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 () => {