fix(smoke): await the server's exit before removing its catalog, and test it

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

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

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

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;

scripts/smoke-web-browser.mjs

@@ -95,7 +95,7 @@ async function shutdown() {
     }
     browser = null;
   }
-  server.stop();
+  await server.stop();
 }
 
 async function fail(message) {

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));
 }