diff --git a/package-lock.json b/package-lock.json index 70b9d70..60e41e6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8967,7 +8967,7 @@ }, "packages/browser": { "name": "@harperfast/prerender-browser", - "version": "1.13.0", + "version": "1.14.0", "license": "Apache-2.0", "dependencies": { "mqtt": "^5.10.4", diff --git a/packages/browser/README.md b/packages/browser/README.md index b79c424..5d86d86 100644 --- a/packages/browser/README.md +++ b/packages/browser/README.md @@ -51,23 +51,23 @@ cache, and starts the worker loop; it resolves once the cache index is built. It Only `harper` is required; everything else has a default. -| Option | Default | Purpose | -| ---------------------------- | ------------------------------------------------- | ----------------------------------------------------------------- | -| `harper` | _(required)_ | `{ mqttOrigin, user, pass, workerId }` — connection + identity | -| `queuePort` | `9926` | Port of the plugin's render-queue HTTP API | -| `bypass` | `{ header: x-harper-renderer-bypass, token: '' }` | Shared origin-bypass header/token (match the plugin) | -| `config` | built-in defaults | Rendering config (deep-partial object _or_ JSON file path) | -| `concurrency` | ~half the CPUs | Max concurrent page renders | -| `rps` | `8` | Max render starts per second | -| `jobClaimLimit` | `concurrency * 2` | Jobs claimed per batch | -| `browserExpirationThreshold` | `200` | Pages a browser renders before being retired | -| `incognitoPages` | `true` | Render each page in a fresh incognito context | -| `contentEncoding` | `gzip` | Encoding used when posting rendered HTML back | -| `chromeArgs` | hardened headless set | Chrome launch flags | -| `browserLaunchOptions` | built from `chromeArgs` | Full Puppeteer launch options (overrides `chromeArgs`) | -| `resourceCache` | enabled, ~8 GB in tmp | On-disk shared sub-resource cache (`enabled`/`dir`/limits) | -| `renderer` | the default renderer | Custom renderer (see below) | -| `installSignalHandlers` | `true` | Install uncaught/SIGTERM handlers; set `false` to own the process | +| Option | Default | Purpose | +| ---------------------------- | ------------------------------------------------- | ------------------------------------------------------------------------------------------- | +| `harper` | _(required)_ | `{ mqttOrigin, user, pass, workerId }` — connection + identity | +| `queuePort` | `9926` | Port of the plugin's render-queue HTTP API | +| `bypass` | `{ header: x-harper-renderer-bypass, token: '' }` | Shared origin-bypass header/token (match the plugin) | +| `config` | built-in defaults | Rendering config (deep-partial object _or_ JSON file path) | +| `concurrency` | ~half the CPUs | Max concurrent page renders | +| `rps` | `8` | Max render starts per second | +| `jobClaimLimit` | `concurrency * 2` | Jobs claimed per batch | +| `browserExpirationThreshold` | `200` | Pages a browser renders before being retired | +| `incognitoPages` | `true` | Render each page in a fresh incognito context | +| `contentEncoding` | `gzip` | Encoding used when posting rendered HTML back | +| `chromeArgs` | hardened headless set | Chrome launch flags | +| `browserLaunchOptions` | built from `chromeArgs` | Full Puppeteer launch options (overrides `chromeArgs`) | +| `resourceCache` | enabled, ~8 GB in tmp | On-disk shared sub-resource cache (`enabled`/`dir`/limits) | +| `renderer` | the default renderer | Custom renderer (see below) | +| `installSignalHandlers` | `true` | Own SIGTERM/SIGINT (drain in-flight renders, then close Chrome); `false` to own the process | ## Rendering config @@ -88,6 +88,10 @@ include what you change: "navigation": { "waitUntil": "domcontentloaded", // 'load' | 'domcontentloaded' | 'networkidle0' | 'networkidle2' "renderBudgetMs": 20000, + // Cap on the initial navigation alone. 0 (default) lets it use the whole renderBudgetMs, so a + // page that stalls before `waitUntil` holds a concurrency slot for the full budget and leaves + // nothing for settle. Set it to fail a stalled navigation fast (counted as failures.navTimeout). + "navigationTimeoutMs": 0, "networkIdleMs": 300, "networkIdleTimeoutMs": 1000, }, diff --git a/packages/browser/package.json b/packages/browser/package.json index 179cc92..eb5beea 100644 --- a/packages/browser/package.json +++ b/packages/browser/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender-browser", - "version": "1.13.0", + "version": "1.14.0", "type": "module", "description": "Headless-browser render library for Harper Prerender: claims render jobs from the @harperfast/prerender queue, renders pages in headless Chrome (Puppeteer), and posts the HTML back. Embedded by a render service and configured entirely via startWorker() options.", "keywords": [ diff --git a/packages/browser/src/ManagedBrowser.ts b/packages/browser/src/ManagedBrowser.ts index 7dd7d43..667d1b8 100644 --- a/packages/browser/src/ManagedBrowser.ts +++ b/packages/browser/src/ManagedBrowser.ts @@ -11,11 +11,39 @@ type ManagedBrowserConfig = ManagedBrowserOptions & { puppeteerLaunchOptions?: LaunchOptions; }; +/** + * All of these mean the browser context is ALREADY gone (its browser is closing, its target died, + * or the connection is down) — so a failed dispose leaked nothing and isn't worth an error. The + * case that IS worth one: a live browser refusing to dispose a context that still exists, since + * that context then survives until the browser exits. + */ +export const contextAlreadyGone = (err: unknown): boolean => { + const message = err instanceof Error ? err.message : String(err); + return ( + message.includes('Failed to find context') || + message.includes('Target closed') || + message.includes('Session closed') || + message.includes('Connection closed') + ); +}; + export default class ManagedBrowser { maxActivePages: number; browser: Browser; + /** + * Set once teardown starts (close/kill), so page-close handlers can tell "the browser is going + * away under me" from a genuine failure. Also stops a caller from re-entering close() while the + * first one is still in flight. + */ + closing = false; + + /** + * Renders currently using this browser — incremented when a job starts on it and decremented + * only once that job's page is closed and its result posted, so `jobRefs === 0` really means + * "nothing is touching this browser" (what the retirement cleanup keys on). + */ jobRefs: number = 0; activePages: number = 0; @@ -80,7 +108,15 @@ export default class ManagedBrowser { try { await context.close(); } catch (err: any) { - logger.error({ err }, 'Failed to close context.'); + // A context dies with its browser, so teardown makes this expected: the page + // 'close' event that got us here can BE the browser closing (worker shutdown, or + // a retired browser being reaped). Only a live browser failing to dispose a + // context that still exists is a real leak. + if (this.closing || !this.browser.connected || contextAlreadyGone(err)) { + logger.debug({ err }, 'browser context already gone at page close'); + } else { + logger.error({ err }, 'Failed to close context.'); + } } } this.activePages--; @@ -98,6 +134,7 @@ export default class ManagedBrowser { } async close() { + this.closing = true; try { await this.browser.close(); } catch (err) { @@ -113,7 +150,22 @@ export default class ManagedBrowser { fallback.unref(); } + /** + * SIGKILL Chrome without awaiting anything, for use immediately before `process.exit()` + * (see RenderWorker.killBrowsersSync). `close()`/`kill()` are the graceful paths; this one + * only guarantees the process isn't left behind. + */ + killSync() { + this.closing = true; + try { + this.browser.process()?.kill('SIGKILL'); + } catch { + // Already exited, or we can't signal it — nothing left to do on the way out. + } + } + async kill() { + this.closing = true; const process = this.browser.process(); if (!process) { diff --git a/packages/browser/src/Worker.ts b/packages/browser/src/Worker.ts index e37d465..5c20d88 100644 --- a/packages/browser/src/Worker.ts +++ b/packages/browser/src/Worker.ts @@ -8,9 +8,14 @@ import { noop } from './util/noop.js'; import { getResourceCache } from './ResourceCache.js'; import { settings } from './settings.js'; import { CpuSampler } from './util/cpu.js'; +import { renderPhaseOf } from './util/renderPhase.js'; export type Renderer = (page: Page, job: RenderJob) => Promise; +// How long a retired browser may sit un-reaped before it's closed regardless of its ref counts. +// Comfortably longer than a render + its result POST, so it only ever fires on stuck bookkeeping. +const RETIRED_BROWSER_MAX_MS = 120000; + type RenderWorkerConfig = { /** * The max number of concurrent page renders @@ -43,7 +48,8 @@ export default class RenderWorker { browserPromise: Promise | null = null; - retiredBrowsers: Set = new Set(); + /** Retired browsers awaiting reaping, each mapped to when it was retired (see closeRetiredBrowsers). */ + retiredBrowsers: Map = new Map(); browserLaunchOptions?: LaunchOptions; @@ -73,7 +79,20 @@ export default class RenderWorker { completed: 0, succeeded: 0, emptyContent: 0, - failures: { timeout: 0, protocol: 0, tooManyRedirects: 0, getPageFailed: 0, other: 0 }, + failures: { + timeout: 0, + // Navigation never reached `waitUntil` (slow origin / starved renderer) — distinct + // from `timeout`, which is a settle-phase or protocol-level timeout. + navTimeout: 0, + protocol: 0, + tooManyRedirects: 0, + getPageFailed: 0, + // Renders killed by our own teardown (drain deadline / browser gone during + // shutdown). Not a render regression — kept out of the other buckets so a + // rollout doesn't read as a failure spike. + shutdownAborted: 0, + other: 0, + }, expiredSkipped: 0, concurrencyBlocked: 0, rpsDelayed: 0, @@ -302,11 +321,34 @@ export default class RenderWorker { // the resulting abort rejection. const ac = new AbortController(); const deadline = setTimeout(deadlineMs, undefined, { signal: ac.signal }).catch(() => {}); - await Promise.race([Promise.allSettled([...this.inflight]), deadline]); + const DRAINED = Symbol('drained'); + const outcome = await Promise.race([Promise.allSettled([...this.inflight]).then(() => DRAINED), deadline]); ac.abort(); + // Whether in-flight renders finished (their results are posted) or were abandoned to the + // deadline (re-rendered later by whoever re-claims the jobs) — the difference matters when + // reading a rollout's logs, so say which happened. + const drained = outcome === DRAINED; + if (!drained) { + logger.warn({ deadlineMs, inflight: this.inflight.size }, 'shutdown drain deadline expired — abandoning renders'); + } + await this.destroy(); - logger.info('worker shutdown complete'); + logger.info({ drained }, 'worker shutdown complete'); + } + + /** + * Best-effort SYNCHRONOUS teardown of every Chrome process, for the forced-exit paths where + * there's no time to await `destroy()` (a second termination signal, or the shutdown deadline + * backstop). Puppeteer's own signal handlers are disabled when we own the drain, so without + * this a forced exit would orphan Chrome. A browser still mid-launch has no PID yet and can't + * be reached from here — the same gap those forced paths already accept. + */ + killBrowsersSync() { + const browsers = [...this.retiredBrowsers.keys(), ...(this.browser ? [this.browser] : [])]; + for (const browser of browsers) { + browser.killSync(); + } } // Async so callers can AWAIT it before process.exit() — otherwise the event loop dies before @@ -328,9 +370,9 @@ export default class RenderWorker { if (this.browserPromise) { closing.push(this.browserPromise.then((b) => b.close().catch(noop)).catch(noop)); } - this.retiredBrowsers.forEach((browser) => { + for (const browser of this.retiredBrowsers.keys()) { closing.push(browser.close().catch(noop)); - }); + } this.browser = null; this.retiredBrowsers.clear(); @@ -338,13 +380,32 @@ export default class RenderWorker { await Promise.all(closing); } + /** + * Reap retired browsers once they're genuinely idle — BOTH counters at zero. Either-or (the + * previous condition) closed a browser out from under open pages: a render drops its job ref + * before its result POST completes, so `jobRefs === 0` while pages are still open is normal, + * and closing there killed those pages mid-flight — which surfaced as "Failed to close + * context" from their close handlers. + * + * The backstop is a deadline, not a looser condition: if a counter is somehow stuck (a page + * whose 'close' never fires), force the close after RETIRED_BROWSER_MAX_MS rather than leak a + * Chrome process forever — and say so, because a stuck counter is a bug worth seeing. + */ closeRetiredBrowsers() { - for (const browser of this.retiredBrowsers) { - if (browser.activePages === 0 || browser.jobRefs === 0) { - browser.close().then(() => { - this.retiredBrowsers.delete(browser); - }); + for (const [browser, retiredAt] of this.retiredBrowsers) { + if (browser.closing) continue; // already being reaped by an earlier tick + const idle = browser.activePages === 0 && browser.jobRefs === 0; + const expired = Date.now() - retiredAt >= RETIRED_BROWSER_MAX_MS; + if (!idle && !expired) continue; + if (!idle) { + logger.warn( + { activePages: browser.activePages, jobRefs: browser.jobRefs, retiredMs: Date.now() - retiredAt }, + 'retired browser never went idle — closing anyway' + ); } + browser.close().then(() => { + this.retiredBrowsers.delete(browser); + }); } } @@ -352,7 +413,7 @@ export default class RenderWorker { if (this.retiredBrowsers.has(browser)) { return; } - this.retiredBrowsers.add(browser); + this.retiredBrowsers.set(browser, Date.now()); this.stats.browserRetirements++; this.browser = null; } @@ -381,9 +442,20 @@ export default class RenderWorker { try { content = await this.renderFn(page, job); } catch (e) { - if (e instanceof TimeoutError) { + if (this.shuttingDown) { + // We tore the browser down under this render (drain deadline, or a browser that + // went away mid-drain): "detached Frame" / "Target closed" / a failed context + // dispose. Our own doing, so it's a warning, not a render failure — and no point + // retiring a browser that's already being closed. + this.stats.failures.shutdownAborted++; + logger.warn({ url: job.url, phase: renderPhaseOf(e), err: e }, 'render aborted by worker shutdown'); + } else if (e instanceof TimeoutError) { this.retireBrowser(browser); - this.stats.failures.timeout++; + if (renderPhaseOf(e) === 'navigation') { + this.stats.failures.navTimeout++; + } else { + this.stats.failures.timeout++; + } } else if (e instanceof ProtocolError) { this.retireBrowser(browser); this.stats.failures.protocol++; @@ -393,13 +465,14 @@ export default class RenderWorker { } else { this.stats.failures.other++; } - logger.error({ url: job.url, err: e }, 'failed to render page'); + if (!this.shuttingDown) { + logger.error({ url: job.url, phase: renderPhaseOf(e), err: e }, 'failed to render page'); + } error = e as Error; } } job.attemptEnded(error, content); - browser.jobRefs--; // Per-interval outcome + latency accounting (drained by logStats). this.stats.completed++; @@ -427,8 +500,15 @@ export default class RenderWorker { return false; }); const closePromise = page ? browser.closePage(page) : Promise.resolve(); - const [posted] = await Promise.all([sendPromise, closePromise]); - if (!posted) this.stats.resultPostFailures++; + try { + const [posted] = await Promise.all([sendPromise, closePromise]); + if (!posted) this.stats.resultPostFailures++; + } finally { + // Released only now — not when the render finished. A retired browser is reaped once its + // refs hit zero, so dropping the ref before the page is closed let the reaper close the + // browser under an open page. + browser.jobRefs--; + } } async getBrowser(): Promise { diff --git a/packages/browser/src/config.ts b/packages/browser/src/config.ts index 0b88ad6..3d5447b 100644 --- a/packages/browser/src/config.ts +++ b/packages/browser/src/config.ts @@ -51,6 +51,18 @@ export type NavigationConfig = { waitUntil: PuppeteerLifeCycleEvent | PuppeteerLifeCycleEvent[]; /** Default per-render time budget (ms) used when a job doesn't specify one. */ renderBudgetMs: number; + /** + * Cap (ms) on the initial navigation alone — the wait for `waitUntil`. Without it the + * `goto` timeout is the *whole* remaining render budget, so a page that stalls before + * `waitUntil` burns a concurrency slot for the full budget and, when it does eventually + * load, leaves nothing for the settle phase (the waits below all clamp to what's left). + * A sub-budget fails a stalled navigation fast, frees the slot, and separates the two + * causes in the worker's stats (`failures.navTimeout` vs `failures.timeout`). + * + * `0` disables the cap (navigation may use the entire budget) — the default, preserving + * prior behavior. Values above the remaining budget have no effect; the smaller wins. + */ + navigationTimeoutMs: number; /** Idle window for the post-navigation/scroll network-idle waits (ms). */ networkIdleMs: number; /** Max time to wait for network idle (ms). */ @@ -219,6 +231,7 @@ export const defaultConfig = (): PrerenderConfig => ({ navigation: { waitUntil: 'domcontentloaded', renderBudgetMs: 20000, + navigationTimeoutMs: 0, networkIdleMs: 300, networkIdleTimeoutMs: 1000, domStableMs: 0, @@ -289,9 +302,9 @@ const validate = (config: PrerenderConfig): PrerenderConfig => { throw new Error(`prerender config: navigation.${field} must be a positive number`); } } - // domStableMs may be 0 (disabled) and domStableTolerance 0 (exact match), so these - // only have to be non-negative numbers. - for (const field of ['domStableMs', 'domStableTolerance'] as const) { + // domStableMs may be 0 (disabled), domStableTolerance 0 (exact match), and + // navigationTimeoutMs 0 (no navigation sub-cap), so these only have to be non-negative. + for (const field of ['domStableMs', 'domStableTolerance', 'navigationTimeoutMs'] as const) { if (typeof config.navigation[field] !== 'number' || config.navigation[field] < 0) { throw new Error(`prerender config: navigation.${field} must be a non-negative number`); } diff --git a/packages/browser/src/errorHandler.ts b/packages/browser/src/errorHandler.ts index 34b71e0..cf714e3 100644 --- a/packages/browser/src/errorHandler.ts +++ b/packages/browser/src/errorHandler.ts @@ -5,19 +5,38 @@ export type ErrorHandlerOptions = { onTerminate?: () => Promise | void; /** Hard cap on the graceful drain before forcing exit(0). Default 12s. */ shutdownDeadlineMs?: number; + /** + * Synchronous last-resort cleanup run immediately before a forced `process.exit()` — a + * second termination signal, or the drain blowing through `shutdownDeadlineMs`. Neither + * path can await `onTerminate`, so this is where anything that would otherwise be orphaned + * (the Chrome processes) gets killed. Must not be async: nothing after it is awaited. + */ + onForceExit?: () => void; }; export class ErrorHandler { private onTerminate?: () => Promise | void; + private onForceExit?: () => void; private shutdownDeadlineMs: number; private terminating = false; constructor(options: ErrorHandlerOptions = {}) { this.onTerminate = options.onTerminate; + this.onForceExit = options.onForceExit; this.shutdownDeadlineMs = options.shutdownDeadlineMs ?? 12000; this.setupGlobalHandlers(); } + /** Run the sync last-resort cleanup; never let it prevent the exit it precedes. */ + private forceExit(code: number): never { + try { + this.onForceExit?.(); + } catch (err) { + logger.error({ err }, 'error during forced-exit cleanup'); + } + process.exit(code); + } + private setupGlobalHandlers() { process.on('uncaughtException', this.handleUncaughtException.bind(this)); process.on('unhandledRejection', this.handleUnhandledRejection.bind(this)); @@ -57,14 +76,14 @@ export class ErrorHandler { // A second signal (e.g. an impatient Ctrl+C) forces an immediate, unclean exit // instead of waiting out the drain. logger.warn({ signal }, 'Second termination signal — forcing exit'); - process.exit(1); + this.forceExit(1); } this.terminating = true; logger.info({ signal }, 'Termination signal received'); // Hard backstop: if the drain hangs, exit before the supervisor SIGKILLs us — with a // NON-zero code so the orchestrator sees an unclean/timed-out shutdown, not a clean one. - const backstop = setTimeout(() => process.exit(1), this.shutdownDeadlineMs); + const backstop = setTimeout(() => this.forceExit(1), this.shutdownDeadlineMs); backstop.unref(); let exitCode = 0; try { diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index e87c5dc..0d2e983 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -20,11 +20,16 @@ import RenderWorker, { Renderer } from './Worker.js'; import defaultRenderer from './renderer.js'; import logger from './util/Logger.js'; -import { applySettings, settings, defaultLaunchOptions } from './settings.js'; +import { applySettings, settings, workerLaunchOptions } from './settings.js'; import type { BrowserOptions } from './settings.js'; import { initResourceCache } from './ResourceCache.js'; import { ErrorHandler } from './errorHandler.js'; +// How long in-flight renders get to finish after a termination signal before they're abandoned. +// Must stay comfortably under the supervisor's termination grace period (Kubernetes' default is +// 30s) so the drain + browser close complete before SIGKILL. +const SHUTDOWN_DRAIN_MS = 12000; + export type StartWorkerOptions = BrowserOptions & { /** Renderer to use instead of the built-in default. */ renderer?: Renderer; @@ -60,18 +65,27 @@ export async function startWorker(options: StartWorkerOptions): Promise worker.shutdown() }); + if (ownsSignals) { + new ErrorHandler({ + // The forced-exit backstop must land strictly AFTER the drain deadline: with both at + // the same value it can fire in the same tick the drain gives up, cutting off the + // graceful browser close that follows it. + shutdownDeadlineMs: SHUTDOWN_DRAIN_MS + 3000, + onTerminate: () => worker.shutdown(SHUTDOWN_DRAIN_MS), + onForceExit: () => worker.killBrowsersSync(), + }); } worker.run(); diff --git a/packages/browser/src/renderer.ts b/packages/browser/src/renderer.ts index d1e6134..1b055bc 100644 --- a/packages/browser/src/renderer.ts +++ b/packages/browser/src/renderer.ts @@ -4,6 +4,7 @@ import { settings } from './settings.js'; import { CACHE_REPLAY_HEADER, getResourceCache } from './ResourceCache.js'; import type { PostProcessConfig } from './config.js'; import { canonicalizeUrl, canonicalAllowsIndex } from './util/url.js'; +import { markRenderPhase } from './util/renderPhase.js'; const noop = () => {}; @@ -174,11 +175,23 @@ const renderer: Renderer = async (page, job) => { const remainingTimer = new RemainingTimer(job.renderBudget || config.navigation.renderBudgetMs); navStart = Date.now(); - const finalRes = await page.goto(navigationUrl.href, { - waitUntil: config.navigation.waitUntil, - timeout: remainingTimer.remaining, - signal: ac.signal, - }); + // Navigation gets the smaller of its own sub-cap and what's left of the render budget, so a + // stalled origin fails fast instead of consuming the whole budget (see navigationTimeoutMs). + const navigationTimeout = config.navigation.navigationTimeoutMs + ? Math.min(remainingTimer.remaining, config.navigation.navigationTimeoutMs) + : remainingTimer.remaining; + let finalRes; + try { + finalRes = await page.goto(navigationUrl.href, { + waitUntil: config.navigation.waitUntil, + timeout: navigationTimeout, + signal: ac.signal, + }); + } catch (e) { + // Tag the phase so the worker separates "never reached waitUntil" from a settle-phase + // timeout — same TimeoutError, different cause. + throw markRenderPhase(e, 'navigation'); + } timings.navTotal = Date.now() - navStart; const settleStart = Date.now(); diff --git a/packages/browser/src/settings.ts b/packages/browser/src/settings.ts index 84c7f25..86b733f 100644 --- a/packages/browser/src/settings.ts +++ b/packages/browser/src/settings.ts @@ -306,3 +306,21 @@ export const defaultLaunchOptions = (): LaunchOptions => ({ ignoreDefaultArgs: ['--disable-dev-shm-usage'], args: settings.chromeArgs, }); + +/** + * Launch options for the long-lived worker's browser: the consumer's options (or the defaults) + * plus, when the worker owns process signals, puppeteer's own signal handlers turned OFF. + * + * Those handlers close Chrome the moment SIGTERM/SIGHUP lands — and `process.exit(130)` on + * SIGINT — which races the worker's graceful drain: in-flight renders die mid-evaluate + * ("Attempted to use detached Frame" / "Target closed"), their results are lost, and the + * page-close handlers then fail to dispose their browser contexts. The worker closes the browser + * itself (`destroy()`, with a SIGKILL fallback, plus `killBrowsersSync()` on forced exits), so + * disabling them orphans nothing. They're applied LAST so a consumer-supplied `handleSIGTERM` + * can't silently reinstate the race; when the embedding app owns signals we don't touch them, + * since there puppeteer's handlers may be the only teardown. + */ +export const workerLaunchOptions = (ownsSignals: boolean): LaunchOptions => ({ + ...(settings.browserLaunchOptions ?? defaultLaunchOptions()), + ...(ownsSignals ? { handleSIGTERM: false, handleSIGINT: false, handleSIGHUP: false } : {}), +}); diff --git a/packages/browser/src/util/renderPhase.ts b/packages/browser/src/util/renderPhase.ts new file mode 100644 index 0000000..6677435 --- /dev/null +++ b/packages/browser/src/util/renderPhase.ts @@ -0,0 +1,25 @@ +/** + * Which phase of a render an error came from, tagged on the error itself so the worker can + * attribute a failure without re-deriving it from the message. Puppeteer throws the same + * `TimeoutError` for a navigation that never reached `waitUntil` and for a settle wait that + * ran out — those have very different causes (slow origin vs. slow in-browser work), so the + * phase is what makes the failure counters actionable. + */ +export type RenderPhase = 'navigation'; + +const PHASE_KEY = '__prerenderPhase'; + +/** Tag `err` with the phase it came from and return it (so it can be re-thrown inline). */ +export function markRenderPhase(err: E, phase: RenderPhase): E { + if (err !== null && typeof err === 'object') { + (err as Record)[PHASE_KEY] = 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; + const phase = (err as Record)[PHASE_KEY]; + return phase === 'navigation' ? phase : undefined; +} diff --git a/packages/browser/test/browserRetirement.test.ts b/packages/browser/test/browserRetirement.test.ts new file mode 100644 index 0000000..e09f3dc --- /dev/null +++ b/packages/browser/test/browserRetirement.test.ts @@ -0,0 +1,135 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import RenderWorker from '../dist/Worker.js'; +import { contextAlreadyGone } from '../dist/ManagedBrowser.js'; + +// A render drops its job ref only after its result POST and page close, and the reaper waits for +// BOTH counters — otherwise a retired browser gets closed out from under open pages, which surfaces +// as "Failed to close context" from their close handlers. + +type StubBrowser = { + activePages: number; + jobRefs: number; + closing: boolean; + closed: boolean; + close: () => Promise; +}; + +const stubBrowser = (activePages: number, jobRefs: number): StubBrowser => { + const browser: StubBrowser = { + activePages, + jobRefs, + closing: false, + closed: false, + close: async () => { + browser.closing = true; + browser.closed = true; + }, + }; + return browser; +}; + +// A worker with no browser of its own: closeRetiredBrowsers only reads the retired map. +const stubWorker = () => new RenderWorker({ renderer: async () => undefined }); + +const retire = (worker: RenderWorker, browser: StubBrowser, retiredAt: number) => { + worker.retiredBrowsers.set(browser as never, retiredAt); +}; + +test('a retired browser with open pages is not closed, even at zero job refs', async () => { + const worker = stubWorker(); + try { + // The result-POST window: the render released its ref, its page is still open. + const browser = stubBrowser(1, 0); + retire(worker, browser, Date.now()); + + worker.closeRetiredBrowsers(); + await Promise.resolve(); + + assert.equal(browser.closed, false, 'must not close a browser that still has an open page'); + assert.equal(worker.retiredBrowsers.size, 1); + } finally { + await worker.destroy(); + } +}); + +test('a retired browser with job refs but no open pages is not closed either', async () => { + const worker = stubWorker(); + try { + // A claimed job that hasn't opened its page yet. + const browser = stubBrowser(0, 1); + retire(worker, browser, Date.now()); + + worker.closeRetiredBrowsers(); + await Promise.resolve(); + + assert.equal(browser.closed, false); + } finally { + await worker.destroy(); + } +}); + +test('a retired browser is reaped once both counters reach zero', async () => { + const worker = stubWorker(); + try { + const browser = stubBrowser(0, 0); + retire(worker, browser, Date.now()); + + worker.closeRetiredBrowsers(); + await Promise.resolve(); + + assert.equal(browser.closed, true); + assert.equal(worker.retiredBrowsers.size, 0, 'reaped browsers leave the retired map'); + } finally { + await worker.destroy(); + } +}); + +test('a browser whose counters never drain is force-closed after the retirement deadline', async () => { + const worker = stubWorker(); + try { + // Stuck bookkeeping (a page whose 'close' never fired) — a leaked Chrome process is worse + // than a hard close, so the deadline wins. + const browser = stubBrowser(2, 1); + retire(worker, browser, Date.now() - 10 * 60 * 1000); + + worker.closeRetiredBrowsers(); + await Promise.resolve(); + + assert.equal(browser.closed, true); + } finally { + await worker.destroy(); + } +}); + +test('a browser already being reaped is not closed again on the next tick', async () => { + const worker = stubWorker(); + try { + const browser = stubBrowser(0, 0); + browser.closing = true; + retire(worker, browser, Date.now()); + + worker.closeRetiredBrowsers(); + await Promise.resolve(); + + assert.equal(browser.closed, false, 'an in-flight close must not be re-entered'); + } finally { + await worker.destroy(); + } +}); + +test('context-close failures that mean "already gone" are told from real ones', () => { + // The shapes seen in production, from both teardown paths. + assert.equal( + contextAlreadyGone( + new Error('Protocol error (Target.disposeBrowserContext): Failed to find context with id 9E2964…') + ), + true + ); + assert.equal(contextAlreadyGone(new Error('Protocol error (Target.disposeBrowserContext): Target closed')), true); + assert.equal(contextAlreadyGone(new Error('Connection closed.')), true); + assert.equal(contextAlreadyGone(new Error('Session closed. Most likely the page has been closed.')), true); + // A live browser that won't dispose an existing context is a genuine leak — keep it loud. + assert.equal(contextAlreadyGone(new Error('Runtime.callFunctionOn timed out')), false); + assert.equal(contextAlreadyGone(undefined), false); +}); diff --git a/packages/browser/test/config.test.ts b/packages/browser/test/config.test.ts index 1043a3d..2897d27 100644 --- a/packages/browser/test/config.test.ts +++ b/packages/browser/test/config.test.ts @@ -21,6 +21,8 @@ test('defaults reproduce the original hardcoded behavior', () => { assert.deepEqual(config.block.resourceTypes, ['image', 'media', 'font']); assert.deepEqual(config.block.urlPatterns, []); assert.equal(config.navigation.waitUntil, 'domcontentloaded'); + // no navigation sub-cap by default: goto may use the whole render budget + assert.equal(config.navigation.navigationTimeoutMs, 0); assert.equal(config.scroll.enabled, true); assert.equal(config.scroll.topSettleMs, 300); assert.equal(config.scroll.stepFraction, 0.5); @@ -59,6 +61,17 @@ test('deep-merges a file over defaults, preserving untouched nested fields', () () => mergeConfig({ scroll: { stepFraction: 'half' as unknown as number } }), /scroll\.stepFraction must be a positive number/ ); + // navigation.navigationTimeoutMs may be 0 (no sub-cap) but not negative / non-numeric + assert.equal(mergeConfig({ navigation: { navigationTimeoutMs: 0 } }).navigation.navigationTimeoutMs, 0); + assert.equal(mergeConfig({ navigation: { navigationTimeoutMs: 8000 } }).navigation.navigationTimeoutMs, 8000); + assert.throws( + () => mergeConfig({ navigation: { navigationTimeoutMs: -1 } }), + /navigation\.navigationTimeoutMs must be a non-negative number/ + ); + assert.throws( + () => mergeConfig({ navigation: { navigationTimeoutMs: '8s' as unknown as number } }), + /navigation\.navigationTimeoutMs must be a non-negative number/ + ); // arrays replace wholesale assert.deepEqual(config.block.urlPatterns, ['google-analytics.com']); assert.deepEqual(config.block.resourceTypes, ['image', 'media', 'font']); diff --git a/packages/browser/test/shutdownSignals.test.ts b/packages/browser/test/shutdownSignals.test.ts new file mode 100644 index 0000000..c386b99 --- /dev/null +++ b/packages/browser/test/shutdownSignals.test.ts @@ -0,0 +1,79 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import ManagedBrowser from '../dist/ManagedBrowser.js'; +import { resolveSettings, workerLaunchOptions, defaultLaunchOptions } from '../dist/settings.js'; +import { markRenderPhase, renderPhaseOf } from '../dist/util/renderPhase.js'; + +// Puppeteer installs its OWN SIGTERM/SIGINT/SIGHUP listeners that close Chrome the instant the +// signal lands, which races the worker's graceful drain: in-flight renders die mid-evaluate +// ("Attempted to use detached Frame" / "Target closed") and their results are lost. The worker +// owns Chrome's lifecycle, so it launches with those handlers off — that's what these cover. + +test('workerLaunchOptions disables puppeteer signal handlers when the worker owns signals', () => { + resolveSettings({}, { requireHarper: false }); + const owned = workerLaunchOptions(true); + assert.equal(owned.handleSIGTERM, false); + assert.equal(owned.handleSIGINT, false); + assert.equal(owned.handleSIGHUP, false); + // ...while keeping everything else the launch defaults provide + assert.equal(owned.headless, defaultLaunchOptions().headless); + assert.equal(owned.protocolTimeout, defaultLaunchOptions().protocolTimeout); +}); + +test('workerLaunchOptions leaves puppeteer signal handlers alone when the app owns signals', () => { + resolveSettings({}, { requireHarper: false }); + const notOwned = workerLaunchOptions(false); + assert.equal(notOwned.handleSIGTERM, undefined); + assert.equal(notOwned.handleSIGINT, undefined); + assert.equal(notOwned.handleSIGHUP, undefined); +}); + +test('a consumer cannot reinstate the signal-handler race through browserLaunchOptions', () => { + resolveSettings( + { browserLaunchOptions: { headless: 'shell', handleSIGTERM: true, handleSIGINT: true } }, + { requireHarper: false } + ); + const owned = workerLaunchOptions(true); + assert.equal(owned.handleSIGTERM, false); + assert.equal(owned.handleSIGINT, false); + // the consumer's other options still win + assert.equal(owned.headless, 'shell'); + resolveSettings({}, { requireHarper: false }); +}); + +test('render phase tags survive on the error and default to undefined', () => { + const err = markRenderPhase(new Error('Navigation timeout of 20000 ms exceeded'), 'navigation'); + assert.equal(renderPhaseOf(err), 'navigation'); + assert.equal(renderPhaseOf(new Error('boom')), undefined); + assert.equal(renderPhaseOf(undefined), undefined); + assert.equal(renderPhaseOf('not an error'), undefined); +}); + +// The behavioral half: prove the launch options actually keep Chrome alive across a SIGTERM, so an +// in-flight render can finish (what RenderWorker.shutdown's drain depends on). Uses a real browser +// like renderOnce.test.ts does. +test('an in-flight page evaluate survives SIGTERM under the worker launch options', async () => { + resolveSettings({}, { requireHarper: false }); + // Keep the default "terminate the process" action away from the test runner while we self-signal. + const keepAlive = () => {}; + process.on('SIGTERM', keepAlive); + + const managed = await ManagedBrowser.launch({ puppeteerLaunchOptions: workerLaunchOptions(true) }); + try { + const page = await managed.getPage(); + await page.goto('about:blank'); + + // Stand-in for a render in flight when the signal lands. + const inflight = page.evaluate( + () => new Promise((resolve) => setTimeout(() => resolve(document.querySelectorAll('*').length), 1000)) + ); + process.kill(process.pid, 'SIGTERM'); + + assert.ok((await inflight) > 0, 'in-flight evaluate should complete after SIGTERM'); + assert.equal(managed.browser.connected, true, 'browser should still be connected after SIGTERM'); + await managed.closePage(page); + } finally { + process.off('SIGTERM', keepAlive); + await managed.close(); + } +});