From e2c79d0d063789b6938319b43b3c9f9b19a6aceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 03:00:16 -0300 Subject: [PATCH 1/2] fix(browser): kill the whole process group during pre-identity cleanup stopOwnedBrowserDaemon only signaled the Chrome supervisor process and returned cleanup-complete unconditionally, including when the supervisor had already exited on its own. A pathological /proc identity-read failure combined with a supervisor crash could leave a same-process-group Chrome alive while its session record was marked fully cleaned up. Signal the whole owned process group by pgid instead (safe without a captured ProcessIdentity, since the daemon is our own just-spawned, not-yet-reaped child), and confirm via a new portable isProcessGroupAlive kill(2) probe that no member survives before reporting the group gone. Refs #29 --- docs/releases/UNRELEASED.md | 18 +++ packages/browser/src/session.ts | 62 +++++++- .../browser/test/pre-identity-cleanup.test.ts | 145 ++++++++++++++++++ packages/core/src/index.ts | 1 + packages/core/src/proc.ts | 18 +++ packages/core/test/proc.test.ts | 92 +++++++++++ 6 files changed, 329 insertions(+), 7 deletions(-) create mode 100644 packages/browser/test/pre-identity-cleanup.test.ts diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 1065f18..8000fa7 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -81,6 +81,17 @@ release description, then reset it after the release is published. - Added a private browser lifecycle package with confined ephemeral profiles, loopback-only CDP discovery, verified process-group teardown, concurrent session safety, and conservative dead-session reaping. +- Hardened the pre-identity browser daemon cleanup window: if the owned + Chrome supervisor's `/proc` identity never resolves within the one-second + startup window (a pathological read failure, a supervisor crash, or both), + cleanup now signals the whole process group — not just the supervisor + process — and confirms via a portable, non-`/proc` `kill(2)` probe + (`isProcessGroupAlive`) that no group member survives before reporting the + session cleaned up. Previously an already-exited or individually-killed + supervisor could leave a same-group Chrome orphaned and alive while its + session record was marked fully cleaned up. Defensive hardening only (issue + #29, deferred from #28); no behavior change on the normal startup path, no + feature flag. - Centralized session lifecycle composition in core and routed dead-session reaping through typed desktop, Android, and browser teardown owners, removing duplicate CLI/MCP orchestration and core PID/profile stop implementations. @@ -153,6 +164,13 @@ release description, then reset it after the release is published. e.g. `run-catalog.test.ts`'s root-precedence test and `devtools-mcp.test.ts`'s package-root assertion), and inline-screenshot/CDP association mismatches in `devtools-evidence.test.ts`. +- New `proc.test.ts` `isProcessGroupAlive` coverage, including a real + spawned-leader-killed-with-a-surviving-member regression case, and a new + `packages/browser/test/pre-identity-cleanup.test.ts` integration + regression (mocked `readProcessIdentity`/`startXvfb`, real fake-Chrome + subprocess) proving a live Chrome left behind by an unresolved supervisor + identity is actually killed, not just reported clean; verified this + regression test fails against the pre-fix code and passes against the fix. - `bun run build` ### Not tested yet diff --git a/packages/browser/src/session.ts b/packages/browser/src/session.ts index f6139d6..4af3360 100644 --- a/packages/browser/src/session.ts +++ b/packages/browser/src/session.ts @@ -7,6 +7,7 @@ import { getSession, isDisplaySocketAlive, isPidAlive, + isProcessGroupAlive, isProfileConfined, processIdentityMatches, reapDeadRunningSessions, @@ -182,17 +183,64 @@ async function waitForOwnedIdentity( } } +const OWNED_GROUP_CONFIRM_TIMEOUT_MS = 1_000; +const OWNED_GROUP_POLL_INTERVAL_MS = 20; + +/** + * Kill the daemon's whole process group, not just the daemon process itself. + * The supervisor spawns Chrome as a plain (non-detached) child, so Chrome + * shares the supervisor's process group (its pgid equals the supervisor's + * pid, set at spawn via `detached: true`). Signaling only the supervisor can + * orphan a live Chrome in the same group; signaling the group by its pgid + * reaches every current member regardless of whether the supervisor itself + * has already exited (e.g. crashed) by the time we act. + */ +function killOwnedBrowserProcessGroup(daemon: OwnedDaemonHandle): void { + try { + process.kill(-daemon.pid, "SIGKILL"); + return; + } catch { + // Group signal failed (already gone, or unsupported); fall through to a + // direct kill so the supervisor itself is at least reaped. + } + try { + daemon.child.kill("SIGKILL"); + } catch { + // already gone + } +} + +async function confirmOwnedGroupGone(pgid: number): Promise { + const deadline = Date.now() + OWNED_GROUP_CONFIRM_TIMEOUT_MS; + while (isProcessGroupAlive(pgid)) { + if (Date.now() >= deadline) return false; + await sleep(OWNED_GROUP_POLL_INTERVAL_MS); + } + return true; +} + +/** + * Stop an owned browser daemon before its `/proc`-backed identity has been + * captured (or ever will be, if the daemon crashed outright). `daemon.pid` is + * safe to signal without a verified `ProcessIdentity`: it is our own + * just-spawned, not-yet-reaped child, so it cannot be a reused pid, and it + * doubles as the process-group id because `startDaemon` spawns it detached + * (making it its own session/group leader). Returns true only once the whole + * group is confirmed to have no live members, so a crashed supervisor can + * never leave an orphaned Chrome reported as cleaned up. + */ async function stopOwnedBrowserDaemon( daemon: OwnedDaemonHandle, ): Promise { try { - if (ownedDaemonExited(daemon)) return true; - const closed = new Promise((resolve) => { - daemon.child.once("close", () => resolve()); - }); - daemon.child.kill("SIGKILL"); - if (!ownedDaemonExited(daemon)) await closed; - return true; + const closed = ownedDaemonExited(daemon) + ? Promise.resolve() + : new Promise((resolve) => { + daemon.child.once("close", () => resolve()); + }); + killOwnedBrowserProcessGroup(daemon); + await closed; + return await confirmOwnedGroupGone(daemon.pid); } catch { return false; } finally { diff --git a/packages/browser/test/pre-identity-cleanup.test.ts b/packages/browser/test/pre-identity-cleanup.test.ts new file mode 100644 index 0000000..6f3c519 --- /dev/null +++ b/packages/browser/test/pre-identity-cleanup.test.ts @@ -0,0 +1,145 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + REAPER_CLEANUP_PENDING_META_KEY, + destroySessionRecord, + isPidAlive, + listSessions, + type EnvLike, +} from "@pickforge/picklab-core"; + +// A sentinel Xvfb pid/display, standing in for a real X server: this suite +// only exercises the pre-identity Chrome cleanup path, so Xvfb itself is +// mocked away (no Xvfb binary required, unlike the fake-binary suites in +// session.test.ts). +const FAKE_XVFB_PID = 4_194_301; +const FAKE_DISPLAY = ":244"; + +vi.mock("@pickforge/picklab-desktop-linux", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + startXvfb: vi.fn( + async (opts: { + onSpawn?: (partial: { + pid: number; + display: string; + startTimeTicks: number; + width: number; + height: number; + }) => void | Promise; + }) => { + const partial = { + pid: FAKE_XVFB_PID, + display: FAKE_DISPLAY, + startTimeTicks: 456, + width: 1280, + height: 800, + }; + await opts.onSpawn?.(partial); + return { + ...partial, + logPath: "/tmp/fake-browser-pre-identity-xvfb.log", + }; + }, + ), + }; +}); + +// Simulate the "pathological /proc identity-read failure" the issue +// describes: the owned Chrome supervisor is alive, but its own identity can +// never be captured. +vi.mock("@pickforge/picklab-core", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + readProcessIdentity: vi.fn(() => undefined), + }; +}); + +import { createBrowserSession, browserSessionLogDir } from "../src/session.js"; +import { writeFakeChrome } from "./fakes.js"; + +const TEST_TIMEOUT_MS = 15_000; + +let root: string | undefined; + +afterEach(() => { + if (root !== undefined) fs.rmSync(root, { recursive: true, force: true }); + root = undefined; +}); + +describe("pre-identity browser daemon cleanup", () => { + it( + "kills a live Chrome left behind when the supervisor's own identity never resolves", + async () => { + root = fs.mkdtempSync( + path.join(os.tmpdir(), "picklab-browser-pre-identity-"), + ); + const home = path.join(root, "home"); + const binDir = path.join(root, "bin"); + fs.mkdirSync(home, { recursive: true }); + // "stall" never exits and never publishes a DevTools port on its own, + // so it stays alive for the full identity-wait window like a real + // Chrome would, and only our teardown code can end it. + writeFakeChrome(binDir, "stall"); + const env: EnvLike = { + ...process.env, + HOME: home, + PICKLAB_HOME: path.join(root, "picklab-home"), + PATH: `${binDir}${path.delimiter}${process.env.PATH ?? ""}`, + }; + + await expect( + createBrowserSession({ + projectDir: path.join(root, "project"), + registryEnv: env, + env, + }), + ).rejects.toThrow(/could not be identified during startup/); + + const records = await listSessions(env); + expect(records).toHaveLength(1); + const record = records[0]!; + expect(record.status).toBe("error"); + // Identity was never captured, so nothing was ever recorded to retry + // against later; the only way this can be a true "cleanup complete" is + // if the group was actually confirmed dead synchronously. + expect(record.browser).toBeUndefined(); + expect(record.meta?.[REAPER_CLEANUP_PENDING_META_KEY]).toBeUndefined(); + + const sessionDir = browserSessionLogDir(record.id, env); + const pidFile = path.join(sessionDir, "chrome.pid"); + const pidDeadline = Date.now() + 5_000; + while (!fs.existsSync(pidFile) && Date.now() < pidDeadline) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + expect(fs.existsSync(pidFile)).toBe(true); + const chromePid = Number(fs.readFileSync(pidFile, "utf8").trim()); + expect(Number.isSafeInteger(chromePid)).toBe(true); + + try { + // The fix: the whole process group (supervisor + Chrome) is killed + // and confirmed empty before cleanup is reported complete, so by now + // the fake Chrome must already be dead. Before the fix, only the + // supervisor was signaled, leaving this process running. + expect(isPidAlive(chromePid)).toBe(false); + } finally { + if (isPidAlive(chromePid)) { + try { + process.kill(chromePid, "SIGKILL"); + } catch { + // already gone + } + } + } + + await destroySessionRecord(record.id, env); + }, + TEST_TIMEOUT_MS, + ); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fdd0b09..1b0c126 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -122,6 +122,7 @@ export { export { CommandError, isPidAlive, + isProcessGroupAlive, listProcessGroupMembers, processIdentityMatches, readProcessIdentity, diff --git a/packages/core/src/proc.ts b/packages/core/src/proc.ts index 7be2c4b..7f5cd88 100644 --- a/packages/core/src/proc.ts +++ b/packages/core/src/proc.ts @@ -420,6 +420,24 @@ export function processIdentityMatches(identity: ProcessIdentity): boolean { return startTicks !== undefined && startTicks === identity.startTicks; } +/** + * Check whether any process in the given process group still exists, via a + * `kill(2)` signal-0 probe. Unlike `listProcessGroupMembers`, this does not + * read `/proc`, so it works on any POSIX platform (including Darwin) at the + * cost of being a yes/no existence check rather than a member list. Useful to + * confirm a group is empty when no `/proc`-backed `ProcessIdentity` for its + * leader is available yet, such as during the brief window between spawning + * an owned daemon and capturing its identity. + */ +export function isProcessGroupAlive(pgid: number): boolean { + try { + process.kill(-pgid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code === "EPERM"; + } +} + /** List the PIDs whose process group id equals `pgid`. */ export function listProcessGroupMembers(pgid: number): number[] { let entries: string[]; diff --git a/packages/core/test/proc.test.ts b/packages/core/test/proc.test.ts index 316f104..200e48b 100644 --- a/packages/core/test/proc.test.ts +++ b/packages/core/test/proc.test.ts @@ -5,6 +5,7 @@ import os from "node:os"; import path from "node:path"; import { isPidAlive, + isProcessGroupAlive, listProcessGroupMembers, parseProcStat, processIdentityMatches, @@ -612,3 +613,94 @@ describe("process identity and group termination", () => { } }); }); + +// isProcessGroupAlive is a kill(2) signal-0 probe, not a /proc read, so +// (unlike listProcessGroupMembers) it is exercised for real here and runs on +// every platform, including Darwin. +describe("isProcessGroupAlive", () => { + it("is true for a live group and false once every member is gone", async () => { + const leader = spawn(node, ["-e", "setInterval(() => {}, 1000);"], { + detached: true, + stdio: "ignore", + }); + const pid = leader.pid; + if (pid === undefined) { + throw new Error("child process did not expose a pid"); + } + try { + expect(isProcessGroupAlive(pid)).toBe(true); + process.kill(-pid, "SIGKILL"); + const deadline = Date.now() + 3000; + while (isProcessGroupAlive(pid) && Date.now() < deadline) { + await delay(20); + } + expect(isProcessGroupAlive(pid)).toBe(false); + } finally { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // already gone + } + } + }); + + it("stays true when a leader is killed but a group member survives it, matching a crashed-supervisor-with-live-child scenario", async () => { + // The leader's own script spawns a member process in the same group + // (inherited by default, since the member itself is not detached), marks + // a readiness file once the member is actually up, then parks so it does + // not exit on its own. + const readyFile = path.join( + os.tmpdir(), + `picklab-proc-group-ready-${process.pid}-${Date.now()}`, + ); + const script = [ + 'const { spawn } = require("node:child_process");', + 'const fs = require("node:fs");', + 'const member = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000);"], { stdio: "ignore" });', + `member.once("spawn", () => fs.writeFileSync(${JSON.stringify(readyFile)}, "ready"));`, + "setInterval(() => {}, 1000);", + ].join("\n"); + const leader = spawn(node, ["-e", script], { + detached: true, + stdio: "ignore", + }); + const pid = leader.pid; + if (pid === undefined) { + throw new Error("child process did not expose a pid"); + } + try { + const readyDeadline = Date.now() + 3000; + while (!fs.existsSync(readyFile) && Date.now() < readyDeadline) { + await delay(20); + } + expect(fs.existsSync(readyFile)).toBe(true); + expect(isProcessGroupAlive(pid)).toBe(true); + + // Kill only the leader directly (by pid, not by group), simulating a + // supervisor that crashed on its own while its child kept running. + leader.kill("SIGKILL"); + await new Promise((resolve) => leader.once("exit", () => resolve())); + + // The group (identified by the now-dead leader's former pid) is still + // alive because its member is still running: a naive check that only + // asks "did the leader exit" would wrongly call this cleaned up. + expect(isProcessGroupAlive(pid)).toBe(true); + + // The fix: signal the group by pgid, which still reaches the survivor, + // then confirm it is actually empty. + process.kill(-pid, "SIGKILL"); + const killDeadline = Date.now() + 3000; + while (isProcessGroupAlive(pid) && Date.now() < killDeadline) { + await delay(20); + } + expect(isProcessGroupAlive(pid)).toBe(false); + } finally { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // already gone + } + fs.rmSync(readyFile, { force: true }); + } + }); +}); From 1bed6bfc5a6983ccb6119e24dc3345951868839d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 03:39:37 -0300 Subject: [PATCH 2/2] fix(browser): document pid-reuse honesty and add missing-supervisor test The exited-branch doc comment on stopOwnedBrowserDaemon overstated its pid-reuse safety: Node/libuv can reap a child (making its pid OS-reusable) before exitCode/signalCode observably go non-null, so that branch is not provably safe the way the not-yet-exited branch is. Split the doc comment per branch to state the real guarantee each one relies on, and add an isProcessGroupAlive pre-check before signaling in the exited branch so a fully vacated group is never blindly signaled (this narrows, but does not close, the reuse gap). Add the missing-supervisor integration regression the issue checklist asks for: a stand-in supervisor that exits immediately after spawning Chrome, exercising stopOwnedBrowserDaemon's already-exited branch end-to-end through createBrowserSession. Captures Chrome's pid synchronously from its own spawn() return rather than waiting on Chrome's script to self-report, after the wait-based version proved flaky under a fully parallel `bun run test`. Refs #29 --- docs/releases/UNRELEASED.md | 26 ++- packages/browser/src/session.ts | 61 ++++-- .../browser/test/pre-identity-cleanup.test.ts | 205 ++++++++++++------ 3 files changed, 210 insertions(+), 82 deletions(-) diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 8000fa7..1418775 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -91,7 +91,11 @@ release description, then reset it after the release is published. supervisor could leave a same-group Chrome orphaned and alive while its session record was marked fully cleaned up. Defensive hardening only (issue #29, deferred from #28); no behavior change on the normal startup path, no - feature flag. + feature flag. The already-exited branch is documented as an accepted, + bounded pid-reuse residual (Node/libuv can reap before we observe the + exit, so a verified identity is not obtainable there by the issue's own + premise) and now pre-checks `isProcessGroupAlive` before signaling so a + fully vacated group is never blindly signaled. - Centralized session lifecycle composition in core and routed dead-session reaping through typed desktop, Android, and browser teardown owners, removing duplicate CLI/MCP orchestration and core PID/profile stop implementations. @@ -166,11 +170,21 @@ release description, then reset it after the release is published. association mismatches in `devtools-evidence.test.ts`. - New `proc.test.ts` `isProcessGroupAlive` coverage, including a real spawned-leader-killed-with-a-surviving-member regression case, and a new - `packages/browser/test/pre-identity-cleanup.test.ts` integration - regression (mocked `readProcessIdentity`/`startXvfb`, real fake-Chrome - subprocess) proving a live Chrome left behind by an unresolved supervisor - identity is actually killed, not just reported clean; verified this - regression test fails against the pre-fix code and passes against the fix. + `packages/browser/test/pre-identity-cleanup.test.ts` with two integration + regressions (mocked `readProcessIdentity`/`startXvfb`, real fake-Chrome + subprocess): a live-supervisor case (identity read never resolves) and a + missing-supervisor case (a stand-in supervisor exits immediately after + spawning Chrome, exercising `stopOwnedBrowserDaemon`'s already-exited + branch). Both prove a live Chrome left behind is actually killed, not just + reported clean; verified both fail against the pre-fix code and pass + against the fix. The missing-supervisor case captures Chrome's pid + synchronously from its own `spawn()` return rather than waiting on Chrome's + script to self-report, after the wait-based version proved flaky under a + fully parallel `bun run test` on this dev sandbox; the live-supervisor case + still depends on Chrome's own self-reported marker (unavoidable without + changing production code) and can occasionally need extra wall-clock time + under the same full-parallel load — a scheduling artifact, not a logic + defect, and it stays within the pre-existing 43-45 Darwin noise band above. - `bun run build` ### Not tested yet diff --git a/packages/browser/src/session.ts b/packages/browser/src/session.ts index 4af3360..88fcbb9 100644 --- a/packages/browser/src/session.ts +++ b/packages/browser/src/session.ts @@ -194,6 +194,10 @@ const OWNED_GROUP_POLL_INTERVAL_MS = 20; * orphan a live Chrome in the same group; signaling the group by its pgid * reaches every current member regardless of whether the supervisor itself * has already exited (e.g. crashed) by the time we act. + * + * This function itself does not decide whether `daemon.pid` is still safe to + * treat as this daemon's group id — see `stopOwnedBrowserDaemon`'s doc + * comment for the per-branch reasoning its two call sites rely on. */ function killOwnedBrowserProcessGroup(daemon: OwnedDaemonHandle): void { try { @@ -221,25 +225,54 @@ async function confirmOwnedGroupGone(pgid: number): Promise { /** * Stop an owned browser daemon before its `/proc`-backed identity has been - * captured (or ever will be, if the daemon crashed outright). `daemon.pid` is - * safe to signal without a verified `ProcessIdentity`: it is our own - * just-spawned, not-yet-reaped child, so it cannot be a reused pid, and it - * doubles as the process-group id because `startDaemon` spawns it detached - * (making it its own session/group leader). Returns true only once the whole - * group is confirmed to have no live members, so a crashed supervisor can - * never leave an orphaned Chrome reported as cleaned up. + * captured (or ever will be, if the daemon crashed outright before capture + * could finish). Returns true only once the whole process group is confirmed + * to have no live members, so a crashed supervisor can never leave an + * orphaned Chrome reported as cleaned up. + * + * `daemon.pid` doubles as the process-group id because `startDaemon` spawns + * it detached (making it its own session/group leader). Signaling it without + * a verified `ProcessIdentity` is only safe in one of the two branches below, + * and for different reasons: + * + * - Not yet exited (`!ownedDaemonExited(daemon)`): safe by construction, not + * by verification. There is no `await` between that check and the group + * signal, so no event-loop turn runs in between — the daemon cannot exit, + * get reaped by libuv, and have its pid recycled to an unrelated process + * within that synchronous gap. Keep this branch synchronous; an `await` + * inserted between the check and the signal would reopen the gap. + * - Already exited: NOT provably safe against pid reuse. Node/libuv reaps a + * child on SIGCHLD independent of any handle we hold, and `exitCode`/ + * `signalCode` only become non-null after that reap, so by the time this + * branch runs the pid is OS-reusable in principle. This is an accepted, + * bounded residual risk rather than a reason to skip cleanup: both call + * sites live inside `createBrowserSession`'s synchronous startup flow, + * within the identity capture's own <=1s deadline; no reaper or other + * late path ever reaches this function; and no verified identity is + * obtainable here by the issue's own premise (identity capture is exactly + * what failed). The `isProcessGroupAlive` pre-check below avoids blindly + * signaling a group that has already been fully vacated, but it does not + * close the reuse gap: an unrelated process that happened to land on this + * pid and also happened to already be its own group leader would still be + * signaled. */ async function stopOwnedBrowserDaemon( daemon: OwnedDaemonHandle, ): Promise { try { - const closed = ownedDaemonExited(daemon) - ? Promise.resolve() - : new Promise((resolve) => { - daemon.child.once("close", () => resolve()); - }); - killOwnedBrowserProcessGroup(daemon); - await closed; + if (ownedDaemonExited(daemon)) { + if (isProcessGroupAlive(daemon.pid)) { + killOwnedBrowserProcessGroup(daemon); + } + } else { + const closed = new Promise((resolve) => { + daemon.child.once("close", () => resolve()); + }); + // No `await` between the exit check above and this signal: see the + // doc comment for why that is what keeps this branch safe. + killOwnedBrowserProcessGroup(daemon); + await closed; + } return await confirmOwnedGroupGone(daemon.pid); } catch { return false; diff --git a/packages/browser/test/pre-identity-cleanup.test.ts b/packages/browser/test/pre-identity-cleanup.test.ts index 6f3c519..d1ced04 100644 --- a/packages/browser/test/pre-identity-cleanup.test.ts +++ b/packages/browser/test/pre-identity-cleanup.test.ts @@ -61,10 +61,59 @@ vi.mock("@pickforge/picklab-core", async (importOriginal) => { }; }); +// Wrapped (not replaced) so the default behavior is the real supervisor; only +// the "missing supervisor" test below swaps in a stand-in for the duration of +// a single call. +vi.mock("../src/supervisor.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + buildSupervisedBrowserCommand: vi.fn(actual.buildSupervisedBrowserCommand), + }; +}); + import { createBrowserSession, browserSessionLogDir } from "../src/session.js"; +import { buildSupervisedBrowserCommand } from "../src/supervisor.js"; import { writeFakeChrome } from "./fakes.js"; -const TEST_TIMEOUT_MS = 15_000; +// Generous: a full parallel `bun run test` run can starve these spawned +// processes of CPU for several seconds before they get scheduled at all. +const TEST_TIMEOUT_MS = 30_000; +const MARKER_WAIT_MS = 20_000; +const mockedBuildSupervisedBrowserCommand = vi.mocked( + buildSupervisedBrowserCommand, +); + +// Spawns the given browser binary as a plain (non-detached) child — so it +// shares this script's own process group, exactly like the real supervisor's +// child — then exits immediately without waiting for it or forwarding any +// signal, standing in for a supervisor that crashed right after spawning +// Chrome. Writes the "chrome.pid" marker itself, synchronously, from the +// `spawn()` return value, instead of relying on fake Chrome's own script (see +// fakes.ts) to schedule and self-report it: under a fully parallel `bun run +// test`, waiting on a second process to actually get CPU time to run its own +// startup code before this script exits was measurably flaky, whereas the +// pid is already known the instant `spawn()` returns. +const CRASHING_SUPERVISOR_SCRIPT = [ + 'const { spawn } = require("node:child_process");', + 'const fs = require("node:fs");', + 'const path = require("node:path");', + "const [binary, ...args] = process.argv.slice(1);", + "let profile = null;", + "for (const a of args) {", + ' if (a.startsWith("--user-data-dir=")) profile = a.slice("--user-data-dir=".length);', + "}", + 'const pidFile = profile ? path.join(path.dirname(profile), "chrome.pid") : null;', + 'const child = spawn(binary, args, { stdio: "ignore" });', + "if (pidFile && child.pid !== undefined) {", + " try {", + " fs.mkdirSync(path.dirname(pidFile), { recursive: true });", + " fs.writeFileSync(pidFile, String(child.pid));", + " } catch {}", + "}", + "process.exit(0);", +].join("\n"); let root: string | undefined; @@ -73,72 +122,104 @@ afterEach(() => { root = undefined; }); +async function setUpStalledFakeChrome(): Promise { + root = fs.mkdtempSync( + path.join(os.tmpdir(), "picklab-browser-pre-identity-"), + ); + const home = path.join(root, "home"); + const binDir = path.join(root, "bin"); + fs.mkdirSync(home, { recursive: true }); + // "stall" never exits and never publishes a DevTools port on its own, so it + // stays alive for the full identity-wait window like a real Chrome would, + // and only our teardown code can end it. + writeFakeChrome(binDir, "stall"); + return { + ...process.env, + HOME: home, + PICKLAB_HOME: path.join(root, "picklab-home"), + PATH: `${binDir}${path.delimiter}${process.env.PATH ?? ""}`, + }; +} + +/** + * Run `createBrowserSession` to its expected pre-identity rejection, then + * assert the record and process-level cleanup semantics that must hold + * either way: no retry was recorded (since no identity was ever captured to + * retry against) and the surviving fake Chrome is confirmed dead. + */ +async function expectPreIdentityCleanupConfirmed(env: EnvLike): Promise { + await expect( + createBrowserSession({ + projectDir: path.join(root!, "project"), + registryEnv: env, + env, + }), + ).rejects.toThrow(/could not be identified during startup/); + + const records = await listSessions(env); + expect(records).toHaveLength(1); + const record = records[0]!; + expect(record.status).toBe("error"); + // Identity was never captured, so nothing was ever recorded to retry + // against later; the only way this can be a true "cleanup complete" is if + // the group was actually confirmed dead synchronously. + expect(record.browser).toBeUndefined(); + expect(record.meta?.[REAPER_CLEANUP_PENDING_META_KEY]).toBeUndefined(); + + const sessionDir = browserSessionLogDir(record.id, env); + const pidFile = path.join(sessionDir, "chrome.pid"); + const pidDeadline = Date.now() + MARKER_WAIT_MS; + while (!fs.existsSync(pidFile) && Date.now() < pidDeadline) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + expect(fs.existsSync(pidFile)).toBe(true); + const chromePid = Number(fs.readFileSync(pidFile, "utf8").trim()); + expect(Number.isSafeInteger(chromePid)).toBe(true); + + try { + expect(isPidAlive(chromePid)).toBe(false); + } finally { + if (isPidAlive(chromePid)) { + try { + process.kill(chromePid, "SIGKILL"); + } catch { + // already gone + } + } + } + + await destroySessionRecord(record.id, env); +} + describe("pre-identity browser daemon cleanup", () => { it( "kills a live Chrome left behind when the supervisor's own identity never resolves", async () => { - root = fs.mkdtempSync( - path.join(os.tmpdir(), "picklab-browser-pre-identity-"), - ); - const home = path.join(root, "home"); - const binDir = path.join(root, "bin"); - fs.mkdirSync(home, { recursive: true }); - // "stall" never exits and never publishes a DevTools port on its own, - // so it stays alive for the full identity-wait window like a real - // Chrome would, and only our teardown code can end it. - writeFakeChrome(binDir, "stall"); - const env: EnvLike = { - ...process.env, - HOME: home, - PICKLAB_HOME: path.join(root, "picklab-home"), - PATH: `${binDir}${path.delimiter}${process.env.PATH ?? ""}`, - }; - - await expect( - createBrowserSession({ - projectDir: path.join(root, "project"), - registryEnv: env, - env, - }), - ).rejects.toThrow(/could not be identified during startup/); - - const records = await listSessions(env); - expect(records).toHaveLength(1); - const record = records[0]!; - expect(record.status).toBe("error"); - // Identity was never captured, so nothing was ever recorded to retry - // against later; the only way this can be a true "cleanup complete" is - // if the group was actually confirmed dead synchronously. - expect(record.browser).toBeUndefined(); - expect(record.meta?.[REAPER_CLEANUP_PENDING_META_KEY]).toBeUndefined(); - - const sessionDir = browserSessionLogDir(record.id, env); - const pidFile = path.join(sessionDir, "chrome.pid"); - const pidDeadline = Date.now() + 5_000; - while (!fs.existsSync(pidFile) && Date.now() < pidDeadline) { - await new Promise((resolve) => setTimeout(resolve, 20)); - } - expect(fs.existsSync(pidFile)).toBe(true); - const chromePid = Number(fs.readFileSync(pidFile, "utf8").trim()); - expect(Number.isSafeInteger(chromePid)).toBe(true); - - try { - // The fix: the whole process group (supervisor + Chrome) is killed - // and confirmed empty before cleanup is reported complete, so by now - // the fake Chrome must already be dead. Before the fix, only the - // supervisor was signaled, leaving this process running. - expect(isPidAlive(chromePid)).toBe(false); - } finally { - if (isPidAlive(chromePid)) { - try { - process.kill(chromePid, "SIGKILL"); - } catch { - // already gone - } - } - } + const env = await setUpStalledFakeChrome(); + // The fix: the whole process group (supervisor + Chrome) is killed and + // confirmed empty before cleanup is reported complete. Before the fix, + // only the supervisor was signaled directly, leaving Chrome running. + await expectPreIdentityCleanupConfirmed(env); + }, + TEST_TIMEOUT_MS, + ); - await destroySessionRecord(record.id, env); + it( + "kills a surviving Chrome when the supervisor has already exited (crashed) before cleanup runs", + async () => { + const env = await setUpStalledFakeChrome(); + mockedBuildSupervisedBrowserCommand.mockImplementationOnce( + (nodePath, binaryPath, browserArgs) => ({ + command: nodePath, + args: ["-e", CRASHING_SUPERVISOR_SCRIPT, binaryPath, ...browserArgs], + }), + ); + // The fix's exited branch: `ownedDaemonExited` is already true by the + // time `stopOwnedBrowserDaemon` runs (the stand-in supervisor exits + // right after spawning Chrome), yet Chrome is still alive in the same + // process group. Before the fix this branch returned cleanup-complete + // unconditionally without ever signaling that surviving group. + await expectPreIdentityCleanupConfirmed(env); }, TEST_TIMEOUT_MS, );