diff --git a/plugins/browser-automation/process.test.ts b/plugins/browser-automation/process.test.ts index e2a390d8a4..e72ff295cc 100644 --- a/plugins/browser-automation/process.test.ts +++ b/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 { + 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,15 +47,15 @@ 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(); @@ -33,7 +63,8 @@ describe("process ownership", () => { { 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); }); diff --git a/plugins/browser-automation/process.ts b/plugins/browser-automation/process.ts index 718687fbd9..324d00c848 100644 --- a/plugins/browser-automation/process.ts +++ b/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 | null = null; + const close = (): Promise => { + if (closing !== null) return closing; + child.stdin.end(); + closing = new Promise((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, }; }