diff --git a/CHANGELOG.md b/CHANGELOG.md index 752ce884..e2158f1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,4 @@ +- Fixed PreToolUse and Stop hook HTTP 502 errors (regression in claude-code 2.1.118) - Fixed toolbar button focusing an existing session tab instead of opening a new one when all open tabs have active sessions - Reorganized General and Advanced settings tabs: moved technical settings (hang timeout, MCP port, Enable MCP integration, session list limit, debug mode) to Advanced - Fixed prompt area being collapsible to zero height by dragging the divider diff --git a/claude-launch-tests/test_hook_concurrent.py b/claude-launch-tests/test_hook_concurrent.py new file mode 100644 index 00000000..f33d20bd --- /dev/null +++ b/claude-launch-tests/test_hook_concurrent.py @@ -0,0 +1,234 @@ +#!/usr/bin/env python3 +"""Verify that concurrent PreToolUse hook requests are all served without 502 errors. + +Regression test for the thread-exhaustion bug in HookServlet: when multiple +Write hooks fire simultaneously (each blocking a Jetty thread waiting for +user approval), subsequent hook or Stop hook requests were returned as HTTP 502. + +The test uses a mock hook server that delays each response by DELAY_SECONDS +(simulating the user taking time to approve/reject in the diff dialog), then +eventually responds with "allow". Claude is asked to write several files in one +prompt; all hooks should get 200 responses and the PTY output must contain no +"502" errors. + +Exit codes: + 0 – PASS: all hooks received 200, no 502 in PTY output + 1 – FAIL +""" + +import http.server +import json +import os +import pathlib +import select +import signal +import socket +import subprocess +import sys +import tempfile +import threading +import time +import pty + +# ── tunables ────────────────────────────────────────────────────────────────── + +DELAY_SECONDS = 3 # seconds each hook response is delayed (simulates user think-time) +EXPECTED_HOOKS = 3 # number of files claude is asked to write +TIMEOUT_SECONDS = 90 # total test timeout + +# ── hook server ─────────────────────────────────────────────────────────────── + +hooks_received = [] +hooks_lock = threading.Lock() + +ALLOW_JSON = json.dumps({ + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "allow" + } +}).encode() + + +class DelayedHookHandler(http.server.BaseHTTPRequestHandler): + """Delays the allow response by DELAY_SECONDS to simulate pending user approval.""" + + def log_message(self, fmt, *args): + pass + + def do_POST(self): + length = int(self.headers.get("Content-Length", 0)) + raw = self.rfile.read(length) + try: + body = json.loads(raw) + except Exception: + body = {"raw": raw.decode(errors="replace")} + + with hooks_lock: + idx = len(hooks_received) + 1 + hooks_received.append(body) + tool = body.get("tool_name", "?") + print(f"[hook #{idx}] received: tool={tool}", flush=True) + + # Simulate the user taking DELAY_SECONDS to click Accept in the diff dialog + time.sleep(DELAY_SECONDS) + + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(ALLOW_JSON))) + self.end_headers() + self.wfile.write(ALLOW_JSON) + print(f"[hook #{idx}] responded 200 allow", flush=True) + + +class ThreadedHTTPServer(http.server.ThreadingHTTPServer): + """Serves each hook request in its own thread — mirrors the plugin's Jetty config.""" + pass + + +def free_port(): + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +HOOK_PORT = free_port() +hook_server = ThreadedHTTPServer(("127.0.0.1", HOOK_PORT), DelayedHookHandler) +threading.Thread(target=hook_server.serve_forever, daemon=True).start() +print(f"[test] hook server on port {HOOK_PORT}, delay={DELAY_SECONDS}s per hook", flush=True) + +# ── work directory ──────────────────────────────────────────────────────────── + +workdir = tempfile.mkdtemp(prefix="hook_concurrent_") +claude_dir = os.path.join(workdir, ".claude") +os.makedirs(claude_dir, exist_ok=True) + +settings = { + "hooks": { + "PreToolUse": [{ + "matcher": "Edit|Write|MultiEdit", + "hooks": [{"type": "http", "url": f"http://127.0.0.1:{HOOK_PORT}/hook"}] + }] + } +} +settings_path = os.path.join(claude_dir, "settings.local.json") +with open(settings_path, "w") as f: + json.dump(settings, f, indent=2) + +print(f"[test] workdir: {workdir}", flush=True) + +# ── spawn Claude in TUI mode ────────────────────────────────────────────────── + +master_fd, slave_fd = pty.openpty() +env = os.environ.copy() +env["TERM"] = "xterm-256color" +proc = subprocess.Popen( + ["claude", "--dangerously-skip-permissions"], + cwd=workdir, + stdin=slave_fd, + stdout=slave_fd, + stderr=slave_fd, + env=env, + preexec_fn=os.setsid, + close_fds=True, +) +os.close(slave_fd) + +pty_output_parts = [] +pty_lock = threading.Lock() +stop_reader = threading.Event() + + +def reader(): + while not stop_reader.is_set(): + try: + r, _, _ = select.select([master_fd], [], [], 0.5) + if r: + chunk = os.read(master_fd, 4096).decode("utf-8", errors="replace") + with pty_lock: + pty_output_parts.append(chunk) + print(chunk, end="", flush=True) + except OSError: + break + + +reader_thread = threading.Thread(target=reader, daemon=True) +reader_thread.start() + +# Wait for initial prompt (trust warning / input ready) +time.sleep(5) +os.write(master_fd, b"2") # accept bypass warning if present +time.sleep(3) + +# Ask Claude to write three files in a single prompt so hooks fire concurrently +prompt = ( + "Write three files: file1.txt containing 'alpha', " + "file2.txt containing 'beta', " + "file3.txt containing 'gamma'. " + "Just create them, no explanation." +) +print(f"\n[test] sending prompt", flush=True) +os.write(master_fd, prompt.encode() + b"\r") + +# Wait long enough for all DELAY_SECONDS-delayed hooks to complete +deadline = time.time() + TIMEOUT_SECONDS +while time.time() < deadline: + with hooks_lock: + n = len(hooks_received) + if n >= EXPECTED_HOOKS: + break + time.sleep(1) + +# Give claude a moment to finish writing +time.sleep(5) + +# ── teardown ────────────────────────────────────────────────────────────────── + +stop_reader.set() +proc.send_signal(signal.SIGTERM) +time.sleep(1) +hook_server.shutdown() +reader_thread.join(timeout=3) + +with pty_lock: + pty_output = "".join(pty_output_parts) + +# ── verdict ─────────────────────────────────────────────────────────────────── + +print("\n" + "=" * 60, flush=True) + +with hooks_lock: + n_hooks = len(hooks_received) + +# Check for 502 error text in PTY output +error_patterns = ["HTTP 502", "502 Bad Gateway", "hook error"] +found_errors = [p for p in error_patterns if p.lower() in pty_output.lower()] + +passed = True + +if n_hooks < EXPECTED_HOOKS: + print(f"FAIL: only {n_hooks}/{EXPECTED_HOOKS} hooks received within {TIMEOUT_SECONDS}s") + passed = False +else: + print(f"OK: {n_hooks}/{EXPECTED_HOOKS} hooks received") + +if found_errors: + print(f"FAIL: PTY output contains error pattern(s): {found_errors}") + passed = False +else: + print("OK: no 502/hook errors in PTY output") + +# Verify the files were actually written +for fname in ("file1.txt", "file2.txt", "file3.txt"): + fpath = pathlib.Path(workdir) / fname + if fpath.exists(): + print(f"OK: {fname} exists") + else: + print(f"WARN: {fname} not found (hook may have been delayed past claude timeout)") + +print("=" * 60, flush=True) +if passed: + print("\nRESULT: PASS") + sys.exit(0) +else: + print("\nRESULT: FAIL") + sys.exit(1) diff --git a/claude-launch-tests/test_hook_ipv6_localhost.py b/claude-launch-tests/test_hook_ipv6_localhost.py new file mode 100644 index 00000000..a1491904 --- /dev/null +++ b/claude-launch-tests/test_hook_ipv6_localhost.py @@ -0,0 +1,227 @@ +#!/usr/bin/env python3 +"""Verify that using '127.0.0.1' in the hook URL fixes 502 errors when +'localhost' resolves to IPv6 (::1) but the hook server only listens on IPv4. + +Root cause: Bun (the JS runtime in claude.exe) updated in 2.1.118 to strictly +follow getaddrinfo order — on systems where 'localhost' resolves to '::1' first, +hook HTTP requests connect to ::1:PORT, but the Java (Jetty) server listening on +0.0.0.0:PORT (IPv4 only) refuses the connection, and claude reports "HTTP 502". + +The test runs two variants back-to-back: + FAIL variant – hook server binds IPv4-only, hook URL uses 'localhost' → expects 502 in PTY + PASS variant – hook server binds IPv4-only, hook URL uses '127.0.0.1' → expects no 502 + +Exit codes: + 0 – both variants behaved as expected (FAIL variant got 502, PASS variant did not) + 1 – unexpected result +""" + +import http.server +import json +import os +import pty +import select +import signal +import socket +import subprocess +import sys +import tempfile +import threading +import time + +TIMEOUT = 60 # seconds per variant + +ALLOW_JSON = json.dumps({ + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "allow" + } +}).encode() + + +# ───────────────────────────────────────────────────────────────────────────── +# Helpers +# ───────────────────────────────────────────────────────────────────────────── + +def free_port_ipv4() -> int: + """Allocate a free port on IPv4 only — like Jetty's ServerConnector.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(("0.0.0.0", 0)) + return s.getsockname()[1] + + +class AllowHookHandler(http.server.BaseHTTPRequestHandler): + def log_message(self, fmt, *args): + pass + + def do_POST(self): + length = int(self.headers.get("Content-Length", 0)) + self.rfile.read(length) + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(ALLOW_JSON))) + self.end_headers() + self.wfile.write(ALLOW_JSON) + self.wfile.flush() + + +def start_ipv4_only_server(port: int): + """Start an HTTP server bound ONLY to 0.0.0.0 (IPv4) — mirrors Jetty default.""" + server = http.server.ThreadingHTTPServer(("0.0.0.0", port), AllowHookHandler) + t = threading.Thread(target=server.serve_forever, daemon=True) + t.start() + return server + + +def run_variant(hook_url: str, port: int, label: str) -> tuple[bool, str]: + """ + Run claude, write one file, watch for hook 502 errors. + Returns (got_502: bool, pty_output: str). + """ + workdir = tempfile.mkdtemp(prefix=f"hook_ipv6_{label}_") + claude_dir = os.path.join(workdir, ".claude") + os.makedirs(claude_dir) + + settings = { + "hooks": { + "PreToolUse": [{ + "matcher": "Write", + "hooks": [{"type": "http", "url": hook_url}] + }] + } + } + with open(os.path.join(claude_dir, "settings.local.json"), "w") as f: + json.dump(settings, f) + + master_fd, slave_fd = pty.openpty() + env = os.environ.copy() + env["TERM"] = "xterm-256color" + # Remove HTTP_PROXY so proxy is not a factor — we're testing IPv4/IPv6 only + env.pop("HTTP_PROXY", None) + env.pop("HTTPS_PROXY", None) + env.pop("http_proxy", None) + env.pop("https_proxy", None) + + proc = subprocess.Popen( + ["claude", "--dangerously-skip-permissions"], + cwd=workdir, + stdin=slave_fd, stdout=slave_fd, stderr=slave_fd, + env=env, + preexec_fn=os.setsid, + close_fds=True, + ) + os.close(slave_fd) + + parts = [] + stop_ev = threading.Event() + + def reader(): + while not stop_ev.is_set(): + try: + r, _, _ = select.select([master_fd], [], [], 0.3) + if r: + chunk = os.read(master_fd, 4096).decode("utf-8", errors="replace") + parts.append(chunk) + print(chunk, end="", flush=True) + except OSError: + break + + threading.Thread(target=reader, daemon=True).start() + + # Wait for initial trust/ready prompt + time.sleep(5) + os.write(master_fd, b"2") # accept bypass warning if present + time.sleep(3) + + print(f"\n[{label}] sending Write prompt", flush=True) + os.write(master_fd, b"Write 'hello' to test.txt\r") + + # Wait for hook response or timeout + deadline = time.time() + TIMEOUT + while time.time() < deadline: + combined = "".join(parts) + if "502" in combined or "hook error" in combined.lower(): + break + # Also stop if claude seems done (prompt reappears) + if combined.count(">") >= 4: + break + time.sleep(1) + + time.sleep(3) + stop_ev.set() + try: + os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + except Exception: + pass + proc.wait(timeout=5) + + output = "".join(parts) + got_502 = "502" in output or "hook error" in output.lower() + return got_502, output + + +# ───────────────────────────────────────────────────────────────────────────── +# Main +# ───────────────────────────────────────────────────────────────────────────── + +def main(): + # Check that localhost actually resolves to ::1 on this system + addrs = socket.getaddrinfo("localhost", 80) + preferred = addrs[0][0] # first address family preferred by OS + localhost_is_ipv6 = (preferred == socket.AF_INET6) + print(f"[info] localhost preferred family: {addrs[0][0].name} ({addrs[0][4][0]})") + if not localhost_is_ipv6: + print("[info] localhost does NOT resolve to IPv6 first on this system.") + print("[info] The bug may not be reproducible here — skipping FAIL variant check.") + + port = free_port_ipv4() + server = start_ipv4_only_server(port) + print(f"[info] IPv4-only hook server on 0.0.0.0:{port}", flush=True) + + # ── FAIL variant: localhost URL ────────────────────────────────────────── + localhost_url = f"http://localhost:{port}/hook" + print(f"\n{'='*60}", flush=True) + print(f"[FAIL-variant] hook URL = {localhost_url}", flush=True) + print(f"{'='*60}", flush=True) + got_502_localhost, out_localhost = run_variant(localhost_url, port, "localhost") + print(f"\n[FAIL-variant] got_502={got_502_localhost}", flush=True) + + # ── PASS variant: 127.0.0.1 URL ───────────────────────────────────────── + ipv4_url = f"http://127.0.0.1:{port}/hook" + print(f"\n{'='*60}", flush=True) + print(f"[PASS-variant] hook URL = {ipv4_url}", flush=True) + print(f"{'='*60}", flush=True) + got_502_ipv4, out_ipv4 = run_variant(ipv4_url, port, "127.0.0.1") + print(f"\n[PASS-variant] got_502={got_502_ipv4}", flush=True) + + server.shutdown() + + # ── Verdict ────────────────────────────────────────────────────────────── + print(f"\n{'='*60}", flush=True) + if localhost_is_ipv6: + # On IPv6-first systems: localhost should fail, 127.0.0.1 should succeed + if got_502_localhost and not got_502_ipv4: + print("PASS: localhost caused 502 (IPv6 connect to IPv4-only server)," + " 127.0.0.1 worked correctly.") + sys.exit(0) + elif not got_502_localhost: + print("UNEXPECTED: localhost did NOT cause 502 — hypothesis may be wrong.") + sys.exit(1) + elif got_502_ipv4: + print("FAIL: 127.0.0.1 also caused 502 — 127.0.0.1 fix does NOT solve the problem.") + sys.exit(1) + else: + print("FAIL: unexpected combination.") + sys.exit(1) + else: + # On IPv4-first systems: neither should fail + if not got_502_localhost and not got_502_ipv4: + print("PASS (IPv4-first system): neither URL caused 502 — consistent with hypothesis.") + sys.exit(0) + else: + print("FAIL: unexpected 502 on IPv4-first system.") + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/pom.xml b/pom.xml index b4ef043c..b6fb94dd 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ io.github.nbclaudecodegui netbeans-claude-code-gui - 0.22.17-SNAPSHOT + 0.22.19-SNAPSHOT nbm Netbeans Claude Code GUI NetBeans plugin that provides a GUI for Claude Code CLI — run claude code sessions directly inside the IDE. diff --git a/src/main/java/io/github/nbclaudecodegui/mcp/MCPSseServer.java b/src/main/java/io/github/nbclaudecodegui/mcp/MCPSseServer.java index 69162e6a..227af9e2 100644 --- a/src/main/java/io/github/nbclaudecodegui/mcp/MCPSseServer.java +++ b/src/main/java/io/github/nbclaudecodegui/mcp/MCPSseServer.java @@ -98,11 +98,17 @@ public boolean start(int port) { ctx.setContextPath("/"); ctx.addServlet(new ServletHolder(new SseServlet()), "/sse"); ctx.addServlet(new ServletHolder(new MessagesServlet()), "/messages"); - ctx.addServlet(new ServletHolder(new HookServlet()), "/hook"); + ServletHolder hookHolder = new ServletHolder(new HookServlet()); + hookHolder.getRegistration().setAsyncSupported(true); + ctx.addServlet(hookHolder, "/hook"); ctx.addServlet(new ServletHolder(new StopServlet()), "/stop"); ctx.addServlet(new ServletHolder(new PermissionRequestServlet()), "/permission-request"); server.setHandler(ctx); + server.setRequestLog((request, response) -> + LOGGER.log(Level.FINE, "Jetty: {0} {1} → {2}", + new Object[]{request.getMethod(), request.getRequestURI(), + response.getCommittedMetaData().getStatus()})); server.start(); mcpHandler.setBroadcaster(this::broadcast); @@ -163,13 +169,24 @@ void broadcast(String msg) { // ------------------------------------------------------------------------- /** - * Receives PreToolUse hook calls from Claude Code CLI. + * Receives PreToolUse hook calls from Claude Code CLI using Jetty async dispatch. * - *

