fix(showcase): correct browser-pool recovery docs and contain recovery throws

Reconcile the stale stats()/reinit()/acquire() comments: all-pending recovery
goes through relaunchPendingSlots, not the size-0 reinit backstop (which is the
truly-empty never-launched case). Rename the misleading reinit test to assert
the actual relaunchPendingSlots mechanism. Chain track()'s cleanup so a future
recovery throw cannot float as an unhandled rejection, and log a -1 sentinel
slotIndex for the never-added reinit close. Fix the self-contradicting
drainMicrotasks JSDoc.

Author: jpr5

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

@@ -117,12 +117,10 @@ function makeFakeLogger(): {
 }
 
 /**
- * `recycleSlot` kicks off an async relaunch via a fire-and-forget IIFE.
- * Tests that observe post-recycle state need to await the in-flight
- * promise to settle. `shutdown()` already awaits `inFlightRecycles`, so
- * this helper is just `await pool.shutdown()` from the test side; here
- * we use a small drain helper for tests that want to observe state
- * without tearing the pool down.
+ * Flush pending microtasks `times` times so post-recycle state settles
+ * enough to observe without tearing the pool down. It does NOT advance real
+ * `setTimeout` backoff timers — tests that depend on backoff elapsing use
+ * `waitFor` instead.
  */
 async function drainMicrotasks(times = 5): Promise<void> {
   for (let i = 0; i < times; i++) {
@@ -316,13 +314,17 @@ describe("BrowserPool dead-instance detection", () => {
     await pool.shutdown();
   });
 
-  it("re-initializes the pool when every slot has been lost (size reaches 0)", async () => {
-    // 2-slot pool. Both browsers crash and EVERY relaunch attempt fails for
-    // both slots, driving the pool to size 0 so the backstop reinit() path
-    // is actually exercised. Each slot makes RELAUNCH_MAX_ATTEMPTS (=3)
-    // launch calls during recycle, so for 2 slots that is launch calls 3..8
-    // (init used calls 1 and 2). Failing 3..8 exhausts all retries and
-    // evicts both slots to leave size 0. Call 9 (the reinit) succeeds.
+  it("recovers a fully-parked pool (all slots relaunchPending, size 0) via the lazy relaunchPendingSlots path on the next acquire", async () => {
+    // 2-slot pool. Both browsers crash and EVERY immediate relaunch attempt
+    // fails for both slots. Each slot makes RELAUNCH_MAX_ATTEMPTS (=3) launch
+    // calls during recycle, so for 2 slots that is launch calls 3..8 (init
+    // used calls 1 and 2). Failing 3..8 exhausts every immediate retry and
+    // parks BOTH slots as `relaunchPending` — they are NOT evicted from
+    // `this.slots` (capacity is preserved), so `stats().size` reads 0 during
+    // the outage while the slots stay parked. Recovery is therefore via the
+    // lazy `relaunchPendingSlots()` path on the next acquire (launch call 9
+    // succeeds), NOT the `reinit()` backstop (which only fires when
+    // `this.slots` is truly empty). This test asserts that actual mechanism.
     const { launchBrowser, launched } = makeFakeLauncher({
       failAtCalls: [3, 4, 5, 6, 7, 8],
     });
@@ -334,17 +336,17 @@ describe("BrowserPool dead-instance detection", () => {
     launched[0]!.__crash();
     launched[1]!.__crash();
     // The recycle uses real setTimeout backoff between attempts, so give it
-    // a wall-clock window to exhaust all retries and park/evict both slots.
+    // a wall-clock window to exhaust all retries and park both slots.
     await waitFor(() => pool.stats().size === 0, 5_000);
 
-    // Precondition: prove the pool actually drained to zero so reinit() is
-    // the path under test (the old [3,4] fixture self-healed on attempt 2
-    // and never reached this state — the test was vacuous).
+    // Precondition: every slot is parked, so live capacity (`size`) reads 0
+    // during the outage. The slots themselves are retained — recovery comes
+    // through the relaunchPending path, not a from-scratch reinit.
     expect(pool.stats().size).toBe(0);
 
-    // With size 0, the next acquire() must trigger the backstop reinit
-    // (launch call 9 succeeds) and hand back a live browser instead of
-    // hanging until timeout.
+    // The next acquire() recovers the parked slots via relaunchPendingSlots
+    // (launch call 9 succeeds) and hands back a live browser instead of
+    // hanging until timeout. `size` returns to non-zero once recovered.
     const acquired = await pool.acquire(5_000);
     expect(acquired).toBeDefined();
     expect((acquired as unknown as FakeBrowser).isConnected()).toBe(true);

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

@@ -178,18 +178,32 @@ export class BrowserPool {
    */
   private track(promise: Promise<void>): Promise<void> {
     this.inFlightRecycles.add(promise);
-    promise.finally(() => {
-      this.inFlightRecycles.delete(promise);
-    });
-    return promise;
+    // Chain the cleanup onto what we return (and await), and absorb any throw
+    // with a logged `.catch` so it can never surface as an unhandled
+    // rejection. The inner methods swallow their own launch errors today, but
+    // a future throw inside reinitInner/relaunchPendingSlotsInner must stay
+    // contained — a recovery launch failing is non-fatal to the pool.
+    return promise
+      .catch((err: unknown) => {
+        this.logger?.info("browser-pool.recovery-failed", {
+          error: err instanceof Error ? err.message : String(err),
+        });
+      })
+      .finally(() => {
+        this.inFlightRecycles.delete(promise);
+      });
   }
 
   /**
-   * Backstop re-initialization. Called from `acquire()` when the pool has
-   * drained to zero slots. Re-launches up to `poolSize` browsers, tolerating
-   * partial failure — even one fresh slot lets a waiting probe proceed. A
-   * total failure here surfaces to the caller via the normal acquire
-   * timeout / waiter-reject path rather than wedging the pool forever.
+   * Backstop re-initialization for the truly-empty pool. Called from
+   * `acquire()` only when `this.slots` is empty — i.e. nothing was ever
+   * launched (init never ran / launched nothing). This is NOT the all-slots-
+   * crashed recovery path: failed recycles keep their slots in `this.slots`
+   * (parked `relaunchPending`) and are recovered by `relaunchPendingSlots()`,
+   * so they never reach this backstop. Re-launches up to `poolSize` browsers,
+   * tolerating partial failure — even one fresh slot lets a waiting probe
+   * proceed. A total failure here surfaces to the caller via the normal
+   * acquire timeout / waiter-reject path rather than wedging the pool forever.
    */
   private async reinit(): Promise<void> {
     if (this.isShutdown || this.launchBrowser === undefined) return;
@@ -209,7 +223,10 @@ export class BrowserPool {
       try {
         const browser = await this.launchBrowser();
         if (this.isShutdown) {
-          await this.closeBrowser(browser, this.slots.length);
+          // This browser was never added to `this.slots`, so there is no real
+          // originating slot index. Pass -1 rather than a fabricated index
+          // (`this.slots.length`) that would masquerade as a live slot.
+          await this.closeBrowser(browser, -1);
           return;
         }
         const slot: Slot = { browser, contextCount: 0 };
@@ -321,10 +338,14 @@ export class BrowserPool {
       throw new Error("BrowserPool is shut down");
     }
 
-    // Backstop: if every slot has been lost (e.g. a burst of crashes whose
-    // relaunches all failed), the pool would otherwise be permanently empty
-    // and every acquire would time out. Re-initialize from scratch so the
-    // pool self-heals instead of draining to nothing.
+    // Backstop: only when `this.slots` is truly empty — nothing was ever
+    // launched (init() never ran or launched nothing) — does the pool have no
+    // slot to recover, so reinit from scratch. NOTE: a burst of crashes whose
+    // relaunches all fail does NOT empty `this.slots`; those slots are kept
+    // and parked as `relaunchPending`, and are recovered below via
+    // `relaunchPendingSlots()`, not here. (`stats().size` may read 0 in that
+    // state, but `size` does not gate this backstop — `this.slots.length`
+    // does.)
     if (this.slots.length === 0) {
       await this.reinit();
     } else {
@@ -452,9 +473,12 @@ export class BrowserPool {
   stats(): BrowserPoolStats {
     // `size` counts only live (non-pending) capacity. A slot parked as
     // `relaunchPending` holds a dead browser and is being recovered lazily,
-    // so it is not usable capacity and must not inflate `size`/`inUse`. This
-    // also makes the empty-pool backstop observable: when every slot is
-    // pending, `size` reads 0 (the condition under which acquire() recovers).
+    // so it is not usable capacity and must not inflate `size`/`inUse`. When
+    // every slot is pending, `size` reads 0 — that surfaces the outage to
+    // callers, but it is NOT what gates recovery. The parked slots remain in
+    // `this.slots`, so `acquire()` recovers them via `relaunchPendingSlots()`
+    // (the `this.slots.length === 0` reinit backstop is a separate
+    // never-launched case), not via this size reading.
     const liveSlots = this.slots.filter((s) => !s.relaunchPending);
     const availableCount = this.available.length;
     return {