Fix browser supervisor child reaping race

plugins/browser-automation/process.test.ts

@@ -1,9 +1,39 @@
-import { spawn } from "node:child_process";
+import { type ChildProcess, spawn } from "node:child_process";
+import { once } from "node:events";
 import { mkdtemp, readFile, rm } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { describe, expect, it, vi } from "vitest";
-import { supervise } from "./process.js";
+import {
+  PROCESS_REAP_CONFIRMATION_TIMEOUT_MS,
+  ProcessReapingUnconfirmedError,
+  supervise,
+} from "./process.js";
+
+async function stopWorker(worker: ChildProcess): Promise<void> {
+  if (worker.exitCode !== null || worker.signalCode !== null) return;
+  const exited = once(worker, "exit");
+  worker.kill("SIGKILL");
+  await exited;
+}
+
+function killProcessGroup(pid: number): void {
+  try {
+    process.kill(-pid, "SIGKILL");
+  } catch {}
+  try {
+    process.kill(pid, "SIGKILL");
+  } catch {}
+}
+
+function isProcessAlive(pid: number): boolean {
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch {
+    return false;
+  }
+}
 
 describe("process ownership", () => {
   it("worker death closes the supervisor pipe and kills its child", async () => {
@@ -17,23 +47,24 @@ describe("process ownership", () => {
       ["--import", "tsx", "--input-type=module", "-e", code],
       { stdio: "ignore" },
     );
+    let pid = 0;
     try {
-      let pid = 0;
       await vi.waitFor(
         async () => {
           pid = Number(await readFile(file, "utf8"));
         },
         { timeout: 5000 },
       );
-      worker.kill("SIGKILL");
+      await stopWorker(worker);
       await vi.waitFor(
         () => {
           expect(() => process.kill(pid, 0)).toThrow();
         },
         { timeout: 5000 },
       );
     } finally {
-      worker.kill("SIGKILL");
+      await stopWorker(worker);
+      if (pid > 0 && isProcessAlive(pid)) killProcessGroup(pid);
       await rm(root, { recursive: true, force: true });
     }
   }, 12_000);
@@ -71,4 +102,57 @@ describe("process ownership", () => {
       await rm(root, { recursive: true, force: true });
     }
   }, 10_000);
+  it("bounds close while the supervisor independently finishes reaping", async () => {
+    const root = await mkdtemp(join(tmpdir(), "db-close-deadline-"));
+    const file = join(root, "pid");
+    const code =
+      'require("node:fs").writeFileSync(process.argv[1], `${String(process.pid)}:${String(process.ppid)}`); process.on("SIGTERM", () => {}); setInterval(() => {}, 1000);';
+    let expireDeadline = (): void => {
+      throw new Error("Close deadline was not scheduled");
+    };
+    let deadlineScheduled = false;
+    let deadlineMs = 0;
+    const owned = supervise(process.execPath, ["-e", code, file], process.env, {
+      scheduleCloseDeadline(callback, timeoutMs) {
+        expireDeadline = callback;
+        deadlineScheduled = true;
+        deadlineMs = timeoutMs;
+        return { cancel() {} };
+      },
+    });
+    let childPid = 0;
+    let supervisorPid = 0;
+    try {
+      await vi.waitFor(
+        async () => {
+          const pids = (await readFile(file, "utf8")).split(":").map(Number);
+          childPid = pids[0] ?? 0;
+          supervisorPid = pids[1] ?? 0;
+          expect(childPid).toBeGreaterThan(0);
+          expect(supervisorPid).toBeGreaterThan(0);
+        },
+        { timeout: 5000 },
+      );
+      const closing = owned.close();
+      expect(deadlineMs).toBe(PROCESS_REAP_CONFIRMATION_TIMEOUT_MS);
+      expect(deadlineScheduled).toBe(true);
+      const rejected = expect(closing).rejects.toBeInstanceOf(
+        ProcessReapingUnconfirmedError,
+      );
+      expireDeadline();
+      await rejected;
+      await expect(closing).rejects.toThrow("child reaping was not confirmed");
+      expect(owned.alive()).toBe(true);
+      expect(() => process.kill(childPid, 0)).not.toThrow();
+      await vi.waitFor(() => expect(owned.alive()).toBe(false), {
+        timeout: 5000,
+      });
+      expect(() => process.kill(childPid, 0)).toThrow();
+    } finally {
+      if (childPid > 0 && isProcessAlive(childPid)) killProcessGroup(childPid);
+      if (supervisorPid > 0 && isProcessAlive(supervisorPid))
+        process.kill(supervisorPid, "SIGKILL");
+      await rm(root, { recursive: true, force: true });
+    }
+  }, 8_000);
 });

