test(mcp): report why the server did not answer, instead of hanging on it (#350)

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;
 }