fix(browser): isolate timed-out snippets - #118
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Closing in favor of #126, which keeps this PR's core and drops the rest. The diagnosis here is right and important: a timed-out snippet's Promise cannot be preempted, it keeps running against the same Session object the next tool call receives, and the two interleave on one socket. That, plus returning captured output in the timeout error, is exactly what #126 ships — including this PR's identity-checked Why not merge this as-is:
The bar for reopening the Tier-2/3 machinery is a real orphan trace from production showing the minimal version leaking cross-call effects. Happy to review that PR when the trace exists. |
…tput A browser_execute timeout fails the Effect fiber but cannot preempt the snippet's Promise. The orphan kept running against the same Session object the next tool call would receive from SessionStore, letting abandoned code and a fresh call interleave CDP commands on one socket — plausible-but-wrong results, not crashes. Timeouts also discarded all console output. On timeout we now: - retire the exact Session the snippet received (Session.invalidate rejects future connect/_call, closes the socket; in-flight calls are rejected by the existing close handler) and remove it from SessionStore by identity, so a stale caller can never evict a successor Session - freeze the capture buffer (post-timeout console.log and onChunk stop) and return the last 8 KiB of output in the error, cut on a UTF-8 boundary - document the 60s default / 600s max and reconnect-after-timeout in the skill's guardrails Deliberately minimal: no abort plumbing through the connect path (the windows are milliseconds wide and socket close + identity eviction already prevent cross-call interleaving) and no per-session locking (opencode serializes tool calls within an assistant message; concurrent same-session calls have never been observed). Diagnosis and the identity-checked eviction shape come from browser-use#118. Tests (no Chrome required) verify: timeout error carries partial output and the retired Session rejects connect/_call while the store hands out a fresh one; capture and onChunk stop after timeout; tail-capping preserves UTF-8. All three fail with the source change reverted.
Problem
browser_executeruns snippets in-process. Timing out the Effect fiber does not preempt the JavaScript Promise, so abandoned code could reconnect and send CDP commands after the caller received a timeout. Same-session tool calls may also execute concurrently.Change
SessionStore.browser_executeper session ID so a timeout cannot invalidate a healthy concurrent call; queued calls resolve the fresh Session after acquiring the lock.onChunkupdates after timeout.Reproduction
Synthetic CDP tests cover a timed-out snippet that sleeps and later attempts
Runtime.evaluate. The server receives no late command. A concurrently queued call then receives a fresh Session, reconnects, and executes successfully.A separate queue-boundary test puts a 10ms call behind a 50ms same-session call. The queued call expires without running its snippet or invalidating the active Session; the first and a later call both complete normally. Additional tests cover pending waiters, connecting sockets, prompt cancellation of profile polling, UTF-8 byte capping, and stopped post-timeout capture.
Validation
browser-execute.test.ts: 8 pass, 6 environment-gated skipspackages/bcode-browsersuite: 27 pass, 8 environment-gated skipsgit diff --checkpassedThis PR is stacked on #112 (
session-recovery).