Skip to content

fix(browser): isolate timed-out snippets - #118

Closed
MagMueller wants to merge 6 commits into
session-recoveryfrom
timeout-guard
Closed

fix(browser): isolate timed-out snippets#118
MagMueller wants to merge 6 commits into
session-recoveryfrom
timeout-guard

Conversation

@MagMueller

@MagMueller MagMueller commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

browser_execute runs 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

  • Permanently invalidate the exact Session object used by a timed-out snippet and remove it by identity from SessionStore.
  • Reject pending/future transport work, close active and connecting sockets, and stop profile polling promptly on invalidation.
  • Serialize browser_execute per session ID so a timeout cannot invalidate a healthy concurrent call; queued calls resolve the fresh Session after acquiring the lock.
  • Count queue wait against the configured timeout. A waiter that expires runs no code and does not invalidate the active Session.
  • Return recent console progress in the timeout error, capped to 8 KiB of valid UTF-8 with a truncation marker.
  • Stop console capture and onChunk updates after timeout.
  • Document the 60s default, 600s maximum, top-level timeout, and small logged batches in one guardrail.

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

  • Focused browser-execute.test.ts: 8 pass, 6 environment-gated skips
  • Full packages/bcode-browser suite: 27 pass, 8 environment-gated skips
  • Package typecheck passed
  • Pre-push filtered typecheck: 13/13 packages passed
  • git diff --check passed

This PR is stacked on #112 (session-recovery).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread packages/bcode-browser/src/browser-execute.ts
Comment thread packages/bcode-browser/src/cdp/session.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/bcode-browser/src/cdp/session.ts
Comment thread packages/bcode-browser/src/cdp/session.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/bcode-browser/src/cdp/session.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/bcode-browser/src/browser-execute.ts Outdated
@Alezander9

Copy link
Copy Markdown
Member

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 SessionStore.invalidate(sessionID, expected, error) shape, which is the correct way to prevent a stale caller from evicting a successor Session.

Why not merge this as-is:

  1. Stacked on a closed base. The branch sits on session-recovery (fix(browser): recover stale CDP sessions #112, closed) and its session.ts hunks reference fields that only exist there (enabledDomains, activeTargetId, reattachPromise) plus the pre-fix(browser): make waitFor options-only and race-safe #114 positional waitFor. Merging or retargeting would drag the rejected recovery work back in; a rebase is a rewrite either way.

  2. The Semaphore solves an unobserved problem. opencode serializes tool calls within an assistant message, and concurrent same-session browser_execute has never appeared in a production trace. The lock adds ~50 lines plus a genuinely new failure mode — a call timing out in the queue having run nothing. If interleaved same-session calls ever show up in the wild, that trace should drive the design (and be the test fixture). fix(browser): retire timed-out snippet sessions and return partial output #126 keeps the old "acceptable for v1" comment honest by noting the shared-object caveat explicitly.

  3. The abort plumbing guards millisecond windows. Threading AbortController through connect/resolveWsUrl/DevToolsActivePort polling, plus the openingSockets and pendingWaiters sets, defends races whose worst case is already covered by socket close + identity eviction: the orphan cannot affect any future call, which is the property that matters. A timeout landing mid-connect leaves the orphan a half-open socket for milliseconds, touching nothing shared.

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.

@Alezander9 Alezander9 closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants