From 4a0cbb55c04190eb60cb02cbd16bdc751a607d14 Mon Sep 17 00:00:00 2001 From: olddonkey Date: Wed, 26 Aug 2026 16:43:00 -0700 Subject: [PATCH] fix(tests): reap the recovery proxy instead of trusting `stop` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The update-recovery case starts a real detached proxy, and its cleanup ran the reap only when `node ocx.mjs stop` exited non-zero. That exit code is a claim, not proof: `stop` also reports success when it finds no live runtime to stop, which is indistinguishable here from one it failed to stop. On that path nothing killed the proxy, and the block went straight on to `rmSync` the fixture tree. The survivor was reparented to init with its package tree, `src` symlink, and config deleted underneath it — unable to serve, unable to exit. One escapee sat at 99% of a core for three and a half hours, 202 minutes of CPU time, listening on nothing. It also held the test machine lock, which is how it surfaced. Cleanup now resolves the pid before anything destroys the record of it, runs `stop` for its graceful path, then verifies liveness and reaps regardless of what `stop` claimed. `rmSync` moves inside a nested finally so it still always runs, but strictly after the reap: deleting the tree out from under a live detached process is what turned a missed kill into a permanently spinning orphan. Reaping the recorded pid is enough because bin/ocx.mjs mirrors its Bun child's exit, so the node launcher follows it down. Reaping alone would still fail silently, because the case passed the whole time it was leaking. An afterAll now audits the pid, so a future regression is red instead of invisible. Verified by A/B: with `stop` stubbed to exit 0 without killing, the old cleanup reports 15 pass / 0 fail and leaves the orphan, the new one reports 15 pass / 0 fail and leaves nothing, and the old cleanup under the new guard turns red. Co-Authored-By: Claude Opus 5 --- tests/update-stop-first.test.ts | 64 +++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/tests/update-stop-first.test.ts b/tests/update-stop-first.test.ts index 140ed3c491..e575d2ef77 100644 --- a/tests/update-stop-first.test.ts +++ b/tests/update-stop-first.test.ts @@ -1,10 +1,10 @@ -import { describe, expect, test } from "bun:test"; +import { afterAll, describe, expect, test } from "bun:test"; import { chmodSync, copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { createServer } from "node:net"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { runNpmCachePreflight } from "../src/update/npm-cache-preflight.mjs"; -import { killProxy } from "../src/lib/process-control"; +import { isProcessAlive, killProxy } from "../src/lib/process-control"; const repoRoot = join(import.meta.dir, ".."); @@ -63,6 +63,16 @@ const serverSource = readFileSync(join(import.meta.dir, "..", "src", "server", " const dispatchSource = readFileSync(join(import.meta.dir, "..", "src", "cli", "dispatch.ts"), "utf8"); describe("update stops the running proxy before replacing files", () => { + // The recovery case starts a real detached proxy, and its own result says nothing about + // whether cleanup reaped it — it stayed green while an escapee spun on a deleted tree for + // hours. Auditing the pid once the suite is done turns a silent leak back into a red test. + let auditedRecoveryPid: number | undefined; + + afterAll(() => { + if (auditedRecoveryPid === undefined) return; + expect(isProcessAlive(auditedRecoveryPid)).toBe(false); + }); + test("a failed cache pre-flight aborts before the stop callback can run", () => { let stopped = false; const malformedSpawn = (() => ({ status: 0, signal: null, stdout: "not-json", stderr: "" })) as never; @@ -227,24 +237,42 @@ esac expect(runtime.pid).toBeGreaterThan(0); recoveredPid = runtime.pid; } finally { - const stopped = existsSync(launcher) - ? Bun.spawnSync(["node", launcher, "stop"], { - cwd: root, - env, - stdout: "ignore", - stderr: "ignore", - timeout: UPDATE_SPAWN_TIMEOUT_MS, - }) - : null; - if (stopped?.exitCode !== 0) { - if (!recoveredPid) { - try { - recoveredPid = JSON.parse(readFileSync(join(opencodexHome, "runtime-port.json"), "utf8")).pid; - } catch { /* the proxy never wrote runtime state */ } + // Resolve the pid FIRST. `stop` rewrites runtime-port.json and the rmSync below + // deletes it outright, so this is the last moment the detached proxy the recovery + // path started can still be identified at all. + if (!recoveredPid) { + try { + recoveredPid = JSON.parse(readFileSync(join(opencodexHome, "runtime-port.json"), "utf8")).pid; + } catch { /* the proxy never wrote runtime state */ } + } + auditedRecoveryPid = Number.isSafeInteger(recoveredPid) && recoveredPid! > 0 + ? recoveredPid + : undefined; + if (existsSync(launcher)) { + Bun.spawnSync(["node", launcher, "stop"], { + cwd: root, + env, + stdout: "ignore", + stderr: "ignore", + timeout: UPDATE_SPAWN_TIMEOUT_MS, + }); + } + try { + // `stop` exiting 0 is a claim, not proof: it also reports success when it finds no + // live runtime to stop, which is indistinguishable here from one it failed to stop. + // Gating the reap on that exit code let a detached proxy survive, get reparented to + // init, and then spin on a fixture tree this same block had already deleted — one + // escapee burned a full core for hours. Verify liveness and reap regardless. + // bin/ocx.mjs mirrors its Bun child's exit, so reaping the recorded child pid takes + // the node launcher with it. + if (Number.isSafeInteger(recoveredPid) && recoveredPid! > 0 && isProcessAlive(recoveredPid!)) { + killProxy(recoveredPid!); } - if (Number.isSafeInteger(recoveredPid) && recoveredPid! > 0) killProxy(recoveredPid!); + } finally { + // Ordered after the reap on purpose: deleting the tree out from under a live + // detached proxy is what turned a missed kill into a permanently spinning orphan. + rmSync(root, { recursive: true, force: true }); } - rmSync(root, { recursive: true, force: true }); } }, RECOVERY_CASE_TIMEOUT_MS,