fix(browser): don't let puppeteer kill Chrome during the shutdown drain; v1.14.0

[harper-joseph]

The bug

Two of the three error signatures seen in the fleet's logs are the same root cause. Both traces sit on the SIGTERM path (handleTermination → Worker.shutdown → allSettled(inflight)):

"Attempted to use detached Frame 'C8D5…'"      at renderer.js:341 (page.evaluate)  → "failed to render page"
"Target.disposeBrowserContext: Failed to find context with id 9E29…"
                                               at ManagedBrowser.js:59 (context.close)  → "Failed to close context."

Puppeteer installs its own SIGTERM/SIGINT/SIGHUP listeners by default (handleSIGTERM = true in @puppeteer/browsers), and on SIGTERM the handler does void this.close() — killing Chrome the moment the signal lands, concurrently with our graceful drain. Neither defaultLaunchOptions() nor any consumer's browserLaunchOptions turned them off.

So on every rollout / scale-down: Chrome dies → in-flight renders throw detached Frame / Target closed and are logged as render failures → their pages emit close → the incognito context.close() hits a dead browser → the second error. RenderWorker.shutdown's drain — the whole reason in-flight renders are supposed to be posted back rather than dropped and re-queued — could never succeed. SIGINT never even reached the drain: puppeteer's handler calls process.exit(130).

Reproduced, and the new test fails without the fix (TargetCloseError: Protocol error (Runtime.callFunctionOn): Target closed).

The fix

