Skip to content

Commit 969265c

Browse files
authored
fix(showcase): harden dashboard chip semantics + browser-pool capacity logging (CopilotKit#5118)
## Summary Follow-up hardening for the showcase Coverage dashboard and harness browser-pool (deferred items from CopilotKit#5112): - **Gate the green chip on a contiguous depth ladder.** A green `d6:<slug>` aggregate over a red/absent/no-data D5 no longer renders green; a never-run D5 reads gray (pending) instead of red. Decision table over `(d1d4Gate, d5.exists, d5.status, d6.exists, d6.status)` preserves all prior intended outcomes. - **Staleness-gate D1/D2/D4 with per-driver windows.** D4 (`chat`/`tools`, 15-min cadence) uses a 1h window; D1/D2 (`health`/`agent`, 5-min cadence) use 45m — applied in both `resolveD4` and the deprecated `deriveDepth`, so a frozen-green liveness/round-trip row no longer credits depth (mirrors the D3/D5/D6 staleness already shipped). - **`resolveD5` strict on missing mapped sub-rows.** A missing pill in a multi-key family (e.g. `beautiful-chat`) is unverified, not passing → returns `status: null` (caps depth, reads gray), matching `isD5Green`. - **Compute `CellModel.isRegression`** (`ceilingDepth > 0 && achievedDepth < ceilingDepth`) and drive the Coverage regressions filter from the model field. - **Harness:** widen `PoolLogger` to `warn`/`error` and route capacity-loss events (exhausted relaunch, recovery failure, empty-pool reinit) to the proper severity so they reach the warn/error pipeline; add a `reinit()` concurrent-entry guard. ## Test plan - [ ] `showcase/shell-dashboard`: `npm test` (661 pass) incl. new chip-contiguity, per-driver-staleness, strict-D5, and isRegression cases (red-green). - [ ] `showcase/harness`: `npx nx test showcase-harness` (1657 pass) incl. logger-severity-routing and reinit-guard tests. - [ ] Both packages: typecheck + build green.
2 parents 17743cd + c918051 commit 969265c

9 files changed

Lines changed: 790 additions & 101 deletions

File tree

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

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,41 @@ function makeFakeLogger(): {
128128
};
129129
}
130130

