From 09b35489aac970b6dfb1404e32c87f25701dd61d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 12:22:24 -0400 Subject: [PATCH] test(scripts): cover the readiness-timeout teardown path (#2000 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot's review is right that the fix was unproven: smoke:web:app's happy path always receives the announcement, so `npm run ci` never exercised a child that stays alive *through* the readiness timeout — exactly the case that orphaned a live server holding its port. A regression there would be silent in CI and would surface only as a stray process on a later run. Extract the spawn/readiness ownership into scripts/lib/announced-child.mjs (the same move prod-web-server.mjs made for teardownWebServer, and for the same reason), and add scripts/lib/announced-child.test.mjs under `test:scripts`. The tests drive real `node -e` children rather than spies, so the assertion is that a real process is reachable and killable, not that a mock was called: the timeout case asserts the child was published before the wait, is still alive when the throw lands, and is actually stopped by kill. The early-exit and spawn-failure paths are covered alongside it, plus the both-streams scan. smoke-web-app.mjs keeps its behavior — startMcpServer() now delegates the spawn and publishes the child via `onSpawn`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- scripts/lib/announced-child.mjs | 85 ++++++++++++++++++ scripts/lib/announced-child.test.mjs | 128 +++++++++++++++++++++++++++ scripts/smoke-web-app.mjs | 70 +++++---------- 3 files changed, 236 insertions(+), 47 deletions(-) create mode 100644 scripts/lib/announced-child.mjs create mode 100644 scripts/lib/announced-child.test.mjs diff --git a/scripts/lib/announced-child.mjs b/scripts/lib/announced-child.mjs new file mode 100644 index 00000000..50c8f529 --- /dev/null +++ b/scripts/lib/announced-child.mjs @@ -0,0 +1,85 @@ +/** + * Spawn a child process and wait for it to announce readiness on its output. + * + * Extracted from `scripts/smoke-web-app.mjs` so the failure path can be tested + * (#2000). A smoke script only ever exercises its own happy path: the real MCP + * test server always announces, so nothing in `npm run ci` proved that a child + * which stays alive *through* the readiness timeout is still reachable by the + * caller's teardown. That is precisely the case that used to orphan a live + * server holding its port, and it is invisible to the smoke itself. + * + * The ownership rule this encodes: the child is handed to `onSpawn` the moment + * it exists, **before** the readiness wait, so every throw path below leaves + * the caller holding a stoppable handle. Returning it only on success is what + * made the timeout leak — `spawnError` and `exited` were never affected (the + * child is already gone there), but they cost nothing to cover the same way. + * + * Both stdio channels are piped and scanned: a child that announces with + * `console.error` is missed entirely by a stdout-only scan, which then times + * out with an empty diagnostic. Piping both also keeps the child's noise out of + * the caller's output while still making it available in the failure message. + */ + +import { spawn } from "node:child_process"; +import { setTimeout as delay } from "node:timers/promises"; + +/** + * @param {object} opts + * @param {string} opts.command Executable to spawn. + * @param {string[]} opts.args Arguments for it. + * @param {string} [opts.cwd] Working directory. + * @param {RegExp} opts.pattern Matched against the accumulated output; + * the first match ends the wait. + * @param {(child: import("node:child_process").ChildProcess) => void} opts.onSpawn + * Called synchronously with the child immediately after spawn, before any + * waiting. This is the caller's teardown handle. + * @param {string} opts.what Noun used in error messages ("MCP test server"). + * @param {number} [opts.timeoutMs] Readiness budget (default 30s). + * @param {number} [opts.pollMs] Poll interval (default 250ms). + * @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>} + */ +export async function startAnnouncedChild({ + command, + args, + cwd, + pattern, + onSpawn, + what, + timeoutMs = 30_000, + pollMs = 250, +}) { + const child = spawn(command, args, { + cwd, + stdio: ["ignore", "pipe", "pipe"], + }); + // Publish before waiting — this is the whole point of the helper. + onSpawn(child); + + let out = ""; + child.stdout.on("data", (d) => (out += d)); + child.stderr.on("data", (d) => (out += d)); + let exited = false; + let spawnError = null; + // A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` — + // and with no `error` listener Node throws it uncaught, replacing the caller's + // diagnostic with a raw stack. `close` is listened to alongside `exit` for the + // same reason: it fires in cases `exit` does not, so a child that dies without + // an exit event can't leave the poll below spinning for the full budget. + child.on("error", (err) => (spawnError = err)); + child.on("exit", () => (exited = true)); + child.on("close", () => (exited = true)); + + const attempts = Math.max(1, Math.ceil(timeoutMs / pollMs)); + for (let attempt = 0; attempt < attempts; attempt++) { + const match = out.match(pattern); + if (match) return { child, match }; + if (spawnError) { + throw new Error(`could not spawn the ${what}: ${spawnError.message}`); + } + if (exited) throw new Error(`${what} exited early:\n${out}`); + await delay(pollMs); + } + throw new Error( + `${what} did not start within ${Math.round(timeoutMs / 1000)}s:\n${out}`, + ); +} diff --git a/scripts/lib/announced-child.test.mjs b/scripts/lib/announced-child.test.mjs new file mode 100644 index 00000000..f8ff8847 --- /dev/null +++ b/scripts/lib/announced-child.test.mjs @@ -0,0 +1,128 @@ +/** + * Tests for the spawn/readiness ownership helper (#2000). + * + * The invariant under test is the one a smoke script can never check itself: + * a child that stays alive *through* the readiness timeout must still be + * reachable by the caller's teardown. `smoke:web:app`'s happy path always gets + * the announcement, so a regression here would be silent in `npm run ci` and + * would surface only as a stray process holding a port on a later run. + * + * Every case drives a real `node -e` child rather than a spy, so the assertion + * is about a real process being killable, not about a mock's call log. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { startAnnouncedChild } from "./announced-child.mjs"; + +/** Wait for a real child to exit (or resolve immediately if it already has). */ +function waitForExit(child) { + if (child.exitCode !== null || child.signalCode !== null) + return Promise.resolve(); + return new Promise((resolve) => child.once("close", resolve)); +} + +test("hands the child to onSpawn before waiting, and returns the match", async () => { + let seen = null; + const { child, match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("listening at http://127.0.0.1:9999")'], + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: (c) => (seen = c), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal( + seen, + child, + "onSpawn received the same child that was returned", + ); + assert.equal(match[1], "http://127.0.0.1:9999"); + child.kill("SIGKILL"); + await waitForExit(child); +}); + +test("scans stdout as well as stderr", async () => { + const { child, match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.log("ready on 4242")'], + pattern: /ready on (\d+)/, + onSpawn: () => {}, + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal(match[1], "4242"); + child.kill("SIGKILL"); + await waitForExit(child); +}); + +test("a non-announcing child is still reachable and killable after the timeout", async () => { + // The regression: a child that is alive but never announces. The throw must + // not be the only thing that happens — the caller must already hold the + // handle, or `process.exit(1)` leaves this running. + let published = null; + await assert.rejects( + startAnnouncedChild({ + // Sleeps well past the budget and prints nothing matching. + command: process.execPath, + args: ["-e", "setTimeout(() => {}, 60000)"], + pattern: /never going to match/, + onSpawn: (c) => (published = c), + what: "probe", + timeoutMs: 300, + pollMs: 25, + }), + /probe did not start within/, + ); + + assert.ok(published, "the child was published before the readiness wait"); + assert.equal( + published.exitCode, + null, + "and it is still alive, as in the bug", + ); + + published.kill("SIGKILL"); + await waitForExit(published); + assert.ok( + published.exitCode !== null || published.signalCode !== null, + "teardown could actually stop it", + ); +}); + +test("reports a child that exits before announcing, with its output", async () => { + let published = null; + await assert.rejects( + startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("boom: bad config"); process.exit(3)'], + pattern: /never going to match/, + onSpawn: (c) => (published = c), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }), + (err) => + /probe exited early/.test(err.message) && + /boom: bad config/.test(err.message), + ); + assert.ok(published, "published even on the early-exit path"); + await waitForExit(published); +}); + +test("reports a spawn failure rather than throwing it uncaught", async () => { + await assert.rejects( + startAnnouncedChild({ + command: "definitely-not-an-executable-2000", + args: [], + pattern: /never/, + onSpawn: () => {}, + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }), + /could not spawn the probe:/, + ); +}); diff --git a/scripts/smoke-web-app.mjs b/scripts/smoke-web-app.mjs index b7f3a3b4..b849f357 100644 --- a/scripts/smoke-web-app.mjs +++ b/scripts/smoke-web-app.mjs @@ -46,13 +46,13 @@ * demand if missing, as in smoke:cli. */ -import { spawn, spawnSync } from "node:child_process"; +import { spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { createRequire } from "node:module"; -import { setTimeout as delay } from "node:timers/promises"; import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; +import { startAnnouncedChild } from "./lib/announced-child.mjs"; import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); @@ -164,54 +164,30 @@ function ensureTestServer() { /** * Spawn the MCP App test server and wait for it to announce its URL. * - * Both stdio channels are piped and scanned: server-composable.ts announces - * readiness with `console.error`, so watching stdout alone never matches and - * this times out with an empty diagnostic. Piping both also keeps the child's - * noise out of the smoke's own output while still making it available in the - * failure message. + * The spawn/readiness ownership lives in `lib/announced-child.mjs` so the + * failure path is testable: this smoke's happy path always gets the + * announcement, so nothing here could prove that a child which stays alive + * through the readiness timeout is still reachable by `shutdown()` — the case + * that used to orphan a live server holding its port (#2000). `onSpawn` + * publishes the child to `mcpServer` before the wait, so every throw path is + * covered by teardown. * - * The child is published to `mcpServer` the moment it is spawned rather than - * handed back on success, so `shutdown()` can stop it on every throw path. - * Returning it only after the announcement matched left the readiness timeout - * — a child that is alive but never announces — with no reachable handle, so - * `process.exit(1)` orphaned a live server still holding its port. The - * `spawnError` and `exited` paths never had that problem (the child is already - * gone there), but they cost nothing to cover the same way. + * The announced URL is authoritative rather than the config's port: + * createTestServerHttp resolves through findAvailablePort(), which walks upward + * when the configured value is taken. */ async function startMcpServer() { - const child = spawn( - process.execPath, - [composableServer, "--config", appConfig], - { cwd: repoRoot, stdio: ["ignore", "pipe", "pipe"] }, - ); - mcpServer = child; - let out = ""; - child.stdout.on("data", (d) => (out += d)); - child.stderr.on("data", (d) => (out += d)); - let exited = false; - let spawnError = null; - // A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` — - // and with no `error` listener Node throws it uncaught, replacing this smoke's - // diagnostic with a raw stack. `close` is listened to alongside `exit` for the - // same reason: it fires in cases `exit` does not, so a child that dies without - // an exit event can't leave the poll below spinning for the full 30s. - child.on("error", (err) => (spawnError = err)); - child.on("exit", () => (exited = true)); - child.on("close", () => (exited = true)); - - for (let attempt = 0; attempt < 120; attempt++) { - // Take the port the server actually bound, not the one we asked for. - const announced = out.match(/listening at (http:\/\/\S+)/i); - if (announced) return { url: announced[1] }; - if (spawnError) { - throw new Error( - `could not spawn the MCP test server (${composableServer}): ${spawnError.message}`, - ); - } - if (exited) throw new Error(`MCP test server exited early:\n${out}`); - await delay(250); - } - throw new Error(`MCP test server did not start within 30s:\n${out}`); + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: [composableServer, "--config", appConfig], + cwd: repoRoot, + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: (child) => { + mcpServer = child; + }, + what: `MCP test server (${composableServer})`, + }); + return { url: match[1] }; } /** base64url(JSON) — the appArgs encoding the deep link expects. */