• workerLaunchOptions(ownsSignals) (settings.ts) is now the single place the worker's launch options come from, and it turns puppeteer's three signal handlers off — applied last, so a consumer that supplies browserLaunchOptions wholesale can't silently reinstate the race. Left untouched when installSignalHandlers: false, since there puppeteer's handlers may be the only teardown.
• Nothing is orphaned: destroy() still closes the browser (with ManagedBrowser.close()'s SIGKILL fallback), and the forced-exit paths that can't await — a second termination signal, or the drain blowing through the deadline — now run worker.killBrowsersSync() via a new ErrorHandler.onForceExit.
• The ErrorHandler backstop is now drain + 3s instead of an equal 12s, so it can't fire in the same tick the drain gives up and cut off the graceful close.

Legibility of the next teardown

• Renders killed by our own shutdown → failures.shutdownAborted, logged at warn, not counted or logged as render failures. A rollout shouldn't read as a failure spike.
• A context close that fails while the browser is already disconnected → debug. Only a live-browser failure is a real leak.
• worker shutdown complete now carries drained: true|false, with a warn line when the deadline expired.

Separately: navigation.navigationTimeoutMs

The third error — Navigation timeout of 20000 ms exceeded at page.goto — is unrelated: 20s is renderBudgetMs, i.e. the page never reached domcontentloaded within the entire render budget. goto's timeout being the whole remaining budget has two costs: a stalled page holds a concurrency slot for the full budget, and when it does eventually load there's nothing left for settle (every settle wait clamps to what remains).

navigationTimeoutMs caps navigation on its own — 0 (default) keeps prior behavior. Navigation failures are also tagged with their phase (util/renderPhase.ts) and counted as failures.navTimeout, so the stats separate a stalled origin from slow in-browser settle instead of pooling both in failures.timeout.

A consumer that wants the fast-fail sets e.g. navigation: { navigationTimeoutMs: 8000 }; nothing changes for consumers that don't.

Testing

• packages/browser/test/shutdownSignals.test.ts: the launch-option contract (both ownership modes + the consumer-can't-reinstate case), the phase tag, and a real-browser test that an in-flight page.evaluate survives a SIGTERM. That last one fails with TargetCloseError if the flags are removed.
• config.test.ts: navigationTimeoutMs default, 0 allowed, negative/non-numeric rejected.
• Full suite green: 87 tests, 0 failures. npm run lint clean; format:check reports only the pre-existing unformatted CLAUDE.md (untouched here on purpose — it holds a block kept in sync across repos).

Notes for the reviewer

• Version: v1.14.0. Note PR feat(browser): opt-in OpenTelemetry OTLP metrics export; v1.13.0 #30 also claims v1.13.0 in its title, which is already released — whichever lands second needs a re-bump.
• Drain is 12s and the backstop 15s, both comfortably under a 30s termination grace period. Worth a glance at the deployment's terminationGracePeriodSeconds before rollout.
• Deliberately unchanged: a shutdown-aborted render still POSTs its (empty) result. The plugin's processJobResult files that under "Unknown prerender error" and leaves a target-backed job to retry after its lease expires — no target deletion, no reschedule penalty — so this is noise, not damage, and fixing it belongs with a "don't post abandoned results" change.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a new navigationTimeoutMs configuration option to cap initial page navigation and fail fast, preventing stalled pages from consuming the entire render budget. It also refactors the shutdown process by disabling Puppeteer's default signal handlers when the worker owns signals, implementing a synchronous last-resort cleanup (killBrowsersSync) for forced exits, and adding detailed tracking for navigation timeouts and shutdown-aborted renders. Feedback suggests using a WeakMap instead of directly mutating caught exception objects to store error metadata, avoiding potential TypeErrors if the exception object is frozen.

[gemini-code-assist]

Mutating caught exception objects directly by adding arbitrary properties (err[PHASE_KEY] = phase) can throw a TypeError in strict mode if the exception object is frozen or non-extensible (e.g., via Object.freeze or Object.preventExtensions). Using a WeakMap to store metadata about errors is a much safer, non-intrusive pattern that completely avoids mutating the error object.

const phaseMap = new WeakMap<object, RenderPhase>();

/** Tag err with the phase it came from and return it (so it can be re-thrown inline). */
export function markRenderPhase<E>(err: E, phase: RenderPhase): E {
	if (err !== null && typeof err === 'object') {
		phaseMap.set(err, phase);
	}
	return err;
}

/** The phase tagged on err, or undefined if it carries none. */
export function renderPhaseOf(err: unknown): RenderPhase | undefined {
	if (err === null || typeof err !== 'object') return undefined;
	return phaseMap.get(err);
}

[harper-joseph]

Second cause of Failed to close context. — the retirement reaper (581b346)

@joseph pointed out this error also fires away from worker shutdown, when a retired browser is closed. That one wasn't just noisy logging — it was a real bug, and the reason it's frequent:

closeRetiredBrowsers() closed a retired browser when activePages === 0 || browser.jobRefs === 0. Those two counters don't reach zero together. render() dropped its job ref at the end of the render, then awaited the result POST and the page close:

job.attemptEnded(error, content);
browser.jobRefs--;                                  // ← ref gone here
…
const closePromise = page ? browser.closePage(page) : Promise.resolve();
const [posted] = await Promise.all([sendPromise, closePromise]);   // ← page still open here

So the entire result-POST window sits at jobRefs === 0 with pages still open, and the either-or condition made that enough to close the browser. Any 10s cleanup tick landing in that window closed the browser out from under those pages; their close handlers then tried to dispose contexts whose browser was already gone. With N renders finishing together on a retired browser, that's N of these errors per retirement.

Changes:

• jobRefs is released in a finally after the POST and page close, so jobRefs === 0 actually means "nothing is touching this browser".
• The reaper requires both counters at zero, and skips a browser whose close() is already in flight (previously a tick could re-enter close() every 10s until it resolved).
• Force-close backstop: retired browsers are closed regardless after RETIRED_BROWSER_MAX_MS (120s, ≫ a render + its POST), so if a counter ever gets stuck the process is reclaimed rather than leaked. It logs a warn — a stuck counter is a bug worth seeing, not something to swallow. retiredBrowsers became a Map<ManagedBrowser, retiredAt> to carry that deadline.
• The log line now classifies by cause instead of by browser.connected (which is still true early in a browser.close(), so it wouldn't have caught this path): a closing/disconnected browser, or an error saying the context/target/session/connection is already gone, is debug — nothing leaked. A live browser failing to dispose a context that still exists stays an error, since that context survives until the browser exits.

Tests (test/browserRetirement.test.ts, 6 cases): not closed with an open page at zero refs (this one fails against the old ||), not closed with refs and no pages, reaped when both are zero, force-closed past the deadline, no re-entry while closing, and the error classifier against the exact production message shapes.

Full suite: 93 tests, 0 failures. Lint and format clean.

Worth noting the two causes are independent: the SIGTERM fix stops the rollout burst, this stops the steady-state trickle at every browser retirement (browserExpirationThreshold, default 200 pages).