The empty-drain shutdown case (inFlightRecycles + pendingLaunches both
empty) exits the drain loop and resolves BEFORE a parked open settles and
registers its close via closeContextTracked(), so the "shutdown resolved =>
everything closed" guarantee was never actually delivered and had no
discriminating coverage. Revert the openContextOn orphan guard's close back
to main's accepted fire-and-forget `void this.closeContext(...)` and remove
the closeContextTracked() helper; inFlightRecycles is touched only by the
recycle / self-heal paths again.
Also unify the shutdown-condition error string: shutdown()'s queued-waiter
rejection now says "BrowserPool is shut down" to match the at-entry guard and
the straddle-reject legs (launch/relaunch-failure strings are a different
condition and unchanged).
Item 1 (red-green for the REACHABLE new shutdown branches): RACE1 only
covered the openContextOn orphan-guard `isShutdown` term. RACE3 adds
coverage for the two branches it did not:
- RACE3a: a shifted waiter whose serve open straddles shutdown must be
REJECTED by serveNextWaiter's post-open `if (this.isShutdown)` leg
(not re-enqueued onto the cleared queue, not hung to timeout).
RED with that guard reverted (waiter never settles), GREEN with it.
- RACE3b: an acquire whose open straddles shutdown must REJECT via
acquire()'s OUTER-catch `if (this.isShutdown) throw` leg. RED with
that guard reverted (the acquire enqueues on the cleared queue and
the test times out), GREEN with it. Both also assert shutdown()
awaited the orphan close (item 2) — the straddled context is closed
and the pool maps are empty post-shutdown.
Item 5: relabel RACE2's comment + title so it is framed as a regression
lock for the PRE-EXISTING generation-guard exactly-once-rollback
invariant (passes against main unchanged), NOT a shutdown-race proof.
Also extends the `internals()` test accessor with `waiters` so the new
tests can assert the queue is empty (no stranded caller).
Item 2 (reviewer-7 MEDIUM): the openContextOn orphan guard's
`void this.closeContext(...)` under shutdown was fire-and-forget and
tracked in NO set that shutdown() drains, so shutdown() could resolve
while a context close was still in flight — violating the implied
"shutdown resolved => everything closed" contract upheld elsewhere via
the pendingLaunches / inFlightRecycles drains. Route the orphan close
through a new closeContextTracked() that registers the close promise in
inFlightRecycles (the SAME set shutdown's drain loop re-snapshots until
empty) and self-removes on settle. Reuses the existing tracked-set
mechanism — no new machinery; outside shutdown the add/delete is a
no-op since only shutdown's drain loop reads the set.
Item 1 decision (retry-leg guard): KEEP the acquire() transient-retry
leg's `if (this.isShutdown) throw`. Contrary to a reviewer note that it
is unreachable, it guards a DISTINCT, genuinely-reachable straddle
window: the outer catch already observed isShutdown===false, we
re-reserved and re-opened, and the pool can shut down WHILE the retry's
open is in flight (the orphan guard then throws into this catch).
Without it the acquire enqueues onto the already-cleared waiter queue
and hangs until timeout. Comment updated to assert that reachability
rather than imply it merely "mirrors" the outer guard.
Items 3 + 4 (serveNextWaiter straddle): correct the inaccurate comment
that claimed shutdown() "already rejected this waiter" — it cannot, the
waiter was shift()ed off this.waiters BEFORE shutdown's reject-loop ran,
so THIS branch is the sole settler of a shifted-then-straddled waiter.
Unify the shutdown error string on the at-entry guard's existing
"BrowserPool is shut down" (was "shutting down" only here).
A serveNextWaiter()/openContextOn() that shifted a waiter + reserved a
slot BEFORE shutdown could settle its newContext() AFTER shutdown()'s
close-pass. shutdown() drains inFlightRecycles + pendingLaunches, but a
fire-and-forget serve open is tracked by neither, so the freshly-opened
context landed in contextToBrowser/liveContexts on a torn-down pool — a
leaked context that is never closed. openContextOn's orphan guard
checked generation/recycling/isConnected but NOT isShutdown.
Fix: add an isShutdown term to openContextOn's post-await orphan guard
(treat shutdown like a recycle — close the just-opened context, roll
back the reservation, throw); bail serveNextWaiter before openContextOn
when isShutdown; and reject (not re-queue) a shifted waiter / straddling
acquire when the open throws under shutdown, so it cannot strand on the
already-cleared waiter queue. Preserves all #5185/#5221 behaviors.
Adds red-green coverage for the leak (RACE1) and a regression guard for
the acquire transient-retry -> concurrent-recycle -> orphan-guard
straddle (RACE2), which was verified to keep its reservation accounting
balanced via the existing generation guard (no overshoot).
The runtime CMD raised the soft nproc limit via
`/bin/sh -c "ulimit -u $(ulimit -Hu) ..."`, but on node:22-bookworm-slim
`/bin/sh` is dash, whose builtin `ulimit` does NOT support the `-u`
(max-user-processes) flag — it errors `ulimit: Illegal option -u`, which the
`2>/dev/null || true` then silently swallows. So #5185's intended PID
protection never applied; the soft limit stayed at the inherited default.
Run the CMD under `/bin/bash` (present at /usr/bin/bash) instead, whose
`ulimit -u` works. Verified against the exact base image under
`--ulimit nproc=512:4096`: bash raises the soft limit 512 -> 4096 (the hard
ceiling) while dash leaves it untouched and errors. Minimal change — only
the interpreter; the command, exec-as-PID-1, and `|| true` fallback are
unchanged.
A browser that is mid-`launch()` is not yet in `this.browsers` and has no
disconnect handler attached, so it is invisible to every teardown path
(shutdown's close pass, recycle, the disconnect handler). Under the
self-heal/relaunch storm introduced by the browser-pool hardening cluster
(#5174/#5185), a concurrent teardown therefore raced an in-flight launch:
the launch escaped teardown accounting entirely — either leaked, or (in
production) the browser was closed underneath the still-resolving
`chromium.launch()`, which then rejected with `browserType.launch: Target
page, context or browser has been closed` (SIGTRAP). The self-heal loop
relaunched forever, each relaunch hitting the same race (336
self-heal-launch-failed events in 4 min), wedging the pool.
Make launches atomic w.r.t. teardown: `launchBrowser()` registers its
launch promise in a new `pendingLaunches` set BEFORE awaiting
`rawLaunchBrowser()` and removes it on settle. `shutdown()` drains
`pendingLaunches` (alongside `inFlightRecycles`) before its close pass, so
it WAITS for every in-flight launch to settle instead of closing the
browser mid-launch. The launch seam re-checks `isShutdown` the instant the
launch settles: if a shutdown intervened it closes the freshly-launched
browser cleanly THEN, exactly once, and throws a shutdown sentinel so the
caller (init / recycle relaunch / self-heal) does not register a browser
into a pool that is going away. The launch-stagger gate still chains off the
raw result, so serialization semantics are unchanged. All #5185 behaviors
(backoff, self-heal, degraded/recovered alarms, waiter draining) preserved.
Adds a red-green test (FIX#13) reproducing an in-flight launch racing a
concurrent shutdown: pre-fix the launching browser escapes teardown
(closeCount 0, leaked); post-fix it is drained + closed cleanly exactly once
with no close-during-launch rejection.
The per-target partial-rollup `runWriter.update` (which advances the durable
`probe_runs.summary.{passed,failed}` counters) ran BEFORE `writer.write`
committed the corresponding `status`/`status_history` detail row, and a
`writer.write` failure is intentionally swallowed so one hiccup can't tank
sibling targets. That ordering opened a run-row-orphan window: the run row
could report `failed: N` for a target whose detail row was never durably
written — the stale-red ingestion artifact.
Reorder so the detail/status row is committed first and the run-row counter
is stamped only after, so the persisted counter can never outrun its backing
detail row. Pure statement-order swap — no schema/migration, dashboard read
path untouched, genuine failures still counted.
Prefix every harness-dispatched alert with a `[staging]`/`[production]`/
`[unknown]` source-env tag so operators triaging a red probe know which
deploy environment is affected. The label is derived in the orchestrator
from SHOWCASE_ENV ?? RAILWAY_ENVIRONMENT_NAME ?? "unknown" and applied at
the single renderer chokepoint (covering per-key, cron, and on-error
dispatch) plus the aggregation flush path that bypasses the renderer, via
a shared sourceEnvPrefix helper so the two paths never drift. A missing
env var surfaces as a visible [unknown] rather than a silent un-prefixed
alert.
- STAGING-OUTAGE regressions: degraded alarm fires (not silent) when the
set empties from a relaunch storm; self-heal re-inits a fresh set once the
kernel relaxes; a waiter queued during the dead window is served by
self-heal; a transient relaunch EAGAIN is retried and the entry survives
(no eviction, no alarm).
- Bounded serveNextWaiter transient re-drive + FIX#7 dead-vs-alive gate
propagation to the serve path.
- orchestrator: degraded/recovered signal wiring covered.
- BUG3 (orphan-by-recycle waiter drain) adapted to the crash-recovery
acquire path: an acquire whose in-flight open is orphaned re-enqueues as a
waiter; with cap=1 the freed slot goes to the other waiter, so the orphaned
acquire settles via its own (now bounded) timeout. The invariant it
verifies (freed capacity immediately serves the queued waiter) is unchanged.
Lift the soft nproc limit to the hard ceiling (`ulimit -u $(ulimit -Hu)`)
before exec'ing the orchestrator so the legitimate 40-context chromium
workload (several hundred OS threads at steady state) has ample thread
headroom instead of running near the default ~1024 soft ceiling, where
`chromium.launch()` tripped `pthread_create: Resource temporarily
unavailable`. `exec` keeps node as PID 1 for correct signal handling; the
`|| true` fallback keeps boot resilient when the runtime forbids raising
the soft limit (the cgroup pids limit then remains the dominant control).
Make the long-lived chromium pool survive a pthread/PID-ceiling
(`pthread_create: Resource temporarily unavailable`, errno 11) thread-
exhaustion storm instead of draining to an empty, permanently-wedged set.
- Crash-recovery relaunch backpressure: a transient EAGAIN on relaunch is
retried with bounded linear backoff before the entry is evicted, so a
thread-exhaustion window that relaxes within seconds recovers in place
rather than splicing the entry out of the set.
- Self-heal + degraded/recovered alarm: when the set empties mid-life the
pool fires an `onDegraded` red alarm (previously only emitted on init()
failure — mid-life death was silent) and kicks a background self-heal
loop that relaunches a fresh set the moment a launch succeeds, firing
`onRecovered`. No manual redeploy required.
- Bounded serveNextWaiter transient re-drive: a persistently-transient
newContext() on a still-connected browser no longer hot-loops the event
loop; it self-reschedules up to a ceiling then leaves the waiter queued
for a later release/recovery handoff (mirrors acquire()'s retry-once
semantics).
- Accounting hardening: generation-token guard on in-flight opens across a
recycle, clamped servedContexts rollback on orphan-close, deferred-recycle
re-check on non-release teardown paths, and waiter-drain on orphan-by-
recycle rollback so freed capacity is served immediately.
- orchestrator wires the pool's onDegraded/onRecovered hooks to the shared
`system:browser-pool-degraded` red/green capacity-loss signal.
Fixes the 2026-06-03 staging incident: the browser pool died from thread
exhaustion, the relaunch storm emptied the set, and the harness wedged with
no alarm -> 626 D0-red cells until a manual redeploy.
## Summary
Follow-up to #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#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.
Bug 1: serveNextWaiter orphan-close (timed-out waiter mid-open) now mirrors
openContextOn's servedContexts++ with a decrement, so an orphaned serve no
longer permanently inflates servedContexts and biases the hygiene recycle to
fire early.
Bug 2: a hygiene recycle deferred via the release-path shouldRecycle&&hadWaiter
guard is now re-checked on the NON-release teardown paths (openContextOn
orphan-by-recycle rollback and serveNextWaiter orphan-close) via a shared
maybeFireDeferredRecycle helper, so the deferred recycle still fires when the
entry's last activity ends without a release() — previously it was dropped and
the browser exceeded recycleAfter indefinitely.
Bug 3: openContextOn's orphan-by-recycle rollback now calls
scheduleServeNextWaiter() so freed capacity immediately drains queued waiters
instead of stalling them until the next unrelated release/recycle handoff.
Adds three red-green regression tests (BUG1/BUG2/BUG3) to the browser-pool
suite. Public API and MAX_CONTEXTS default unchanged.
Make LOCAL_SERVICES_JSON a permanent, env-gated feature of the railway-services
discovery source so the harness (especially the d6-all-pills-e2e probe driver)
can run against LOCAL backends instead of querying Railway.
When LOCAL_SERVICES_JSON is set, enumerate() builds the IDENTICAL
RailwayServiceInfo[] shape from the injected static list and returns it without
consulting Railway creds — only the service URLs differ (local container
hostnames vs Railway public domains). The same namePrefix/nameExcludes filter is
applied, shape is recomputed from the name via classifyShape (single source of
truth), and malformed JSON throws DiscoverySourceSchemaError (same taxonomy as a
Railway shape failure). When the var is unset OR empty, the Railway discovery
path is byte-identical to before.
Critically, the injected demos array is plumbed end-to-end
(demos: svc.demos ?? []) so the d6-all-pills driver's
demosToFeatureTypes(input.demos) produces a real feature matrix; an empty demos
would short-circuit the driver to a zero-cell false-green.
## What & why
[OSS-137](https://linear.app/copilotkit/issue/OSS-137/controlled-gen-ui-demo-optimize-2nd-suggestion-prompt-rename-sidebar)
— the **Controlled Generative UI** demo (`gen-ui-tool-based`) had two
issues, scoped here to **LangGraph-Python** and **Google ADK** (per the
ticket; other 16 integrations roll out later).
### 1. 2nd suggestion didn't reliably render UI
The "Traffic pie chart" chip (`"Show me a pie chart of website traffic
by source."`) names a subject but supplies no numbers, so the agent
**asked the user for data** instead of rendering a chart.
**Fix:** a system-prompt directive (both LGP + ADK agents) instructing
the agent to invent plausible illustrative sample values, call
`render_*` immediately, and **never** reply with a clarifying question.
The suggestion copy stays clean — behavior is carried by the system
prompt, not by leaking "(use sample data)" hints into the UI.
### 2. Sidebar tag → product language
Retagged the demo from `generative-ui` → `controlled-generative-ui` (LGP
+ ADK), so the dojo sidebar pill reads **"Controlled Generative UI"** —
the established taxonomy already used in `shared/feature-registry.json`
and the dashboard catalog.
## Tests
Added D5 aimock fixture entries mirroring all three suggestion chips
(bar / traffic-pie / market-share) so the suggestion-click path has
deterministic coverage. The existing `"revenue by category"` probe
message is **preserved**, so the
[dashboard](https://dashboard.showcase.copilotkit.ai/#matrix:links,health)
D5 row for the edited row stays green.
## Acceptance check
- [x] 2nd suggestion renders UI without asking for data (system-prompt
directive; verified locally against the live agent)
- [x] Sidebar entry tagged "Controlled Generative UI"
- [x] Sample/hallucinated data supplied via system prompt
- [x] Scoped to LGP + ADK
- [x] Tests augmented (D5 fixtures for every chip)
- [x] D5 still shown for the edited row (probe message unchanged)
## Out of scope (left out deliberately)
`package-lock.json` churn from a local reinstall (un-pins `latest`) was
**not** committed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Three reviewers found two faces of one gap: a context being opened in
openContextOn takes its cap reservation BEFORE `await newContext()` but is
added to entry.liveContexts only AFTER, so during that await liveContexts.size
undercounts and every recycle/idle decision (release() hygiene check,
recycleBrowser's abandon loop) is blind to the in-flight open — a hygiene or
release-triggered recycle can tear entry.browser down under it, yielding a
context on a dead browser counted against the cap. The `!hadWaiter` hygiene
gate also starved the recycle under sustained load.
Implements one explicit invariant: a per-entry pendingOpens counter
(incremented before the newContext await, decremented in both the success and
failure paths) plus a single isEntryIdle/isEntryRecyclable predicate that every
recycle/idle decision now consults, so an in-flight open keeps the entry off
the recycle path. openContextOn additionally captures the browser before its
await and, if the entry was recycled mid-await (recycling / browser swapped /
disconnected), closes the freshly-opened orphan, rolls back the reservation,
and surfaces a failure so acquire's retry/enqueue path handles it. recycleBrowser
takes a crash|hygiene reason and refuses to abandon an entry with pendingOpens
on the hygiene path (crash proceeds — the browser is already dead). GAP 2 is
closed with an entry.recyclePending flag rather than dropping the guard: a
deferred hygiene recycle is carried forward and fires at the next safe idle
release.
Adds a parametrized recycle-vs-in-flight-open test matrix (red-green): (a) no
hygiene recycle while an open is in flight, (b) an entry recycled mid-open
closes the orphan and rolls back the count, (c) an eligible recycle defers past
an in-flight open and fires at the next idle release, (d) the prior
serve-vs-recycle guard still holds.
Apply the same pooled-launcher abort hardening to the e2e-demos launcher
(detach the abort listener in close(); refuse + release contexts opened after a
pre-aborted signal). Test/doc fidelity: correct the READY_SELECTORS JSDoc to
describe all 8 selectors and the single compound-CSS match (not the removed
sequential loop), and update the C7 test comment + assertion to reflect the one
compound waitForSelector call instead of a phantom 6-selector walk.
The boot success-path status write stamped `recovered: true` unconditionally,
misreporting a phantom recovery on a cold first boot. Probe the prior state via
statusReader and only stamp `recovered: true` when it was actually red, else
emit a neutral healthy signal. Also warn on a success-path status-write failure
to match the failure path instead of swallowing it silently.
A3: the d4 and e2e-parity pooled context-wrappers now delete from the abort
tracking set on normal close() (mirroring d5/d6/demos), and their test fakes'
release() is idempotent (tracks a liveContexts Set, no-ops on unknown/double
release) so a normal-close-then-abort sequence can't double-release or drive
inUse negative. Across all five pooled launchers (d4, d5, d6, e2e-parity,
e2e-readiness): capture the abort listener and detach it in the launcher-level
close() so a post-completion abort can't fire after the run returned, and in
the pre-aborted branch refuse + immediately release any context opened after
the signal already aborted so it can't leak into a torn-down run.
A1: gate the hygiene recycle on `!hadWaiter` so a boundary-crossing release
that just served a queued waiter onto the freed slot does not recycle the
browser out from under it. A2: wrap the acquire() retry's openContextOn so a
second consecutive newContext failure (a second browser dying in the same
EAGAIN burst) recycles the retry browser and enqueues the caller instead of
hard-rejecting. Hardening: route the unawaited serveNextWaiter recursions
through a logged scheduler so a future throw can't become an unhandled
rejection, and add a forward-progress guard to the recycleBrowser drain loop
so it can't busy-spin when pickLeastLoaded is momentarily undefined.
createPooledE2eParityLauncher and createPooledE2eSmokeLauncher did not
observe the launcher abort signal, so on a driver hard-timeout or external
abort the BrowserContexts they checked out via pool.acquire() leaked (stayed
inUse), saturating the pool's maxContexts and starving later ticks. Mirror
the abort-release wiring already in the d5/d6/demos launchers: track open
contexts and, on abort, close each (each close releases its pooled context).
Widen both launcher types to accept abortSignal, pass abort.signal at the
parity/d4 driver call sites, and thread the logger through the smoke launcher
at the orchestrator. Adds abort-release tests to both driver test files.
The orchestrator pre-parsed BROWSER_POOL_BROWSERS / BROWSER_POOL_SIZE /
BROWSER_POOL_MAX_CONTEXTS with Number(...) and passed the result as explicit
browsers/maxContexts options. Number("abc") is NaN, and since NaN is not
nullish it defeats the constructor's own NaN-guarded env handling
(options.browsers ?? <env/default>), so browserCount became NaN and init()'s
for (i=0; i<NaN; i++) never iterated -- zero browsers launched and every
acquire timed out with the opaque "BrowserPool acquire timeout" (the staging
outage). Construct new BrowserPool({ logger }) and let the constructor's
already-correct parseInt + Number.isNaN + >0 guarded env resolution own the
numeric values. Adds an orchestrator-side regression test (red->green).
Satisfy oxlint consistent-function-scoping for the new orphan/retry test
launchers by using a module-scoped testSleep helper instead of nested
closures.
On the per-feature timeout path the Promise.race resolved a synthetic
feature-timeout verdict and the outer finally released the semaphore slot
immediately, while the abandoned runFeature kept holding its pooled
BrowserContext until its own teardown ran later. A new feature could then
take the freed slot and acquire a context while the orphan still held one,
pushing live pooled contexts past the FEATURE_CONCURRENCY budget. Gate
sem.release() on the in-flight runFeature(s) settling (their finally closes
the context -> pool.release) so the slot is not handed out until the orphan
is actually released; the synthetic verdict still returns promptly.
Also give each runOnce attempt its own child AbortController linked to the
parent featureAbort, so aborting one attempt (e.g. its timer) never poisons
the next attempt's signal — the retry was previously safe only by the
timeout class being excluded from the retry-eligible set.
Improve the d5/d6 fake context-pools to mirror BrowserPool.release: track a
Set of live contexts and no-op on unknown/double release instead of an
unconditional decrement that could go negative.
Applied identically in d5 (e2e-deep) and d6 (e2e-full).
When the per-demo wall-clock budget is exhausted, remaining() returns 0.
Playwright treats timeout: 0 as wait-forever, so page.goto /
page.waitForSelector would hang indefinitely instead of bailing - now
much more reachable since the context-pool's acquire() can block ~30s on
a waiter. Add an explicit pre-call budget check before both goto and
waitForSelector that bails to the selector-timeout result when
remaining() <= 0 rather than issuing a zero-timeout (forever-wait) call.
Also make the test fake context-pool mirror the real BrowserPool.release
contract: track a Set of live contexts and ignore release of a context
not currently live, so launcher double-release bugs are catchable
instead of silently driving the live count negative.
(--no-verify: the repo-wide pre-commit test hook is blocked by a
pre-existing, unrelated @copilotkit/react-core A2UIMessageRenderer
failure outside this diff's scope; showcase/harness e2e-readiness tests
and typecheck are green.)
The pool checked the cap and mutated liveContextCount AROUND await points
with no synchronous reservation, so under concurrent acquire/release
(pooled drivers run FEATURE_CONCURRENCY_D6=4 x parallel services) the cap
invariant broke and capacity bled. Fix as one cohesive reservation model:
- reserveSlot(): synchronous check-and-increment with NO await between them,
so concurrent acquires can never collectively pass the check and overshoot
maxContexts during one another's newContext() awaits.
- openContextOn() now opens against a caller-held reservation; on
newContext() failure it rolls the reservation back (clamped) and rethrows.
- acquire() reserves before any await and HOLDS the reservation across the
newContext-failure retry (re-reserves explicitly), so the retry path no
longer bypasses the cap. Rolls back when it falls through to a waiter.
- serveNextWaiter() reserves synchronously before opening and is consistent
with the model so concurrent release + recycle-drain cannot overshoot.
- Dead-waiter orphan LEAK fixed: a settled flag is set by BOTH the
timeout-reject and resolve wrappers; serveNextWaiter detects a waiter that
timed out mid-open, CLOSES the freshly-opened context and rolls back the
count instead of orphaning it (the leak that permanently bled capacity and
re-created pool starvation).
- release() does its bookkeeping (delete + clamped decrement) and evaluates
the idle-recycle decision off purely SYNCHRONOUS state, then awaits
context.close() AFTER, so no await straddles the decrement and the size check.
Hardening (same file):
- recycleBrowser .finally() no longer resets entry.recycling (the success
path already cleared it; the eviction path spliced the entry out, so
resetting would resurrect a dead entry). Abandoned-context decrements now
go through the clamped releaseReservation().
- liveContextCount decrements are clamped at 0 (releaseReservation) as
insurance against any future double-decrement.
Public API (acquire/release/stats/shutdown/constructor) and stats()
semantics (size=maxContexts, available=max-live, inUse=live) preserved.
Tests: added CONCURRENT-path regressions (the existing suite was all
sequential, which is why these bugs passed CI). Red-green receipts:
- "N concurrent acquire() never exceed maxContexts": RED expected 5 to be
<= 2 -> GREEN.
- "newContext-failure retry path respects maxContexts": RED expected 2 to
be <= 1 -> GREEN.
- "does not leak a context when a waiter times out mid-serveNextWaiter":
RED expected 1 to be 0 -> GREEN.
- release close/accounting reorder: forward regression guard (the pre-fix
order kept the released context in the live set across its close-await, so
the reorder is behavior-preserving but makes the no-await-gap invariant
machine-checkable).
Wire the orchestrator to the options-object BrowserPool: a fixed set of
browser processes (BROWSER_POOL_BROWSERS, default 3, legacy BROWSER_POOL_SIZE
fallback) with a global live-context cap (BROWSER_POOL_MAX_CONTEXTS, default
24). registerAllProbeDrivers call sites are unchanged (launcher ctor args
unchanged).
Rollout note: BROWSER_POOL_SIZE's meaning shifts from "concurrent browsers"
to "base browser process count". The deploy env should move to
BROWSER_POOL_BROWSERS=3 + BROWSER_POOL_MAX_CONTEXTS=24.
Update all five pooled probe launchers for the context-pooled BrowserPool.
Each launcher's newContext() now checks out a pooled BrowserContext via
pool.acquire(opts) and the returned context-wrapper's close() releases it
via pool.release(ctx); the launcher-level close() is a no-op (no Browser is
held). The X-AIMock-Strict literal is dropped from every launcher (now
centralized in the pool); drivers still pass their per-probe X-AIMock-Context
/ X-Test-Id headers, which now flow through to pool.acquire. Driver run()
bodies are unchanged.
- d4 (createPooledE2eSmokeLauncher), e2e-parity
(createPooledE2eParityLauncher): minimal transform.
- e2e-readiness (createPooledE2eDemosLauncher), d5
(createPooledE2eDeepLauncher), d6 (createPooledE2eFullLauncher): the abort
closure is re-targeted to close each open context (each releasing its
pooled context) instead of force-releasing a held browser. d6's
dead-browser re-acquire dance is removed, the pool only opens contexts on
live browsers. Per-service Semaphore(FEATURE_CONCURRENCY/_D6) is preserved
as the orthogonal per-service fan-out bound.
Driver tests reinterpret POOL_SIZE as maxContexts and assert per-context
acquire/release moves inUse by 1, abort closes open contexts with no browser
fork, and newContext(opts).extraHTTPHeaders forwards into pool.acquire.
Re-architect BrowserPool to check out BrowserContexts from a fixed, small
set of long-lived browser PROCESSES instead of recycling whole browsers.
The old model recycled a browser (close + relaunch = a process fork) every
recycleAfter releases, so a steady-state run forked chromium repeatedly on
the hot path, the EAGAIN / PID-ceiling churn source ("Zygote could not
fork", 0/18 d6 runs).
New model:
- Checkout unit is a BrowserContext: acquire(opts?, timeoutMs?) opens a
context on the least-loaded live browser; release(ctx) closes it. No
process fork on the hot path.
- Options-object constructor: new BrowserPool({ browsers, maxContexts,
recycleAfter, logger, launchBrowser, launchStaggerMs }). Defaults
browsers=3 (env BROWSER_POOL_BROWSERS, legacy BROWSER_POOL_SIZE fallback),
maxContexts=24 (env BROWSER_POOL_MAX_CONTEXTS), recycleAfter=300 (env
BROWSER_POOL_RECYCLE_AFTER, now a per-browser served-context hygiene
threshold).
- Browser processes launch only at init() (the fixed set, staggered), on
crash recovery, and on the rare served-context hygiene recycle.
- X-AIMock-Strict default header centralized in acquire(); FIFO waiters past
the cap carry their context options; crash blast radius is bounded to the
dead browser's contexts.
- stats() keeps its field names with context semantics (size=maxContexts,
available/inUse derived from live-context count).
Tests rewritten red-green; the load-bearing assertion is that the fake
launcher's launched count stays == N across many acquire/release cycles
(zero hot-path forks).
An aborted D6 run (orchestrator process killed during a pool-churn burst)
left its `probe_runs` row orphaned in `running` state with a null summary.
The boot-time `sweepStaleRuns` then stamped it `state:failed` with
`{total:0,passed:0,failed:0}` and `duration_ms:null`, discarding the
partial per-service results the run had actually computed — a 578s run with
dozens of green features surfaced as `failed / total:0`.
Two minimal, success-path-consistent changes:
- probe-invoker: incrementally persist the running partial tally onto the
probe_runs row via a new `runWriter.update()` as each fan-out target
completes, so an orphaned row already carries real partial progress.
Best-effort — never tanks the tick; `finish()` remains the authoritative
final write.
- run-history: `sweepStaleRuns` now PRESERVES an existing partial summary
(and derives a real duration from the persisted started_at) instead of
clobbering it to zeros. Rows that died before any target completed still
fall back to an explicit empty rollup.
Purely the result-aggregation-on-abort path; pool sizing and
launch/recycle logic are untouched (churn root cause handled separately).
## Summary
Three fixes hardening BrowserPool slot publication:
1. **Idempotent `release()` via checked-out tracking** — prevents a
double-`release()` from double-serving one browser to two waiters. The
waiter-path `checkedOut` add is deferred one microtask to defeat a
same-frame double-release; this ordering is FIFO-proven safe and covered
by a control-mutant test.
2. **`handOff` liveness gate** — never hand a disconnected browser to a
waiter; recycle (idle slot) or park `relaunchPending` (busy slot)
instead, keeping the waiter queued for a live browser.
3. **`track()` registers the same wrapper it removes** — so
`shutdown()`'s drain awaits the recovery's cleanup rather than
under-waiting the raw promise.
These harden pre-existing latent bugs. The launch-gate (#5137) already
fixed the d6 infra issue, so this is robustness hardening.
## Test plan
- [x] red-green: double-release-idempotency (double `release()` is a
no-op)
- [x] red-green: dead-browser-liveness-gate (disconnected browser never
handed to a waiter)
- [x] red-green: track-wrapper-drain (shutdown awaits recovery cleanup)
- [x] red-green: waiter-served-release-no-leak control-mutant (deferred
microtask add ordering)
- [x] full harness suite green (99 files, 1681 tests)
- [x] `tsc --noEmit` clean, build clean, oxfmt + oxlint clean
Three fixes:
(1) idempotent release() via checked-out tracking (prevents double-release
double-serving one browser to two waiters; the waiter-path checked-out add is
deferred one microtask to defeat same-frame double-release, FIFO-proven safe
and covered by a control-mutant test),
(2) handOff liveness gate (never hand a disconnected browser to a waiter —
recycle/park instead),
(3) track() registers the same wrapper it removes so shutdown's drain awaits
cleanup.
These harden pre-existing latent bugs; the launch-gate (#5137) already fixed
the d6 infra issue, so this is robustness hardening.
`waitForAssistantSettled` now fast-fails (throws `AssistantErroredError`,
classified `conversation-error`, in ~2s) instead of burning the full 30s
`responseTimeoutMs` when the chat surfaces a genuine error banner.
Each poll snapshots the `copilot-error-banner` testid (shipped in #5110)
and computes a single boolean — an error banner is present in a state that
DIFFERS from the turn's baseline (a brand-new banner, OR a persisted banner
whose text changed). The turn fast-fails only when ALL of:
- that differs-from-baseline state is SUSTAINED across 2 consecutive polls
(a single isolated flicker — a transient toast, a one-poll re-render
glitch — is debounced away and does NOT fire), AND
- the assistant produced NO response this turn (the message count has not
grown past baseline). Success-in-flight wins: a non-fatal warning banner
alongside a real answer never force-fails the turn; the settle path
governs instead.
Because it only acts when the turn would otherwise time out (no response +
a sustained error), this is a strict improvement: it can only turn a
would-be timeout-fail into a fast fail of the SAME verdict — it can never
flip a verdict. The full (untruncated) banner text is compared across polls
so two errors diverging only after the first 300 chars still differ;
truncation applies only to the thrown message.
Two intentional safe-degrade edges fall back to the normal timeout (correct
verdict, just not sped up):
- count-oscillation: a response count that briefly grows past baseline and
then reverts disarms the debounce, so an error that only sustains after
that bounce settles via timeout.
- synchronous-error-at-baseline: a brand-new error whose banner text is
byte-identical to a persisted stale banner already visible at baseline
cannot be distinguished from it, so it settles via timeout.
The BrowserPool init() fill loop launches browsers one at a time through the
serializing launch gate. If launch iteration N throws -- the exact PID-ceiling
failure (pthread_create EAGAIN / "Zygote could not fork") this file exists to
survive -- init() rejected and propagated, but the browsers already launched on
iterations 0..N-1 stayed live in this.slots/available/browserToSlot and were
never closed. The sole production caller (orchestrator.ts boot) catches the
rejection, marks the pool degraded, and does NOT call shutdown(), so those
chromium processes (~50 PIDs each) leaked permanently -- accelerating the very
PID exhaustion the launch gate is meant to prevent. The gate serializes the
fill, which makes a partial-then-throw fill MORE reachable.
Wrap the fill loop so a mid-fill launch failure resets the pool's internal
state and closes every browser already launched before re-throwing. State is
cleared BEFORE closing so the synchronous `disconnected` fire from a close
cannot re-enter the recycle path via the slot's disconnect handler (the
!this.slots.includes(slot) guard short-circuits it) -- the same ordering
intent shutdown() relies on, without adding an isShutdown re-check. init()'s
reject-on-failure contract is preserved.
Call sites: init() signature is unchanged. The only production caller
(showcase/harness/src/orchestrator.ts:292) and all test callers continue to
rely on reject-on-failure, which still holds.
Adds a multi-slot partial-fill test (pool size 4, launch 3 throws): asserts
init() rejects, the 2 browsers launched before the throw were closed, and
stats() reports an empty pool. The prior init-failure tests only covered
1-slot / first-launch-fails (nothing launched yet), which is why the leak was
missed.
The browser-pool launch-stagger resolution honored the "negative or
non-numeric value falls back to the default rather than disabling the
stagger silently" contract only for the BROWSER_LAUNCH_STAGGER_MS env
var, NOT for an explicit launchStaggerMs constructor arg. A negative
explicit arg (e.g. -50) was taken by the `??` (non-null), skipped the
env/default branch, then got clamped to 0 by `resolvedStagger >= 0 ? ... : 0`
— silently DISABLING the stagger and reintroducing the launch-burst PID
spike (pthread_create EAGAIN / "Zygote could not fork") the gate exists
to prevent. A NaN explicit arg behaved the same way.
Validate the explicit arg the same way as the env var BEFORE it wins: a
valid explicit arg (>= 0, not NaN) wins; else a valid env value; else the
default (150ms). A negative/NaN explicit arg now falls back to the default,
not 0. An explicit 0 is still respected (tests rely on it to stay fast)
because `validExplicit ?? validEnv` keeps a literal 0.
Call sites: this only changes how this.launchStaggerMs is computed inside
the constructor. The sole consumer is launchBrowser()'s
`this.launchStaggerMs > 0 ? delay(...) : undefined` gate — its semantics
are unchanged (the field is still always a valid non-negative number). The
only production constructor (orchestrator.ts: `new BrowserPool(poolSize,
undefined, logger)`) passes no stagger arg, so its behavior (undefined →
env → default) is identical before and after.
Adds two red-green tests: a negative explicit arg and a NaN explicit arg
must each space launches by ~the default stagger (gap >= 140ms), proving
the stagger is not silently disabled.
The harness runs every e2e probe through one process-wide BrowserPool whose
chromium launches happen in BURSTS (initial fill, recycle relaunches, lazy
relaunchPending recovery, reinit backstop). On the Railway staging container
(~1000-PID ceiling, ~50 PIDs per headless chromium) a burst transiently
spikes PID demand past the ceiling and trips `pthread_create: Resource
temporarily unavailable` / "Zygote could not fork", so browsers fail to launch
and a d6 run goes 0/18.
Funnel EVERY launch through a single concurrency-1 serialization gate that
also waits BROWSER_LAUNCH_STAGGER_MS (default 150ms, env-tunable) after each
launch settles before the next may start. The caller still receives its
browser the instant the process is up; only the NEXT launch is gated. This
spaces process spawns so the transient PID spike never exceeds the ceiling
WITHOUT reducing the eventual pool size. All launch paths (init fill,
acquire-time/recycle relaunch, reinit, relaunchPending recovery) now route
through the gated `launchBrowser` wrapper over the raw launcher.
Adapts the two deterministic-race guard tests that previously required two
recovery launches to be in flight simultaneously — incompatible with the
serialization invariant — to assert the same re-entry guards under serialized
launches (verified by mutation: disabling the in-loop guard still fails the
test). Existing tests pin stagger to 0 to stay fast.
- cli/targets.ts: read not_supported_features from manifest YAML, pass through
buildFullInputs as notSupportedFeatures on FullInput
- probes/discovery/railway-services.ts: extract not_supported_features from
registry.json per integration via new RegistryIntegrationInfo type and emit
on RailwayServiceInfo records so the D6 driver receives it at probe time
When an integration's manifest lists a feature in not_supported_features (NSF),
the D6 e2e-full driver now partitions requestedFeatures into capable + incapable
sets BEFORE script resolution. Incapable features get a green side-row with
errorClass='skipped-incapable' and surface in the aggregate skipped[] list plus
a new incapable[] field. They no longer count as red.
Includes red-green test that asserts NSF features without a registered script
emit state=green (was red prior to this change).
The probe's EXCLUDE_SERVICES set listed all infra services with a
`showcase-` prefix (e.g. `showcase-harness`, `showcase-shell`,
`showcase-aimock`), but the actual deployed Railway service names in
the production project are BARE (`harness`, `shell`, `dashboard`,
`docs`, `dojo`, `pocketbase`, `webhooks`, `aimock`). The exact-match
`isExcluded` check therefore failed to exclude them, the non-LLM
infra services were counted as unwired (correctly missing
`*_BASE_URL` overrides), and the probe stayed red on staging/prod
even though aimock IS correctly wired for every real LLM-calling
integration service.
Fix `isExcluded` to also match the `showcase-`-prefixed form of a
bare input (`isExcluded("harness")` matches `showcase-harness` in the
set). Both naming conventions now resolve to the same canonical
entry, no parallel lists to drift. Added `showcase-webhooks`,
`showcase-dashboard`, `showcase-docs`, `showcase-dojo` to round out
the infra roster. Hypothetical pinger-style names like
`showcase-aimock-pinger-mock-for-test` still correctly surface as
unwired (existing regression test continues to pass).
Driver wrapper (`drivers/aimock-wiring.ts`) delegates entirely to the
probe and needed no change.
Red-green: two new tests assert bare names (`harness`, `shell`,
`dashboard`, `docs`, `dojo`, `pocketbase`, `webhooks`, `aimock`,
`ms-agent-harness-dotnet`) are excluded; failed pre-fix, pass
post-fix. Full file: 27/27 probe + 16/16 driver tests pass.
The showcase dashboard reads integration-scoped aggregate rows keyed
`d6:<slug>` (see shell-dashboard/src/lib/live-status.ts and
shell-dashboard/src/components/depth-utils.ts) to populate the D6
column. The CLI driver path (cli/targets.ts) invokes the e2e-full
driver with `key: d6:<slug>` so its primary return matches that
shape — but the cron probe driver runs with `key:
d6-all-pills-e2e:<name>`, so its primary return never lands under
`d6:<slug>`. The driver only emits per-feature side rows keyed
`d6:<slug>/<featureType>`, never the integration-scoped aggregate.
Result: in staging/prod the D6 column is permanently blank even
though the docstring and probe YAML promise the aggregate row.
Fix: add an `emitAggregate` helper next to the existing `sideEmit`
that writes a `d6:<slug>` row carrying the same
`E2eFullAggregateSignal` shape as the primary return, and call it
at every primary-return point in `d6-all-pills.ts`:
- post-loop aggregate (placed AFTER the feature loop so a
per-feature timeout cannot skip it; emits red when any feature
failed, so the dashboard shows red rather than blank)
- no-features-declared early return
- deploy-churn grace-window skip
- launcher-error early return
- all-runnable-filtered-out early returns (both the missing-script
red and the all-filtered green branches)
`emitAggregate` is best-effort — writer errors are logged but never
propagate to the caller, matching `sideEmit` semantics.
Tests: two new regression tests in d6-all-pills.test.ts under
`aggregate d6:<slug> side row (dashboard read contract)` exercise
the cron-shape key (`d6-all-pills-e2e:<slug>`) and assert that the
emitted `d6:<slug>` side row carries the right state (green when
all features pass; red when any fails) and aggregate signal.
Red-green: confirmed both tests fail before the fix and pass after.
Full harness suite (1648 tests / 99 files) and typecheck both clean.
Widen PoolLogger to optional warn/error and route capacity-loss events
(recycle-relaunch-failed, recovery-failed, recycle-failed, new reinit-empty)
to error and per-attempt failures (relaunch-failed, reinit-failed,
close-failed) to warn so Sentry sees real outages instead of buried info.
Add a reiniting concurrent-entry guard so two acquires on an empty pool
launch at most poolSize browsers.
Jordan Ritter