fix(cli): close every browser on SIGTERM and SIGINT via one launch owner - #4910

Review at 3c2d37d0. This is a comment, not an approval. I did my own pass and an independent Codex pass in a separate checkout, then checked each finding at this head. I also ran the real CLI (check, validate, snapshot) against a fixture project and signalled it at several points, with base (9a4b2a2d7) call sites as a control.

Blocker: CI is red because of this PR. Test (runtime) and Tests on windows-latest: engine-cli both fail on the same test:

FAIL src/capture/captureCompositionFrame.browserFallback.test.ts
  > openSettledCompositionPage Windows bundled-browser recovery > retries the known managed-shell crash once with system Chrome
Error: System Chrome fallback also failed: browser.process is not a function
  Caused by: TypeError: browser.process is not a function  (launch.ts:62)

That test's mocked launch() returns a browser with no process(), so launchManagedBrowser throws. It passes with base call sites (3/3) and fails at this head on both Linux and Windows. I reproduced it locally: the full CLI suite gives 4,098 passed, 1 failed, 13 skipped. Adding a process() stub to that mock should be all it needs. This is a third stale assertion beyond the two the last commit fixed.

What works (verified on Linux with real Chromium)

• SIGTERM no longer looks like a failure. At base, SIGTERM to a running check exits 1 with ✗ check_runtime_failure: Attempted to use detached Frame …. SIGTERM during the GPU probe is swallowed entirely: the probe falls back to software and check finishes with exit 0. At this head, all three phases (probe, launch, browser up) exit 143 within about 0.1 s.
• SIGINT exits 130 at both base and head.
• No Chrome is left behind. For every Chrome process group the CLI spawned, no member survived 2 s after the exit.
• All six direct launch sites in the CLI are migrated: beats, website capture, composition capture, layout, motion-shot and validate. The four probe sites call resolveManagedGpuMode.
• launch.process.test.ts passes 4/4 against real Chromium. Dropping the SIGINT or SIGTERM listener, re-enabling Puppeteer's SIGINT handler, or not registering the browser each fail it.

Should fix

1. SIGHUP still has no owner, so closing the terminal gives the old behaviour. Only handleSIGINT and handleSIGTERM are turned off, so Puppeteer's handleSIGHUP (default true) still runs void this.close().

   • SIGHUP while check has a browser up: exit 1 with ✗ check_runtime_failure: Attempted to use detached Frame ….
   • SIGHUP during the GPU probe, 2 of 2 runs: ◇ Check passed, exit 0. The hangup is swallowed, which is the same bug this PR fixes for SIGTERM.

   Render's cancellation scope already treats SIGHUP as a cancel signal. Adding SIGHUP to ownSignals (exit 129) and passing handleSIGHUP: false would make this owner match.

2. A cancelled run can still be reported as an ordinary failure. Closing the browser rejects whatever Puppeteer call the command is awaiting. The command's catch, failCommand, then reaches executeCli, which calls reportCommandFailure and finalizeCli({ exitCode: 1 }). finalized is first-wins, so when that happens first, the later requestCliExit(143) only forces the exit code. Telemetry keeps the exit-1 result plus a command_failure event, and the user sees a misleading error line. It happens often in practice:

   • validate with SIGTERM 0.5 s after Chrome starts: 2 of 5 runs printed ✗ Requesting main frame too early! or ✗ net::ERR_ABORTED …, then exited 143.
   • check with SIGTERM or SIGINT 1.2 s in: ✗ check_runtime_failure: Protocol error (Page.captureScreenshot): Target closed or Execution context was destroyed, then exited 143 or 130.

   Making the shutdown state visible to the root error boundary would fix it. For example, set a "cancelled by signal" flag before closing the browsers, and have executeCli skip failure reporting and finalize with 130/143 when it's set. Codex flagged the same race.

