Skip to content

fix(browser): retire timed-out snippet sessions and return partial output - #126

Merged
Alezander9 merged 2 commits into
mainfrom
timeout-isolation
Jul 30, 2026
Merged

fix(browser): retire timed-out snippet sessions and return partial output#126
Alezander9 merged 2 commits into
mainfrom
timeout-isolation

Conversation

@Alezander9

@Alezander9 Alezander9 commented Jul 30, 2026

Copy link
Copy Markdown
Member

Problem

browser_execute runs snippets in-process, and a timeout cannot preempt the snippet's Promise. Two consequences:

  1. Orphan interleaving. The timed-out snippet keeps running against the same Session object that SessionStore hands to the next tool call. Abandoned code and a fresh call then share one CDP socket: the orphan navigates or clicks while the new snippet is mid-scrape, the new snippet's waitFor fires on the orphan's events. It does not crash — it returns plausible-but-wrong results the model builds on.
  2. Lost progress. The timeout error was the string browser_execute timed out. A snippet that logged useful progress for 60s returned none of it.

Change

On timeout:

  • Retire the exact Session the snippet received. Session.invalidate(error) rejects all future connect/_call with the timeout error and closes the socket (the existing close handler rejects in-flight calls). SessionStore.invalidate is identity-checked, so a stale caller can never evict a successor Session a newer call is using. The next tool call gets a fresh Session; the orphan can finish local work but cannot keep driving the browser.
  • Freeze the capture buffer — post-timeout console.log and onChunk updates stop — and return the last 8 KiB of captured output in the error, cut on a UTF-8 sequence boundary, with an explicit truncation marker.
  • Document the timeout contract in the skill guardrails: 60s default / 600s max, top-level timeout is the only knob, keep batches small and log progress, reconnect after a timeout.

What this deliberately does not include

  • No abort-signal plumbing through connect/resolveWsUrl/DevToolsActivePort polling. Those windows are milliseconds wide, and socket close + identity eviction already prevent the failure that matters (cross-call interleaving).
  • No per-session locking/queueing. opencode serializes tool calls within an assistant message; concurrent same-session browser_execute calls have not been observed. If a production trace ever shows one, that is the moment to add serialization — with the trace as the test fixture.

Credit

Supersedes #118, which correctly diagnosed the orphan problem and whose identity-checked eviction shape is kept here. This PR takes the minimal core and drops the speculative machinery; rationale in the #118 close comment.

Validation

  • Three new tests (no Chrome needed): timeout error carries partial output, retired Session rejects connect/_call, store hands out a fresh Session; capture and onChunk stop after timeout; tail-capping preserves multibyte UTF-8. All three fail with the source change reverted.
  • Full packages/bcode-browser suite: 20 pass, 8 env-gated skips, 1 pre-existing failure (workspace-import cache-bust flake, fails identically on clean main).
  • bun typecheck clean.

Summary by cubic

Hardened browser_execute timeouts by retiring the exact timed‑out Session, stopping streaming, and including the last 8 KiB of console output in the error. Also documents a 60s default (600s max) and ensures re-runs get a fresh session and capture buffer.

  • Bug Fixes
    • Retire the exact Session on timeout via Session.invalidate; rejects future connect/_call and closes the socket. SessionStore.invalidate now always retires the expected object; identity check only guards the map delete so successors aren’t evicted.
    • Re-check invalidation during connect/openWs (entry, open, close) and rethrow retirement errors from detection so late sockets can’t open; in‑flight and pre‑open calls fail with the retirement error.
    • Move execute state (Session, capture buffer, timeout) inside Effect.suspend to avoid re-run leakage; freeze capture and onChunk after timeout; attach the last 8 KiB of UTF‑8‑safe console output with a truncation marker.
    • Update SKILL.md with the timeout contract and reconnect‑after‑timeout guidance.

Written for commit b1f6512. Summary will update on new commits.

Review in cubic

…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 #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.

@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.

2 issues found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/bcode-browser/src/browser-execute.ts">

<violation number="1" location="packages/bcode-browser/src/browser-execute.ts:191">
P2: The 8 KiB limit is applied only after the entire console history has been retained. A yielding snippet that logs continuously can grow `captured.output` without bound and then incur another full-buffer allocation in `timeoutOutput()`, defeating the timeout tail cap as a memory safeguard. A bounded rolling tail (with separate handling for successful full output if that behavior must remain) would keep timeout handling from being vulnerable to unbounded log growth.</violation>

