Skip to content

Commit 77f8af7

Browse files
committed
fix(browser): retire timed-out snippet sessions and return partial output
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.
1 parent 3106007 commit 77f8af7

5 files changed

Lines changed: 165 additions & 14 deletions

File tree

packages/bcode-browser/skills/browser-execute/SKILL.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ console.log(JSON.stringify(titles))
197197
## Guardrails
198198
- Top-level `import` statements inside the snippet body are not allowed. Use `await import(...)` instead.
199199
- No CPU-bound infinite loops without `await` — they ignore the timeout. Insert `await new Promise(r => setTimeout(r, 0))` to yield.
200+
- `browser_execute` defaults to 60s (max 600s). For longer work, set the tool's top-level `timeout`; inner CDP timeouts do not extend it. Keep batches small and log progress — timeout errors return recent logs, and a timeout resets the CDP session (reconnect in the next snippet).
200201
201202
## Console
202203
- `console.log`, `console.error`, `console.warn`, `console.info`, `console.debug` are all captured and streamed to the user. Treat them as your stdout. Other `console.*` methods write to bcode's stderr without being captured into the tool result.

packages/bcode-browser/src/browser-execute.ts

Lines changed: 51 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,13 @@
3232
//
3333
// Cancellation: JS Promises are not preemptively cancellable. A snippet
3434
// without `await` yield-points (e.g. `for (let i = 0; i < 1e9; i++) {}`)
35-
// runs to completion before our timeout fiber observes it. `Effect.timeoutOrElse`
36-
// fails the surrounding fiber but the orphan Promise keeps running until it
37-
// finishes. This matches the `uv run` subprocess case (SIGTERM only after
38-
// the Python signal handler yields). Document, don't fix.
35+
// runs to completion before our timeout fiber observes it. When a yielding
36+
// snippet times out, its Promise keeps running as an orphan — so on timeout
37+
// we retire the exact Session object the snippet received (rejects future
38+
// connect/_call, closes the socket) and evict it from SessionStore. The
39+
// orphan can finish local work but cannot keep driving the browser, and the
40+
// next tool call gets a fresh Session instead of sharing a socket with it.
41+
// The timeout error carries the console output captured so far.
3942
//
4043
// Level 1 per decisions.md §1c — substantial implementation lives here. The
4144
// Level-2 hook in packages/opencode is a thin adapter.
@@ -48,6 +51,18 @@ import { Skills } from "./skills"
4851

4952
const DEFAULT_TIMEOUT_MS = 60 * 1000
5053
const MAX_TIMEOUT_MS = 10 * 60 * 1000
54+
const MAX_TIMEOUT_OUTPUT_BYTES = 8 * 1024
55+
const TIMEOUT_OUTPUT_TRUNCATED = "[partial console output truncated; showing final bytes]\n"
56+
57+
// Tail-cap the captured output for the timeout error: last 8 KiB, snapped
58+
// forward to a UTF-8 sequence start so multibyte characters survive the cut.
59+
const timeoutOutput = (output: string) => {
60+
const bytes = Buffer.from(output, "utf8")
61+
if (bytes.length <= MAX_TIMEOUT_OUTPUT_BYTES) return output
62+
let start = bytes.length - (MAX_TIMEOUT_OUTPUT_BYTES - Buffer.byteLength(TIMEOUT_OUTPUT_TRUNCATED))
63+
while (start < bytes.length && (bytes[start]! & 0xc0) === 0x80) start++
64+
return TIMEOUT_OUTPUT_TRUNCATED + bytes.subarray(start).toString("utf8")
65+
}
5166