131+
interface LeveledLog extends CapturedLog {
132+
level: "info" | "warn" | "error";
133+
}
134+
135+
/**
136+
* Fake logger that captures the SEVERITY each event was emitted at. Unlike
137+
* `makeFakeLogger` (info-only), this exposes `warn`/`error` so routing-by-
138+
* severity is observable: a capacity-loss event logged via `logger.error(...)`
139+
* must show up with `level: "error"`, never `level: "info"`.
140+
*/
141+
function makeLeveledLogger(): {
142+
logger: {
143+
info: (msg: string, meta?: Record<string, unknown>) => void;
144+
warn: (msg: string, meta?: Record<string, unknown>) => void;
145+
error: (msg: string, meta?: Record<string, unknown>) => void;
146+
};
147+
events: LeveledLog[];
148+
} {
149+
const events: LeveledLog[] = [];
150+
return {
151+
logger: {
152+
info(msg, meta) {
153+
events.push({ level: "info", event: msg, meta });
154+
},
155+
warn(msg, meta) {
156+
events.push({ level: "warn", event: msg, meta });
157+
},
158+
error(msg, meta) {
159+
events.push({ level: "error", event: msg, meta });
160+
},
161+
},
162+
events,
163+
};
164+
}
165+
131166
/**
132167
* Flush pending microtasks `times` times so post-recycle state settles
133168
* enough to observe without tearing the pool down. It does NOT advance real
@@ -700,6 +735,135 @@ describe("BrowserPool dead-instance detection", () => {
700735
await Promise.allSettled([a, b]);
701736
});
702737

738+
it("routes capacity-loss events to error and per-attempt failures to warn", async () => {
739+
// Single-slot pool. The browser crashes and EVERY immediate relaunch
740+
// attempt fails (calls 2..4 — RELAUNCH_MAX_ATTEMPTS=3), so each per-attempt
741+
// close/relaunch failure must log at warn while the terminal capacity-loss
742+
// event (`recycle-relaunch-failed`) must log at error.
743+
const { launchBrowser, launched } = makeFakeLauncher({
744+
failAtCalls: [2, 3, 4],
745+
});
746+
const { logger, events } = makeLeveledLogger();
747+
const pool = new BrowserPool(1, 100, logger, launchBrowser);
748+
await pool.init();
749+
expect(launched).toHaveLength(1);
750+
751+
launched[0]!.__crash();
752+
await waitFor(() => pool.stats().size === 0, 3_000);
753+
754+
// Terminal capacity-loss after all retries → error.
755+
const recycleFailed = events.find(
756+
(e) => e.event === "browser-pool.recycle-relaunch-failed",
757+
);
758+
expect(recycleFailed).toBeDefined();
759+
expect(recycleFailed?.level).toBe("error");
760+
761+
// The capacity-loss event must NOT also have been emitted at info.
762+
expect(
763+
events.some(
764+
(e) =>
765+
e.event === "browser-pool.recycle-relaunch-failed" &&
766+
e.level === "info",
767+
),
768+
).toBe(false);
769+
770+
await pool.shutdown();
771+
});
772+
773+
it("routes per-attempt relaunch-failed (lazy recovery) to warn", async () => {
774+
// Single-slot pool. Crash + all immediate retries fail (calls 2..4) parks
775+
// the slot. A first acquire's lazy relaunchPendingSlots attempt (call 5)
776+
// ALSO fails → `relaunch-failed` must log at warn (per-attempt, not a
777+
// terminal capacity loss).
778+
const { launchBrowser, launched } = makeFakeLauncher({
779+
failAtCalls: [2, 3, 4, 5],
780+
});
781+
const { logger, events } = makeLeveledLogger();
782+
const pool = new BrowserPool(1, 100, logger, launchBrowser);
783+
await pool.init();
784+
785+
launched[0]!.__crash();
786+
await waitFor(() => pool.stats().size === 0, 3_000);
787+
788+
// First acquire drives a relaunchPendingSlots attempt (call 5) which fails.
789+
await expect(pool.acquire(50)).rejects.toThrow(
790+
"BrowserPool acquire timeout",
791+
);
792+
793+
const relaunchFailed = events.find(
794+
(e) => e.event === "browser-pool.relaunch-failed",
795+
);
796+
expect(relaunchFailed).toBeDefined();
797+
expect(relaunchFailed?.level).toBe("warn");
798+
799+
await pool.shutdown();
800+
});
801+
802+
it("emits an error when reinit ends with an empty pool (every relaunch failed)", async () => {
803+
// 1-slot pool. After init (call 1), every subsequent launch fails. The
804+
// backstop reinit() can only run when this.slots is truly empty, so we
805+
// construct a never-launched pool: init throws nothing (call 1 succeeds),
806+
// but to reach the empty-pool reinit path we use a 1-slot pool whose ONLY
807+
// slot was never created because init's launch failed. Use failAt=1 so
808+
// init launches nothing, leaving this.slots empty; the next acquire drives
809+
// reinit, whose launch (call 2) also fails, ending the pool empty → error.
810+
const { launchBrowser } = makeFakeLauncher({ failAtCalls: [1, 2, 3] });
811+
const { logger, events } = makeLeveledLogger();
812+
const pool = new BrowserPool(1, 100, logger, launchBrowser);
813+
// init's only launch (call 1) throws — init sets `launchBrowser` BEFORE the
814+
// launch loop, so after catching the throw `this.slots` is empty but the
815+
// pool can still reinit. (init does not swallow launch errors by design.)
816+
await expect(pool.init()).rejects.toThrow("simulated launch failure");
817+
expect(pool.stats().size).toBe(0);
818+
819+
// acquire drives the empty-pool reinit backstop; its launch (call 2) fails,
820+
// so reinitInner's loop ends with this.slots.length === 0 → error emit.
821+
await expect(pool.acquire(50)).rejects.toThrow(
822+
"BrowserPool acquire timeout",
823+
);
824+
825+
const reinitEmpty = events.find(
826+
(e) => e.event === "browser-pool.reinit-empty",
827+
);
828+
expect(reinitEmpty).toBeDefined();
829+
expect(reinitEmpty?.level).toBe("error");
830+
831+
await pool.shutdown();
832+
});
833+
834+
it("does not overshoot poolSize when two concurrent acquires hit an empty pool", async () => {
835+
// Empty pool (init launches nothing). Two concurrent acquire() calls both
836+
// see this.slots.length === 0. WITHOUT a reiniting guard, BOTH drive
837+
// reinit() and each launches up to poolSize browsers — overshoot. WITH the
838+
// guard, the second acquire skips reinit and falls through to the waiter
839+
// queue, so at most poolSize browsers are launched.
840+
const poolSize = 2;
841+
// init = call 1 fails (so this.slots stays empty); reinit launches succeed.
842+
const { launchBrowser, launched } = makeFakeLauncher({ failAtCalls: [1] });
843+
const { logger } = makeLeveledLogger();
844+
const pool = new BrowserPool(poolSize, 100, logger, launchBrowser);
845+
// init's first launch (call 1) throws, leaving this.slots empty but with
846+
// `launchBrowser` already set so reinit can run. init does not swallow.
847+
await expect(pool.init()).rejects.toThrow("simulated launch failure");
848+
expect(pool.stats().size).toBe(0);
849+
850+
const a = pool.acquire(5_000);
851+
const b = pool.acquire(5_000);
852+
853+
const first = await Promise.race([a, b]);
854+
expect(first).toBeDefined();
855+
expect((first as unknown as FakeBrowser).isConnected()).toBe(true);
856+
857+
await drainMicrotasks(30);
858+
859+
// The guard ensures only ONE reinit ran: at most poolSize browsers exist.
860+
expect(launched.length).toBeLessThanOrEqual(poolSize);
861+
expect(pool.stats().size).toBeLessThanOrEqual(poolSize);
862+
863+
await pool.shutdown();
864+
await Promise.allSettled([a, b]);
865+
});
866+
703867
it("shutdown() does not loop the disconnect handler when closing slot browsers", async () => {
704868
const { launchBrowser, launched } = makeFakeLauncher();
705869
const { logger, events } = makeFakeLogger();

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

Lines changed: 64 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,17 @@ export interface BrowserPoolStats {
4242

4343
/**
4444
* Minimal logger surface the pool uses for lifecycle events. Matches the
45-
* harness-wide `Logger` interface but only the `info` method is required.
46-
* Optional so existing callers (tests, legacy boot paths) don't break.
45+
* harness-wide `Logger` interface but only the `info` method is required;
46+
* `warn`/`error` are OPTIONAL so existing callers (tests, legacy boot paths)
47+
* that inject a bare-`info` fake still type-check. The concrete harness logger
48+
* implements all three (warn/error route to stderr → Sentry), so capacity-loss
49+
* events emitted via `error` reach the alert pipeline instead of being buried
50+
* at info. Call the optional methods safely: `this.logger?.warn?.(...)`.
4751
*/
4852
interface PoolLogger {
4953
info(msg: string, meta?: Record<string, unknown>): void;
54+
warn?(msg: string, meta?: Record<string, unknown>): void;
55+
error?(msg: string, meta?: Record<string, unknown>): void;
5056
}
5157