<violation number="2" location="packages/bcode-browser/src/browser-execute.ts:192">
P2: Progress callbacks already forked before timeout can still publish output afterward when `onChunk` contains an async boundary. Track and interrupt these callback fibers (or make the callback effect cancellation-aware) during timeout so the stated capture freeze also covers in-flight updates.</violation>
</file>

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread packages/bcode-browser/src/cdp/session.ts
if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
if (!captured.active) return
captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
if (ctx.onChunk) Effect.runFork(ctx.onChunk(captured.output))

@cubic-dev-ai cubic-dev-ai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Progress callbacks already forked before timeout can still publish output afterward when onChunk contains an async boundary. Track and interrupt these callback fibers (or make the callback effect cancellation-aware) during timeout so the stated capture freeze also covers in-flight updates.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bcode-browser/src/browser-execute.ts, line 192:

<comment>Progress callbacks already forked before timeout can still publish output afterward when `onChunk` contains an async boundary. Track and interrupt these callback fibers (or make the callback effect cancellation-aware) during timeout so the stated capture freeze also covers in-flight updates.</comment>

<file context>
@@ -157,20 +172,24 @@ const serialize = (v: unknown): string => {
-        if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
+        if (!captured.active) return
+        captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
+        if (ctx.onChunk) Effect.runFork(ctx.onChunk(captured.output))
       }
       // Prototype-chain to the real `console` so uncommon methods (`debug`,
</file context>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Declining. A late in-flight onChunk delivery carries the cumulative output that was true at timeout time — never post-timeout content, since the tee freezes the buffer before the error is built — and the tool's final result supersedes any progress update. Fiber tracking would add bookkeeping to suppress a benign, already-stale cosmetic update.

Comment thread packages/bcode-browser/src/browser-execute.ts
output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
if (!captured.active) return
captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"

@cubic-dev-ai cubic-dev-ai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: The 8 KiB limit is applied only after the entire console history has been retained. A yielding snippet that logs continuously can grow captured.output without bound and then incur another full-buffer allocation in timeoutOutput(), defeating the timeout tail cap as a memory safeguard. A bounded rolling tail (with separate handling for successful full output if that behavior must remain) would keep timeout handling from being vulnerable to unbounded log growth.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bcode-browser/src/browser-execute.ts, line 191:

<comment>The 8 KiB limit is applied only after the entire console history has been retained. A yielding snippet that logs continuously can grow `captured.output` without bound and then incur another full-buffer allocation in `timeoutOutput()`, defeating the timeout tail cap as a memory safeguard. A bounded rolling tail (with separate handling for successful full output if that behavior must remain) would keep timeout handling from being vulnerable to unbounded log growth.</comment>

<file context>
@@ -157,20 +172,24 @@ const serialize = (v: unknown): string => {
-        output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
-        if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
+        if (!captured.active) return
+        captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
+        if (ctx.onChunk) Effect.runFork(ctx.onChunk(captured.output))
       }
</file context>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Declining as out of scope. The full buffer is the success path's contract (result.output returns everything, onChunk receives the cumulative output) and predates this PR; growth is time-bounded by the 600s timeout cap. timeoutOutput caps what enters the error message, which is the new surface this PR adds. A rolling tail would change the success-path contract to defend a bound the cap already provides.

Comment thread packages/bcode-browser/src/session-store.ts Outdated
Address cubic review on #126:

- openWs re-checks invalidation (entry, open, close paths) so a connect
  awaiting resolver/detection steps cannot open a late socket for a retired
  Session; the detect-loop rethrows instead of aggregating the retirement
  error. In-flight and pre-open calls now reject with the retirement error
  rather than the generic socket-closed message.
- SessionStore.invalidate always retires the expected Session; the identity
  check now only guards the map delete. Previously a replaced entry left the
  stale object un-retired.
- execute() state (Session, capture buffer, timeout) moved inside
  Effect.suspend: Effect values are re-runnable, and a re-run after a
  timeout must not inherit a retired Session or frozen capture.

Declined (rationale on the PR): interrupting in-flight onChunk fibers (late
delivery carries output that was true at timeout; final result supersedes)
and rolling-tail capping of the capture buffer (success path returns full
output by contract; duration is bounded by the 600s cap).

Four new tests; the connect-entry, replaced-entry, and re-run pins fail
against the previous commit.
@Alezander9
Alezander9 merged commit 00781d5 into main Jul 30, 2026
3 checks passed
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.

1 participant