test: fix the review round on the stress suite

All five findings are against the stress tests added in the previous commit, and three of them are defects in the tests rather than nits. The serious one: the launch-cycle test passed if no helper was ever created. Every assertion in it was an upper bound, so a launch path that silently stopped publishing metadata — a wrong env gate, a proxy fixture that never engaged, `app .` short-circuiting on the fake bin — left every count at zero and the test green, while proving nothing about the accumulation it exists to guard. That is the same defect class as the unexercised selector branch this PR fixes for #668, in the test written to guard against it. It now probes first: one launch with a long idle window, asserting helper metadata actually appears, and that probe helper is torn down before the measurement loop starts. The loop additionally records whether it ever observed a helper, and asserts it did. Verified by mutation: stubbing out both metadata writers in the wrapper now fails the test instead of passing it. The rest: - `waitForExit` waited only on `exit`. A child that never spawns emits `error` and never `exit`, so the promise stayed pending forever — and since the batched helpers await a whole batch concurrently, one failed spawn stalled every sibling and hung the run rather than failing it. It now settles on either event, and the batch is reaped before the PIDs are validated so a throw cannot leak the siblings that did start. - `countHelperMetadata` swallowed every readdir error and reported zero. Only ENOENT is a legitimate zero; anything else — a permissions change, a path that is not a directory — was being reported as a clean sweep, which the upper-bound assertions accept happily. Narrowed to ENOENT, rethrowing the rest. - The reap matrix indexed `running[index]?.ready.pid ?? 0` and passed the fallback to `isProcessAlive`. On POSIX `kill(0, 0)` probes the caller's own process group and succeeds, so a missing helper would have read as alive — and four of the five cases expect alive. Zipped off `running` instead, with the length pinned. - The same test hardcoded `running[1]` as the reaped case and guarded the assertion with `if (reaped)`, so reordering `cases` would have asserted `owner-gone` against a helper meant to survive, and a missing entry would have skipped the only assertion proving *why* the helper died. Looked up by label and asserted unconditionally. The header comment said "four configurations" over a list of five. test/owned-pids-helper.test.ts covers the helpers themselves, including a direct assertion that a child which fails to spawn signals via `error` — the premise the hang fix rests on — with a timeout well inside vitest's so a regression reads as a failure rather than a stuck suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015b4Lew3oHNEYmTZtg7zEWz
ndycode · Aug 13, 2026 · Aug 11, 2026 · Aug 11, 2026 · Aug 11, 2026 · Aug 11, 2026
commit 8f0b578f96df3eb382deb3972142486a27f211d3
@@ -7709,7 +7709,17 @@ describe("codex bin wrapper", () => {
 		let entries: string[] = [];
 		try {
 			entries = readdirSync(multiAuthDir);
-		} catch {
+		} catch (error) {
+			// Only "the directory does not exist yet" is a legitimate zero. Any
+			// other readdir failure — a permissions change, a path that is not a
+			// directory — would otherwise be reported as a clean sweep, and the
+			// bounded-metadata test below asserts upper bounds that a false zero
+			// satisfies perfectly.
+			const code =
+				error && typeof error === "object" && "code" in error
+					? String((error as { code?: unknown }).code)
+					: "unknown";
+			if (code !== "ENOENT") throw error;
 			return { status: 0, owner: 0 };
 		}
 		return {
@@ -7747,8 +7757,49 @@ describe("codex bin wrapper", () => {
 				"utf8",
 			);
 
+			// Before measuring accumulation, prove the fixture actually produces
+			// helpers. Every assertion below is an upper bound, so a launch path
+			// that silently never publishes metadata — a wrong env gate, a proxy
+			// fixture that never engages, `app .` short-circuiting on the fake bin
+			// — would leave every count at zero and pass the whole test while
+			// demonstrating nothing about the leak it exists to guard.
+			const probe = runWrapper(fixtureRoot, ["app", "."], {
+				CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin,
+				CODEX_HOME: originalHome,
+				CODEX_MULTI_AUTH_DIR: multiAuthDir,
+				CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1",
+				// Long enough that the helper is unambiguously still alive when the
+				// probe looks for it.
+				CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "30000",
+				CODEX_MULTI_AUTH_APP_ROTATION_DETACHED_IDLE_MS: "0",
+				OPENAI_API_KEY: undefined,
+			});
+			expect(probe.status).toBe(0);
+			const probeCounts = countHelperMetadata(multiAuthDir);
+			expect(probeCounts.status).toBeGreaterThan(0);
+			expect(probeCounts.owner).toBeGreaterThan(0);
+			// Tear that one down before the accumulation loop starts, so it cannot
+			// be mistaken for a leaked helper later.
+			const probeEntries = readdirSync(multiAuthDir).filter((name) =>
+				name.startsWith("runtime-rotation-app-helper"),
+			);
+			for (const name of probeEntries) {
+				const match = /\.(\d+)\.json$/.exec(name);
+				const pid = match?.[1] ? Number.parseInt(match[1], 10) : null;
+				if (pid !== null && isProcessAlive(pid)) {
+					try {
+						process.kill(pid, "SIGKILL");
+					} catch {
+						// Already gone.
+					}
+				}
+				rmSync(join(multiAuthDir, name), { force: true });
+			}
+			await sleep(200);
+
 			const cycles = 30;
 			const counts: number[] = [];
+			let sawHelperDuringLoop = false;
 			for (let cycle = 0; cycle < cycles; cycle += 1) {
 				const result = runWrapper(fixtureRoot, ["app", "."], {
 					CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin,
@@ -7763,7 +7814,9 @@ describe("codex bin wrapper", () => {
 				});
 				expect(result.status).toBe(0);
 				await sleep(120);
-				counts.push(countHelperMetadata(multiAuthDir).owner);
+				const sample = countHelperMetadata(multiAuthDir);
+				if (sample.owner > 0 || sample.status > 0) sawHelperDuringLoop = true;
+				counts.push(sample.owner);
 			}
 
 			// Give the last cycle's helper time to exit and one more launch to sweep
@@ -7782,6 +7835,9 @@ describe("codex bin wrapper", () => {
 
 			const final = countHelperMetadata(multiAuthDir);
 			const peak = Math.max(...counts);
+			// The loop has to have observed at least one helper at some point,
+			// otherwise the upper bounds below are vacuous.
+			expect(sawHelperDuringLoop).toBe(true);
 			// The pre-fix behaviour was one owner file per launch, kept forever, so
 			// the count climbed with the cycle count. A few concurrent files are
 			// expected — each helper outlives the launch that spawned it by its
@@ -7872,9 +7928,9 @@ describe("codex bin wrapper", () => {
 	it.skipIf(process.platform === "win32")(
 		"stress: the reap matrix reaps exactly the stranded helpers and no others",
 		async () => {
-			// The whole lifecycle contract in one run, with all four configurations
-			// live at the same time so a rule that fires on the wrong one is visible
-			// as a divergence rather than as a single red test.
+			// The whole lifecycle contract in one run, with every configuration live
+			// at the same time so a rule that fires on the wrong one is visible as a
+			// divergence rather than as a single red test.
 			const fixtureRoot = createWrapperFixture();
 			createRuntimeRotationProxyFixtureModule(fixtureRoot);
 			const originalHome = join(fixtureRoot, "codex-home");
@@ -7957,24 +8013,40 @@ describe("codex bin wrapper", () => {
 					// been, and anything that survives this has survived on a rule.
 					await sleep(4_000);
 
-					const actual = cases.map((testCase, index) => ({
-						label: testCase.label,
-						expected: testCase.survives,
-						alive: isProcessAlive(running[index]?.ready.pid ?? 0),
-					}));
+					// Zipped off `running`, not indexed with a fallback: passing `0` to
+					// `isProcessAlive` would probe the caller's own process group on
+					// POSIX and answer true, so a missing helper would read as alive —
+					// and four of these five cases expect exactly that.
+					expect(running).toHaveLength(cases.length);
+					const actual = running.map(({ ready }, index) => {
+						const testCase = cases[index];
+						expect(testCase).toBeDefined();
+						return {
+							label: testCase?.label ?? `case-${index}`,
+							expected: testCase?.survives ?? false,
+							alive: isProcessAlive(ready.pid),
+							statusPath: ready.statusPath,
+						};
+					});
 					// Compared as a whole so a failure names every divergence at once.
 					expect(actual.map((a) => `${a.label}=${a.alive}`)).toEqual(
 						actual.map((a) => `${a.label}=${a.expected}`),
 					);
 
-					// The one that died did so for the stated reason.
-					const reaped = running[1];
-					if (reaped) {
-						const status = JSON.parse(
-							readFileSync(reaped.ready.statusPath, "utf8"),
-						) as { state: string };
-						expect(status.state).toBe("owner-gone");
-					}
+					// The one that died did so for the stated reason. Looked up by
+					// label and asserted unconditionally: hardcoding an index meant
+					// reordering `cases` would silently assert `owner-gone` against a
+					// helper that was supposed to survive, and a guard around it would
+					// let the only assertion that proves *why* it died skip itself.
+					const reaped = actual.find(
+						(a) => a.label === "owner dead, never served",
+					);
+					expect(reaped).toBeDefined();
+					expect(reaped?.alive).toBe(false);
+					const status = JSON.parse(
+						readFileSync(reaped?.statusPath ?? "", "utf8"),
+					) as { state: string };
+					expect(status.state).toBe("owner-gone");
 				} finally {
 					await Promise.all(
 						running.map(({ helper, closed }) =>

@@ -29,7 +29,19 @@ function spawnIdleChild(): ChildProcess {
 async function waitForExit(child: ChildProcess): Promise<void> {
 	if (child.exitCode === null && child.signalCode === null) {
 		await new Promise<void>((resolve) => {
-			child.once("exit", () => resolve());
+			let settled = false;
+			const finish = (): void => {
+				if (settled) return;
+				settled = true;
+				resolve();
+			};
+			child.once("exit", finish);
+			// A child that never spawned emits `error`, never `exit`. Waiting on
+			// `exit` alone leaves this promise pending forever — and the batched
+			// helpers below await a whole batch concurrently, so a single failed
+			// spawn would stall every sibling and hang the run rather than failing
+			// it. Either event means "this child is not running".
+			child.once("error", finish);
 		});
 	}
 	// `exit` fires before the stdio streams are torn down, so the parent's write
@@ -101,14 +113,20 @@ export async function withDeadPids<T>(
 		const size = Math.min(batchSize, count - offset);
 		const children = Array.from({ length: size }, () => spawnIdleChild());
 		const pids = children.map((child) => child.pid);
+		// Reap the whole batch first — including any child that failed to spawn,
+		// which `waitForExit` now settles on `error` — and only then decide whether
+		// the batch was usable. Throwing before the cleanup would leak the
+		// siblings that did start.
 		await Promise.all(
 			children.map(async (child) => {
 				child.kill("SIGKILL");
 				await waitForExit(child);
 			}),
 		);
 		if (pids.some((pid) => pid === undefined)) {
-			throw new Error("failed to spawn a probe process");
+			throw new Error(
+				"owned-pids: failed to spawn a probe process while building a dead-PID batch",
+			);
 		}
 		deadPids.push(...(pids as number[]));
 	}
@@ -153,10 +171,14 @@ export async function withLivePids<T>(
 	try {
 		const pids = children.map((child) => child.pid);
 		if (pids.some((pid) => pid === undefined)) {
-			throw new Error("failed to spawn a probe process");
+			throw new Error(
+				"owned-pids: failed to spawn a probe process while building a live-PID set",
+			);
 		}
 		return await run(pids as number[]);
 	} finally {
+		// Same contract as the dead-PID batch: a child that failed to spawn settles
+		// on `error`, so this cleanup cannot hang on it.
 		await Promise.all(
 			children.map(async (child) => {
 				child.kill("SIGKILL");

@@ -0,0 +1,108 @@
+import { spawn } from "node:child_process";
+import process from "node:process";
+import { describe, expect, it } from "vitest";
+import { withDeadPid, withDeadPids, withLivePid, withLivePids } from "./helpers/owned-pids.js";
+
+// The lifecycle fixtures depend on these helpers being facts rather than
+// approximations, so the helpers themselves need coverage. The hang is the
+// dangerous one: a helper that never resolves turns a test failure into a
+// suite that sits there until the runner's timeout, with no useful output.
+
+function isAlive(pid: number): boolean {
+	try {
+		process.kill(pid, 0);
+		return true;
+	} catch (error) {
+		const code =
+			error && typeof error === "object" && "code" in error ? error.code : null;
+		return code === "EPERM";
+	}
+}
+
+describe("owned-pids", () => {
+	it("hands out a PID that is genuinely dead", async () => {
+		await withDeadPid((pid) => {
+			expect(Number.isInteger(pid)).toBe(true);
+			expect(pid).toBeGreaterThan(0);
+			expect(isAlive(pid)).toBe(false);
+		});
+	});
+
+	it("hands out a PID that is genuinely alive, and reaps it afterwards", async () => {
+		let captured = 0;
+		await withLivePid((pid) => {
+			captured = pid;
+			expect(isAlive(pid)).toBe(true);
+		});
+		// Killed on the way out rather than left for the OS.
+		expect(isAlive(captured)).toBe(false);
+	});
+
+	it("kills the live PID even when the body throws", async () => {
+		let captured = 0;
+		await expect(
+			withLivePid((pid) => {
+				captured = pid;
+				throw new Error("boom");
+			}),
+		).rejects.toThrow("boom");
+		expect(isAlive(captured)).toBe(false);
+	});
+
+	it("produces distinct PIDs in batches larger than one spawn round", async () => {
+		// The batch size is an implementation detail; asking for more than one
+		// batch is what proves the loop stitches them together rather than
+		// returning only the last batch.
+		const count = 40;
+		await withDeadPids(count, (pids) => {
+			expect(pids).toHaveLength(count);
+			expect(new Set(pids).size).toBe(count);
+			for (const pid of pids) {
+				expect(isAlive(pid)).toBe(false);
+			}
+		});
+	}, 60_000);
+
+	it("keeps every PID in a live batch alive for the body and reaps them after", async () => {
+		let captured: number[] = [];
+		await withLivePids(5, (pids) => {
+			captured = [...pids];
+			expect(new Set(pids).size).toBe(pids.length);
+			for (const pid of pids) {
+				expect(isAlive(pid)).toBe(true);
+			}
+		});
+		for (const pid of captured) {
+			expect(isAlive(pid)).toBe(false);
+		}
+	}, 60_000);
+
+	it("settles rather than hanging when a child never spawns", async () => {
+		// A child that fails to spawn emits `error` and never `exit`. The batched
+		// helpers await a whole batch concurrently, so waiting on `exit` alone
+		// meant one failed spawn stalled every sibling and hung the run instead of
+		// failing it. This asserts the settle, with a timeout well under vitest's
+		// so a regression reads as a failure here rather than as a stuck suite.
+		const child = spawn(
+			"definitely-not-a-real-binary-2f8c1d",
+			["--nope"],
+			{ stdio: ["pipe", "ignore", "ignore"] },
+		);
+		const settled = await Promise.race([
+			new Promise<string>((resolve) => {
+				let done = false;
+				const finish = (label: string) => () => {
+					if (done) return;
+					done = true;
+					resolve(label);
+				};
+				child.once("exit", finish("exit"));
+				child.once("error", finish("error"));
+			}),
+			new Promise<string>((resolve) => setTimeout(() => resolve("timeout"), 5_000)),
+		]);
+		// The premise of the fix: this child signals via `error`, not `exit`.
+		expect(settled).toBe("error");
+		child.stdin?.destroy();
+	}, 30_000);
+});