fix(showcase/harness): harden browser-pool non-release teardown paths (CopilotKit#5174)

## Summary

Follow-up to CopilotKit#5173 (bucket-(d)) closing three **pre-existing**
browser-pool concurrency defects on the non-release teardown paths. The
browser-pool is a context-pool over a fixed set of long-lived Chromium
processes; these defects biased the hygiene-recycle cadence and could
strand waiters / leak deferred recycles.

## Fixes (each red-green proven)

1. **`serveNextWaiter` orphan-close leaked `servedContexts`.** A waiter
timing out mid-`openContextOn` cleaned up the reservation/context but
never decremented `servedContexts` (which `openContextOn` had already
`++`'d) → every orphaned-by-timeout serve permanently inflated the count
→ premature hygiene recycles. Fix: decrement `servedContexts` in the
orphan-close block.
2. **Deferred `recyclePending` honored only on `release()`.** A recycle
deferred because `pendingOpens > 0` set `recyclePending`, but the
non-release teardown paths (orphan-by-recycle rollback, orphan-close)
returned the entry to idle without re-checking it → the deferred recycle
was dropped and the browser exceeded `recycleAfter` indefinitely. Fix:
shared `maybeFireDeferredRecycle(entry)` helper called on both
non-release teardown paths.
3. **`openContextOn` rollback didn't drain waiters.** The
orphan-by-recycle rollback freed a reservation but never
`scheduleServeNextWaiter()` → queued waiters could stall with free
capacity until an unrelated release. Fix: `scheduleServeNextWaiter()`
after the rollback.

## Verification
- Red-green for all three (reproduced each bug, then green).
- Full harness vitest: **1702–1705 passed**; browser-pool suite
**27/27** (mutation-tested — reverting fix CopilotKit#3 fails its guard). `tsc
--noEmit` exit 0.
- Public API + `BROWSER_POOL_MAX_CONTEXTS` default untouched.

## Reviewed
7-agent unbiased CR (cr-loop): the three fixes confirmed sound; one
ordering concern on the new code investigated and **refuted**
(sole-browser relaunch-failure rejects the waiter rather than stranding
it).

## Known further hardening (separate effort — NOT in this PR)
The CR surfaced additional **pre-existing** browser-pool reliability
bugs that warrant a dedicated hardening pass, independently flagged by
multiple reviewers:
- `shutdown()` vs in-flight `openContextOn` → leaked context (no
`isShutdown` re-check post-`newContext`); recycles added to
`inFlightRecycles` after shutdown's snapshot not awaited.
- crash-reason `recycleBrowser` abandons live contexts without
`.close()` — leaks on the non-dead `acquire`-retry path.
- `acquire` retry treats a transient `newContext` failure as a full
crash → recycles the whole browser, tearing down unrelated live contexts
(correlated flakiness).
- `launchChain` launch gate has no timeout → a single hung
`chromium.launch()` permanently deadlocks all relaunches.
- `parseInt` env parsing silently accepts trailing garbage
(`MAX_CONTEXTS=24x` → 24); no warning.
- context-close failures swallowed via bare `.catch(() => {})`
(inconsistent with `closeBrowser`'s logged path).
- relaunch-failure eviction only rejects waiters when the pool is fully
empty (doesn't redistribute onto surviving browsers); `pickLeastLoaded`
tie-break concentrates load on browser 0.

Author: jpr5

showcase/harness/src/probes/helpers/browser-pool.test.ts

@@ -1150,4 +1150,220 @@ describe("BrowserPool — context pooling over fixed browser set", () => {
     await pool.release(c2!);
     await pool.shutdown();
   });
+
+  // ── BUCKET-D CONCURRENCY-HARDENING REGRESSIONS ────────────────────────────
+  // Three PRE-EXISTING defects in the non-release teardown paths, each driven
+  // by an interleave the sequential cases above never exercised.
+
+  // Internal-state probe: read a private BrowserEntry field for assertions the
+  // public stats() surface does not expose (servedContexts / recyclePending).
+  // The pool keeps these per-entry; the only way to assert the orphan-close
+  // bookkeeping is to read them directly.
+  const entry0 = (
+    pool: BrowserPool,
+  ): {
+    servedContexts: number;
+    recyclePending: boolean;
+    pendingOpens: number;
+  } =>
+    (
+      pool as unknown as {
+        browsers: Array<{
+          servedContexts: number;
+          recyclePending: boolean;
+          pendingOpens: number;
+        }>;
+      }
+    ).browsers[0]!;
+
+  // BUG 1 — a waiter that times out WHILE serveNextWaiter is opening its
+  // context leaks `servedContexts`. openContextOn did servedContexts++ when it
+  // added the never-delivered context; the orphan-close cleanup decrements the
+  // reservation + drops liveContexts but (pre-fix) does NOT decrement
+  // servedContexts. So every timed-out-mid-serve permanently inflates
+  // servedContexts, biasing the hygiene recycle to fire early. The fix mirrors
+  // the increment with a decrement in the orphan-close block.
+  it("BUG1: a waiter that times out mid-serveNextWaiter does NOT inflate servedContexts", async () => {
+    const { launchBrowser, launched } = makeFakeLauncher({
+      deferNewContextForCalls: [1],
+    });
+    const pool = new BrowserPool({
+      browsers: 1,
+      maxContexts: 1,
+      recycleAfter: 1000, // keep hygiene out of the picture; assert the count
+      launchBrowser,
+      launchStaggerMs: 0,
+    });
+    await pool.init();
+
+    // Fill the cap with one context (parked open → release it).
+    const c1p = pool.acquire();
+    await drainMicrotasks();
+    launched[0]!.__releaseNewContexts();
+    const c1 = await c1p;
+    expect(pool.stats().inUse).toBe(1);
+    // One context genuinely served.
+    expect(entry0(pool).servedContexts).toBe(1);
+
+    // Enqueue a waiter with a SHORT timeout — it pends past the cap.
+    let waiterRejected: Error | undefined;
+    const waiterP = pool
+      .acquire(undefined, 20)
+      .catch((e: Error) => (waiterRejected = e));
+    await drainMicrotasks();
+
+    // Release c1 → serveNextWaiter() picks the waiter, calls newContext()
+    // (parked). While that open is in flight, let the 20ms timeout fire.
+    await pool.release(c1);
+    await drainMicrotasks();
+    await new Promise((r) => setTimeout(r, 40)); // waiter times out now
+    expect(waiterRejected).toBeInstanceOf(Error);
+
+    // Let the parked newContext() for the dead waiter resolve → orphan-close.
+    launched[0]!.__releaseNewContexts();
+    await drainMicrotasks();
+    await waiterP;
+
+    // The orphaned serve must NOT count toward servedContexts: the context was
+    // opened (servedContexts++) but never delivered, then closed + rolled back.
+    // Pre-fix: servedContexts stuck at 2 (the orphan inflated it). Fixed: 1.
+    expect(entry0(pool).servedContexts).toBe(1);
+    expect(pool.stats().inUse).toBe(0);
+
+    await pool.shutdown();
+  });
+
+  // BUG 2 — a hygiene recycle deferred via the `shouldRecycle && hadWaiter`
+  // guard sets recyclePending=true, expecting "the next idle release honors it."
+  // But if the just-served waiter's lifecycle ends via a NON-release teardown
+  // (here: the waiter times out mid-serve → serveNextWaiter orphan-close), no
+  // release() ever fires, so the deferred recycle is dropped and the browser
+  // exceeds recycleAfter indefinitely. The fix re-checks the deferred recycle on
+  // the non-release teardown paths.
+  it("BUG2: a deferred hygiene recycle still fires when the just-served waiter ends via a non-release teardown", async () => {
+    const { launchBrowser, launched } = makeFakeLauncher({
+      deferNewContextForCalls: [1], // browser0 defers every newContext
+    });
+    const { logger } = makeLeveledLogger();
+    const pool = new BrowserPool({
+      browsers: 1,
+      maxContexts: 1,
+      recycleAfter: 1, // eligible after the first served context
+      logger,
+      launchBrowser,
+      launchStaggerMs: 0,
+    });
+    await pool.init();
+    expect(launched.length).toBe(1);
+
+    // Fill the cap with one context (parked open → release the park).
+    const c1p = pool.acquire();
+    await drainMicrotasks();
+    launched[0]!.__releaseNewContexts();
+    const c1 = await c1p;
+    expect(pool.stats().inUse).toBe(1);
+    expect(entry0(pool).servedContexts).toBe(1); // >= recycleAfter
+
+    // Queue a waiter W past the cap with a SHORT timeout.
+    let wRejected: Error | undefined;
+    const wP = pool.acquire(undefined, 20).catch((e: Error) => (wRejected = e));
+    await drainMicrotasks();
+
+    // Release c1: the boundary-crossing release (servedContexts>=recycleAfter,
+    // entry momentarily idle) computes shouldRecycle=true AND has a queued waiter
+    // → it serves W (parks in deferred newContext) and DEFERS the recycle, setting
+    // recyclePending=true. No recycle fires now.
+    await pool.release(c1);
+    await drainMicrotasks();
+    expect(pool.stats().totalRecycles).toBe(0);
+    expect(entry0(pool).recyclePending).toBe(true);
+
+    // W now times out WHILE its serve's newContext() is still parked in flight.
+    await new Promise((r) => setTimeout(r, 40));
+    expect(wRejected).toBeInstanceOf(Error);
+
+    // Resolve the parked open for the dead waiter → serveNextWaiter orphan-close,
+    // a NON-release teardown that returns the entry to idle. Pre-fix: nothing
+    // re-checks the deferred recycle here, so recyclePending stays true forever
+    // and the browser is never recycled (totalRecycles stuck at 0). The fix
+    // re-checks isEntryRecyclable && recyclePending on the orphan-close path and
+    // fires the deferred recycle.
+    launched[0]!.__releaseNewContexts();
+    await wP;
+    await waitFor(() => pool.stats().totalRecycles === 1);
+    expect(pool.stats().totalRecycles).toBe(1);
+    expect(entry0(pool).recyclePending).toBe(false);
+
+    await pool.shutdown();
+  });
+
+  // BUG 3 — when an in-flight open is orphaned by a recycle and rolls back via
+  // releaseReservation(), it does NOT drain the waiter queue. So a queued waiter
+  // can stall with free capacity until the next unrelated release/recycle
+  // handoff. The fix calls scheduleServeNextWaiter() after the rollback so freed
+  // capacity immediately serves the waiter.
+  it("BUG3: an open orphaned by recycle drains a queued waiter from the freed capacity", async () => {
+    // browser0 (init) defers its first newContext so we can park an in-flight
+    // open and orphan it; its relaunch (call 2) does NOT defer, so a waiter
+    // served onto the fresh browser resolves immediately.
+    const { launchBrowser, launched } = makeFakeLauncher({
+      deferNewContextForCalls: [1],
+    });
+    const { logger } = makeLeveledLogger();
+    const pool = new BrowserPool({
+      browsers: 1,
+      maxContexts: 1,
+      recycleAfter: 1000, // hygiene out of the picture
+      logger,
+      launchBrowser,
+      launchStaggerMs: 0,
+    });
+    await pool.init();
+    expect(launched.length).toBe(1);
+
+    // Acquire A parks in newContext() on browser0 (pendingOpens=1, reservation
+    // held → cap saturated at 1).
+    let aDone = false;
+    const aP = pool
+      .acquire(undefined, 5_000)
+      .then(() => (aDone = true))
+      .catch(() => {});
+    await drainMicrotasks();
+    expect(launched[0]!.__pendingNewContexts).toBe(1);
+    expect(pool.stats().inUse).toBe(1);
+
+    // Acquire B pends as a WAITER past the cap (cap=1, A holds the only slot).
+    let b: BrowserContext | undefined;
+    const bP = pool
+      .acquire(undefined, 5_000)
+      .then((c) => (b = c))
+      .catch(() => {});
+    await drainMicrotasks();
+    expect(b).toBeUndefined();
+
+    // CRASH browser0 → recovery recycle reassigns entry.browser to a fresh,
+    // NON-deferring process (call 2). Resolve A's parked open on the ORIGINAL
+    // (now-replaced) browser: the orphan guard closes it and rolls back the
+    // reservation — freeing the only cap slot.
+    launched[0]!.__crash();
+    await waitFor(() => launched.length === 2);
+    launched[0]!.__releaseNewContexts(); // A's open resolves → orphaned + rolled back
+    await drainMicrotasks();
+
+    // BUG3 INVARIANT: the freed slot must immediately serve waiter B from the
+    // rollback's scheduleServeNextWaiter(). Pre-fix the rollback did not drain
+    // the queue, so B stalled with free capacity. The crash-recovery relaunch's
+    // own drain loop would EVENTUALLY serve B, so to isolate the rollback-drain
+    // we assert B resolves promptly after the rollback without any further pool
+    // activity.
+    await waitFor(() => b !== undefined, 1_000);
+    expect(b).toBeDefined();
+    expect(pool.stats().inUse).toBe(1); // exactly B is live
+
+    void aDone;
+    await aP;
+    await bP;
+    await pool.release(b!);
+    await pool.shutdown();
+  });
 });

showcase/harness/src/probes/helpers/browser-pool.ts

@@ -395,6 +395,28 @@ export class BrowserPool {
     return !entry.recycling && this.isEntryIdle(entry);
   }
 
+  /**
+   * Fire a hygiene recycle that was previously DEFERRED (carried on
+   * `entry.recyclePending`) if the entry is now genuinely recyclable. This is
+   * the shared "honor the deferred recycle" check that EVERY path which returns
+   * an entry to idle must run — not just `release()`. The release path defers a
+   * hygiene recycle (e.g. because a waiter was just served onto the freed slot)
+   * and sets `recyclePending`, expecting the next idle release to honor it; but
+   * an entry can also return to idle via NON-release teardowns (the
+   * `openContextOn` orphan-by-recycle rollback and the `serveNextWaiter`
+   * orphan-close), where no `release()` ever fires. Without re-checking here the
+   * deferred recycle is dropped and the browser exceeds `recycleAfter`
+   * indefinitely. Race-safe: the predicate + flag clear + recycle dispatch are
+   * all synchronous (no await straddles them), and `recycleBrowser` itself
+   * re-guards on `recycling`/`pendingOpens`.
+   */
+  private maybeFireDeferredRecycle(entry: BrowserEntry): void {
+    if (entry.recyclePending && this.isEntryRecyclable(entry)) {
+      entry.recyclePending = false;
+      this.recycleBrowser(entry, "hygiene");
+    }
+  }
+
   /**
    * Open a context on the given live browser against a reservation the caller
    * ALREADY took via `reserveSlot()`. Centralizes the `X-AIMock-Strict` default
@@ -455,6 +477,15 @@ export class BrowserPool {
       this.logger?.warn?.("browser-pool.open-orphaned-by-recycle", {
         browserIndex: this.browsers.indexOf(entry),
       });
+      // NON-release teardown: this in-flight open ended without a release(), so
+      // the release-path's deferred-recycle re-check (recyclePending) and waiter
+      // drain never run for the capacity this rollback just freed. Mirror them
+      // here. (Bug 2) honor a recycle that was deferred behind this very open,
+      // now that pendingOpens dropped and the entry may be idle; (Bug 3) drain a
+      // queued waiter onto the freed slot instead of stalling it until the next
+      // unrelated release/recycle handoff. Both run on synchronous state.
+      this.maybeFireDeferredRecycle(entry);
+      this.scheduleServeNextWaiter();
       throw new Error("browser-pool: context open orphaned by recycle");
     }
 
@@ -657,11 +688,25 @@ export class BrowserPool {
     if (waiter.settled) {
       this.contextToBrowser.delete(context);
       entry.liveContexts.delete(context);
+      // openContextOn already did `entry.servedContexts++` for this context.
+      // Since it is being orphan-closed and never delivered, mirror that
+      // increment with a decrement — otherwise every timed-out-mid-serve
+      // permanently inflates servedContexts and biases the hygiene recycle
+      // (servedContexts >= recycleAfter) to fire early, wasting PID budget on
+      // relaunches. (Bug 1)
+      entry.servedContexts--;
       this.releaseReservation();
       void context.close().catch(() => {});
       this.logger?.warn?.("browser-pool.serve-waiter-orphan-closed", {
         browserIndex: this.browsers.indexOf(entry),
       });
+      // NON-release teardown: returning this entry to idle here without a
+      // release() means the release-path's deferred-recycle re-check never runs.
+      // Honor a hygiene recycle that was deferred behind this serve (set via the
+      // release-path `shouldRecycle && hadWaiter` guard) now that the entry is
+      // idle again — otherwise the deferred recycle is dropped and the browser
+      // exceeds recycleAfter indefinitely. (Bug 2)
+      this.maybeFireDeferredRecycle(entry);
       if (this.waiters.length > 0) this.scheduleServeNextWaiter();
       return;
     }