plugins/browser-automation/process.ts

@@ -1,18 +1,54 @@
 import { spawn } from "node:child_process";
 
+interface CloseDeadline {
+  cancel(): void;
+}
+
+interface SuperviseOptions {
+  scheduleCloseDeadline?: (
+    callback: () => void,
+    timeoutMs: number,
+  ) => CloseDeadline;
+}
+
+export const PROCESS_REAP_CONFIRMATION_TIMEOUT_MS = 5_000;
+
+export class ProcessReapingUnconfirmedError extends Error {
+  constructor(timeoutMs: number) {
+    super(
+      `Browser process shutdown exceeded ${String(timeoutMs)} ms; child reaping was not confirmed`,
+    );
+    this.name = "ProcessReapingUnconfirmedError";
+  }
+}
+
+function scheduleCloseDeadline(
+  callback: () => void,
+  timeoutMs: number,
+): CloseDeadline {
+  const timer = setTimeout(callback, timeoutMs);
+  timer.unref();
+  return { cancel: () => clearTimeout(timer) };
+}
+
 const supervisor = `
 const { spawn } = require('node:child_process');
 const child = spawn(process.argv[1], process.argv.slice(2), { detached: true, stdio: 'ignore', env: process.env });
 let stopping = false;
+let childClosed = false;
 function kill(signal) { if (child.pid) { try { process.kill(-child.pid, signal); } catch {} } }
+function finish(code) {
+  if (childClosed) process.exit(code);
+  child.once('close', () => process.exit(code));
+}
 function stop() {
   if (stopping) return;
   stopping = true;
   kill('SIGTERM');
-  setTimeout(() => { kill('SIGKILL'); setTimeout(() => process.exit(0), 100); }, 1500);
+  setTimeout(() => { kill('SIGKILL'); finish(0); }, 1500);
 }
 child.on('error', () => process.exit(1));
-child.on('exit', () => { if (!stopping) { kill('SIGKILL'); process.exit(1); } });
+child.on('close', () => { childClosed = true; if (!stopping) { kill('SIGKILL'); process.exit(1); } });
 process.stdin.resume();
 process.stdin.on('end', stop);
 process.on('SIGTERM', stop);
@@ -23,6 +59,7 @@ export function supervise(
   command: string,
   args: string[],
   env: NodeJS.ProcessEnv,
+  options: SuperviseOptions = {},
 ) {
   const child = spawn(process.execPath, ["-e", supervisor, command, ...args], {
     env,
@@ -40,12 +77,39 @@ export function supervise(
       resolve();
     });
   });
+  let closing: Promise<void> | null = null;
+  const close = (): Promise<void> => {
+    if (closing !== null) return closing;
+    child.stdin.end();
+    closing = new Promise<void>((resolve, reject) => {
+      let settled = false;
+      let cancelDeadline = (): void => {};
+      completion.then(() => {
+        if (settled) return;
+        settled = true;
+        cancelDeadline();
+        resolve();
+      });
+      cancelDeadline = (options.scheduleCloseDeadline ?? scheduleCloseDeadline)(
+        () => {
+          if (settled) return;
+          settled = true;
+          child.stdin.destroy();
+          child.unref();
+          reject(
+            new ProcessReapingUnconfirmedError(
+              PROCESS_REAP_CONFIRMATION_TIMEOUT_MS,
+            ),
+          );
+        },
+        PROCESS_REAP_CONFIRMATION_TIMEOUT_MS,
+      ).cancel;
+    });
+    return closing;
+  };
   return {
     alive: () => !exited,
-    async close() {
-      child.stdin.end();
-      await completion;
-    },
+    close,
   };
 }