Merge pull request #37 from funkypenguin/fix/bound-restart-and-reclaim-stalled

apps/api/src/config.ts

@@ -13,6 +13,16 @@ 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")
+// 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

apps/api/src/deps.ts

@@ -3,13 +3,16 @@ import type { OrchestratorDeps } from "@trawl/tiers"
 import type { SessionData } from "@trawl/types"
 import {
   ACQUIRE_TIMEOUT_MS,
+  CLOSE_TIMEOUT_MS,
   CONTENT_PROCESSES,
+  LAUNCH_TIMEOUT_MS,
   POOL_SIZE,
   proxyPool,
   RECYCLE_AFTER_TEMPORARY_CONTEXTS,
   REDIS_URL,
   residentialProxyPool,
   SESSION_TTL,
+  STALL_TIMEOUT_MS,
 } from "./config"
 
 const state: {
@@ -36,6 +39,9 @@ export const initPool = async (): Promise<void> => {
     acquireTimeoutMs: ACQUIRE_TIMEOUT_MS,
     recycleAfterTemporaryContexts: RECYCLE_AFTER_TEMPORARY_CONTEXTS,
     contentProcesses: CONTENT_PROCESSES,
+    stallAfterMs: STALL_TIMEOUT_MS,
+    closeTimeoutMs: CLOSE_TIMEOUT_MS,
+    launchTimeoutMs: LAUNCH_TIMEOUT_MS,
   })
   await state.pool.init()
   state.pool.startHealthCheck()
@@ -54,8 +60,8 @@ export const getDeps = (): OrchestratorDeps => {
   const sc = state.sessionCache
   const pcc = state.persistentContextCache
   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),
     loadSession: (d: string) => (sc ? sc.load(d).catch(() => undefined) : Promise.resolve(undefined)),
     saveSession: (d: string, data: SessionData) => (sc ? sc.save(d, data).catch(() => {}) : Promise.resolve()),
     invalidateSession: (d: string) => (sc ? sc.invalidate(d).catch(() => {}) : Promise.resolve()),

apps/api/src/routes/health.test.ts

@@ -0,0 +1,36 @@
+import { describe, expect, test } from "bun:test"
+import type { PoolStats } from "@trawl/types"
+import { healthRoute } from "./health"
+
+const stats = (live: number): PoolStats => ({
+  total: 1,
+  busy: live ? 1 : 0,
+  available: 0,
+  restarts: 0,
+  avgRestarts: 0,
+  stalled: live ? 0 : 1,
+  live,
+})
+
+describe("GET /health", () => {
+  test("returns 200 while the pool has live capacity", async () => {
+    const response = await healthRoute(() => stats(1)).handle(new Request("http://localhost/health"))
+
+    expect(response.status).toBe(200)
+    expect(await response.json()).toMatchObject({ status: "ok", pool: { live: 1 } })
+  })
+
+  test("returns 503 when no live capacity remains", async () => {
+    const response = await healthRoute(() => stats(0)).handle(new Request("http://localhost/health"))
+
+    expect(response.status).toBe(503)
+    expect(await response.json()).toMatchObject({ status: "starting", pool: { live: 0 } })
+  })
+
+  test("returns 503 before the pool is available", async () => {
+    const response = await healthRoute(() => undefined).handle(new Request("http://localhost/health"))
+
+    expect(response.status).toBe(503)
+    expect(await response.json()).toMatchObject({ status: "starting", pool: { total: 0 } })
+  })
+})

apps/api/src/routes/health.ts

@@ -3,21 +3,36 @@ import { Elysia } from "elysia"
 import { startTime } from "../config"
 import { getPool } from "../deps"
 
-export function healthRoute() {
+export function healthRoute(getStats = () => getPool()?.getStats()) {
   return new Elysia().get("/health", ({ set }) => {
-    const pool = getPool()
-    if (!pool) set.status = 503
+    const stats = 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),
     }
   })

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

apps/docs/api-reference/health-stats.md

@@ -46,22 +46,26 @@ Full system health check. Used by Docker Compose health checks and monitoring sy
     "busy": 1,
     "available": 4,
     "restarts": 0,
