From 16c96d44c33d876df1bc6badbd426909a299fdae Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 11 Aug 2026 14:25:09 -0400 Subject: [PATCH] fix(smoke): await the server's exit before removing its catalog, and test it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Copilot review comments on #1978. 1. `stop()` removed the catalog dir immediately after a bare `child.kill()`, which only *delivers* SIGTERM. That is precisely the #1801 race `child-cleanup.mjs:11-17` documents, and it says both halves are needed: `stopChild()` closes the race on the normal path, `removeSafe()` makes the residual case harmless. I had used only the second. `smoke-web-app.mjs` already used `stopChild` for its MCP test-server child, so the helper was inconsistent with its own sibling. `stop()` is now async — it awaits `stopChild` and then removes — and all three callers await it (`smoke-web.mjs`'s `fail()` became async to match, so every call site had to `await fail(...)` or execution would run past it instead of exiting). 2. The isolation contract had no test. The smokes exit the process right after teardown, so a regression reintroducing the shared catalog — or dropping cleanup — would leave them all green. Split the two pure pieces out (`createTempCatalog`, `buildWebServerEnv`) and cover them in a sibling `prod-web-server.test.mjs`: a unique dir per run, no pre-created catalog file, the dir is removable, and an inherited `MCP_CATALOG_PATH` (or `MCP_AUTO_OPEN_ENABLED`) cannot beat the explicit one — the spread-then-assign ordering that guard depends on. `startProdWebServer` itself stays untested by design; it spawns a real launcher, which is the smokes' job. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- scripts/lib/prod-web-server.mjs | 84 ++++++++++++++++++----- scripts/lib/prod-web-server.test.mjs | 99 ++++++++++++++++++++++++++++ scripts/smoke-web-app.mjs | 2 +- scripts/smoke-web-browser.mjs | 2 +- scripts/smoke-web.mjs | 17 +++-- 5 files changed, 177 insertions(+), 27 deletions(-) create mode 100644 scripts/lib/prod-web-server.test.mjs diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index 2991d588..6365a155 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/scripts/lib/prod-web-server.mjs @@ -20,12 +20,64 @@ import { setTimeout as delay } from "node:timers/promises"; import { fileURLToPath } from "node:url"; import { dirname, join, resolve } from "node:path"; -import { removeSafe } from "./child-cleanup.mjs"; +import { removeSafe, stopChild } from "./child-cleanup.mjs"; const libDir = dirname(fileURLToPath(import.meta.url)); const repoRoot = resolve(libDir, "..", ".."); const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js"); +/** + * Mint a fresh throwaway catalog for one server run. + * + * `mkdtemp` (not a fixed name) is what makes concurrent runs safe: the three web + * smokes run back-to-back today, but a fixed path would have them share — and + * silently reintroduce the cross-run bleed this whole change removes. + * + * The catalog *file* is deliberately not created. The backend seeds an empty + * catalog on first use, which is exactly the first-run state we want every run + * to start from. + * + * @returns {{ dir: string, path: string }} + */ +export function createTempCatalog() { + const dir = mkdtempSync(join(tmpdir(), "smoke-web-catalog-")); + return { dir, path: join(dir, "catalog.json") }; +} + +/** + * Build the child env for the prod web server. + * + * Split out from the spawn so the isolation contract is unit-testable without + * starting a server (#1977). The ordering is load-bearing: `MCP_CATALOG_PATH` + * is assigned *after* the `process.env` spread, so an inherited value from the + * developer's shell is overridden rather than silently winning — which would + * put the smoke straight back on whatever catalog that variable names. + * + * @param {object} opts + * @param {string} opts.host + * @param {string} opts.port + * @param {string} opts.token + * @param {string} opts.catalogPath + * @param {NodeJS.ProcessEnv} [opts.baseEnv] + */ +export function buildWebServerEnv({ + host, + port, + token, + catalogPath, + baseEnv = process.env, +}) { + return { + ...baseEnv, + CLIENT_PORT: port, + HOST: host, + MCP_INSPECTOR_API_TOKEN: token, + MCP_CATALOG_PATH: catalogPath, + // Don't pop a browser in CI. + MCP_AUTO_OPEN_ENABLED: "false", + }; +} + /** * Spawn `mcp-inspector --web` (prod, no `--dev`) against the built * `clients/web/dist` and return handles for readiness + teardown. @@ -56,20 +108,10 @@ const launcherEntry = resolve(repoRoot, "clients/launcher/build/index.js"); export function startProdWebServer({ host, port, token, label = "smoke:web" }) { const baseUrl = `http://${host}:${port}`; - // The file need not exist — the backend seeds an empty catalog on first run. - const catalogDir = mkdtempSync(join(tmpdir(), "smoke-web-catalog-")); - const catalogPath = join(catalogDir, "catalog.json"); + const { dir: catalogDir, path: catalogPath } = createTempCatalog(); const child = spawn(process.execPath, [launcherEntry, "--web"], { - env: { - ...process.env, - CLIENT_PORT: port, - HOST: host, - MCP_INSPECTOR_API_TOKEN: token, - MCP_CATALOG_PATH: catalogPath, - // Don't pop a browser in CI. - MCP_AUTO_OPEN_ENABLED: "false", - }, + env: buildWebServerEnv({ host, port, token, catalogPath }), stdio: ["ignore", "inherit", "inherit"], }); @@ -151,11 +193,17 @@ export function startProdWebServer({ host, port, token, label = "smoke:web" }) { catalogPath, waitForReady, whenChildExits, - stop: () => { - if (!exited) child.kill("SIGTERM"); - // SIGTERM is not awaited, so the server may still hold the catalog for a - // moment. `removeSafe` warns rather than throws, so the worst case is a - // leaked temp dir the OS reclaims — never a red smoke over cleanup. + /** + * Terminate the server, then remove its catalog dir. **Await this** — the + * two halves are the documented pair in `child-cleanup.mjs`, and both are + * needed. `stopChild` closes the #1801 race on the normal path (a bare + * `kill()` only *delivers* the signal, so a synchronous remove can hit + * ENOTEMPTY when the server writes the catalog on its way out); `removeSafe` + * then makes the residual case harmless, warning instead of throwing so a + * leftover temp dir can never turn a passing smoke red. + */ + stop: async () => { + await stopChild(child, { label, what: "prod web server" }); removeSafe(catalogDir, { label }); }, }; diff --git a/scripts/lib/prod-web-server.test.mjs b/scripts/lib/prod-web-server.test.mjs new file mode 100644 index 00000000..bcf65f25 --- /dev/null +++ b/scripts/lib/prod-web-server.test.mjs @@ -0,0 +1,99 @@ +/** + * Unit tests for the prod-web-server helper's catalog-isolation contract (#1977). + * + * The smokes themselves cannot guard this: they exercise startup and then exit + * the process immediately after teardown, so a regression that reintroduced the + * shared catalog — or that stopped cleaning up — would still leave every smoke + * green. These lock down the three properties that make isolation real: each run + * gets its own catalog, an inherited `MCP_CATALOG_PATH` cannot win, and the temp + * dir is removable on stop. + * + * `startProdWebServer` itself is not unit-tested — it spawns a real launcher, so + * its contract is the smokes' job. What is tested here is exactly the part that + * decides *which catalog the server sees*, which is pure. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { dirname } from "node:path"; + +import { buildWebServerEnv, createTempCatalog } from "./prod-web-server.mjs"; +import { removeSafe } from "./child-cleanup.mjs"; + +const BASE = { + host: "127.0.0.1", + port: "6299", + token: "test-token", + catalogPath: "/tmp/example/catalog.json", +}; + +test("createTempCatalog: mints a unique dir per call", () => { + const a = createTempCatalog(); + const b = createTempCatalog(); + try { + assert.notEqual(a.dir, b.dir, "two runs must not share a catalog dir"); + assert.equal(dirname(a.path), a.dir); + assert.ok(existsSync(a.dir)); + assert.ok(existsSync(b.dir)); + } finally { + removeSafe(a.dir); + removeSafe(b.dir); + } +}); + +test("createTempCatalog: does not create the catalog file itself", () => { + // The backend seeds an empty catalog on first use; pre-creating the file would + // hand it a zero-byte file to parse instead of the clean first-run state. + const { dir, path } = createTempCatalog(); + try { + assert.equal(existsSync(path), false); + } finally { + removeSafe(dir); + } +}); + +test("createTempCatalog: its dir is removable by removeSafe", () => { + const { dir, path } = createTempCatalog(); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, "{}"); + assert.equal(removeSafe(dir), true); + assert.equal(existsSync(dir), false); +}); + +test("buildWebServerEnv: points the server at the given catalog", () => { + const env = buildWebServerEnv({ ...BASE, baseEnv: {} }); + assert.equal(env.MCP_CATALOG_PATH, BASE.catalogPath); +}); + +test("buildWebServerEnv: overrides an inherited MCP_CATALOG_PATH", () => { + // The regression this guards: assigning before the spread would let a + // developer's exported MCP_CATALOG_PATH win, putting the smoke back on a + // real catalog while every test still passed. + const env = buildWebServerEnv({ + ...BASE, + baseEnv: { MCP_CATALOG_PATH: "/home/dev/.mcp-inspector/mcp.json" }, + }); + assert.equal(env.MCP_CATALOG_PATH, BASE.catalogPath); +}); + +test("buildWebServerEnv: passes through unrelated inherited vars", () => { + const env = buildWebServerEnv({ ...BASE, baseEnv: { PATH: "/usr/bin" } }); + assert.equal(env.PATH, "/usr/bin"); +}); + +test("buildWebServerEnv: sets host, port, token, and disables auto-open", () => { + const env = buildWebServerEnv({ ...BASE, baseEnv: {} }); + assert.equal(env.HOST, BASE.host); + assert.equal(env.CLIENT_PORT, BASE.port); + assert.equal(env.MCP_INSPECTOR_API_TOKEN, BASE.token); + assert.equal(env.MCP_AUTO_OPEN_ENABLED, "false"); +}); + +test("buildWebServerEnv: an inherited auto-open setting cannot re-enable it", () => { + const env = buildWebServerEnv({ + ...BASE, + baseEnv: { MCP_AUTO_OPEN_ENABLED: "true" }, + }); + assert.equal(env.MCP_AUTO_OPEN_ENABLED, "false"); +}); diff --git a/scripts/smoke-web-app.mjs b/scripts/smoke-web-app.mjs index dce98805..c5f2f8a6 100644 --- a/scripts/smoke-web-app.mjs +++ b/scripts/smoke-web-app.mjs @@ -120,7 +120,7 @@ async function shutdown() { } browser = null; } - server.stop(); + await server.stop(); if (mcpServer) { const child = mcpServer; mcpServer = null; diff --git a/scripts/smoke-web-browser.mjs b/scripts/smoke-web-browser.mjs index dbc90844..ade88c54 100644 --- a/scripts/smoke-web-browser.mjs +++ b/scripts/smoke-web-browser.mjs @@ -95,7 +95,7 @@ async function shutdown() { } browser = null; } - server.stop(); + await server.stop(); } async function fail(message) { diff --git a/scripts/smoke-web.mjs b/scripts/smoke-web.mjs index 2fbcfb44..867ae4ff 100644 --- a/scripts/smoke-web.mjs +++ b/scripts/smoke-web.mjs @@ -37,31 +37,34 @@ const server = startProdWebServer({ label: "smoke:web", }); -function fail(message) { +// Async because `server.stop()` awaits the child's exit before removing its temp +// catalog — so every call site must `await fail(...)`, or execution would run on +// past it instead of exiting. +async function fail(message) { console.error(`smoke:web FAILED — ${message}`); - server.stop(); + await server.stop(); process.exit(1); } try { const res = await server.waitForReady(); if (res.status !== 200) { - fail(`GET / returned HTTP ${res.status}, expected 200`); + await fail(`GET / returned HTTP ${res.status}, expected 200`); } const body = await res.text(); if (!body.includes(TOKEN_GLOBAL)) { - fail( + await fail( `served HTML is missing the ${TOKEN_GLOBAL} global (token not injected)`, ); } if (!body.includes(TOKEN)) { - fail("served HTML is missing the injected auth-token value"); + await fail("served HTML is missing the injected auth-token value"); } console.log( `smoke:web OK — GET / => 200 with injected ${TOKEN_GLOBAL} at ${server.baseUrl}`, ); - server.stop(); + await server.stop(); process.exit(0); } catch (err) { - fail(err instanceof Error ? err.message : String(err)); + await fail(err instanceof Error ? err.message : String(err)); }