5258
/**
@@ -73,6 +79,15 @@ export class BrowserPool {
7379
// the same slot — which would leak one process and/or publish the slot to
7480
// `available` twice.
7581
private relaunchingSlots = new Set<Slot>();
82+
// Re-entry guard for the empty-pool reinit backstop, mirroring the
83+
// relaunchingSlots/recyclingSlots idiom. Two concurrent acquire() calls
84+
// against a truly-empty pool (this.slots.length === 0) would each drive
85+
// reinit() and each launch up to poolSize browsers — overshooting capacity.
86+
// Setting this synchronously before reinit's first await makes the second
87+
// acquire skip reinit and fall through to the waiter queue, where the first
88+
// reinit's handOff serves it. Cleared in a finally so a thrown reinit can't
89+
// wedge the guard set forever.
90+
private reiniting = false;
7691
private isShutdown = false;
7792
private readonly logger?: PoolLogger;
7893
private readonly injectedLaunchBrowser?: LaunchBrowser;
@@ -156,7 +171,7 @@ export class BrowserPool {
156171
try {
157172
await browser.close();
158173
} catch (err) {
159-
this.logger?.info("browser-pool.close-failed", {
174+
this.logger?.warn?.("browser-pool.close-failed", {
160175
slotIndex,
161176
error: err instanceof Error ? err.message : String(err),
162177
});
@@ -178,7 +193,7 @@ export class BrowserPool {
178193
// contained — a recovery launch failing is non-fatal to the pool.
179194
return promise
180195
.catch((err: unknown) => {
181-
this.logger?.info("browser-pool.recovery-failed", {
196+
this.logger?.error?.("browser-pool.recovery-failed", {
182197
error: err instanceof Error ? err.message : String(err),
183198
});
184199
})
@@ -200,10 +215,20 @@ export class BrowserPool {
200215
*/
201216
private async reinit(): Promise<void> {
202217
if (this.isShutdown || this.launchBrowser === undefined) return;
203-
// Track the whole reinit so a shutdown racing this launch loop drains it
204-
// before closing slots, rather than leaking a browser that resolves after
205-
// shutdown already awaited the in-flight set.
206-
await this.track(this.reinitInner());
218+
// Set the concurrent-entry guard SYNCHRONOUSLY before the first `await`
219+
// (before `this.track(...)`) so a second acquire reaching its empty-pool
220+
// check while this reinit is in flight skips reinit and parks a waiter
221+
// instead of launching its own duplicate set of browsers. Cleared in the
222+
// `finally` so a thrown reinit can't leave the guard latched forever.
223+
this.reiniting = true;
224+
try {
225+
// Track the whole reinit so a shutdown racing this launch loop drains it
226+
// before closing slots, rather than leaking a browser that resolves after
227+
// shutdown already awaited the in-flight set.
228+
await this.track(this.reinitInner());
229+
} finally {
230+
this.reiniting = false;
231+
}
207232
}
208233