5267
// Field order matters: providers stream tool-call args in schema-declared
5368
// order, so the model commits to whichever field comes first. `code` is the
@@ -157,20 +172,24 @@ const serialize = (v: unknown): string => {
157172
export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string) {
158173
const skillsDir = yield* Effect.promise(() => Skills.resolveSkillsDir(dataDir))
159174

160-
const execute = (args: Parameters, ctx: ExecuteContext) =>
161-
Effect.gen(function* () {
162-
const session = SessionStore.get(ctx.sessionID)
175+
const execute = (args: Parameters, ctx: ExecuteContext) => {
176+
// Resolved outside the generator so the timeout handler below can retire
177+
// the exact Session this snippet received and freeze its capture buffer.
178+
const session = SessionStore.get(ctx.sessionID)
179+
const captured = { active: true, output: "" }
180+
const timeout = Math.min(args.timeout ?? DEFAULT_TIMEOUT_MS, MAX_TIMEOUT_MS)
181+
return Effect.gen(function* () {
163182
yield* Effect.promise(() => fs.mkdir(ctx.workspaceDir, { recursive: true }))
164183

165184
const wrapped = yield* Effect.try({
166185
try: () => new AsyncFunction("session", "console", args.code),
167186
catch: (err) => new Error(`syntax error in browser_execute snippet: ${err}`),
168187
})
169188

170-
let output = ""
171189
const tee = (...a: unknown[]) => {
172-
output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
173-
if (ctx.onChunk) Effect.runFork(ctx.onChunk(output))
190+
if (!captured.active) return
191+
captured.output += a.map((x) => (typeof x === "string" ? x : serialize(x))).join(" ") + "\n"
192+
if (ctx.onChunk) Effect.runFork(ctx.onChunk(captured.output))
174193
}
175194
// Prototype-chain to the real `console` so uncommon methods (`debug`,
176195
// `dir`, `trace`, `table`, `group`, …) don't throw when a snippet calls
@@ -223,14 +242,33 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string)
223242
catch: (err) => new Error(`browser_execute snippet threw: ${err instanceof Error ? err.stack ?? err.message : String(err)}`),
224243
}).pipe(Effect.ensuring(Effect.sync(() => unsubscribe())))
225244

226-
return { output, result: serialize(ran), screenshots } satisfies ExecuteResult
245+
return { output: captured.output, result: serialize(ran), screenshots } satisfies ExecuteResult
227246
}).pipe(
228247
Effect.scoped,
229248
Effect.timeoutOrElse({
230-
duration: Math.min(args.timeout ?? DEFAULT_TIMEOUT_MS, MAX_TIMEOUT_MS),
231-
orElse: () => Effect.fail(new Error("browser_execute timed out")),
249+
duration: timeout,
250+
orElse: () =>
251+
Effect.suspend(() => {
252+
captured.active = false
253+
const output = timeoutOutput(captured.output)
254+
const error = new Error(
255+
[
256+
`browser_execute timed out after ${timeout} ms; CDP session was reset — reconnect in the next snippet`,
257+
output.trim() ? `Partial console output before timeout:\n${output.trimEnd()}` : "",
258+
]
259+
.filter(Boolean)
260+
.join("\n\n"),
261+
)
262+
// Identity-checked: only removes the store entry if it still maps
263+
// to this snippet's Session. A concurrent same-sessionID call
264+
// would share the retired object — acceptable for v1, opencode
265+
// serializes tool calls within an assistant message.
266+
SessionStore.invalidate(ctx.sessionID, session, error)
267+
return Effect.fail(error)
268+
}),
232269
}),
233270
)
271+
}
234272

