diff --git a/AGENTS.md b/AGENTS.md index d68142c17..a63b8d0d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -185,7 +185,13 @@ v2/main/ │ # pack-and-verify.mjs, and lib/ shared helpers │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program -│ # through — #1965; resolve-node-bin.mjs resolves a +│ # through — #1965; announced-child.mjs owns the +│ # spawn-and-wait-for-a-readiness-line step, publishing +│ # the child via `onSpawn` BEFORE the wait so the +│ # caller's teardown reaches it on every throw path — +│ # the readiness timeout included, which is what used +│ # to orphan a live server holding its port — #2000; +│ # resolve-node-bin.mjs resolves a │ # package's bin through its own package.json, so the │ # verify/smoke scripts spawn it with `process.execPath` │ # rather than the `npx` `.cmd` shim Node refuses to @@ -773,7 +779,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). -- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. +- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. diff --git a/README.md b/README.md index 672d86a69..e88002727 100644 --- a/README.md +++ b/README.md @@ -368,7 +368,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | diff --git a/clients/web/.npmignore b/clients/web/.npmignore index 133cadc22..ca99459ae 100644 --- a/clients/web/.npmignore +++ b/clients/web/.npmignore @@ -8,11 +8,15 @@ # load. (The other clients don't hit this because none of them ship a nested # .gitignore.) # -# Crucially this file does NOT list `build` or `dist`, so both the prod runner -# (build/) and the SPA (dist/) are packed. The root "files" allowlist already -# restricts publishing to those two directories, so everything else in -# clients/web (src, configs, node_modules, coverage, storybook-static) stays out -# regardless — the entries below are just belt-and-suspenders. +# Crucially this file does NOT list `build`, `dist`, or `static`, so all three +# are packed: the prod runner (build/), the SPA (dist/), and the MCP Apps +# sandbox proxy page (static/sandbox_proxy.html — a committed source file, read +# from disk at runtime by server/sandbox-controller.ts as +# `/../static/sandbox_proxy.html`, so it must ship at exactly that +# path; #1859). The root "files" allowlist names those three directories and +# nothing else under clients/web, so the rest (src, configs, node_modules, +# coverage, storybook-static) stays out regardless — the entries below are just +# belt-and-suspenders. node_modules coverage storybook-static diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index c0d5260c3..06eaa2156 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -875,7 +875,9 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - return { ok: true, body: null } as unknown as Response; + // A real Response constructed from `null` has a null `.body`, + // so the guard is exercised through the actual Response API. + return new Response(null, { status: 200 }); } return h.fetchFn(url, init); }, @@ -894,12 +896,14 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: () => Promise.reject(new Error("stream broke")), - }), - }; - return { ok: true, body } as unknown as Response; + // A real stream whose first pull throws — `reader.read()` then + // rejects exactly as a broken network body would, with no cast. + const body = new ReadableStream({ + pull() { + throw new Error("stream broke"); + }, + }); + return new Response(body, { status: 200 }); } return h.fetchFn(url, init); }, @@ -928,22 +932,18 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: async () => { - reads += 1; - if (reads === 1) { - // Two frames in one chunk → one background refresh. - return { - done: false, - value: encoder.encode("event: change\n\n\n\n"), - }; - } - return { done: true, value: undefined }; - }, - }), - }; - return { ok: true, body } as unknown as Response; + const body = new ReadableStream({ + pull(controller) { + reads += 1; + if (reads === 1) { + // Two frames in one chunk → one background refresh. + controller.enqueue(encoder.encode("event: change\n\n\n\n")); + return; + } + controller.close(); + }, + }); + return new Response(body, { status: 200 }); } return h.fetchFn(url, init); }, @@ -1138,22 +1138,24 @@ describe("useServers", () => { const fetchFn: typeof fetch = async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: async () => { - reads += 1; - // Priming comment only — no `event:` / `data:` line. - if (reads === 1) { - return { done: false, value: encoder.encode(":\n\n") }; - } - // Hold the stream open so the loop can't end and let a - // teardown-time settle hide a queued refresh. - await secondRead; - return { done: true, value: undefined }; - }, - }), - }; - return { ok: true, body } as unknown as Response; + // `pull` runs once per read, so the counting and the blocking + // second read work the same way they would on a hand-rolled reader + // double — while staying type-checked against the Response API. + const body = new ReadableStream({ + async pull(controller) { + reads += 1; + // Priming comment only — no `event:` / `data:` line. + if (reads === 1) { + controller.enqueue(encoder.encode(":\n\n")); + return; + } + // Hold the stream open so the loop can't end and let a + // teardown-time settle hide a queued refresh. + await secondRead; + controller.close(); + }, + }); + return new Response(body, { status: 200 }); } if (url.endsWith("/api/servers")) listGets += 1; return h.fetchFn(url, init); diff --git a/scripts/lib/announced-child.mjs b/scripts/lib/announced-child.mjs new file mode 100644 index 000000000..11303a199 --- /dev/null +++ b/scripts/lib/announced-child.mjs @@ -0,0 +1,94 @@ +/** + * 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)); + + // A deadline loop, not a fixed attempt count: state is re-read *after* every + // wait, including the last one. Counting attempts and throwing straight after + // the final `delay` leaves the whole last polling interval unobserved, so an + // announcement landing at 29.9s of a 30s budget is reported as a timeout — + // and a spawn error or early exit in that window is misattributed the same + // way. The final wait is also clamped to the deadline so the budget is a + // real bound rather than one poll longer. + const deadline = Date.now() + timeoutMs; + for (;;) { + 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}`); + const remaining = deadline - Date.now(); + if (remaining <= 0) break; + await delay(Math.min(pollMs, remaining)); + } + 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 000000000..e6404c804 --- /dev/null +++ b/scripts/lib/announced-child.test.mjs @@ -0,0 +1,163 @@ +/** + * 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)); +} + +/** + * Build an `onSpawn` that registers teardown the moment the child exists, so it + * is killed on every path out of the test — a failed assertion included. + * + * This is the helper's own ownership rule applied to the suite: killing only on + * the success path would leak a 60-second child into the runner precisely when + * the regression being guarded has come back, which is the one run where a leak + * matters most. Killing an already-exited child is a no-op, so the early-exit + * cases pay nothing for it. + */ +function killOnTeardown(t, capture = () => {}) { + return (child) => { + capture(child); + t.after(async () => { + child.kill("SIGKILL"); + await waitForExit(child); + }); + }; +} + +test("hands the child to onSpawn before waiting, and returns the match", async (t) => { + 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: killOnTeardown(t, (c) => (seen = c)), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal(seen, child, "onSpawn got the child that was returned"); + assert.equal(match[1], "http://127.0.0.1:9999"); +}); + +test("scans stdout as well as stderr", async (t) => { + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.log("ready on 4242")'], + pattern: /ready on (\d+)/, + onSpawn: killOnTeardown(t), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal(match[1], "4242"); +}); + +test("sees an announcement that lands inside the final polling interval", async (t) => { + // The off-by-one this guards: a fixed attempt count checks the output and + // then sleeps, so nothing ever observes the last interval — an announcement + // at 29.9s of a 30s budget reads as a timeout. + // + // Setting pollMs === timeoutMs makes that deterministic rather than a race: + // the attempt-counted loop gets exactly one check, at t≈0 before the child + // has even finished starting, and then throws. The deadline loop re-reads + // after the wait, so *any* announcement before the deadline is seen. The + // child announces immediately, ~2s inside the window on either side, so a + // loaded runner cannot flip the outcome. (Verified: this fails against the + // attempt-counted loop and passes against the deadline loop.) + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("listening at http://127.0.0.1:8080")'], + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: killOnTeardown(t), + what: "probe", + timeoutMs: 2000, + pollMs: 2000, + }); + assert.equal(match[1], "http://127.0.0.1:8080"); +}); + +test("a non-announcing child is still reachable and killable after the timeout", async (t) => { + // 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: killOnTeardown(t, (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 (t) => { + 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: killOnTeardown(t, (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"); +}); + +test("reports a spawn failure rather than throwing it uncaught", async (t) => { + await assert.rejects( + startAnnouncedChild({ + command: "definitely-not-an-executable-2000", + args: [], + pattern: /never/, + onSpawn: killOnTeardown(t), + 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 4dafa18f9..b849f357f 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,45 +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 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"] }, - ); - 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 { child, 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. */ @@ -247,7 +232,9 @@ try { } ensureTestServer(); - ({ child: mcpServer, url: mcpUrl } = await startMcpServer()); + // `startMcpServer` publishes the child to `mcpServer` itself, so teardown + // reaches it even when this throws before returning. + ({ url: mcpUrl } = await startMcpServer()); await server.waitForReady(); browser = await loadChromium(); const page = await browser.newPage();