-    "avgRestarts": 0
+    "avgRestarts": 0,
+    "stalled": 0,
+    "live": 5
   }
 }
 ```
 
 | Field              | Type   | Description                              |
 | ------------------ | ------ | ---------------------------------------- |
-| `status`           | `"ok"` | Always `"ok"` when the API is reachable  |
+| `status`           | string | `"ok"` when the pool has live capacity; otherwise `"starting"` |
 | `uptime`           | number | Seconds since the API process started    |
 | `pool.total`       | number | Total browser instances in the pool      |
 | `pool.busy`        | number | Browsers currently processing a request  |
 | `pool.available`   | number | Browsers ready to accept a request       |
 | `pool.restarts`    | number | Total browser restarts since worker boot |
 | `pool.avgRestarts` | number | Average restarts per browser             |
+| `pool.stalled`     | number | Checked-out browsers past their deadline |
+| `pool.live`        | number | Connected, non-stalled browser capacity  |
 
-Pool stats are read directly from the browser pool. If the pool hasn't initialised yet, pool values will be zero.
+`/health` returns HTTP 503 while the pool is warming up or has no live browser capacity. A saturated but healthy pool remains ready because active, connected requests still count as live.
 
 ### Curl
 
@@ -82,7 +86,9 @@ Lightweight public stats for dashboards and landing pages.
   "browsers": 5,
   "available": 4,
   "busy": 1,
-  "restarts": 0
+  "restarts": 0,
+  "stalled": 0,
+  "live": 5
 }
 ```
 
@@ -92,6 +98,8 @@ Lightweight public stats for dashboards and landing pages.
 | `available` | number | Idle browsers                        |
 | `busy`      | number | Browsers in use                      |
 | `restarts`  | number | Total browser restarts since startup |
+| `stalled`   | number | Checked-out browsers past their deadline |
+| `live`      | number | Connected, non-stalled browser capacity |
 
 ### Curl
 

apps/docs/architecture/browser-pool.md

@@ -45,7 +45,10 @@ new BrowserPool({
   acquireTimeoutMs: 15000,    // BROWSER_ACQUIRE_TIMEOUT_MS — 15s default
   pollIntervalMs: 100,        // how often to re-check for an idle browser
   recycleAfterTemporaryContexts: 8,
-  contentProcesses: 2,         // BROWSER_CONTENT_PROCESSES — caps Firefox content procs
+  contentProcesses: 2,        // BROWSER_CONTENT_PROCESSES — caps Firefox content procs
+  stallAfterMs: 180000,       // BROWSER_STALL_TIMEOUT_MS
+  closeTimeoutMs: 10000,      // BROWSER_CLOSE_TIMEOUT_MS
+  launchTimeoutMs: 90000,     // BROWSER_LAUNCH_TIMEOUT_MS
 })
 ```
 
@@ -64,17 +67,9 @@ See issue #13 (original bug), #17 (recycle-on-suspect trade-off discussion), and
 
 ## Self-healing
 
-A health check runs every 30 seconds:
+A health check runs every 30 seconds. Disconnected idle browsers are relaunched in place, and checkouts that exceed the request budget plus `BROWSER_STALL_TIMEOUT_MS` are reclaimed. Lease tokens prevent a late release from an abandoned request from freeing a replacement checkout.
 
-```typescript
-for (const entry of this.entries) {
-  if (entry.busy) continue
-  const connected = entry.browser?.isConnected() ?? false
-  if (!connected) await this.restartEntry(entry)
-}
-```
-
-`browser.isConnected()` is a synchronous check. A disconnected browser is relaunched in place. `restartCount` increments so you can monitor via `/health`.
+Browser/context close and browser launch operations are bounded by `BROWSER_CLOSE_TIMEOUT_MS` and `BROWSER_LAUNCH_TIMEOUT_MS`. This keeps a wedged Firefox process from leaving a pool entry permanently stuck in restart. `/health` reports 503 when no connected, non-stalled capacity remains.
 
 ## Why Camoufox Firefox, not Chromium?
 

apps/docs/getting-started/configuration.md

@@ -85,6 +85,16 @@ BROWSER_CONTENT_PROCESSES=2   # default - conservative cap, lowest RAM/CPU
 BROWSER_CONTENT_PROCESSES=4   # raise if CF/Imperva challenges stall
 ```
 
+### Browser recovery timeouts
+
+| Variable | Default | Purpose |
+| --- | ---: | --- |
+| `BROWSER_STALL_TIMEOUT_MS` | `180000` | Grace period after a request's own timeout before its browser checkout is reclaimed |
+| `BROWSER_CLOSE_TIMEOUT_MS` | `10000` | Maximum wait for a wedged browser or context to close |
+| `BROWSER_LAUNCH_TIMEOUT_MS` | `90000` | Maximum wait for Camoufox to launch |
+
+These bounds keep an unresponsive Firefox process from permanently consuming a pool slot. The defaults are suitable for most installations.
+
 ## Session Cache
 
 ### `SESSION_TTL_SECONDS`