From 678184ed3e94aa0df9509ac9c87015c52273ded7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 11 Aug 2026 15:17:56 -0400 Subject: [PATCH] fix(smoke): pin the teardown ordering, and correct a docblock the test refuted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both round-3 review points, both mine. 1. The docblock claimed deleting `removeSafe` would leave "both the unit suite and all three smokes green" — written before the test existed and never updated, so it contradicted the test added in the same PR. Only the smokes would stay green; the unit tests are precisely what fails on it. 2. `assert.ok(hasExited(child))` after the await proved only that the child was dead by the time teardown returned. It passes just as happily if the catalog were removed *first*, or while an un-awaited `stopChild` was still pending — so it did not lock down the #1801 ordering it claimed to. Sample the directory from the child's own `exit` handler instead, registered before teardown so it runs ahead of stopChild's listener and observes the world at the instant the child dies. `removeSafe` cannot have run by then: it is sequenced after the promise stopChild resolves from that very event. Mutation-checked against both defects the old assertion missed — removing before stopping, and dropping the await. Each now fails the test; neither did before. 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 | 9 +++++---- scripts/lib/prod-web-server.test.mjs | 24 +++++++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/scripts/lib/prod-web-server.mjs b/scripts/lib/prod-web-server.mjs index 44986c2c..895d9509 100644 --- a/scripts/lib/prod-web-server.mjs +++ b/scripts/lib/prod-web-server.mjs @@ -89,10 +89,11 @@ export function buildWebServerEnv({ * never turn a passing smoke red. * * Exported separately from `startProdWebServer` so the teardown *contract* is - * testable against a stand-in child, without booting a real launcher. Testing it - * matters more than it looks: deleting the `removeSafe` call would leave both the - * unit suite and all three smokes green (the smokes exit immediately after - * teardown), which is precisely the regression the tests exist to catch. + * testable against a stand-in child, without booting a real launcher. That split + * is what makes the leak detectable at all: the smokes exit immediately after + * teardown, so deleting the `removeSafe` call below would leave all three of them + * green. The focused unit tests in `prod-web-server.test.mjs` are the only thing + * that fails on it. * * @param {object} opts * @param {import("node:child_process").ChildProcess} opts.child diff --git a/scripts/lib/prod-web-server.test.mjs b/scripts/lib/prod-web-server.test.mjs index d3f46671..0888d887 100644 --- a/scripts/lib/prod-web-server.test.mjs +++ b/scripts/lib/prod-web-server.test.mjs @@ -84,18 +84,32 @@ test("teardownWebServer: removes the catalog dir it was given", async () => { assert.equal(existsSync(dir), false, "teardown must remove the catalog dir"); }); -test("teardownWebServer: waits for the child to exit before removing", async () => { - // The #1801 race in one assertion: a bare kill() only *delivers* SIGTERM, so a - // teardown that removed synchronously could unlink the dir while the server was - // still writing to it. If teardown resolves with the child still alive, the - // await on stopChild has been lost. +test("teardownWebServer: the catalog outlives the child", async () => { + // The #1801 ordering, pinned at the one instant that distinguishes it. Simply + // asserting `hasExited` after the await is too weak — that also passes if the + // dir were removed *first*, or while an un-awaited stopChild was still pending. + // So sample the dir from the child's own `exit` handler: registering it before + // teardown puts it ahead of stopChild's listener, so it observes the world at + // the moment the child dies. removeSafe cannot have run yet — it is sequenced + // after the promise stopChild resolves from this very event. const child = spawnIdleChild(); const { dir } = createTempCatalog(); assert.equal(hasExited(child), false, "child should start alive"); + let dirAtChildExit = null; + child.once("exit", () => { + dirAtChildExit = existsSync(dir); + }); + await teardownWebServer({ child, catalogDir: dir, label: "test" }); + assert.equal( + dirAtChildExit, + true, + "catalog must still exist when the child exits — removing earlier is the #1801 race", + ); assert.ok(hasExited(child), "teardown must await the child's exit"); + assert.equal(existsSync(dir), false, "and must remove it afterwards"); }); test("teardownWebServer: an already-dead child is not an error", async () => {