3. The tests don't cover the slower paths. These mutations still pass all 4 process tests:

   • removing the SIGKILL fallback;
   • removing the abort() of pending launches;
   • removing the "CLI is stopping" admission guard;
   • removing await exited;
   • replacing browser.close() with nothing.

   Related gaps:

   • Launch and probe cases may not catch Chrome mid-launch. The fixture writes launching or probing before it calls the launcher, and the test signals as soon as it reads that. So the "during launch" and "during probe" cases can run before any Chrome exists. The surviving abort mutation is consistent with that.

   • The fixture's exit path differs from the real one. The fixture's root requester SIGKILLs itself, and the test discards the {code, signal} it captures. So no test checks the real exit status or the finalizeCli path (item 2). cli.ts calls process.exit, which also runs Puppeteer's exit hook.

   • The "no orphans" check depends on which Chrome binary gets used. browserPids matches --user-data-dir in the command line.

     • With system Chromium, which the fixture picked up on this box, every helper process carries it.
     • With chrome-headless-shell, which the real CLI used here, only 1 of 7 processes does. The zygotes, GPU, utility and renderer processes don't.

     Chrome is spawned detached, so its process ID is also its group ID. Checking that group covers both cases.

   • A failing run leaks Chrome. When a process test fails, afterEach SIGKILLs only the fixture, and its detached Chrome keeps running. My four failing mutation runs left four Chromium trees (8 processes each) alive 20 minutes later; I killed them. Having the fixture write the browser's process ID, and having afterEach kill that process group, would stop that. Codex found this too.

   • Nothing stops a new direct launch. As a check, I reverted validate.ts to a direct puppeteer.default.launch(...) with the import still in place. check:cli-process-ownership and the 199 tests in the touched suites all still passed. The ownership script only looks at process.exit/exitCode. A rule against .launch( on Puppeteer outside browser/launch.ts would close that.

Lower priority

• The forced kill only kills Chrome's main process (launch.ts:93). Puppeteer kills the whole group on POSIX (kill(-pid)) and uses taskkill /T /F on Windows. On Linux, I SIGKILLed only the main process of a live browser, and all 7 group members were gone within 5 s, so I don't see orphans there. After that, Puppeteer's exit hook skips its group kill, because the main process no longer exists. Windows is unverified.
  • The 1 s grace is also skipped if the command is already in its own browser.close(). Puppeteer's close callback returns at once while a close is in progress, so Promise.race resolves and the SIGKILL fires immediately.
  • processTree.ts already has a group/tree kill you could reuse. Codex rated this blocking; I'd call it low on Linux, given the experiment.
• The "Before" description doesn't match what I measured. With base call sites, I didn't get any Chromium left behind after SIGTERM or SIGINT in any phase. Puppeteer's default handlers close it. The problems I measured at base are the SIGTERM ones above: a failure message with exit 1, or exit 0 when it lands during the probe. If you have a repro for the orphan (macOS, Bun, a particular command), please add it to the description. Otherwise I'd reword it to the exit-code and swallowed-signal fix. Your note that removing the two process.on lines leaves Chromium behind is a different case: that mutation also keeps handleSIGTERM: false, so nothing handles the signal at all.
• Two owners during the probe. The probe Chrome keeps Puppeteer's handlers, as the description says. SIGINT during the probe therefore exits through Puppeteer's synchronous process.exit(130), not through this owner. The exit code is the same, and the exit handler still flushes telemetry. "One SIGTERM/SIGINT handler" is true for managed browsers only.
• Not covered: benchmark. It runs the producer with no render cancellation scope, so it keeps the old behaviour. That's outside the stated scope, but worth a line next to "render" and "studio preview".

Nits

• launch.ts:63: a launched browser with no process() throws without closing it. That's unreachable with launch(), but void browser.close() before throwing is free.
• The handlers stay installed for the rest of the process. That's fine for a one-shot CLI, but it's worth a sentence in the doc comment for anyone who imports these helpers into a long-lived process.

— Somu

Second pass at 0724e393: delta re-check of 3c2d37d0→0724e393. That's one commit touching one test file; I didn't re-review the full PR. This is a comment, not an approval.

The blocker is fixed. The fake browser in captureCompositionFrame.browserFallback.test.ts now has a process: () => new EventEmitter() stub, so launchManagedBrowser no longer throws browser.process is not a function on the system-Chrome fallback path. That's the whole delta: +2 lines, test only, no production code changed. CI at this exact SHA is green (89 pass, 5 skipped), including both jobs that failed before: Test (runtime) in CI run 37041485309 and Tests on windows-latest: engine-cli in Windows render verification run 37041485315. I checked both runs' head SHA against 0724e393. I didn't re-run the suite locally.

Still open from my first pass (none of these is a blocker, and this commit doesn't touch them):

1. SIGHUP has no owner. handleSIGHUP is still Puppeteer's default, so closing the terminal while check runs exits 1 with a detached-frame error, or exits 0 if it lands during the GPU probe. Adding SIGHUP to ownSignals (129) and passing handleSIGHUP: false would fix it.
2. A cancelled run can still be reported as a command failure. failCommand can reach finalizeCli({exitCode: 1}) before requestCliExit(143), so telemetry records a command_failure and the user sees a misleading ✗ line.
3. Test gaps. In the first pass, 5 mutations survived the process tests: no SIGKILL fallback, no launch abort, no admission guard, no await exited, no close(). The "no orphans" check misses chrome-headless-shell helpers, and a failing test leaks its detached Chrome. Nothing stops a new direct puppeteer.launch(.

The "Before" line in the description still says SIGTERM orphaned Chromium. With the base call sites I couldn't reproduce an orphan. What I measured was exit 1 with an error, or a swallowed signal. A repro, or rewording it to match, would make the description accurate. The new "Known follow-ups" (the preview server's browser, and the GPU probe on SIGINT) don't cover items 1 or 2.

— Somu

Reviewed at 0724e393: browser/launch.ts, every call site it replaced, the fixture and process test, requestCliExit / registerRootExitRequester, and the other signal handlers in the CLI. Approving.

Every CLI browser goes through one owner. After this change, packages/cli/src has one direct puppeteer.launch(, inside launchManagedBrowser. The call sites that moved over:

• beats
• capture
• openSettledCompositionPage, which serves snapshot, compare, grade-compare and check
• layout, motionShot and validate

Each launch passes handleSIGINT/SIGTERM: false, so Puppeteer's own handlers can't race the CLI's.

What happens on a signal:

• The first SIGINT or SIGTERM sets shutdown. Any later launchManagedBrowser throws instead of starting a browser.
• Pending launches are aborted. The signal reaches @puppeteer/browsers' launch (BrowserLauncher.js:128), so the half-started process is killed and its launch settles.
• Each live browser gets a 1 s close(), then SIGKILL, and the code waits for its exit.
• Then requestCliExit(130 | 143) runs, which goes through finalizeCli and process.exit in cli.ts.
• Running ownSignals() in resolveManagedGpuMode before the probe closes the window where a signal during the GPU probe was swallowed. The test checks the "probing" case.
• A launch that resolves while shutdown is waiting is still closed. Its browsers.add runs before allSettled resumes, so the later snapshot of live browsers includes it.

Nothing else clashes. preview keeps its own SIGINT/SIGTERM handlers. Its thumbnail browser comes from the engine's acquireBrowser, not this owner, so preview never installs these handlers. That browser is the follow-up the body names. render has its own setup-worker signal handling and doesn't call this module.

Tests:

• CI's Test (runtime) job at this SHA ran launch.process.test.ts, 4 of 4 in 5.8 s.
• Locally, the capture and motionShot suites pass, 36 of 36.
• The process tests time out on my devbox because of a local Chrome issue, not this change. With the test's fresh home folder, ensureBrowser falls back to the system Chrome, which won't start under Puppeteer there at all: a plain puppeteer.launch outside this code times out after 25 s too.
• Run by hand with my normal home folder, the fixture reaches "ready" in 1 s. SIGTERM then records exit 143 and leaves no process with that profile.

Nits (not blocking):

1. A second Ctrl+C does nothing while shutdown runs (if (shutdown) return). That's bounded: aborted launches settle quickly, then there's 1 s of grace and a SIGKILL. So it's fine, but worth a comment, since a user's usual escape hatch is now a no-op.
2. Once the handlers are installed they stay for the rest of the command. So a Ctrl+C after every browser has closed, for example during the ffmpeg part of a capture, also exits 130 through finalizeCli. That seems right. Just noting that the handler's lifetime is the process, not the browser's.
3. The orphan check (browserPids) matches on the profile directory. That covers Chromium and its helpers on Linux only, and the suite skips everywhere else. The follow-ups already listed in the body cover the rest of the test gaps.

Verdict: APPROVE
Reasoning: Every CLI browser now launches through one owner, which aborts pending launches, kills live ones after a short grace, and exits 130 or 143 through the CLI's own exit path. The process test passes on Linux CI at this SHA. The remaining gaps (SIGHUP, the preview browser, the GPU probe's own browser, how a cancelled run is reported) are listed as follow-ups.

— Rames Jusso

Third pass at a6706044: delta re-check of 0724e393→a6706044. That's 2 commits touching only packages/cli/src/browser/launch.fixture.ts (+6/-3); I didn't re-review the full PR. This is a comment, not an approval.

No new findings. The race fix is correct. Both resolveManagedGpuMode and launchManagedBrowser are async, so their bodies run synchronously up to the first await. By the time the fixture writes probing or launching, ownSignals() has already installed the SIGINT/SIGTERM listeners (launch.ts:28-33, :40, :50). In the launch case, puppeteer.launch() has also started, and its abort controller is registered in pending. So a test that signals as soon as it reads the ready file can't land before the handler exists. That was the ENOENT on ready.exit: the signal killed the fixture with the default handler, before it could write the exit file.

This also partly closes the test gap I raised in my first pass. "launching" used to be written before any launch had started. Now the "during launch" case signals while a launch is actually pending, so the pending-launch abort() path is exercised. I didn't re-run my abort mutation to confirm the test now catches it.

Checked by reading, against the source at this SHA; nothing run locally. CI at this head was still in progress when I checked (72 pass, 20 pending, 1 skipped, nothing red). My open should-fixes from the first pass (SIGHUP, cancelled run reported as command_failure, the remaining test gaps) are still tracked under Known follow-ups and aren't touched here.

— Somu

Re-reviewed at a6706044. My previous approval was at 0724e393, which is an ancestor of this head. The only change since is packages/cli/src/browser/launch.fixture.ts (+6/−3, two commits). No product code changed. Approving.

Why the fix holds.

• Probe and launch. resolveManagedGpuMode and launchManagedBrowser are async functions, so everything before their first await runs during the call itself:
  • ownSignals() installs the SIGINT/SIGTERM listeners in both.
  • launchManagedBrowser also calls puppeteer.launch and registers its abort controller in pending.
• Marker order. The fixture now calls each one first and writes probing / launching afterwards.
• Result. The test can't send its signal before a listener exists. Before, it could: the marker was written first, so an early SIGTERM got Node's default handling. The process died without writing ready.exit, which is the ENOENT CI hit.
• Bonus. In the launching case the signal now arrives while a launch really is pending, so it exercises the abort-pending path rather than a launch that hasn't started yet.

Verdict: APPROVE
Reasoning: The delta is test-only. It orders the fixture's readiness markers after the synchronous parts of the calls that own the signals, which closes the race behind the ENOENT. The product code I approved at 0724e393 is unchanged.

— Rames Jusso