feat(serve)!: admit concurrent runs with a bounded queue instead of refusing them - #400
Open
frontierkodiak wants to merge 4 commits into
Open
feat(serve)!: admit concurrent runs with a bounded queue instead of refusing them#400frontierkodiak wants to merge 4 commits into
frontierkodiak wants to merge 4 commits into
Conversation
…efusing them `oracle serve` was single-flight: a second caller got HTTP 409 `busy` and was expected to invent a retry policy. That shape fits a service where runs are short. These runs are not — a Pro answer can take ten minutes, and nearly all of it is waiting for the model rather than driving the browser. Nothing in the browser stack required the restriction. Runs hold their own CDP page connection, clipboard capture is page-local, input and uploads are per-target, and temp directories are per-run; the composer section that genuinely must be serialized already is, by the profile run lock. What was missing was a place for the next caller to wait. So: a bounded number of concurrent runs (4 by default) and a FIFO queue for the rest. A queued caller is told its position over the existing `log` event, so older clients ignore it rather than breaking. Refusal is now reserved for a full queue — 503 with `Retry-After` — because a caller told "later" can wait, while a caller told "no" has to guess. Cancellation had no representation at all: the service never observed client disconnect. It does now, and a disconnect frees whatever the caller held — its place in the queue, or its slot. Without that a long-lived service leaks capacity to clients that walked away until it stops accepting work. Two isolation defects that single-flight was hiding are fixed with it. The client's session slug was used verbatim as the key for the server's own artifact directory, and slugs are prompt-derived, so two callers could collide; the server now namespaces per run. And the browser tab cap is pinned to what the service admits, so extra callers wait in the queue where the wait is visible rather than inside the lease loop where it is not. `/health` reports active, queued, and capacity so a caller can decide when to send work instead of discovering the answer by being queued. BREAKING: a second concurrent caller is now served rather than receiving 409 `busy`. A client that treated 409 as its signal to back off will no longer see one; saturation is 503 `queue_full`. Claude-Session: https://claude.ai/code/session_01HsXirqcfqtr1Cae9zYCLDk
`oracle serve` observed client disconnects and freed the queue slot, but nothing reached the run itself: `BrowserRunOptions` had no way to express cancellation, so a disconnected caller's run continued to completion and its capacity came back only by accident of finishing. Measured before this change, a client killed ten seconds into a thirty-second run held its slot for the remaining twenty. That is the wrong shape for runs this long. A browser run holds a tab and a slot on a shared profile for minutes, and the caller is the only party that knows it has stopped caring. `signal` joins the existing disconnect race, so every awaited step honours it and the existing finally does the unwinding it already knew how to do — releasing the tab lease, closing the owned tab, stopping the monitors. Cancellation raises `BrowserRunCancelledError` rather than a generic failure, because a caller that walked away is not a run that went wrong, and a reader of the session record should not go looking for a fault. Verified live: the same interrupt now releases the slot 2s after disconnect instead of 20s, and the service records "cancelled: the caller disconnected" rather than a completion. Claude-Session: https://claude.ai/code/session_01HsXirqcfqtr1Cae9zYCLDk
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 availability
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 4:13 AM ET / 08:13 UTC. ClawSweeper reviewWhat this changesThe branch replaces single-flight remote browser runs with bounded concurrent admission, FIFO queueing, disconnect cancellation, health counters, and per-run artifact isolation. Merge readiness⛔ Blocked until stronger real behavior proof is added - 7 items remain Keep open: this is still distinct from current main, but two previously identified P1 defects remain in the introduced admission and cancellation paths, and the supplied live claim does not prove the latest fixes. Priority: P2 Review scores
Verification
How this fits together
flowchart LR
A[Remote callers] --> B[Serve request handler]
B --> C[Request validation]
C --> D[Admission slots]
D --> E[Browser run]
E --> F[Shared Chrome profile]
E --> G[Result stream]
D --> H[Health counters]
Decision needed
Why: This is a deliberate remote-service contract and default-policy change; correctness repairs cannot determine whether existing client backoff behavior should be preserved or migrated. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt the bounded-queue contract only after slot acquisition atomically rejects excess waiters, cancellation covers all browser setup with late-resource cleanup, and the chosen upgrade behavior is explicitly confirmed. Do we have a high-confidence way to reproduce the issue? Yes, by source: fill active slots, then send multiple valid requests whose body reads complete after the pre-read saturation check; each can join the waiting array despite the configured queue cap. Is this the best way to solve the issue? No: the intended queue needs atomic reservation or rejection after validation, and the cancellation implementation must race Chrome setup rather than only the tab-lease wait. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against bbc1b3b0261d. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
…ng the tab cap Three gaps in the first pass, all raised in review and all correct. **The tab cap is not the service's to overwrite.** Pinning `browserConfig.maxConcurrentTabs` to the admission limit silently replaced an operator's lower choice — exactly what someone staying under an account's throttling would have set. The dependency runs the other way: the tab cap is the physical constraint on a shared profile, so the service now reads the host's configured cap and admits at most that many, logging when it clamps. **`signal` has to mean the same thing on both sides of the bridge.** The remote executor never observed it, so a caller aborting a remote run cancelled nothing: the request stayed open, the service never saw a disconnect, and the run kept its slot and its browser tab until it finished on its own. That is worse than not having cancellation, because the caller believes it worked. The executor now destroys its request on abort and refuses to send one that was aborted first. **Cancellation arrived too late to matter.** The abort race was installed after the tab-lease wait, Chrome startup, and the CDP connection — the slowest part of a cold run, and the part most likely to be waiting on a peer. Several abandoned requests could hold every slot until their browser timeouts. The race is now built before setup begins. A lease granted after the caller gave up is handed back rather than leaked, since a slot abandoned mid-queue would otherwise sit for six hours. Claude-Session: https://claude.ai/code/session_01HsXirqcfqtr1Cae9zYCLDk
Contributor
Author
Review follow-upAll three P1 findings were correct and are fixed in Preserve the configured shared-browser tab cap. You're right that the dependency was backwards. The tab cap is the physical constraint on a shared profile and belongs to the host; pinning it to whatever the service admits silently discards an operator's lower choice — and "lower to avoid account throttling" is precisely the case, since ChatGPT rate-limits well below what the transport can drive. The service now reads the host's configured Forward AbortSignal to remote requests. Also right, and the worst of the three: a caller aborting a remote run cancelled nothing while believing it had. The executor now destroys its HTTP request on abort — which is how the service learns to cancel, via its own disconnect handling — and refuses to send a request whose signal was already aborted. Make pre-connection setup cancellable. The race was installed after the tab-lease wait, Chrome startup, and the CDP connection: the slowest stretch of a cold run and the one most likely to be waiting on a peer. It is now built before setup begins. On your parenthetical about leaking — losing the race does not cancel the acquisition, so a lease granted after the caller gave up is now handed back explicitly. Otherwise cancelling during a queue wait burns a slot on the shared profile for the six-hour stale window, which is worse than not honouring the cancellation at all. Two tests added for the bridge case: a caller aborting a remote run is observed as an abort inside the run, and an already-aborted caller never sends the request. Full suite: 1774 passed / 43 skipped. @clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
oracle servewas single-flight: a second caller got HTTP 409busyand was expected to invent a retry policy. That shape fits a service where runs are short. These are not — a Pro answer can take ten minutes, and nearly all of it is waiting for the model rather than driving the browser.Nothing in the browser stack required the restriction. Runs hold their own CDP page connection (
connectToNewTarget), clipboard capture is page-local JS monkey-patching insideRuntime.evaluate, input and uploads are per-target, temp dirs are per-run, and a repo-wide sweep finds no module-scope mutable state insrc/browserbeyond frozen constants. The composer section that genuinely must be serialized already is, by the profile run lock. What was missing was a place for the next caller to wait.Approach
A bounded number of concurrent runs (4 by default) and a FIFO queue for the rest. A queued caller is told its position over the existing
logevent, so older clients ignore it rather than breaking. Refusal is reserved for a full queue — 503 withRetry-After— because a caller told "later" can wait, while a caller told "no" has to guess.Cancellation had no representation at all: the service never observed client disconnect, and
BrowserRunOptionshad no way to express it. Measured before this change, a client killed ten seconds into a thirty-second run held its browser tab and its slot for the remaining twenty.signalnow joins the existing disconnect race, so every awaited step honours it and the existing unwinding releases the tab lease and closes the owned tab. It raisesBrowserRunCancelledError, since a caller that walked away is not a run that went wrong.Two isolation defects that single-flight was hiding are fixed with it: the client's session slug was used verbatim as the key for the server's own artifact directory (slugs are prompt-derived, so two callers could collide), and the browser tab cap is now pinned to what the service admits, so extra callers wait in the queue where the wait is visible rather than inside the lease loop where it is not.
Real behavior
Five concurrent callers against one browser, watched through
/health:All five exited 0, each got its own answer, and each landed in a distinct conversation — five ids, no cross-talk.
Cancellation, same setup: a client killed ten seconds into a run now releases the slot 2s after disconnect (previously 20s, i.e. natural completion), and the service records
cancelled: the caller disconnectedrather than a completion.Worth knowing
ChatGPT itself rate-limits well below what the transport can drive: six conversations opened at once tripped its "Too many requests" modal repeatedly on a Pro account, while five did not. So the default cap of 4 is deliberately under that. (#395 makes that modal report itself as a rate limit instead of as a missing model.)
Breaking
A second concurrent caller is now served rather than receiving 409
busy. A client that treated 409 as its back-off signal will no longer see one; saturation is 503queue_full.Tests
Ten added: admission up to the limit, the caller past the limit waiting rather than failing, FIFO order, saturation only when the queue is full too, cancellation while queued and while running, double-release safety, an end-to-end concurrency run through the real HTTP path, per-run session-id isolation, and the tab cap being pinned to what the service admits.
Full suite green: 1772 passed / 43 skipped.