diff --git a/packages/cli/src/browser/chromeTree.mocked.test.ts b/packages/cli/src/browser/chromeTree.mocked.test.ts new file mode 100644 index 0000000000..b3d3a19f67 --- /dev/null +++ b/packages/cli/src/browser/chromeTree.mocked.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from "vitest"; + +const execFile = vi.hoisted(() => vi.fn()); +vi.mock("node:child_process", () => ({ execFile })); + +import { chromeDescendants, reapChromeTree } from "./chromeTree.js"; + +const psFails = (error: Error & { killed?: boolean }) => + execFile.mockImplementation((...args: unknown[]) => { + (args.at(-1) as (e: Error) => void)(error); + }); + +describe("chromeTree ps call", () => { + it("is bounded, and a ps killed at the timeout records no descendants", async () => { + psFails(Object.assign(new Error("ps timed out"), { killed: true })); + await expect(chromeDescendants(1)).resolves.toEqual([]); + expect(execFile.mock.calls[0]?.[2]).toMatchObject({ timeout: expect.any(Number) }); + }); + + it("does not hide other ps failures", async () => { + psFails(new Error("spawn ps ENOENT")); + await expect(chromeDescendants(1)).rejects.toThrow("ENOENT"); + }); + + it("reaps nothing without running ps when no member was recorded", async () => { + execFile.mockClear(); + await reapChromeTree([]); + expect(execFile).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/browser/chromeTree.test.ts b/packages/cli/src/browser/chromeTree.test.ts new file mode 100644 index 0000000000..fe60e5d7f3 --- /dev/null +++ b/packages/cli/src/browser/chromeTree.test.ts @@ -0,0 +1,45 @@ +import { spawn } from "node:child_process"; +import { expect, it } from "vitest"; +import type { Browser } from "puppeteer-core"; +import { waitForTestCondition } from "../utils/processTestUtils.js"; +import { closeManagedBrowser } from "./launch.js"; + +const alive = (pid: number) => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}; + +it.runIf(process.platform !== "win32")( + "closing a browser also ends the helper processes it leaves behind", + async () => { + const main = spawn( + process.execPath, + [ + "-e", + `const h=require("node:child_process").spawn("sleep",["60"],{detached:true,stdio:"ignore"});h.unref();console.log(h.pid);setInterval(()=>{},1e6)`, + ], + { stdio: ["ignore", "pipe", "ignore"] }, + ); + const helper = Number( + await new Promise((resolve) => main.stdout!.once("data", (d) => resolve(String(d)))), + ); + expect(alive(helper)).toBe(true); + const browser = { + process: () => main, + close: async () => { + main.kill("SIGKILL"); + }, + } as unknown as Browser; + try { + await closeManagedBrowser(browser); + await waitForTestCondition(() => !alive(helper), 5_000); + } finally { + if (alive(helper)) process.kill(helper, "SIGKILL"); + } + }, + 30_000, +); diff --git a/packages/cli/src/browser/chromeTree.ts b/packages/cli/src/browser/chromeTree.ts new file mode 100644 index 0000000000..a9df5f3008 --- /dev/null +++ b/packages/cli/src/browser/chromeTree.ts @@ -0,0 +1,57 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +const PS_TIMEOUT_MS = 2_000; + +export interface TreeMember { + pid: number; + command: string; +} + +async function processTable(): Promise> { + let stdout: string; + try { + ({ stdout } = await promisify(execFile)("ps", ["-axo", "pid=,ppid=,command="], { + maxBuffer: 16 * 1024 * 1024, + timeout: PS_TIMEOUT_MS, + })); + } catch (error) { + // A stalled ps is killed at the timeout: nothing recorded, so the close stays bounded. + if ((error as { killed?: boolean }).killed) return []; + throw error; + } + return stdout.split("\n").flatMap((line) => { + const row = /^\s*(\d+)\s+(\d+)\s+(.*)$/.exec(line); + return row ? [{ pid: Number(row[1]), ppid: Number(row[2]), command: row[3]! }] : []; + }); +} + +/** Every process under `rootPid` now, so they can be ended after the root is gone and they are reparented to init. */ +export async function chromeDescendants(rootPid: number): Promise { + const table = await processTable(); + const found: TreeMember[] = []; + const queue = [rootPid]; + while (queue.length > 0) { + const parent = queue.shift()!; + for (const row of table) { + if (row.ppid !== parent) continue; + found.push({ pid: row.pid, command: row.command }); + queue.push(row.pid); + } + } + return found; +} + +/** Ends the members still running under the command they had, so a reused pid is left alone. */ +export async function reapChromeTree(members: readonly TreeMember[]): Promise { + if (members.length === 0) return; + const commands = new Map((await processTable()).map((row) => [row.pid, row.command])); + for (const { pid, command } of members) { + if (commands.get(pid) !== command) continue; + try { + process.kill(pid, "SIGKILL"); + } catch { + /* already gone */ + } + } +} diff --git a/packages/cli/src/browser/launch.ts b/packages/cli/src/browser/launch.ts index 2c9ef8144f..ab2dd39949 100644 --- a/packages/cli/src/browser/launch.ts +++ b/packages/cli/src/browser/launch.ts @@ -9,6 +9,7 @@ import { import { normalizeErrorMessage } from "../utils/errorMessage.js"; import { requestCliExit } from "../utils/commandResult.js"; import { processIsAlive } from "../utils/processTree.js"; +import { chromeDescendants, reapChromeTree } from "./chromeTree.js"; import { watchChromeTree, type ChromeWatchdog } from "./windowsChromeWatchdog.js"; import { inAgentSandbox, @@ -259,10 +260,16 @@ async function closeBrowser(browser: Browser): Promise { } } +async function descendantsToReap(pid: number | undefined) { + if (pid === undefined || process.platform === "win32") return []; + return chromeDescendants(pid).catch(() => []); +} + async function closedWithoutKill(browser: Browser): Promise { const child = browser.process(); if (!child) throw new Error("A managed browser lost its owned process."); if (child.exitCode !== null || child.signalCode !== null) return false; + const tree = await descendantsToReap(child.pid); const exited = new Promise((resolve) => child.once("exit", () => resolve())); let timer: NodeJS.Timeout | undefined; let poll: NodeJS.Timeout | undefined; @@ -289,6 +296,7 @@ async function closedWithoutKill(browser: Browser): Promise { }, EXIT_POLL_MS); }); await Promise.race([exited, goneBeforeExitEvent, exitLimit]); + await reapChromeTree(tree).catch(() => undefined); return clean; } finally { clearTimeout(timer);