From 4416a41cba0a0d711185bb90166dbebc6427ecb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 27 Oct 2026 13:29:56 +0000 Subject: [PATCH] fix(cli): every managed browser closes within a second even if Chrome never exits (#6553) * fix(cli): every managed browser closes within a second even if Chrome never exits Commands called browser.close() directly, which waits for the Chrome process to exit without a limit. A headless shell that acknowledged Browser.close and then stayed alive left the CLI hanging after its work was done. The managed browser's close() is now the bounded close that kills a browser that has not exited. * test(cli): browser stand-ins carry the close() a real browser has --- .../src/browser/launch.boundedClose.test.ts | 20 +++++++++++++++++++ .../cli/src/browser/launch.noListen.test.ts | 2 +- .../cli/src/browser/launch.pipeEof.test.ts | 2 +- packages/cli/src/browser/launch.ts | 11 +++++++++- .../cli/src/browser/sandboxDenied.test.ts | 3 ++- .../launchCaptureBrowser.signal.test.ts | 14 ++++++------- 6 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 packages/cli/src/browser/launch.boundedClose.test.ts diff --git a/packages/cli/src/browser/launch.boundedClose.test.ts b/packages/cli/src/browser/launch.boundedClose.test.ts new file mode 100644 index 0000000000..7607926b27 --- /dev/null +++ b/packages/cli/src/browser/launch.boundedClose.test.ts @@ -0,0 +1,20 @@ +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import type { Browser, LaunchOptions, PuppeteerNode } from "puppeteer-core"; +import { expect, it } from "vitest"; +import { launchManagedBrowser } from "./launch.js"; + +it("kills a browser that acknowledged close but never exits, so every caller's close() returns", async () => { + const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 60000)"], { stdio: "pipe" }); + const browser = { process: () => child, close: () => once(child, "exit") } as unknown as Browser; + const puppeteer = { + launch: async (_options: LaunchOptions) => browser, + } as unknown as PuppeteerNode; + try { + const launched = await launchManagedBrowser(puppeteer, { headless: true }, { diagnose: false }); + await launched.close(); + expect(child.signalCode).toBe("SIGKILL"); + } finally { + child.kill("SIGKILL"); + } +}); diff --git a/packages/cli/src/browser/launch.noListen.test.ts b/packages/cli/src/browser/launch.noListen.test.ts index 81e5637881..31a3d2b60d 100644 --- a/packages/cli/src/browser/launch.noListen.test.ts +++ b/packages/cli/src/browser/launch.noListen.test.ts @@ -15,7 +15,7 @@ vi.mock("./gpuPolicy.js", async (actual) => ({ const puppeteer = { launch: async (options: LaunchOptions) => { probe.options.push(options); - return { process: () => new EventEmitter() }; + return { process: () => new EventEmitter(), close: async () => {} }; }, } as unknown as PuppeteerNode; await hooks.launch(puppeteer, { headless: true, args: ["--headless"] }); diff --git a/packages/cli/src/browser/launch.pipeEof.test.ts b/packages/cli/src/browser/launch.pipeEof.test.ts index bb8ab2c8c6..a2d2523215 100644 --- a/packages/cli/src/browser/launch.pipeEof.test.ts +++ b/packages/cli/src/browser/launch.pipeEof.test.ts @@ -15,7 +15,7 @@ it("closes the browser's pipes when it exits, so an orphaned helper cannot hold stdio: ["pipe", "pipe", "pipe", "pipe", "pipe"], }); const puppeteer = { - launch: async (_options: LaunchOptions) => ({ process: () => child }), + launch: async (_options: LaunchOptions) => ({ process: () => child, close: async () => {} }), } as unknown as PuppeteerNode; await launchManagedBrowser(puppeteer, { headless: true }, { diagnose: false }); await expect diff --git a/packages/cli/src/browser/launch.ts b/packages/cli/src/browser/launch.ts index 22076656c9..e9469dd60f 100644 --- a/packages/cli/src/browser/launch.ts +++ b/packages/cli/src/browser/launch.ts @@ -26,6 +26,7 @@ export class BrowserCleanupError extends AggregateError {} const browsers = new Set(); const closing = new WeakMap>(); const watchdogs = new WeakMap(); +const gracefulClose = new WeakMap Promise>(); const pending = new Map, AbortController>(); const diagnosing = new Set(); const sessionCleanups = new Set<() => Promise>(); @@ -193,6 +194,8 @@ async function launchOwnedBrowser( browsers.add(browser); const watchdog = watchChromeTree(child, launchedAt); if (watchdog) watchdogs.set(browser, watchdog); + gracefulClose.set(browser, browser.close.bind(browser)); + browser.close = () => closeManagedBrowser(browser); child.once("exit", () => { browsers.delete(browser); destroyBrowserPipes(child); @@ -273,6 +276,10 @@ async function descendantsToReap(pid: number | undefined) { return chromeDescendants(pid).catch(() => []); } +function closeGracefully(browser: Browser): Promise { + return (gracefulClose.get(browser) ?? (() => browser.close()))(); +} + async function closedWithoutKill(browser: Browser): Promise { const child = browser.process(); if (!child) throw new Error("A managed browser lost its owned process."); @@ -286,7 +293,9 @@ async function closedWithoutKill(browser: Browser): Promise { }); try { await Promise.race([ - browser.close().catch((error: unknown) => console.warn("Browser close failed:", error)), + closeGracefully(browser).catch((error: unknown) => + console.warn("Browser close failed:", error), + ), grace, ]); const clean = child.exitCode !== null || child.signalCode !== null; diff --git a/packages/cli/src/browser/sandboxDenied.test.ts b/packages/cli/src/browser/sandboxDenied.test.ts index 64f5c2a05b..2689eb96f4 100644 --- a/packages/cli/src/browser/sandboxDenied.test.ts +++ b/packages/cli/src/browser/sandboxDenied.test.ts @@ -37,7 +37,8 @@ describe("a browser launch a sandbox refuses", () => { }); }); -const fakeBrowser = () => ({ process: () => new EventEmitter() }) as unknown as Browser; +const fakeBrowser = () => + ({ process: () => new EventEmitter(), close: async () => {} }) as unknown as Browser; function recordingPuppeteer(failures: (string | Error)[]) { const launches: LaunchOptions[] = []; diff --git a/packages/cli/src/capture/launchCaptureBrowser.signal.test.ts b/packages/cli/src/capture/launchCaptureBrowser.signal.test.ts index acfb8e9975..07ffe27602 100644 --- a/packages/cli/src/capture/launchCaptureBrowser.signal.test.ts +++ b/packages/cli/src/capture/launchCaptureBrowser.signal.test.ts @@ -33,14 +33,12 @@ it.each(["SIGINT", "SIGTERM"] as const)( exitCode: null as number | null, signalCode: null, }); - const browser = Object.assign(new EventEmitter(), { - process: () => child, - close: vi.fn(async () => { - child.exitCode = 0; - child.emit("exit", 0); - browser.emit("disconnected"); - }), + const close = vi.fn(async () => { + child.exitCode = 0; + child.emit("exit", 0); + browser.emit("disconnected"); }); + const browser = Object.assign(new EventEmitter(), { process: () => child, close }); mocks.launch.mockResolvedValue(browser); mocks.proxyClose.mockResolvedValue(undefined); const { launchCaptureBrowser } = await import("./launchCaptureBrowser.js"); @@ -62,6 +60,6 @@ it.each(["SIGINT", "SIGTERM"] as const)( expect(await exit).toEqual({ code: signal === "SIGINT" ? 130 : 143, profileExists: false }); await session.close(); await session.close(); - expect(browser.close).toHaveBeenCalledOnce(); + expect(close).toHaveBeenCalledOnce(); }, );