235273
return { parameters, execute, skillsDir }
236274
})

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ export type DetectedBrowser = {
4343

4444
export class Session implements Transport {
4545
private ws?: WebSocket;
46+
private invalidatedError?: Error;
4647
private nextId = 1;
4748
private pending = new Map<number, Pending>();
4849
private activeSessionId: string | undefined;
@@ -79,6 +80,7 @@ export class Session implements Transport {
7980
* and we connect directly to the supplied endpoint.
8081
*/
8182
async connect(opts: ConnectOptions = {}): Promise<void> {
83+
if (this.invalidatedError) throw this.invalidatedError;
8284
const timeoutMs = opts.timeoutMs ?? 5_000;
8385
if (opts.wsUrl || opts.profileDir) {
8486
const wsUrl = await resolveWsUrl(opts, timeoutMs);
@@ -137,13 +139,33 @@ export class Session implements Transport {
137139
}
138140

139141
isConnected(): boolean {
140-
return this.ws?.readyState === WebSocket.OPEN;
142+
return !this.invalidatedError && this.ws?.readyState === WebSocket.OPEN;
141143
}
142144

143145
close(): void {
144146
this.ws?.close();
145147
}
146148

149+
/**
150+
* Permanently retire this Session object.
151+
*
152+
* `browser_execute` timeouts cannot preempt the snippet's Promise — the
153+
* orphan keeps running and would otherwise share this object (and its
154+
* socket) with the next tool call, interleaving two authors on one
155+
* transport. Invalidation rejects all future `connect`/`_call` attempts
156+
* and closes the socket (the close handler rejects in-flight calls);
157+
* `SessionStore.invalidate` removes the entry so the next call gets a
158+
* fresh Session.
159+
*/
160+
invalidate(error: Error): void {
161+
if (this.invalidatedError) return;
162+
this.invalidatedError = error;
163+
const ws = this.ws;
164+
this.ws = undefined;
165+
this.activeSessionId = undefined;
166+
try { ws?.close(); } catch { /* ignore */ }
167+
}
168+
147169
/**
148170
* Pick a target and make subsequent calls auto-route to it.
149171
* Uses Target.attachToTarget with flatten:true (single-WS, sessionId-on-message).
@@ -239,6 +261,7 @@ export class Session implements Transport {
239261

240262
// Transport implementation. Called by the generated domain bindings.
241263
_call(method: string, params: unknown = {}): Promise<unknown> {
264+
if (this.invalidatedError) return Promise.reject(this.invalidatedError);
242265
if (!this.ws || this.ws.readyState !== WebSocket.OPEN) {
243266
return Promise.reject(new Error('Not connected. Call session.connect(...) first.'));
244267
}

packages/bcode-browser/src/session-store.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ export const get = (sessionID: string): Session => {
2727
return fresh
2828
}
2929

30+
// Retire a specific Session object after a browser_execute timeout. Identity-
31+
// checked so a stale caller can never evict a successor Session that a newer
32+
// call is already using.
33+
export const invalidate = (sessionID: string, expected: Session, error: Error): void => {
34+
const entry = sessions.get(sessionID)
35+
if (entry !== expected) return
36+
sessions.delete(sessionID)
37+
entry.invalidate(error)
38+
}
39+
3040
export const evict = async (sessionID: string): Promise<void> => {
3141
const entry = sessions.get(sessionID)
3242
if (!entry) return

packages/bcode-browser/test/browser-execute.test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,85 @@ test("console.debug is captured; uncommon methods fall through without throwing"
224224
await Promise.all([data, ws].map((d) => fs.rm(d, { recursive: true, force: true })))
225225
})
226226

227+
// Timeout isolation: a timed-out snippet keeps running as an orphan (JS
228+
// Promises are not preemptible), so the tool must retire the Session object
229+
// the snippet received and surface captured output in the error. No Chrome
230+
// required — the snippets sleep without touching the browser.
231+
const runTimeout = async (id: string, code: string, timeout: number, onChunk?: (o: string) => Effect.Effect<void>) => {
232+
const data = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-"))
233+
const ws = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-ws-"))
234+
const err = await Effect.runPromise(
235+
Effect.scoped(
236+
Effect.gen(function* () {
237+
const impl = yield* BrowserExecute.make(data)
238+
return yield* impl.execute(
239+
{ description: "timeout test", code, timeout },
240+
{ sessionID: id, workspaceDir: ws, onChunk },
241+
)
242+
}),
243+
),
244+
).then(
245+
() => { throw new Error("expected timeout") },
246+
(e: unknown) => String(e),
247+
)
248+
await Promise.all([data, ws].map((d) => fs.rm(d, { recursive: true, force: true })))
249+
return err
250+
}
251+
252+
test("timeout returns partial output and retires the session", async () => {
253+
const id = "timeout-isolation-test"
254+
const before = SessionStore.get(id)
255+
const err = await runTimeout(
256+
id,
257+
`console.log("progress-marker");
258+
await new Promise((r) => setTimeout(r, 60_000));`,
259+
100,
260+
)
261+
expect(err).toContain("timed out after 100 ms")
262+
expect(err).toContain("Partial console output before timeout:")
263+
expect(err).toContain("progress-marker")
264+
// The orphan's Session is permanently dead...
265+
expect(before.isConnected()).toBe(false)
266+
await expect(before.connect({ wsUrl: "ws://127.0.0.1:9/nope" })).rejects.toThrow(/timed out after 100 ms/)
267+
await expect(before.domains.Runtime.evaluate({ expression: "1" })).rejects.toThrow(/timed out after 100 ms/)
268+
// ...and the next tool call gets a fresh one.
269+
expect(SessionStore.get(id)).not.toBe(before)
270+
await SessionStore.evict(id)
271+
})
272+
273+
test("console capture and onChunk stop after timeout", async () => {
274+
const chunks: string[] = []
275+
const err = await runTimeout(
276+
"timeout-capture-test",
277+
`console.log("early");
278+
await new Promise((r) => setTimeout(r, 250));
279+
console.log("late");`,
280+
100,
281+
(o) => Effect.sync(() => { chunks.push(o) }),
282+
)
283+
expect(err).toContain("early")
284+
// Let the orphan's late log fire, then confirm it was not captured.
285+
await new Promise((r) => setTimeout(r, 400))
286+
expect(chunks.some((c) => c.includes("early"))).toBe(true)
287+
expect(chunks.some((c) => c.includes("late"))).toBe(false)
288+
await SessionStore.evict("timeout-capture-test")
289+
})
290+
291+
test("timeout output is tail-capped to valid UTF-8", async () => {
292+
// ~25 KiB of multibyte lines, all logged before the sleep.
293+
const err = await runTimeout(
294+
"timeout-truncate-test",
295+
`for (let i = 0; i < 300; i++) console.log("é".repeat(40) + "-line-" + i);
296+
await new Promise((r) => setTimeout(r, 60_000));`,
297+
100,
298+
)
299+
expect(err).toContain("[partial console output truncated; showing final bytes]")
300+
expect(err).toContain("-line-299")
301+
expect(err).not.toContain("-line-0\n")
302+
expect(err).not.toContain("\uFFFD")
303+
await SessionStore.evict("timeout-truncate-test")
304+
})
305+
227306
// Concurrency safety: two overlapping execute() calls (different sessionIDs)
228307
// must each capture their own console output without leaking into each other
229308
// or into the real global console. No Chrome required — the snippets never

0 commit comments

Comments
 (0)