From 4af38c719c81fda10eb9d9bb977890ca9e578066 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 22:01:08 -0400 Subject: [PATCH] fix(scripts): don't arm a survival timer after the verdict is settled (#2147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4. `exit` can be observed before the child's last output drains — `done()` already keeps reading until `close` for exactly that reason — so `onData` can run after the verdict is settled. Recording `markerAt` there is still right: the message is a thunk rendered at `finish()`, and a marker still sitting in the pipe when `exit` fired was painted all the same. Arming a survival timer is not. It could not change the result (`done` is a no-op once settled), but it holds the event loop open for the whole window, which under `node --test` is a suite lingering seconds for nothing. That implies something the comment did not say, and it had already bitten this PR: if `markerAt` can be set after the exit was observed, any wording derived from observation ORDER is unstable. The old message read "rendered at Nms and then exited Mms later", where M was `since() - markerAt` evaluated at finish — not the paint-to-exit gap at all, and incoherent whenever the marker surfaced during the drain. The test asserted on /survival window/, which the losing side of that race could drop. It could have flaked, and the flake would have looked like the bug this PR fixes. So the message now makes no ordering claim and reports both times as the observations they are, with `exitAt` captured in the exit handler rather than recomputed later. Assertions moved to the race-stable phrase, plus a test that runs the paint-then-die stub 12x asserting verdict AND wording are identical every time — one pass proves nothing about a race. The ordering could not be forced on darwin, so the guard is reasoned rather than reproduced, and the reply says so. Two attempts and what they actually showed: a 4MB payload with the marker last never gets there, because `process.exit()` discards pending stdout writes (the "before rendering" branch is correctly taken); marker on stderr with a graceful exitCode was observed first in all 30 runs, none lingering near the window. `done()` exists precisely because exit and close are separate events, so treating their order as guaranteed was worth removing anyway. Also (suppressed comment): the README's "Two more do the same" clause now lists four suites, not two, after this PR added render-smoke and pty. Signed-off-by: Cliff Hall Signed-off-by: cliffhall --- README.md | 2 +- scripts/lib/render-smoke.mjs | 26 +++++++++++++++++++++++--- scripts/lib/render-smoke.test.mjs | 18 +++++++++++++++++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ddca719f..7f73f4ea 100644 --- a/README.md +++ b/README.md @@ -490,7 +490,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `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:bundle-externals` | Guards the must-not-bundle invariant (#2067): for each tsup-bundled client it reads the **built** `build/` output and fails if any package that must stay external was inlined anyway. Candidates are the union of the client's own `external` array **and the root manifest's `dependencies`** — the latter because #2067 was a *missing* `external` entry, which a self-referential check would have passed. Detection is via esbuild's `// ` module banners, so it covers both shapes — a separate `-HASH.js` chunk (what a dynamically `import()`ed CommonJS package produces) and a statically-imported package folded straight into `index.js`, which emits no chunk at all. A build with no banners fails as such rather than passing clean, so enabling `minify` cannot silently retire the check. `undici` was declared only in the root and `clients/cli` manifests, and tsup auto-externalizes only what the _nearest_ manifest declares, so the web and TUI bundles inlined 1.05MB of it — and CommonJS inlined into an ESM bundle throws `Dynamic require of "assert" is not supported` on first use, from a specifier no user-side install can satisfy. Reads the output rather than the config because those two disagreed for four releases. 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 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. Two more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. 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` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → Storybook. A strict superset of GitHub CI — `smoke:web:firefox` (and `smoke:tui`) run only here. | diff --git a/scripts/lib/render-smoke.mjs b/scripts/lib/render-smoke.mjs index e61c3f02..f156f123 100644 --- a/scripts/lib/render-smoke.mjs +++ b/scripts/lib/render-smoke.mjs @@ -246,6 +246,15 @@ export function runRenderSmoke({ // " — quoting an output tail that visibly contains the marker. // First paint has happened; its deadline no longer has a question to ask. clearTimeout(renderTimer); + // `exit` can beat the last of the child's buffered output, and `done` + // then keeps reading until `close` — so this can run *after* the verdict + // is settled. Recording `markerAt` is still right (the deferred message + // below reads it, and a marker that only surfaced in the drain was still + // painted). Arming a timer is not: `done` is a no-op once settled, so it + // could not change the result, but it holds the event loop open for the + // whole survival window — which under `node --test` is a suite that + // hangs on for two seconds per case for no reason. + if (settled) return; // First paint is not the verdict — it only starts the clock on the // question this smoke actually asks. surviveTimer = setTimeout(() => { @@ -267,14 +276,25 @@ export function runRenderSmoke({ child.on("exit", (code, signal) => { childExited = true; + const exitAt = since(); if (settled) return; const how = signal ? `signal ${signal}` : `code ${code}`; + // Thunked: rendered at `finish()`, i.e. after `close`, so `markerAt` is + // whatever the FULL output showed — a marker still sitting in the pipe + // when `exit` fired is accounted for by the time this is read. + // + // Deliberately makes no claim about the order in which this process + // *observed* the two. `exit` and the last `data` event race, so a + // "N ms later" phrased off observation times would flip wording (and + // sign) run to run on the same child. The times are reported as the + // observations they are and the verdict does not rest on them. done(1, () => markerAt === null ? `child exited (${how}) before rendering "${marker}"\n${outputTail(output)}` - : `child rendered "${marker}" at ${markerAt}ms and then exited (${how}) ` + - `${since() - markerAt}ms later, inside the ${surviveMs}ms survival window — ` + - `it painted one frame, it did not run\n${outputTail(output)}`, + : `child rendered "${marker}" and then exited (${how}) inside the ` + + `${surviveMs}ms survival window — it painted one frame, it did not run ` + + `(first paint seen at ${markerAt}ms, exit at ${exitAt}ms)` + + `\n${outputTail(output)}`, ); }); diff --git a/scripts/lib/render-smoke.test.mjs b/scripts/lib/render-smoke.test.mjs index d631c62a..e3b12f1b 100644 --- a/scripts/lib/render-smoke.test.mjs +++ b/scripts/lib/render-smoke.test.mjs @@ -36,12 +36,26 @@ test("fails when the child paints the marker and then exits non-zero", async () const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); assert.equal(r.code, 1, `expected failure, got: ${r.message}`); assert.match(r.message, /code 1/); - assert.match(r.message, /survival window/); + assert.match(r.message, /it painted one frame, it did not run/); // The diagnostic must not read as a crash *before* first paint — that was a // different defect, and conflating them sends the reader to the wrong place. assert.doesNotMatch(r.message, /before rendering/); }); +// `exit` races the child's last `data` event, so this stub can be observed +// either way round on the same machine. Both orders must produce the same +// verdict AND the same wording — a message phrased off observation order would +// flip run to run, which is how a stable failure gets written off as flake. +// Repeated because one pass proves nothing about a race. +test("the verdict does not depend on whether exit beats the final output", async () => { + for (let i = 0; i < 12; i++) { + const r = await runRenderSmoke({ ...opts, ...paintThenExit(1) }); + assert.equal(r.code, 1, `run ${i}: expected failure, got: ${r.message}`); + assert.match(r.message, /it painted one frame, it did not run/, `run ${i}`); + assert.doesNotMatch(r.message, /before rendering/, `run ${i}`); + } +}); + // Exiting 0 right after painting is the same failure: the app did not run. It // is called out separately because `script(1)` under busybox reports 0 for a // crashed child, so the verdict cannot rest on the exit code. @@ -159,10 +173,12 @@ test("a zero survival window falls back rather than passing instantly", async () surviveMs: Number(""), }); assert.equal(r.code, 1, `expected failure, got: ${r.message}`); + // The window named in the message is the normalized one, not the 0 passed in. assert.match( r.message, new RegExp(`${DEFAULTS.surviveMs}ms survival window`), ); + assert.doesNotMatch(r.message, /\b0ms survival window/); }); test("outputTail drops a leading partial line but keeps short output whole", () => {