209234
private async reinitInner(): Promise<void> {
@@ -229,12 +254,25 @@ export class BrowserPool {
229254
this.handOff(slot, browser);
230255
} catch (err) {
231256
// Best effort — keep trying the remaining slots. If none succeed
232-
// the pool stays empty and the next acquire retries reinit.
233-
this.logger?.info("browser-pool.reinit-failed", {
257+
// the pool stays empty and the next acquire retries reinit. Per-attempt
258+
// failure is a warn: a single slot failing to relaunch is recoverable
259+
// (other slots may succeed, or a later acquire retries).
260+
this.logger?.warn?.("browser-pool.reinit-failed", {
234261
error: err instanceof Error ? err.message : String(err),
235262
});
236263
}
237264
}
265+
266+
// The genuine empty-pool capacity-loss signal: reinit ran but every launch
267+
// failed, so the pool ends with zero slots. Unlike the per-attempt warn
268+
// above, this is the terminal "pool ended empty" outage and must reach the
269+
// error/Sentry pipeline. Per-attempt reinit-failed stays warn (recoverable);
270+
// this distinct end-of-loop emit fires once when the backstop drained empty.
271+
if (this.slots.length === 0) {
272+
this.logger?.error?.("browser-pool.reinit-empty", {
273+
poolSize: this.poolSize,
274+
});
275+
}
238276
}
239277

240278
/**
@@ -309,8 +347,10 @@ export class BrowserPool {
309347
slotIndex: this.slots.indexOf(slot),
310348
});
311349
} catch (err) {
312-
// Still failing — leave it pending for the next acquire to retry.
313-
this.logger?.info("browser-pool.relaunch-failed", {
350+
// Still failing — leave it pending for the next acquire to retry. This
351+
// is a per-attempt failure (the slot stays parked and recoverable), so
352+
// warn rather than error.
353+
this.logger?.warn?.("browser-pool.relaunch-failed", {
314354
slotIndex: this.slots.indexOf(slot),
315355
error: err instanceof Error ? err.message : String(err),
316356
});
@@ -333,16 +373,21 @@ export class BrowserPool {
333373
// `relaunchPendingSlots()`, not here. (`stats().size` may read 0 in that
334374
// state, but `size` does not gate this backstop — `this.slots.length`
335375
// does.)
336-
if (this.slots.length === 0) {
376+
if (this.slots.length === 0 && !this.reiniting) {
337377
await this.reinit();
338-
} else {
378+
} else if (this.slots.length > 0) {
339379
// Lazily relaunch any slot whose last relaunch failed. This is the
340380
// recovery path for a transient launch failure: the slot was kept
341381
// (capacity preserved) but parked as `relaunchPending`; the next
342382
// acquire re-attempts the launch before falling through to the
343383
// normal available-slot scan.
344384
await this.relaunchPendingSlots();
345385
}
386+
// The remaining case — `slots.length === 0 && this.reiniting` — is the
387+
// concurrent-acquire skip: another acquire's reinit is already launching
388+
// up to poolSize browsers, so this call does NOT launch its own duplicate
389+
// set. It falls through to the waiter-queue path below; the in-flight
390+
// reinit's `handOff` serves this waiter as soon as a fresh browser lands.
346391

347392
// Skip zombie slots whose browser has disconnected but whose disconnect
348393
// handler hasn't yet completed the recycle. Without this loop, a single
@@ -601,7 +646,10 @@ export class BrowserPool {
601646
// launch (see relaunchPendingSlots), and the empty-pool backstop in
602647
// acquire() re-initializes if every slot ends up pending/lost.
603648
slot.relaunchPending = true;
604-
this.logger?.info("browser-pool.recycle-relaunch-failed", {
649+
// Capacity loss after exhausting ALL immediate retries — the slot is now
650+
// parked dead and only a later lazy acquire can recover it. This is a
651+
// genuine capacity-loss signal that must reach error/Sentry, not info.
652+
this.logger?.error?.("browser-pool.recycle-relaunch-failed", {
605653
slotIndex: this.slots.indexOf(slot),
606654
attempts: RELAUNCH_MAX_ATTEMPTS,
607655
poolSize: this.slots.length,
@@ -623,7 +671,7 @@ export class BrowserPool {
623671
// never surface as an unhandled rejection.
624672
recyclePromise
625673
.catch((err: unknown) => {
626-
this.logger?.info("browser-pool.recycle-failed", {
674+
this.logger?.error?.("browser-pool.recycle-failed", {
627675
slotIndex: this.slots.indexOf(slot),
628676
error: err instanceof Error ? err.message : String(err),
629677
});

0 commit comments

Comments
 (0)