Blocks until {@link NetBeansMCPHandler#handlePreToolUse} resolves the - * {@link CompletableFuture} (user clicks Allow/Deny in the diff view), then - * writes the hook decision JSON back to Claude. The hook timeout is 600 s - * (Claude's default); if the future times out we return {@code "ask"} so - * Claude falls back to its built-in PTY dialog instead of hanging. + *

The Jetty thread is released immediately after reading the request body. + * {@link jakarta.servlet.AsyncContext} is used to hold the HTTP connection open + * while the user decides in the diff-view tab (which may take up to 590 s). + * The response is written by {@code future.whenComplete()} on whichever thread + * resolves the {@link CompletableFuture} — typically the EDT when the user + * clicks Accept/Reject/Cancel in {@code FileDiffTab}. + * + *

This prevents Jetty thread-pool exhaustion: previously each pending diff + * dialog blocked one Jetty thread for up to 590 s. With many concurrent Write + * hooks (e.g. claude writing multiple files at once) and a Stop hook firing + * after every response, the thread pool could be saturated, causing HTTP 502. + * + *

Async support is enabled on the {@link ServletHolder} in {@link #start}. + * The async timeout is set to 590 000 ms (just under Claude's 600 s hook + * timeout); on expiry the servlet container calls {@code onTimeout} and we + * return {@code "ask"} so Claude falls back to its built-in PTY dialog. */ private class HookServlet extends HttpServlet { @@ -184,25 +201,50 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) String body = new String(bodyBytes, StandardCharsets.UTF_8); LOGGER.log(Level.INFO, "PreToolUse hook received: {0}", body); - String responseJson; + jakarta.servlet.AsyncContext asyncCtx = req.startAsync(req, resp); + asyncCtx.setTimeout(590_000L); + + CompletableFuture future; try { - CompletableFuture future = mcpHandler.handlePreToolUse(body); - // Wait up to 590 s — just under Claude's 600 s hook timeout - responseJson = future.get(590, TimeUnit.SECONDS); - } catch (java.util.concurrent.TimeoutException e) { - LOGGER.warning("PreToolUse hook timed out; returning 'ask'"); - responseJson = ASK_JSON; + future = mcpHandler.handlePreToolUse(body); } catch (Exception e) { - LOGGER.log(Level.SEVERE, "Error handling PreToolUse hook", e); - responseJson = ASK_JSON; + LOGGER.log(Level.SEVERE, "Error starting PreToolUse hook handler", e); + asyncCtx.complete(); + return; } - byte[] responseBytes = responseJson.getBytes(StandardCharsets.UTF_8); + // Timeout: resolve with ASK_JSON so Claude falls back to its PTY dialog + asyncCtx.addListener(new jakarta.servlet.AsyncListener() { + @Override public void onTimeout(jakarta.servlet.AsyncEvent ev) { + LOGGER.warning("PreToolUse hook async timeout; returning 'ask'"); + future.complete(ASK_JSON); + } + @Override public void onError(jakarta.servlet.AsyncEvent ev) { + future.complete(ASK_JSON); + } + @Override public void onComplete(jakarta.servlet.AsyncEvent ev) {} + @Override public void onStartAsync(jakarta.servlet.AsyncEvent ev) {} + }); - resp.setStatus(HttpServletResponse.SC_OK); - resp.setContentType("application/json"); - resp.setContentLength(responseBytes.length); - resp.getOutputStream().write(responseBytes); + future.whenComplete((responseJson, ex) -> { + if (ex != null || responseJson == null) { + responseJson = ASK_JSON; + } + try { + HttpServletResponse r = (HttpServletResponse) asyncCtx.getResponse(); + byte[] bytes = responseJson.getBytes(StandardCharsets.UTF_8); + r.setStatus(HttpServletResponse.SC_OK); + r.setContentType("application/json"); + r.setContentLength(bytes.length); + r.getOutputStream().write(bytes); + r.getOutputStream().flush(); + } catch (IOException e) { + LOGGER.log(Level.WARNING, "Error writing async hook response", e); + } finally { + asyncCtx.complete(); + } + }); + // Jetty thread is released here — no blocking } } @@ -226,6 +268,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) resp.setStatus(HttpServletResponse.SC_OK); resp.setContentType("application/json"); resp.getOutputStream().write("{}".getBytes(StandardCharsets.UTF_8)); + resp.getOutputStream().flush(); } } @@ -245,6 +288,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) resp.setStatus(HttpServletResponse.SC_OK); resp.setContentType("application/json"); resp.getOutputStream().write("{}".getBytes(StandardCharsets.UTF_8)); + resp.getOutputStream().flush(); } } diff --git a/src/main/java/io/github/nbclaudecodegui/mcp/NetBeansMCPHandler.java b/src/main/java/io/github/nbclaudecodegui/mcp/NetBeansMCPHandler.java index d792f5e4..da5035f5 100644 --- a/src/main/java/io/github/nbclaudecodegui/mcp/NetBeansMCPHandler.java +++ b/src/main/java/io/github/nbclaudecodegui/mcp/NetBeansMCPHandler.java @@ -658,18 +658,14 @@ public void handlePermissionRequest(String payload) { findSessionByCwd(cwd).ifPresent(ClaudeSessionTab::triggerPromptScan)); } - private static String extractCwdFromPayload(String payload) { + private String extractCwdFromPayload(String payload) { if (payload == null) return null; - // Simple extraction without full JSON parse - int idx = payload.indexOf("\"cwd\""); - if (idx < 0) return null; - int colon = payload.indexOf(':', idx); - if (colon < 0) return null; - int q1 = payload.indexOf('"', colon + 1); - if (q1 < 0) return null; - int q2 = payload.indexOf('"', q1 + 1); - if (q2 < 0) return null; - return payload.substring(q1 + 1, q2); + try { + JsonNode node = objectMapper.readTree(payload); + return node.has("cwd") ? node.get("cwd").asText(null) : null; + } catch (Exception e) { + return null; + } } private static java.util.Optional findSessionByCwd(String cwd) { diff --git a/src/main/java/io/github/nbclaudecodegui/process/ClaudeProcess.java b/src/main/java/io/github/nbclaudecodegui/process/ClaudeProcess.java index 2155cbec..e9d73196 100644 --- a/src/main/java/io/github/nbclaudecodegui/process/ClaudeProcess.java +++ b/src/main/java/io/github/nbclaudecodegui/process/ClaudeProcess.java @@ -443,13 +443,17 @@ static String mergeSettingsJson(String existingJson, int port, ClaudeProfile pro } } - // Add fresh entry for this port + // Add fresh entry for this port. + // Use 127.0.0.1, NOT localhost: on systems where localhost resolves to ::1 (IPv6) + // first, claude-code's HTTP client (Bun) connects to ::1:PORT, but Jetty's + // ServerConnector binds to 0.0.0.0 (IPv4 only) and refuses the connection. + // 127.0.0.1 is always IPv4 and requires no DNS resolution. ObjectNode ourEntry = MAPPER.createObjectNode(); ourEntry.put("matcher", OUR_HOOK_MATCHER); ArrayNode hooksArr = MAPPER.createArrayNode(); ObjectNode httpHook = MAPPER.createObjectNode(); httpHook.put("type", "http"); - httpHook.put("url", "http://localhost:" + port + "/hook"); + httpHook.put("url", "http://127.0.0.1:" + port + "/hook"); hooksArr.add(httpHook); ourEntry.set("hooks", hooksArr); filtered.add(ourEntry); @@ -462,7 +466,7 @@ static String mergeSettingsJson(String existingJson, int port, ClaudeProfile pro ArrayNode stopHooksArr = MAPPER.createArrayNode(); ObjectNode stopHook = MAPPER.createObjectNode(); stopHook.put("type", "http"); - stopHook.put("url", "http://localhost:" + port + "/stop"); + stopHook.put("url", "http://127.0.0.1:" + port + "/stop"); stopHooksArr.add(stopHook); stopEntry.set("hooks", stopHooksArr); ArrayNode stopArr = MAPPER.createArrayNode(); @@ -475,7 +479,7 @@ static String mergeSettingsJson(String existingJson, int port, ClaudeProfile pro ArrayNode permHooksArr = MAPPER.createArrayNode(); ObjectNode permHook = MAPPER.createObjectNode(); permHook.put("type", "http"); - permHook.put("url", "http://localhost:" + port + "/permission-request"); + permHook.put("url", "http://127.0.0.1:" + port + "/permission-request"); permHooksArr.add(permHook); permEntry.set("hooks", permHooksArr); ArrayNode permArr = MAPPER.createArrayNode(); @@ -730,7 +734,7 @@ static Path writeMcpConfigTempFile(int port) throws IOException { * Builds the JSON string to pass as {@code --mcp-config} to the Claude CLI. */ static String buildMcpConfigJson(int port) { - return "{\"mcpServers\":{\"" + OUR_MCP_KEY + "\":{\"type\":\"sse\",\"url\":\"http://localhost:" + port + "/sse\"}}}"; + return "{\"mcpServers\":{\"" + OUR_MCP_KEY + "\":{\"type\":\"sse\",\"url\":\"http://127.0.0.1:" + port + "/sse\"}}}"; } /** @@ -762,18 +766,19 @@ static List parseArgs(String args) { return result; } + // 127.0.0.1 is used instead of localhost — see comment in mergeSettingsJson. private static String buildSettingsLocalJson(int port) { return "{" + "\"hooks\":{" + "\"PreToolUse\":[" + "{\"matcher\":\"Edit|Write|MultiEdit\"," - + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://localhost:" + port + "/hook\"}]}]," + + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://127.0.0.1:" + port + "/hook\"}]}]," + "\"Stop\":[" + "{\"matcher\":\".*\"," - + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://localhost:" + port + "/stop\"}]}]," + + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://127.0.0.1:" + port + "/stop\"}]}]," + "\"PermissionRequest\":[" + "{\"matcher\":\".*\"," - + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://localhost:" + port + "/permission-request\"}]}]" + + "\"hooks\":[{\"type\":\"http\",\"url\":\"http://127.0.0.1:" + port + "/permission-request\"}]}]" + "}}"; } } diff --git a/src/test/java/io/github/nbclaudecodegui/mcp/MCPSseServerPerSessionTest.java b/src/test/java/io/github/nbclaudecodegui/mcp/MCPSseServerPerSessionTest.java index 57549c16..34d6a4cc 100644 --- a/src/test/java/io/github/nbclaudecodegui/mcp/MCPSseServerPerSessionTest.java +++ b/src/test/java/io/github/nbclaudecodegui/mcp/MCPSseServerPerSessionTest.java @@ -9,8 +9,13 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.AfterEach; @@ -202,10 +207,164 @@ void testSessionCountTracked() throws Exception { sessionB.disconnect(); } + // ----------------------------------------------------------------------- + // TC-5: HookServlet async — Jetty thread not blocked while future is pending + // ----------------------------------------------------------------------- + + /** + * A POST to {@code /hook} must return (HTTP connection must be completed) + * only AFTER the CompletableFuture is resolved, but the Jetty worker thread + * must NOT be held during the wait. + * + *

The test verifies async behaviour indirectly: a second concurrent request + * to a different endpoint ({@code /stop}) must be served while the hook future + * is still pending. If the servlet were synchronous, the single Jetty thread + * handling the hook would be blocked and the /stop request would time out. + */ + @Test + void testHookServletAsyncDoesNotBlockThread() throws Exception { + CompletableFuture pendingFuture = new CompletableFuture<>(); + MCPSseServer hookServer = startWithHandler(new NetBeansMCPHandler() { + @Override + public CompletableFuture handlePreToolUse(String hookJson) { + return pendingFuture; + } + }); + try { + int hookPort = hookServer.getPort(); + + // Fire the hook request — it should NOT block the Jetty thread + CountDownLatch hookDone = new CountDownLatch(1); + AtomicInteger hookStatus = new AtomicInteger(-1); + new Thread(() -> { + try { + hookStatus.set(postToHook(hookPort, "{}")); + } catch (Exception e) { /* ignore */ } + hookDone.countDown(); + }).start(); + + // While hook is pending, /stop must still be served within 2 seconds + int stopStatus = postToStop(hookPort, "{}"); + assertEquals(200, stopStatus, "/stop must return 200 while hook is pending"); + + // Now resolve the future and verify hook response is delivered + pendingFuture.complete("{\"hookSpecificOutput\":{\"hookEventName\":\"PreToolUse\",\"permissionDecision\":\"allow\"}}"); + assertTrue(hookDone.await(5, TimeUnit.SECONDS), "hook response not delivered after future resolved"); + assertEquals(200, hookStatus.get(), "/hook must return 200 after future resolved"); + } finally { + hookServer.stop(); + } + } + + // ----------------------------------------------------------------------- + // TC-6: N concurrent hook requests — all served without 502 + // ----------------------------------------------------------------------- + + /** + * Sends N concurrent {@code /hook} requests (each backed by a pending + * {@link CompletableFuture}) and verifies that ALL return HTTP 200 after + * their futures are resolved. This is the regression test for thread-pool + * exhaustion: the synchronous implementation would have blocked N Jetty + * threads; if N exceeded the pool size some requests would have returned 502. + */ + @Test + void testConcurrentHookRequestsAllServed() throws Exception { + int n = 10; + List> futures = new ArrayList<>(); + MCPSseServer hookServer = startWithHandler(new NetBeansMCPHandler() { + @Override + public CompletableFuture handlePreToolUse(String hookJson) { + CompletableFuture f = new CompletableFuture<>(); + synchronized (futures) { futures.add(f); } + return f; + } + }); + String allowJson = "{\"hookSpecificOutput\":{\"hookEventName\":\"PreToolUse\",\"permissionDecision\":\"allow\"}}"; + try { + int hookPort = hookServer.getPort(); + ExecutorService pool = Executors.newFixedThreadPool(n); + List> results = new ArrayList<>(); + for (int i = 0; i < n; i++) { + results.add(pool.submit(() -> postToHook(hookPort, "{}"))); + } + pool.shutdown(); + + // Wait until all N futures are registered + long deadline = System.currentTimeMillis() + 5000; + while (System.currentTimeMillis() < deadline) { + synchronized (futures) { if (futures.size() == n) break; } + Thread.sleep(50); + } + synchronized (futures) { + assertEquals(n, futures.size(), n + " futures must be registered"); + futures.forEach(f -> f.complete(allowJson)); + } + + // All N responses must be 200 + AtomicInteger failures = new AtomicInteger(0); + for (Future r : results) { + int status = r.get(10, TimeUnit.SECONDS); + if (status != 200) failures.incrementAndGet(); + } + assertEquals(0, failures.get(), "all " + n + " hook requests must return 200"); + } finally { + hookServer.stop(); + } + } + + // ----------------------------------------------------------------------- + // TC-7: POST /stop returns 200 with {} body + // ----------------------------------------------------------------------- + + /** + * A well-formed {@code POST /stop} must return {@code 200 OK} with body + * {@code {}}. Regression guard for a missing {@code flush()} call. + */ + @Test + void testStopHookReturns200() throws Exception { + String payload = "{\"hook_event_name\":\"Stop\",\"cwd\":\"/tmp\",\"session_id\":\"abc\"}"; + int status = postToStop(port, payload); + assertEquals(200, status, "/stop must return 200"); + } + // ----------------------------------------------------------------------- // Helpers // ----------------------------------------------------------------------- + /** Starts a fresh MCPSseServer on a free port backed by the given handler. */ + private MCPSseServer startWithHandler(NetBeansMCPHandler handler) throws Exception { + int p = findFreePort(); + MCPSseServer s = new MCPSseServer(handler); + assertTrue(s.start(p), "server with custom handler must start"); + return s; + } + + private int postToHook(int hookPort, String json) throws Exception { + URL url = new URL("http://localhost:" + hookPort + "/hook"); + HttpURLConnection conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("POST"); + conn.setDoOutput(true); + conn.setRequestProperty("Content-Type", "application/json"); + conn.setConnectTimeout(3000); + conn.setReadTimeout(15000); + byte[] body = json.getBytes(StandardCharsets.UTF_8); + try (OutputStream out = conn.getOutputStream()) { out.write(body); } + return conn.getResponseCode(); + } + + private int postToStop(int stopPort, String json) throws Exception { + URL url = new URL("http://localhost:" + stopPort + "/stop"); + HttpURLConnection conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("POST"); + conn.setDoOutput(true); + conn.setRequestProperty("Content-Type", "application/json"); + conn.setConnectTimeout(3000); + conn.setReadTimeout(5000); + byte[] body = json.getBytes(StandardCharsets.UTF_8); + try (OutputStream out = conn.getOutputStream()) { out.write(body); } + return conn.getResponseCode(); + } + private void postMessage(String messagesUrl, String json) throws Exception { URL url = new URL(messagesUrl); HttpURLConnection conn = (HttpURLConnection) url.openConnection(); diff --git a/src/test/java/io/github/nbclaudecodegui/process/ClaudeProcessTest.java b/src/test/java/io/github/nbclaudecodegui/process/ClaudeProcessTest.java index 2d1b2bf4..8f461b9b 100644 --- a/src/test/java/io/github/nbclaudecodegui/process/ClaudeProcessTest.java +++ b/src/test/java/io/github/nbclaudecodegui/process/ClaudeProcessTest.java @@ -100,7 +100,7 @@ void testMergeEmptyFileAddsOurEntries() { assertFalse(result.contains("\"netbeans\""), "mcpServers.netbeans should NOT appear in settings.local.json"); assertFalse(result.contains("/sse"), "SSE URL should NOT appear in settings.local.json"); assertTrue(result.contains("PreToolUse"), "should add PreToolUse hook"); - assertTrue(result.contains("http://localhost:8990/hook"), "should contain hook URL"); + assertTrue(result.contains("http://127.0.0.1:8990/hook"), "should contain hook URL"); assertTrue(result.contains("Edit|Write|MultiEdit"), "should contain matcher"); } @@ -117,7 +117,7 @@ void testMergePreservesOtherMcpServers() { void testMergeUpdatesPortWhenAlreadyPresent() { String existing = ClaudeProcess.mergeSettingsJson("{}", 8888, null); String result = ClaudeProcess.mergeSettingsJson(existing, 9999, null); - assertTrue(result.contains("localhost:9999/hook"), "hook URL should use new port"); + assertTrue(result.contains("127.0.0.1:9999/hook"), "hook URL should use new port"); assertFalse(result.contains("localhost:8888"), "should not keep old port"); assertEquals(1, countOccurrences(result, "Edit|Write|MultiEdit")); } @@ -246,7 +246,7 @@ void testWriteMcpConfigTempFile() throws Exception { void testBuildMcpConfigJsonContainsNetbeans() { String json = ClaudeProcess.buildMcpConfigJson(9000); assertTrue(json.contains("\"netbeans\""), "should contain netbeans key"); - assertTrue(json.contains("http://localhost:9000/sse"), "should contain SSE URL"); + assertTrue(json.contains("http://127.0.0.1:9000/sse"), "should contain SSE URL"); assertTrue(json.contains("\"type\":\"sse\""), "should be SSE type"); } @@ -277,13 +277,13 @@ void testMergeReplacesOurHookWithUpdatedPort() { String result = ClaudeProcess.mergeSettingsJson(existing, 9999, null); assertEquals(1, countOccurrences(result, "Edit|Write|MultiEdit"), "our hook matcher should appear exactly once"); - assertTrue(result.contains("localhost:9999/hook"), "hook URL should be updated"); + assertTrue(result.contains("127.0.0.1:9999/hook"), "hook URL should be updated"); } @Test void testMergeRemovesStaleNetbeansEntry() { // Simulate a file left by old plugin (< 0.19.22) that has mcpServers.netbeans - String stale = "{\"mcpServers\":{\"netbeans\":{\"type\":\"sse\",\"url\":\"http://localhost:8888/sse\"}}}"; + String stale = "{\"mcpServers\":{\"netbeans\":{\"type\":\"sse\",\"url\":\"http://127.0.0.1:8888/sse\"}}}"; String result = ClaudeProcess.mergeSettingsJson(stale, 9000, null); assertFalse(result.contains("\"netbeans\""), "stale mcpServers.netbeans should be removed on merge"); assertFalse(result.contains("mcpServers"), "mcpServers should be gone if netbeans was the only entry"); @@ -292,7 +292,7 @@ void testMergeRemovesStaleNetbeansEntry() { @Test void testMergeRemovesStaleNetbeansButKeepsOtherMcpServers() { String stale = "{\"mcpServers\":{" - + "\"netbeans\":{\"type\":\"sse\",\"url\":\"http://localhost:8888/sse\"}," + + "\"netbeans\":{\"type\":\"sse\",\"url\":\"http://127.0.0.1:8888/sse\"}," + "\"other\":{\"type\":\"stdio\",\"command\":\"foo\"}}}"; String result = ClaudeProcess.mergeSettingsJson(stale, 9000, null); assertFalse(result.contains("\"netbeans\""), "stale netbeans entry should be removed"); @@ -306,7 +306,7 @@ void testMergeRemovesStaleNetbeansButKeepsOtherMcpServers() { @Test void testCleanupRemovesStaleNetbeansEntry() { // File has stale mcpServers.netbeans from old plugin + our hooks - String stale = "{\"mcpServers\":{\"netbeans\":{\"type\":\"sse\",\"url\":\"http://localhost:8888/sse\"}}," + String stale = "{\"mcpServers\":{\"netbeans\":{\"type\":\"sse\",\"url\":\"http://127.0.0.1:8888/sse\"}}," + "\"hooks\":{\"Stop\":[{\"matcher\":\".*\",\"hooks\":[]}]}}"; String cleaned = ClaudeProcess.cleanedSettingsJson(stale); assertNull(cleaned, "should delete file — only plugin-owned content after cleanup"); @@ -315,7 +315,7 @@ void testCleanupRemovesStaleNetbeansEntry() { @Test void testCleanupRemovesStaleNetbeansButKeepsUserMcpServers() { String stale = "{\"mcpServers\":{" - + "\"netbeans\":{\"type\":\"sse\",\"url\":\"http://localhost:8888/sse\"}," + + "\"netbeans\":{\"type\":\"sse\",\"url\":\"http://127.0.0.1:8888/sse\"}," + "\"user-server\":{\"type\":\"stdio\",\"command\":\"bar\"}}}"; String cleaned = ClaudeProcess.cleanedSettingsJson(stale); assertNotNull(cleaned, "should keep file — user-server remains");