From 3aa15ab02b182cbb0e0fe397210d0c0fc559495b Mon Sep 17 00:00:00 2001
From: Ercan Er <ercan.er@medeniyet.edu.tr>
Date: Fri, 11 Sep 2026 04:27:15 +0300
Subject: [PATCH] test(mcp): report why the server did not answer, instead of
 hanging on it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

rpc() gave up after a fixed 15s, returned whatever had arrived, and dropped the
child's stderr. Callers dereferenced into `undefined`, so a slow server and a
crashed one both surfaced as `Cannot read properties of undefined (reading
'result')` — naming neither the timeout nor the request.

The crash case was worse than badly reported. `await once(child, 'exit')` after
`child.kill()` only settles while the child is still alive to be killed; a
server that died on its own has already fired `exit` and will not fire it
again, so the await hangs forever. Replacing the server with something that
writes to stderr and exits 3: the file was still running when I killed it at
120s, with none of its five tests having reported. It now fails in 0.1s with
the exit code and the server's own stderr in the message.

- Throw instead of returning short: how many of how many, and how long it took.
- Keep the child's stderr and put it in that error. It was piped already; it
  was just never read.
- Await a `close` promise created at spawn time, so it settles whether the
  child crashed or was killed. This is the hang.
- Stop waiting the moment the child quits — nothing can arrive after that.
- Deadline 15s to 60s, named. Only a live-but-silent server can reach it now,
  and the loop leaves as soon as the replies are in, so it costs nothing when
  things work. 15s was the number that flaked: ~2s of work has less headroom
  than it looks with ~30 test files running in parallel.

`close` rather than `exit` throughout: it fires once stdio is drained, so a
server that answers and then quits is not misread as having quit without
answering.

Fixes #350

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 test/mcp-server.test.ts | 49 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

diff --git a/test/mcp-server.test.ts b/test/mcp-server.test.ts
index f0610e034..7f383ad2c 100644
--- a/test/mcp-server.test.ts
+++ b/test/mcp-server.test.ts
@@ -7,10 +7,39 @@ import { join } from 'node:path';
 import { once } from 'node:events';
 import { buildGraph } from '../src/graph/build.js';
 
+/**
+ * How long to wait for the server's replies. Generous on purpose: the loop below
+ * leaves the moment the replies are in — or the moment the child quits — so this
+ * is only ever paid by a server that is up and genuinely silent.
+ */
+const RPC_TIMEOUT_MS = 60_000;
+
+/**
+ * Drive the MCP server over stdio and return its replies.
+ *
+ * Throws rather than returning short. Returning the partial array meant every
+ * caller dereferenced into `undefined` — `Cannot read properties of undefined
+ * (reading 'result')` — which named neither the timeout nor the request, and
+ * read the same whether the server had been slow, had crashed on startup, or had
+ * answered something unparseable. The child's stderr is kept for the same
+ * reason: it is the only place a crash says why, and it was being piped and then
+ * dropped.
+ *
+ * A crashed server used to be worse than badly reported, it hung the run: with
+ * the child already gone, `await once(child, 'exit')` waits for an event that
+ * has fired and will not fire again. The `close` promise below is created while
+ * the child is still alive, so it settles whenever the child ends, whether that
+ * is a crash of its own or the kill at the end.
+ */
 async function rpc(messages: object[], dir: string, expected: number): Promise<any[]> {
   const child = spawn(process.execPath, ['--import', 'tsx', 'src/cli.ts', 'mcp', dir], { stdio: ['pipe', 'pipe', 'pipe'] });
+  const closed = once(child, 'close');
   const responses: any[] = [];
   let buf = '';
+  let stderr = '';
+  // `close`, not `exit`: it fires once the child's stdio is drained, so a server
+  // that answers and then quits is not read as having quit without answering.
+  let died: string | null = null;
   child.stdout.on('data', (d) => {
     buf += d.toString();
     let i;
@@ -20,11 +49,25 @@ async function rpc(messages: object[], dir: string, expected: number): Promise<a
       if (line) responses.push(JSON.parse(line));
     }
   });
+  child.stderr.on('data', (d) => (stderr += d.toString()));
+  child.on('close', (code, signal) => (died ??= `exit code ${code}, signal ${signal}`));
   for (const m of messages) child.stdin.write(`${JSON.stringify(m)}\n`);
-  const deadline = Date.now() + 15000;
-  while (responses.length < expected && Date.now() < deadline) await new Promise((r) => setTimeout(r, 50));
+  const started = Date.now();
+  const deadline = started + RPC_TIMEOUT_MS;
+  while (responses.length < expected && died === null && Date.now() < deadline) {
+    await new Promise((r) => setTimeout(r, 50));
+  }
+  const waited = ((Date.now() - started) / 1000).toFixed(1);
+  const quitFirst = died;
   child.kill();
-  await once(child, 'exit').catch(() => {});
+  await closed.catch(() => {});
+  if (responses.length < expected) {
+    throw new Error(
+      `mcp server answered ${responses.length} of ${expected} in ${waited}s — ` +
+        (quitFirst ? `it quit first (${quitFirst})` : 'it was still running') +
+        (stderr.trim() ? `\n--- its stderr ---\n${stderr.trim()}` : '\nIt wrote nothing to stderr.'),
+    );
+  }
   return responses;
 }