test(scripts): cover the readiness-timeout teardown path (#2000 review)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>

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}`,
+  );
+}

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:/,
+  );
+});

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
+ * The spawn/readiness ownership lives in `lib/announced-child.mjs` so the
- * readiness with `console.error`, so watching stdout alone never matches and
+ * failure path is testable: this smoke's happy path always gets the
- * this times out with an empty diagnostic. Piping both also keeps the child's
+ * announcement, so nothing here could prove that a child which stays alive
- * noise out of the smoke's own output while still making it available in the
+ * through the readiness timeout is still reachable by `shutdown()` — the case
- * failure message.
+ * 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
+ * The announced URL is authoritative rather than the config's port:
- * handed back on success, so `shutdown()` can stop it on every throw path.
+ * createTestServerHttp resolves through findAvailablePort(), which walks upward
- * Returning it only after the announcement matched left the readiness timeout
+ * when the configured value is taken.
- * — 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.
  */
 async function startMcpServer() {
-  const child = spawn(
+  const { match } = await startAnnouncedChild({
-    process.execPath,
+    command: process.execPath,
-    [composableServer, "--config", appConfig],
+    args: [composableServer, "--config", appConfig],
-    { cwd: repoRoot, stdio: ["ignore", "pipe", "pipe"] },
+    cwd: repoRoot,
-  );
+    pattern: /listening at (http:\/\/\S+)/i,
-  mcpServer = child;
+    onSpawn: (child) => {
-  let out = "";
+      mcpServer = child;
-  child.stdout.on("data", (d) => (out += d));
+    },
-  child.stderr.on("data", (d) => (out += d));
+    what: `MCP test server (${composableServer})`,
-  let exited = false;
+  });
-  let spawnError = null;
+  return { url: match[1] };
-  // 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}`);
 }
 
 /** base64url(JSON) — the appArgs encoding the deep link expects. */