fix(browser): kill the whole process group during pre-identity cleanup

docs/releases/UNRELEASED.md

@@ -81,6 +81,21 @@ 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. 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.
@@ -153,6 +168,23 @@ 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` 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

packages/browser/src/session.ts

@@ -7,6 +7,7 @@ import {
   getSession,
   isDisplaySocketAlive,
   isPidAlive,
+  isProcessGroupAlive,
   isProfileConfined,
   processIdentityMatches,
   reapDeadRunningSessions,
@@ -182,17 +183,97 @@ 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.
+ *
+ * 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 {
+    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<boolean> {
+  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 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<boolean> {
   try {
-    if (ownedDaemonExited(daemon)) return true;
-    const closed = new Promise<void>((resolve) => {
-      daemon.child.once("close", () => resolve());
-    });
-    daemon.child.kill("SIGKILL");
-    if (!ownedDaemonExited(daemon)) await closed;
-    return true;
+    if (ownedDaemonExited(daemon)) {
+      if (isProcessGroupAlive(daemon.pid)) {
+        killOwnedBrowserProcessGroup(daemon);
+      }
+    } else {
+      const closed = new Promise<void>((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;
   } finally {

packages/browser/test/pre-identity-cleanup.test.ts

@@ -0,0 +1,226 @@
+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<typeof import("@pickforge/picklab-desktop-linux")>();
+  return {
+    ...actual,
+    startXvfb: vi.fn(
+      async (opts: {
+        onSpawn?: (partial: {
+          pid: number;
+          display: string;
+          startTimeTicks: number;
+          width: number;
+          height: number;
+        }) => void | Promise<void>;
+      }) => {
+        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<typeof import("@pickforge/picklab-core")>();
+  return {
+    ...actual,
+    readProcessIdentity: vi.fn(() => undefined),
+  };
+});
+
+// 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<typeof import("../src/supervisor.js")>();
+  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";
+
+// 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;
+
+afterEach(() => {
+  if (root !== undefined) fs.rmSync(root, { recursive: true, force: true });
+  root = undefined;
+});
+
+async function setUpStalledFakeChrome(): Promise<EnvLike> {
+  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<void> {
+  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 () => {
+      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,
+  );
+
+  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,
+  );
+});

packages/core/src/index.ts

@@ -122,6 +122,7 @@ export {
 export {
   CommandError,
   isPidAlive,
+  isProcessGroupAlive,
   listProcessGroupMembers,
   processIdentityMatches,
   readProcessIdentity,