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

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,
 	},

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": [

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) {

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<string | undefined>;
 
+// 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<ManagedBrowser> | null = null;
 
-	retiredBrowsers: Set<ManagedBrowser> = new Set();
+	/** Retired browsers awaiting reaping, each mapped to when it was retired (see closeRetiredBrowsers). */
+	retiredBrowsers: Map<ManagedBrowser, number> = 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,31 +370,50 @@ 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();
 
 		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);
+			});
 		}
 	}
 
 	retireBrowser(browser: ManagedBrowser) {
 		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<ManagedBrowser> {