From 961579724a24532eba47cd945c3eeb0eec9d32d3 Mon Sep 17 00:00:00 2001 From: David Young Date: Tue, 21 Jul 2026 12:34:38 +1200 Subject: [PATCH 1/2] fix(api): gate /health on real pool capacity, not available + busy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/health` returns 200 as soon as `pool` is non-null, which happens before `await pool.init()` has warmed any browser — so a readiness probe on /health passes before the process can solve anything. The obvious fix, `available + busy > 0`, is also wrong, and fails in a much worse way. A request that hangs mid-solve never reaches the orchestrator's `finally`, so it never calls `release()` and its entry stays `busy` for the life of the process. `busy` therefore counts dead entries as capacity, and /health can report 200/"ok" indefinitely on a pool with zero usable browsers — the failure is completely invisible to any external check. Adds `stalled` and `live` to PoolStats: * an entry is `stalled` once its checkout outlives the caller's own budget (req.maxTimeout, threaded through acquire()) plus a grace period, so a slow but genuinely live request is never miscounted * `live` counts entries that can serve work now or are genuinely mid-request: idle-and-connected, plus busy-and-connected-and-not-stalled /health now gates on `live > 0`. A fully utilised pool still reports ready, so this does not flap under load, but a wedged one cannot report ready at all. `isUsable()` also checks `browser.isConnected()` rather than trusting the `healthy` flag, which is only refreshed on the 30s health-check tick and is never refreshed at all for busy entries. --- apps/api/src/config.ts | 4 ++++ apps/api/src/deps.ts | 2 ++ apps/api/src/routes/health.ts | 22 +++++++++++++++++++--- apps/api/src/routes/stats.ts | 4 ++++ packages/browser/src/pool.ts | 19 +++++++++++++++++++ packages/types/src/index.ts | 8 ++++++++ 6 files changed, 56 insertions(+), 3 deletions(-) diff --git a/apps/api/src/config.ts b/apps/api/src/config.ts index 09fa472..b1dedb9 100644 --- a/apps/api/src/config.ts +++ b/apps/api/src/config.ts @@ -13,6 +13,10 @@ export const RECYCLE_AFTER_TEMPORARY_CONTEXTS = Number(process.env.BROWSER_RECYC // minimal while still allowing CF/Imperva challenges to resolve. Raise if specific // targets fail with empty content (rare). export const CONTENT_PROCESSES = Number(process.env.BROWSER_CONTENT_PROCESSES ?? "2") +// How long a browser may stay checked out before the pool calls it wedged rather than +// busy. A scrape's own budget is req.maxTimeout (default 60s), so 3x that is well clear +// of anything legitimate while still catching a hung checkout within a few minutes. +export const STALL_TIMEOUT_MS = Number(process.env.BROWSER_STALL_TIMEOUT_MS ?? "180000") // PROXY_URL / RESIDENTIAL_PROXY_URL accept a comma-separated list of proxy URLs (a single // URL still works — it's just a 1-element list). *_LIST_FILE is an alternative source diff --git a/apps/api/src/deps.ts b/apps/api/src/deps.ts index 76d4399..b109eb7 100644 --- a/apps/api/src/deps.ts +++ b/apps/api/src/deps.ts @@ -8,6 +8,7 @@ import { REDIS_URL, residentialProxyPool, SESSION_TTL, + STALL_TIMEOUT_MS, } from "./config" // Single embedded pool — no BullMQ / worker process required. @@ -36,6 +37,7 @@ export async function initPool() { acquireTimeoutMs: ACQUIRE_TIMEOUT_MS, recycleAfterTemporaryContexts: RECYCLE_AFTER_TEMPORARY_CONTEXTS, contentProcesses: CONTENT_PROCESSES, + stallAfterMs: STALL_TIMEOUT_MS, }) await pool.init() pool.startHealthCheck() diff --git a/apps/api/src/routes/health.ts b/apps/api/src/routes/health.ts index ae9fc0c..033e33c 100644 --- a/apps/api/src/routes/health.ts +++ b/apps/api/src/routes/health.ts @@ -6,18 +6,34 @@ import { getPool } from "../deps" export function healthRoute() { return new Elysia().get("/health", ({ set }) => { const pool = getPool() - if (!pool) set.status = 503 + const stats = pool?.getStats() + // `pool` is assigned before `await pool.init()` completes, so a non-null pool does + // not mean any browser is warm — gate readiness on real capacity instead. + // + // `available + busy > 0` looks like the right test — a busy browser is still a live + // browser — but it isn't. A request that hangs mid-solve never reaches the + // orchestrator's `finally`, so it never calls release() and its browser stays `busy` + // for the life of the process. `busy` therefore counts dead entries, and the pod can + // report 200/"ok" indefinitely with zero usable browsers. + // + // `live` excludes both restarting entries (no browser attached) and entries whose + // checkout has outlived the stall threshold, so it cannot be propped up by a wedge. + // It still counts genuinely in-flight work, so a merely saturated pool won't flap. + const ready = Boolean(stats && stats.live > 0) + if (!ready) set.status = 503 return { - status: pool ? "ok" : "starting", + status: ready ? "ok" : "starting", uptime: Math.floor((Date.now() - startTime) / 1000), pool: - pool?.getStats() ?? + stats ?? ({ total: 0, busy: 0, available: 0, restarts: 0, avgRestarts: 0, + stalled: 0, + live: 0, } satisfies PoolStats), } }) diff --git a/apps/api/src/routes/stats.ts b/apps/api/src/routes/stats.ts index 65a2605..b25a98f 100644 --- a/apps/api/src/routes/stats.ts +++ b/apps/api/src/routes/stats.ts @@ -9,11 +9,15 @@ export function statsRoute() { available: 0, restarts: 0, avgRestarts: 0, + stalled: 0, + live: 0, } return { browsers: stats.total, available: stats.available, busy: stats.busy, + stalled: stats.stalled, + live: stats.live, restarts: stats.restarts, queueDepth: 0, } diff --git a/packages/browser/src/pool.ts b/packages/browser/src/pool.ts index 95fad3a..65ac385 100644 --- a/packages/browser/src/pool.ts +++ b/packages/browser/src/pool.ts @@ -40,6 +40,7 @@ export class BrowserPool { private pollIntervalMs: number private recycleAfterTemporaryContexts: number private contentProcesses!: number + private stallAfterMs: number private browserFactory?: BrowserFactory private healthInterval: ReturnType | null = null @@ -49,6 +50,7 @@ export class BrowserPool { pollIntervalMs = 100, recycleAfterTemporaryContexts = 8, contentProcesses = 2, + stallAfterMs = 180_000, browserFactory, }: { poolSize: number @@ -56,6 +58,7 @@ export class BrowserPool { pollIntervalMs?: number recycleAfterTemporaryContexts?: number contentProcesses?: number + stallAfterMs?: number browserFactory?: BrowserFactory }) { this.poolSize = poolSize @@ -63,9 +66,17 @@ export class BrowserPool { this.pollIntervalMs = pollIntervalMs this.recycleAfterTemporaryContexts = recycleAfterTemporaryContexts this.contentProcesses = contentProcesses + this.stallAfterMs = stallAfterMs this.browserFactory = browserFactory } + // A checkout longer than this is not slow, it's wedged: the orchestrator's own budget + // is req.maxTimeout (default 60s), so nothing legitimate holds a browser for minutes. + private isStalled(entry: PoolEntry, now = Date.now()): boolean { + if (!entry.busy || entry.busySince === undefined) return false + return now - entry.busySince > this.stallAfterMs + } + async init(): Promise { for (let i = 0; i < this.poolSize; i++) { // Pick a fingerprint for this instance; the picked OS drives the browser's @@ -188,6 +199,7 @@ export class BrowserPool { if (!entry) return false if (!entry.context || !entry.browser) return false entry.busy = true + entry.busySince = Date.now() entry.lastDomain = domain entry.lastUsedAt = Date.now() resolve({ @@ -244,6 +256,7 @@ export class BrowserPool { const entry = this.entries.find((e) => e.id === id) if (!entry) return entry.busy = false + entry.busySince = undefined // Keep the context alive — CF cookies (cf_clearance, __cf_bm) and browser cache // accumulate, making subsequent challenges faster. Cookies are domain-scoped. if (entry.context) { @@ -307,8 +320,10 @@ export class BrowserPool { } getStats(): PoolStats { + const now = Date.now() const busy = this.entries.filter((e) => e.busy).length const available = this.entries.filter((e) => !e.busy && !e.restarting && e.healthy && e.context).length + const stalled = this.entries.filter((e) => this.isStalled(e, now)).length const totalRestarts = this.entries.reduce((sum, e) => sum + e.restartCount, 0) return { total: this.poolSize, @@ -316,6 +331,10 @@ export class BrowserPool { available, restarts: totalRestarts, avgRestarts: totalRestarts / this.poolSize, + stalled, + // Entries that can serve work now or are genuinely mid-request. Excludes entries + // that are restarting (no browser attached) and entries wedged in a dead checkout. + live: available + (busy - stalled), } } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7f51821..aba11e3 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -59,6 +59,9 @@ export interface SessionData { export interface PoolBrowser { id: number busy: boolean + // When the current checkout started, or undefined when idle. Used to tell a browser + // that is busy doing work from one whose request wedged and left it busy forever. + busySince?: number lastDomain?: string lastUsedAt?: number restartCount: number @@ -71,6 +74,11 @@ export interface PoolStats { available: number restarts: number avgRestarts: number + // Subset of `busy` that has been checked out longer than the pool's stall threshold. + // A stalled entry is counted in `busy` but is not real capacity — its request wedged + // and will never call release(). `live` is the honest capacity number. + stalled: number + live: number } // Per-instance HTTP-level fingerprint (User-Agent + matching navigator.platform / From 7dae35710324fce847e5c7a1deb4884133b2c646 Mon Sep 17 00:00:00 2001 From: David Young Date: Tue, 21 Jul 2026 12:42:32 +1200 Subject: [PATCH 2/2] fix(browser): bound every await in restartEntry; reclaim stalled checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BrowserPool.restartEntry` awaited `context.close()`, `browser.close()` and the Camoufox launch with no timeout on any of them. Camoufox hangs on close when a content process is wedged — tiers/3.ts and tiers/4.ts already guard their *temporary* contexts against exactly this with a 5s `Promise.race` — but the persistent context and browser the pool owns had no such guard, and launches can hang too. When any one of those hangs, the entry is pinned at `restarting = true` forever. From then on the health check hits its own `if (entry.restarting) return` guard, so every 30s tick logs "browser N disconnected, restarting" and does nothing. The pool silently loses that slot permanently: `restartCount` never increments, so the restart counter sits frozen while the log implies furious activity. With enough uptime every entry ends up in this state and the pool is inert. Changes: * every await in `restartEntry`, `init()` and `shutdown()` is bounded. On timeout the entry is left unhealthy with `restarting` cleared, so the next health-check tick retries it from scratch instead of wedging. * `runHealthCheck` reclaims checkouts past their deadline. Previously busy entries were skipped entirely, so an entry whose request wedged was never examined again. * a per-checkout `lease`, returned on the handle and passed back to `release()`, so a request that outlives its checkout cannot free — or recycle, via `noteTemporaryContext` — a browser the pool has since handed to someone else. * `release()` hands its in-flight page closes to `restartEntry` rather than racing them, since closing a context underneath in-flight `page.close()` calls is one way to wedge the transport in the first place. * abandoned launches are counted and capped. A timeout can only stop *waiting* for a launch, not cancel it, so retrying without a cap could pile up hung Firefox processes; past the cap the entry stays down and `live` reflects it. Timeouts are configurable (`closeTimeoutMs`, `launchTimeoutMs`, `stallAfterMs`, `healthIntervalMs`) with the API exposing them as BROWSER_*_MS env vars. Adds regression tests for the hung close, the hung launch, stall accounting, budget-aware stall deadlines, disconnected-but-busy entries, and stale releases. The hung-close and hung-launch tests both fail against the unpatched pool. --- apps/api/src/config.ts | 6 + apps/api/src/deps.ts | 8 +- packages/browser/src/pool.ts | 261 ++++++++++++++++++++++++---- packages/browser/tests/pool.test.ts | 217 ++++++++++++++++++++++- packages/tiers/src/orchestrator.ts | 10 +- packages/types/src/index.ts | 3 + 6 files changed, 466 insertions(+), 39 deletions(-) diff --git a/apps/api/src/config.ts b/apps/api/src/config.ts index b1dedb9..d79af03 100644 --- a/apps/api/src/config.ts +++ b/apps/api/src/config.ts @@ -17,6 +17,12 @@ export const CONTENT_PROCESSES = Number(process.env.BROWSER_CONTENT_PROCESSES ?? // busy. A scrape's own budget is req.maxTimeout (default 60s), so 3x that is well clear // of anything legitimate while still catching a hung checkout within a few minutes. export const STALL_TIMEOUT_MS = Number(process.env.BROWSER_STALL_TIMEOUT_MS ?? "180000") +// Upper bound on a single browser/context close during a recycle. Camoufox can hang on +// close when a content process is wedged; past this we abandon the close and relaunch. +export const CLOSE_TIMEOUT_MS = Number(process.env.BROWSER_CLOSE_TIMEOUT_MS ?? "10000") +// Upper bound on a browser launch. A cold Camoufox start is a few seconds, but launches +// have been observed to hang indefinitely — without a bound that strands the pool entry. +export const LAUNCH_TIMEOUT_MS = Number(process.env.BROWSER_LAUNCH_TIMEOUT_MS ?? "90000") // PROXY_URL / RESIDENTIAL_PROXY_URL accept a comma-separated list of proxy URLs (a single // URL still works — it's just a 1-element list). *_LIST_FILE is an alternative source diff --git a/apps/api/src/deps.ts b/apps/api/src/deps.ts index b109eb7..f9cc2da 100644 --- a/apps/api/src/deps.ts +++ b/apps/api/src/deps.ts @@ -1,7 +1,9 @@ import { BrowserPool, SessionCache } from "@trawl/browser" import { ACQUIRE_TIMEOUT_MS, + CLOSE_TIMEOUT_MS, CONTENT_PROCESSES, + LAUNCH_TIMEOUT_MS, POOL_SIZE, proxyPool, RECYCLE_AFTER_TEMPORARY_CONTEXTS, @@ -38,6 +40,8 @@ export async function initPool() { recycleAfterTemporaryContexts: RECYCLE_AFTER_TEMPORARY_CONTEXTS, contentProcesses: CONTENT_PROCESSES, stallAfterMs: STALL_TIMEOUT_MS, + closeTimeoutMs: CLOSE_TIMEOUT_MS, + launchTimeoutMs: LAUNCH_TIMEOUT_MS, }) await pool.init() pool.startHealthCheck() @@ -49,8 +53,8 @@ export function getDeps() { const p = pool const sc = sessionCache return { - acquireBrowser: (d: string) => p.acquire(d), - releaseBrowser: (id: number) => p.release(id), + acquireBrowser: (d: string, budgetMs?: number) => p.acquire(d, budgetMs), + releaseBrowser: (id: number, lease?: number) => p.release(id, lease), // Session cache ops are no-ops when Redis is unavailable loadSession: (d: string) => (sc ? sc.load(d).catch(() => null) : Promise.resolve(null)), saveSession: (d: string, data: unknown) => (sc ? sc.save(d, data as never).catch(() => {}) : Promise.resolve()), diff --git a/packages/browser/src/pool.ts b/packages/browser/src/pool.ts index 65ac385..f213620 100644 --- a/packages/browser/src/pool.ts +++ b/packages/browser/src/pool.ts @@ -11,6 +11,34 @@ type Browser = any // biome-ignore lint/suspicious/noExplicitAny: see comment above type BrowserContext = any +// Closing a Camoufox browser or context can hang indefinitely when a content process is +// wedged — tier3/tier4 already guard their temporary contexts this way. 10s is far above +// the typical sub-second close path. +const CLOSE_TIMEOUT_MS = 10_000 +// A cold Camoufox start is a few seconds; camoufox-js also does a public-IP lookup for +// `geoip` before handing off to Playwright, which adds network time that Playwright's own +// launch timeout does not cover. 90s is generous but finite. +const LAUNCH_TIMEOUT_MS = 90_000 + +// Resolves when `p` settles or `ms` elapses, whichever comes first. Never rejects, and +// never leaves an unhandled rejection behind when `p` fails after we stopped waiting. +function settleWithin(p: Promise | undefined | null, ms: number): Promise { + if (!p) return Promise.resolve() + const swallowed = p.then( + () => {}, + () => {}, + ) + let timer: ReturnType | undefined + return Promise.race([ + swallowed, + new Promise((resolve) => { + timer = setTimeout(resolve, ms) + }), + ]).finally(() => { + if (timer) clearTimeout(timer) + }) +} + export class PoolExhaustedError extends Error { constructor() { super("Browser pool exhausted: all browsers are busy") @@ -23,9 +51,18 @@ export class PoolExhaustedError extends Error { export type { BrowserHandle } from "@trawl/types" interface PoolEntry extends PoolBrowser { + // Monotonic per-checkout token. Bumped on every acquire and every restart so a + // release() arriving from an abandoned request can be recognised and ignored. + lease: number browser: Browser | null context: BrowserContext | null temporaryContextUses: number + // Page closes started by release(); restartEntry lets them settle before tearing the + // context down, so it isn't closing a context underneath in-flight page.close() calls. + pendingPageCloses?: Promise + // Wall-clock instant past which this checkout is considered wedged. Set on acquire from + // the caller's budget; undefined when idle. + stallAt?: number restartReason?: string restarting?: boolean fingerprint: (typeof FINGERPRINT_POOL)[number] @@ -41,8 +78,15 @@ export class BrowserPool { private recycleAfterTemporaryContexts: number private contentProcesses!: number private stallAfterMs: number + private closeTimeoutMs: number + private launchTimeoutMs: number + private healthIntervalMs: number private browserFactory?: BrowserFactory private healthInterval: ReturnType | null = null + // Launch attempts we timed out on and can no longer cancel. Decremented if the attempt + // ever settles. Retrying past maxAbandonedLaunches would just stack up more of them. + private abandonedLaunches = 0 + private maxAbandonedLaunches: number constructor({ poolSize, @@ -51,6 +95,10 @@ export class BrowserPool { recycleAfterTemporaryContexts = 8, contentProcesses = 2, stallAfterMs = 180_000, + closeTimeoutMs = CLOSE_TIMEOUT_MS, + launchTimeoutMs = LAUNCH_TIMEOUT_MS, + healthIntervalMs = 30_000, + maxAbandonedLaunches = 3, browserFactory, }: { poolSize: number @@ -59,6 +107,10 @@ export class BrowserPool { recycleAfterTemporaryContexts?: number contentProcesses?: number stallAfterMs?: number + closeTimeoutMs?: number + launchTimeoutMs?: number + healthIntervalMs?: number + maxAbandonedLaunches?: number browserFactory?: BrowserFactory }) { this.poolSize = poolSize @@ -67,14 +119,21 @@ export class BrowserPool { this.recycleAfterTemporaryContexts = recycleAfterTemporaryContexts this.contentProcesses = contentProcesses this.stallAfterMs = stallAfterMs + this.closeTimeoutMs = closeTimeoutMs + this.launchTimeoutMs = launchTimeoutMs + this.healthIntervalMs = healthIntervalMs + this.maxAbandonedLaunches = maxAbandonedLaunches this.browserFactory = browserFactory } - // A checkout longer than this is not slow, it's wedged: the orchestrator's own budget - // is req.maxTimeout (default 60s), so nothing legitimate holds a browser for minutes. + // A checkout past its deadline is not slow, it's wedged. The deadline is the caller's + // own budget (req.maxTimeout) plus a full stallAfterMs of grace, so a request is never + // reclaimed while it is still inside the time it asked for — callers may legitimately + // pass a maxTimeout larger than stallAfterMs. Without a budget we fall back to + // stallAfterMs alone. private isStalled(entry: PoolEntry, now = Date.now()): boolean { - if (!entry.busy || entry.busySince === undefined) return false - return now - entry.busySince > this.stallAfterMs + if (!entry.busy || entry.stallAt === undefined) return false + return now > entry.stallAt } async init(): Promise { @@ -83,10 +142,14 @@ export class BrowserPool { // navigator.platform, locale, timezone, and the HTTP UA the orchestrator sends. // Shuffled pool (not sequential) so 4 browsers don't all get the same fingerprint. const fingerprint = FINGERPRINT_POOL[i % FINGERPRINT_POOL.length] - const { browser, context } = await this.launchBrowser(fingerprint) + // Bounded like every other launch: an unbounded hang here leaves init() pending + // forever with the HTTP listener already up, so the pod never becomes ready and + // never fails either. Throwing lets the startup probe restart the container. + const { browser, context } = await this.launchWithin(fingerprint, this.launchTimeoutMs) this.entries.push({ id: i, busy: false, + lease: 0, restartCount: 0, healthy: true, browser, @@ -192,24 +255,34 @@ export class BrowserPool { return context } - acquire(domain?: string): Promise { + // `budgetMs` is the caller's own deadline for this checkout (the orchestrator passes + // req.maxTimeout). It only ever extends how long the checkout is tolerated, never + // shortens it below stallAfterMs. + acquire(domain?: string, budgetMs?: number): Promise { return new Promise((resolve, reject) => { const tryAcquire = () => { const entry = this.pickEntry(domain) if (!entry) return false if (!entry.context || !entry.browser) return false + const now = Date.now() entry.busy = true - entry.busySince = Date.now() + entry.busySince = now + entry.stallAt = now + Math.max(budgetMs ?? 0, 0) + this.stallAfterMs + entry.lease++ entry.lastDomain = domain entry.lastUsedAt = Date.now() resolve({ id: entry.id, + lease: entry.lease, context: entry.context, browser: entry.browser, fingerprint: entry.fingerprint, - noteTemporaryContext: (reason: string) => { + // Captured lease: a reclaimed request that resumes later must not attribute its + // failure to the replacement browser now occupying this entry. + noteTemporaryContext: ((lease: number) => (reason: string) => { + if (entry.lease !== lease) return this.noteTemporaryContext(entry, reason) - }, + })(entry.lease), }) return true } @@ -252,29 +325,59 @@ export class BrowserPool { } } - release(id: number): void { + release(id: number, lease?: number): void { const entry = this.entries.find((e) => e.id === id) if (!entry) return + // A checkout the health check already reclaimed must not free the entry a second + // time — by now it may be restarting, or handed to a different request. The lease + // identifies *which* checkout is being released; a mismatch means this one is stale. + if (lease !== undefined && entry.lease !== lease) return + if (!entry.busy) return entry.busy = false entry.busySince = undefined + entry.stallAt = undefined // Keep the context alive — CF cookies (cf_clearance, __cf_bm) and browser cache // accumulate, making subsequent challenges faster. Cookies are domain-scoped. - if (entry.context) { - const pages: unknown[] = entry.context.pages() ?? [] - for (const p of pages) (p as { close: () => Promise }).close().catch(() => {}) - } + const pages: unknown[] = entry.context?.pages() ?? [] + entry.pendingPageCloses = Promise.all( + pages.map((p) => (p as { close: () => Promise }).close().catch(() => {})), + ) if (entry.restartReason) { - void this.restartEntry(entry, entry.restartReason) + const reason = entry.restartReason + entry.restartReason = undefined + // Called synchronously, not deferred behind the page closes: restartEntry sets + // `restarting` before its first await, and that flag is what stops another request + // acquiring this entry in the window before the browser is actually torn down. + // restartEntry waits on pendingPageCloses itself. + void this.restartEntry(entry, reason) } } startHealthCheck(): void { - this.healthInterval = setInterval(() => this.runHealthCheck(), 30_000) + this.healthInterval = setInterval(() => this.runHealthCheck(), this.healthIntervalMs) } private async runHealthCheck(): Promise { + const now = Date.now() for (const entry of this.entries) { - if (entry.busy) continue + // A restart already in flight will finish or fail on its own deadline. Re-entering + // here only produced the "disconnected, restarting" log every 30s that made a dead + // pool look like a busy one. + if (entry.restarting) continue + + if (entry.busy) { + // We can't probe a checked-out browser — closing it would kill a live request. + // But a checkout past the stall threshold is not a request any more: it never + // reached the orchestrator's `finally`, so nothing will ever release it. Left + // alone, the entry is subtracted from the pool for the rest of the process. + if (this.isStalled(entry, now)) { + const heldSec = Math.round((now - (entry.busySince ?? now)) / 1000) + console.warn(`[pool] browser ${entry.id} stalled — checked out for ${heldSec}s, reclaiming`) + await this.restartEntry(entry, "checkout stalled") + } + continue + } + if (!(entry.browser?.isConnected() ?? false)) { console.warn(`[pool] browser ${entry.id} disconnected, restarting`) await this.restartEntry(entry, "browser disconnected") @@ -284,6 +387,62 @@ export class BrowserPool { } } + // Runs `launchBrowser` under a hard deadline. Playwright's own launch timeout does not + // cover camoufox-js's pre-launch work (the `geoip` public-IP lookup), so a launch can + // outlive it; and an unbounded launch here is unrecoverable — see restartEntry. + private async launchWithin( + fingerprint: (typeof FINGERPRINT_POOL)[number], + ms: number, + ): Promise<{ browser: Browser; context: BrowserContext }> { + let timedOut = false + // Playwright exposes no way to cancel an in-flight launch, so a timeout here can only + // stop *waiting* — the attempt keeps running. Count the ones we abandon so a browser + // that hangs on every launch can't have attempts piled on it forever. + this.abandonedLaunches++ + const launch = this.launchBrowser(fingerprint).then( + (result) => { + if (!timedOut) { + this.abandonedLaunches-- + return result + } + // We already gave up on this launch — don't leak the browser it finally produced. + // Stay charged until that close *actually* settles, with no timeout: releasing the + // slot on a bound would let genuinely unkillable Firefox processes accumulate + // silently, one per retry. Holding it means a doubly-wedged entry (launch hung, + // then close hung) stays down and the pool reports reduced `live` — the readiness + // gate surfaces that, which is the outcome we want over a quiet process leak. + void Promise.resolve(result.browser?.close()).then( + () => { + this.abandonedLaunches-- + }, + () => { + this.abandonedLaunches-- + }, + ) + return null + }, + (err) => { + this.abandonedLaunches-- + if (timedOut) return null + throw err + }, + ) + let timer: ReturnType | undefined + const result = await Promise.race([ + launch, + new Promise((resolve) => { + timer = setTimeout(() => { + timedOut = true + resolve(null) + }, ms) + }), + ]).finally(() => { + if (timer) clearTimeout(timer) + }) + if (!result) throw new Error(`browser launch exceeded ${ms}ms`) + return result + } + private async restartEntry(entry: PoolEntry, reason = "manual restart"): Promise { if (entry.restarting) { entry.restartReason ??= reason @@ -291,21 +450,48 @@ export class BrowserPool { } entry.restarting = true entry.healthy = false + // Drop any checkout this entry was holding. Either release() already cleared it, or + // we are reclaiming a stalled one — in both cases the entry is ours now, and the + // bumped lease makes a late release() from the abandoned request a no-op. + entry.busy = false + entry.busySince = undefined + entry.stallAt = undefined + entry.lease++ entry.restartReason = undefined console.warn(`[pool] browser ${entry.id} restarting: ${reason}`) - try { - await entry.context?.close() - } catch {} - try { - await entry.browser?.close() - } catch {} - entry.browser = null + + const dyingContext = entry.context + const dyingBrowser = entry.browser + const pendingPageCloses = entry.pendingPageCloses entry.context = null + entry.browser = null + entry.pendingPageCloses = undefined + + // Every await below is bounded, and that is the whole point. Camoufox/Firefox hangs + // on close when a content process is wedged (the hazard tier3/tier4 already guard + // their temporary contexts against), and camoufox-js's launch path can hang too. An + // unbounded await anywhere in here strands the entry with restarting=true forever: + // it is then excluded from `available` in getStats() and short-circuited by the + // `if (entry.restarting)` guard at the top, so the 30s health check can only log + // "disconnected, restarting" about it, never actually restart it. That is how a pool + // reaches zero live browsers while its restart counter sits frozen and the process + // looks perfectly healthy from the outside. + await settleWithin(pendingPageCloses, this.closeTimeoutMs) + await settleWithin(dyingContext?.close(), this.closeTimeoutMs) + await settleWithin(dyingBrowser?.close(), this.closeTimeoutMs) + try { + // Refuse to pile another attempt onto a backlog of launches we already gave up + // waiting for — each one may still be holding a real Firefox process we can't + // cancel. The entry stays unhealthy, so `live` drops and the readiness gate takes + // the pod out of rotation instead of quietly leaking processes. + if (this.abandonedLaunches >= this.maxAbandonedLaunches) { + throw new Error(`${this.abandonedLaunches} launches already abandoned; not starting another until one settles`) + } // On restart, keep the entry's original fingerprint so this browser instance // keeps its identity across restart cycles (otherwise cross-session correlation // becomes trivial). - const { browser, context } = await this.launchBrowser(entry.fingerprint) + const { browser, context } = await this.launchWithin(entry.fingerprint, this.launchTimeoutMs) entry.browser = browser entry.context = context entry.healthy = true @@ -313,17 +499,28 @@ export class BrowserPool { entry.restartCount++ console.log(`[pool] browser ${entry.id} restarted (total: ${entry.restartCount})`) } catch (err) { + // Leave the entry unhealthy with no browser attached. `restarting` clears in the + // finally, so the next health-check tick retries this entry from scratch. console.error(`[pool] browser ${entry.id} failed to restart:`, err) } finally { entry.restarting = false } } + // A browser only counts if its transport is actually up. `healthy` is only refreshed + // every health-check tick, and busy entries are never probed at all, so without this a + // checkout whose browser died reads as capacity until its stall deadline passes. + private isUsable(entry: PoolEntry): boolean { + return Boolean(entry.context) && Boolean(entry.browser?.isConnected?.() ?? false) + } + getStats(): PoolStats { const now = Date.now() const busy = this.entries.filter((e) => e.busy).length - const available = this.entries.filter((e) => !e.busy && !e.restarting && e.healthy && e.context).length + const available = this.entries.filter((e) => !e.busy && !e.restarting && e.healthy && this.isUsable(e)).length const stalled = this.entries.filter((e) => this.isStalled(e, now)).length + // Busy entries that are still genuinely working: inside their deadline AND connected. + const busyLive = this.entries.filter((e) => e.busy && !this.isStalled(e, now) && this.isUsable(e)).length const totalRestarts = this.entries.reduce((sum, e) => sum + e.restartCount, 0) return { total: this.poolSize, @@ -332,17 +529,19 @@ export class BrowserPool { restarts: totalRestarts, avgRestarts: totalRestarts / this.poolSize, stalled, - // Entries that can serve work now or are genuinely mid-request. Excludes entries - // that are restarting (no browser attached) and entries wedged in a dead checkout. - live: available + (busy - stalled), + // Real capacity: idle-and-connected plus in-flight-and-connected. Excludes + // restarting entries, wedged checkouts, and checkouts whose browser has died. + live: available + busyLive, } } async shutdown(): Promise { if (this.healthInterval) clearInterval(this.healthInterval) for (const entry of this.entries) { - await entry.context?.close().catch(() => {}) - await entry.browser?.close().catch(() => {}) + // Bounded for the same reason as restartEntry — an unbounded close here hangs + // SIGTERM handling until the supervisor's grace period expires and force-kills us. + await settleWithin(entry.context?.close(), this.closeTimeoutMs) + await settleWithin(entry.browser?.close(), this.closeTimeoutMs) } this.entries = [] } diff --git a/packages/browser/tests/pool.test.ts b/packages/browser/tests/pool.test.ts index fa503a6..a2034a8 100644 --- a/packages/browser/tests/pool.test.ts +++ b/packages/browser/tests/pool.test.ts @@ -1,8 +1,8 @@ import { describe, expect, test } from "bun:test" import { BrowserPool } from "../src/pool" -const waitFor = async (predicate: () => boolean) => { - const deadline = Date.now() + 1000 +const waitFor = async (predicate: () => boolean, budgetMs = 1000) => { + const deadline = Date.now() + budgetMs while (Date.now() < deadline) { if (predicate()) return await new Promise((resolve) => setTimeout(resolve, 10)) @@ -10,6 +10,8 @@ const waitFor = async (predicate: () => boolean) => { throw new Error("timed out waiting for condition") } +const NEVER = () => new Promise(() => {}) + type MockBrowser = { closed: boolean isConnected: () => boolean @@ -144,3 +146,214 @@ describe("BrowserPool recycling", () => { expect(pool.getStats().total).toBe(1) }) }) + +// Regression tests for a wedge seen in long-running deployments: /health kept reporting +// 200/"ok" with zero usable browsers, while the pool's restart counter stayed frozen and +// the health check logged "browser N disconnected, restarting" forever without restarting. +describe("BrowserPool wedge recovery", () => { + test("a browser whose close() never resolves does not strand the entry in restarting", async () => { + // The failure: restartEntry awaited context.close() with no bound, the + // close never settled, and `restarting` stayed true forever. From then on the 30s + // health check hit the `if (entry.restarting) return` guard and could only log — + // the entry was never rebuilt and never counted as available again. + const browsers: MockBrowser[] = [] + const factory = async () => { + const browser: MockBrowser = { + closed: false, + isConnected() { + return !this.closed + }, + // First browser hangs on close, exactly like Camoufox with a wedged content + // process. Replacements close normally. + close: + browsers.length === 0 + ? NEVER + : async function (this: MockBrowser) { + this.closed = true + }, + } + const context: MockContext = { + closed: false, + pages: () => [], + close: + browsers.length === 0 + ? NEVER + : async function (this: MockContext) { + this.closed = true + }, + } + browsers.push(browser) + return { browser, context } + } + + const pool = new BrowserPool({ + poolSize: 1, + recycleAfterTemporaryContexts: 1, + closeTimeoutMs: 50, + browserFactory: factory, + }) + await pool.init() + + const handle = await pool.acquire("example.com") + handle.noteTemporaryContext?.("tier4 blocked") + pool.release(handle.id, handle.lease) + + // Before the fix this never happened — the pool sat at restarts=0, available=0. + await waitFor(() => pool.getStats().restarts === 1) + expect(pool.getStats().available).toBe(1) + expect(pool.getStats().live).toBe(1) + expect(browsers).toHaveLength(2) + }) + + test("a launch that never resolves fails the restart instead of hanging it", async () => { + let launches = 0 + const factory = async () => { + launches++ + // Second launch (the restart) hangs — camoufox-js can block before Playwright's + // own launch timeout ever applies. + if (launches === 2) await NEVER() + const browser: MockBrowser = { + closed: false, + isConnected() { + return !this.closed + }, + async close() { + this.closed = true + }, + } + const context: MockContext = { + closed: false, + pages: () => [], + async close() { + this.closed = true + }, + } + return { browser, context } + } + + const pool = new BrowserPool({ + poolSize: 1, + recycleAfterTemporaryContexts: 1, + closeTimeoutMs: 20, + launchTimeoutMs: 50, + healthIntervalMs: 30, + browserFactory: factory, + }) + await pool.init() + pool.startHealthCheck() + + const handle = await pool.acquire("example.com") + handle.noteTemporaryContext?.("tier4 blocked") + pool.release(handle.id, handle.lease) + + // Wait for the restart to actually be in flight (entry detached, no capacity) before + // asserting recovery — otherwise this passes on the pre-restart state and proves + // nothing. + await waitFor(() => pool.getStats().live === 0, 2000) + // The hung launch is then abandoned, `restarting` clears, and the next health-check + // tick retries the entry from scratch — so the pool heals rather than wedging here. + await waitFor(() => pool.getStats().live === 1, 3000) + await pool.shutdown() + }) + + test("a stalled checkout is not counted as live capacity", async () => { + // This is the exact arithmetic that defeated the old `available + busy > 0` gate: + // total=1, busy=1, available=0 — which read as "ok" despite nothing being usable. + const { factory } = makeFactory() + const pool = new BrowserPool({ poolSize: 1, stallAfterMs: 40, browserFactory: factory }) + await pool.init() + + const handle = await pool.acquire("example.com") + expect(pool.getStats().busy).toBe(1) + expect(pool.getStats().live).toBe(1) // genuinely in-flight work still counts + + await new Promise((r) => setTimeout(r, 60)) + + const stats = pool.getStats() + expect(stats.busy).toBe(1) + expect(stats.available).toBe(0) + expect(stats.stalled).toBe(1) + expect(stats.live).toBe(0) // …and the old gate would have said "ok" here + expect(handle.id).toBe(0) + }) + + test("a busy entry whose browser died is not counted as live capacity", async () => { + // The health check never probes busy entries, so a checkout whose browser dies would + // otherwise read as capacity right up until its stall deadline — the same "200 with + // nothing usable" failure the gate exists to prevent, just on a timer. + const { factory, browsers } = makeFactory() + const pool = new BrowserPool({ poolSize: 1, stallAfterMs: 60_000, browserFactory: factory }) + await pool.init() + + const handle = await pool.acquire("example.com", 60_000) + expect(pool.getStats().live).toBe(1) + + // Browser dies mid-request; nothing releases it and it is nowhere near its deadline. + browsers[0].closed = true + + const stats = pool.getStats() + expect(stats.busy).toBe(1) + expect(stats.stalled).toBe(0) // still inside its budget… + expect(stats.live).toBe(0) // …but not usable, so not capacity + expect(handle.id).toBe(0) + }) + + test("a checkout inside the caller's own budget is never reclaimed", async () => { + // Callers may pass req.maxTimeout larger than the stall threshold. Reclaiming on the + // threshold alone would close the browser out from under a request that is still + // well inside the time it asked for. + const { factory } = makeFactory() + const pool = new BrowserPool({ + poolSize: 1, + stallAfterMs: 40, + healthIntervalMs: 20, + browserFactory: factory, + }) + await pool.init() + pool.startHealthCheck() + + // Budget of 5s dwarfs the 40ms stall threshold — this checkout must survive. + const handle = await pool.acquire("example.com", 5000) + await new Promise((r) => setTimeout(r, 300)) + + const stats = pool.getStats() + expect(stats.stalled).toBe(0) + expect(stats.restarts).toBe(0) + expect(stats.busy).toBe(1) + expect(stats.live).toBe(1) + + pool.release(handle.id, handle.lease) + await pool.shutdown() + }) + + test("the health check reclaims a stalled checkout, and its late release is ignored", async () => { + const { factory } = makeFactory() + const pool = new BrowserPool({ + poolSize: 1, + stallAfterMs: 40, + healthIntervalMs: 20, + browserFactory: factory, + }) + await pool.init() + pool.startHealthCheck() + + // A request that wedges mid-solve: acquired, never released. + const abandoned = await pool.acquire("example.com") + + await waitFor(() => pool.getStats().restarts === 1, 2000) + expect(pool.getStats().live).toBe(1) + + // Someone else now holds the rebuilt browser. + const current = await pool.acquire("example.com") + expect(pool.getStats().busy).toBe(1) + + // The abandoned request finally unwinds and calls release(). Its lease is stale, so + // it must not free the checkout that `current` is holding. + pool.release(abandoned.id, abandoned.lease) + expect(pool.getStats().busy).toBe(1) + + pool.release(current.id, current.lease) + expect(pool.getStats().busy).toBe(0) + await pool.shutdown() + }) +}) diff --git a/packages/tiers/src/orchestrator.ts b/packages/tiers/src/orchestrator.ts index 86d9584..a4daaec 100644 --- a/packages/tiers/src/orchestrator.ts +++ b/packages/tiers/src/orchestrator.ts @@ -35,8 +35,8 @@ export function shouldFlagForRecycle(status: TierResult["status"]): boolean { } export interface OrchestratorDeps { - acquireBrowser(domain: string): Promise - releaseBrowser(id: number): void + acquireBrowser(domain: string, budgetMs?: number): Promise + releaseBrowser(id: number, lease?: number): void loadSession(domain: string): Promise saveSession(domain: string, data: SessionData): Promise invalidateSession(domain: string): Promise @@ -96,7 +96,9 @@ export async function scrape(req: ScrapeRequest, deps: OrchestratorDeps): Promis } // Acquire browser for tiers 2-4 - const handle = await deps.acquireBrowser(domain) + // Pass our own budget so the pool's stall detector doesn't reclaim this browser + // while the request is still inside the time the caller asked for. + const handle = await deps.acquireBrowser(domain, maxTimeout) try { // Tier 2: browser with cached session @@ -249,6 +251,6 @@ export async function scrape(req: ScrapeRequest, deps: OrchestratorDeps): Promis throw new ScrapeError(`All tiers exhausted. Last failure: ${t4.reason ?? t4.status}`, timings) } finally { - deps.releaseBrowser(handle.id) + deps.releaseBrowser(handle.id, handle.lease) } } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index aba11e3..ba54b02 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -97,6 +97,9 @@ export interface BrowserFingerprint { // (consumers call .newPage()/.newContext()/.cookies() etc directly on these fields). export interface BrowserHandle { id: number + // Identifies this specific checkout. Pass it back to release() so a request that + // outlived its checkout can't free a browser the pool has since reclaimed. + lease: number // biome-ignore lint/suspicious/noExplicitAny: see comment above context: any // biome-ignore lint/suspicious/noExplicitAny: see comment above