fix(browser): preserve session after tool timeout

Author: MagMueller

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

@@ -35,20 +35,18 @@
 //
 // Cancellation: JS Promises are not preemptively cancellable. A snippet
 // without `await` yield-points (e.g. `for (let i = 0; i < 1e9; i++) {}`)
-// runs to completion before our timeout fiber observes it. When a yielding
-// snippet times out, its Promise keeps running as an orphan — so on timeout
-// we retire the exact Session object the snippet received (rejects future
-// connect/_call, closes the socket) and evict it from SessionStore. The
-// orphan can finish local work but cannot keep driving the browser, and the
-// next tool call gets a fresh Session instead of sharing a socket with it.
-// The timeout error carries the console output captured so far.
+// runs to completion before our timeout fiber observes it. A yielding snippet
+// keeps running as an orphan after timeout, so each call receives a scoped
+// Session view. The view rejects methods after its deadline while the real
+// Session and its tabs remain available to the next call.
 //
 // Level 1 per decisions.md §1c — substantial implementation lives here. The
 // Level-2 hook in packages/opencode is a thin adapter.
 
 import fs from "fs/promises"
 import path from "path"
 import { Effect, Schema } from "effect"
+import { withSessionExecution } from "./cdp/session"
 import { SessionStore } from "./session-store"
 import { Skills } from "./skills"
 
@@ -182,6 +180,7 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string)
     Effect.suspend(() => {
     const session = SessionStore.get(ctx.sessionID)
     const captured = { active: true, output: "" }
+    const sessionExecution = { active: true }
     const timeout = Math.min(args.timeout ?? DEFAULT_TIMEOUT_MS, MAX_TIMEOUT_MS)
     return Effect.gen(function* () {
       yield* Effect.promise(() => fs.mkdir(ctx.workspaceDir, { recursive: true }))
@@ -248,7 +247,7 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string)
       })
 
       const ran = yield* Effect.tryPromise({
-        try: () => wrapped(session, snippetConsole),
+        try: () => withSessionExecution(sessionExecution, () => wrapped(session, snippetConsole)),
         catch: (err) => new Error(`browser_execute snippet threw: ${err instanceof Error ? err.stack ?? err.message : String(err)}`),
       }).pipe(Effect.ensuring(Effect.sync(() => unsubscribe())))
 
@@ -260,21 +259,16 @@ export const make = Effect.fn("BrowserExecute.make")(function* (dataDir: string)
         orElse: () =>
           Effect.suspend(() => {
             captured.active = false
+            sessionExecution.active = false
             const output = timeoutOutput(captured.output)
             const error = new Error(
               [
-                `browser_execute timed out after ${timeout} ms; CDP session was reset — reconnect in the next snippet`,
+                `browser_execute timed out after ${timeout} ms; the browser session remains connected`,
                 output.trim() ? `Partial console output before timeout:\n${output.trimEnd()}` : "",
               ]
                 .filter(Boolean)
                 .join("\n\n"),
             )
-            // Always retires this snippet's Session; the identity check
-            // inside only guards the store delete, so a successor Session is
-            // never evicted. A concurrent same-sessionID call would share the
-            // retired object — acceptable for v1, opencode serializes tool
-            // calls within an assistant message.
-            SessionStore.invalidate(ctx.sessionID, session, error)
             return Effect.fail(error)
           }),
       }),
@@ -304,9 +298,8 @@ async function ensureCloudConnected(sessionID: string, session: ReturnType<typeo
     await connecting
   } finally {
     // One automatic attempt per logical BrowserCode session. A later disconnect
-    // (including after timeout replacement) must be surfaced:
-    // BU_CDP_WS is the browser selected at run start, not necessarily a newer
-    // browser the agent explicitly switched to during this run.
+    // must be surfaced: BU_CDP_WS is the browser selected at run start, not
+    // necessarily a newer browser the agent explicitly switched to during this run.
     v4Bootstrapped.add(sessionID)
     if (v4Connections.get(sessionID) === connecting) v4Connections.delete(sessionID)
   }

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

@@ -6,13 +6,29 @@
  * Target.sendMessageToTarget envelopes).
  */
 
+import { AsyncLocalStorage } from 'node:async_hooks';
 import { bindDomains, type Domains, type Transport } from './generated.ts';
 
 type Pending = {
   resolve: (v: unknown) => void;
   reject: (e: unknown) => void;
 };
 
+export type SessionExecution = { active: boolean };
+
+const sessionExecution = new AsyncLocalStorage<SessionExecution>();
+
+export const withSessionExecution = <T>(
+  execution: SessionExecution,
+  run: () => T,
+): T => sessionExecution.run(execution, run);
+
+const assertExecutionActive = (): void => {
+  if (sessionExecution.getStore()?.active === false) {
+    throw new Error('browser_execute call already timed out');
+  }
+};
+
 export type ConnectOptions = {
   /** Full WS URL: ws://host:port/devtools/browser/<id>. Escape hatch. */
   wsUrl?: string;
@@ -80,6 +96,7 @@ export class Session implements Transport {
    * and we connect directly to the supplied endpoint.
    */
   async connect(opts: ConnectOptions = {}): Promise<void> {
+    assertExecutionActive();
     if (this.invalidatedError) throw this.invalidatedError;
     const timeoutMs = opts.timeoutMs ?? 5_000;
     if (opts.wsUrl || opts.profileDir) {
@@ -117,6 +134,7 @@ export class Session implements Transport {
   }
 
   private openWs(wsUrl: string, timeoutMs: number): Promise<void> {
+    assertExecutionActive();
     // Re-checked here (not only in connect) because connect awaits resolver/
     // detection steps first — an invalidation landing during those must not
     // open a late socket for a retired Session.
@@ -170,6 +188,7 @@ export class Session implements Transport {
   }
 
   close(): void {
+    assertExecutionActive();
     this.ws?.close();
   }
 
@@ -185,6 +204,7 @@ export class Session implements Transport {
    * fresh Session.
    */
   invalidate(error: Error): void {
+    assertExecutionActive();
     if (this.invalidatedError) return;
     this.invalidatedError = error;
     const ws = this.ws;
@@ -199,12 +219,14 @@ export class Session implements Transport {
    */
   async use(targetId: string): Promise<string> {
     const r = await this._call('Target.attachToTarget', { targetId, flatten: true }) as { sessionId: string };
+    assertExecutionActive();
     this.activeSessionId = r.sessionId;
     return r.sessionId;
   }
 
   /** Set the active sessionId directly (e.g. one you already attached). */
   setActiveSession(sessionId: string | undefined): void {
+    assertExecutionActive();
     this.activeSessionId = sessionId;
   }
 
@@ -214,6 +236,7 @@ export class Session implements Transport {
 
   /** Subscribe to all CDP events. Returns an unsubscribe fn. */
   onEvent(fn: (method: string, params: unknown, sessionId?: string) => void): () => void {
+    assertExecutionActive();
     this.eventListeners.push(fn);
     return () => {
       this.eventListeners = this.eventListeners.filter(x => x !== fn);
@@ -231,6 +254,7 @@ export class Session implements Transport {
    * agnostic of any one method's semantics.
    */
   onCallResult(fn: (method: string, params: unknown, result: unknown) => void): () => void {
+    assertExecutionActive();
     this.callResultListeners.push(fn);
     return () => {
       this.callResultListeners = this.callResultListeners.filter(x => x !== fn);
@@ -249,6 +273,7 @@ export class Session implements Transport {
     opts: { predicate?: (params: T) => boolean; timeoutMs?: number } = {},
     ...rest: never[]
   ): Promise<T> {
+    assertExecutionActive();
     // Both legacy positional shapes fail loudly rather than silently reverting
     // to the 30s default: `(method, predicate)` lands on the first guard,
     // `(method, predicate?, timeoutMs)` on the second. Snippets are written at
@@ -288,6 +313,7 @@ export class Session implements Transport {
 
   // Transport implementation. Called by the generated domain bindings.
   _call(method: string, params: unknown = {}): Promise<unknown> {
+    assertExecutionActive();
     if (this.invalidatedError) return Promise.reject(this.invalidatedError);
     if (!this.ws || this.ws.readyState !== WebSocket.OPEN) {
       return Promise.reject(new Error('Not connected. Call session.connect(...) first.'));

packages/bcode-browser/test/browser-auto-connect.test.ts

@@ -276,8 +276,10 @@ test("an explicit browser switch retires the old socket and target attachment",
   );
 });
 
-test("a timeout replacement does not auto-attach the run-start browser again", async () => {
+test("a timeout preserves the same browser and target", async () => {
   connections = 0;
+  attachedCalls = 0;
+  pageCallsWithSession = 0;
   await withEnv(
     { V4_RUN_ID: "run-timeout", BU_CDP_WS: wsUrl, BU_CDP_URL: undefined },
     () =>
@@ -287,26 +289,34 @@ test("a timeout replacement does not auto-attach the run-start browser again", a
             impl.execute(
               {
                 description: "Time out after initial V4 bootstrap",
-                code: "await new Promise(resolve => setTimeout(resolve, 100))",
+                code: `
+                  const navigate = session.Page.navigate
+                  setTimeout(async () => {
+                    try { await navigate({ url: "https://too-late.example" }) } catch {}
+                  }, 30)
+                  await new Promise(resolve => setTimeout(resolve, 100))
+                `,
                 timeout: 10,
               },
               { sessionID, workspaceDir },
             ),
           ),
         ).rejects.toThrow("browser_execute timed out");
 
-        await expect(
-          Effect.runPromise(
-            impl.execute(
-              {
-                description: "Do not silently return to the run-start browser",
-                code: "return await session.Page.navigate({ url: 'https://sap.com' })",
-              },
-              { sessionID, workspaceDir },
-            ),
+        const recovered = await Effect.runPromise(
+          impl.execute(
+            {
+              description: "Continue on the same browser",
+              code: "return await session.Page.navigate({ url: 'https://sap.com' })",
+            },
+            { sessionID, workspaceDir },
           ),
-        ).rejects.toThrow("Not connected. Call session.connect(...) first.");
+        );
+        expect(JSON.parse(recovered.result)).toEqual({});
+        await new Promise((resolve) => setTimeout(resolve, 40));
         expect(connections).toBe(1);
+        expect(attachedCalls).toBe(1);
+        expect(pageCallsWithSession).toBe(1);
       }),
   );
 });

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

@@ -225,9 +225,9 @@ test("console.debug is captured; uncommon methods fall through without throwing"
 })
 
 // Timeout isolation: a timed-out snippet keeps running as an orphan (JS
-// Promises are not preemptible), so the tool must retire the Session object
-// the snippet received and surface captured output in the error. No Chrome
-// required — the snippets sleep without touching the browser.
+// Promises are not preemptible), so its scoped Session view must stop working
+// while the persistent Session remains available to the next call. Browser
+// behavior is covered by browser-auto-connect; these snippets need no Chrome.
 const runTimeout = async (id: string, code: string, timeout: number, onChunk?: (o: string) => Effect.Effect<void>) => {
   const data = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-"))
   const ws = await fs.mkdtemp(path.join(os.tmpdir(), "bcode-to-ws-"))
@@ -249,7 +249,7 @@ const runTimeout = async (id: string, code: string, timeout: number, onChunk?: (
   return err
 }
 
-test("timeout returns partial output and retires the session", async () => {
+test("timeout returns partial output and preserves the session", async () => {
   const id = "timeout-isolation-test"
   const before = SessionStore.get(id)
   const err = await runTimeout(
@@ -261,12 +261,10 @@ test("timeout returns partial output and retires the session", async () => {
   expect(err).toContain("timed out after 100 ms")
   expect(err).toContain("Partial console output before timeout:")
   expect(err).toContain("progress-marker")
-  // The orphan's Session is permanently dead...
+  // The next call gets the exact same persistent Session. The orphan only had
+  // a scoped view, whose post-timeout CDP behavior is covered by the V4 test.
   expect(before.isConnected()).toBe(false)
-  await expect(before.connect({ wsUrl: "ws://127.0.0.1:9/nope" })).rejects.toThrow(/timed out after 100 ms/)
-  await expect(before.domains.Runtime.evaluate({ expression: "1" })).rejects.toThrow(/timed out after 100 ms/)
-  // ...and the next tool call gets a fresh one.
-  expect(SessionStore.get(id)).not.toBe(before)
+  expect(SessionStore.get(id)).toBe(before)
   await SessionStore.evict(id)
 })
 
@@ -294,7 +292,7 @@ test("re-running the execute effect after a timeout gets fresh state", async ()
   const impl = await Effect.runPromise(BrowserExecute.make(data))
   // One Effect value, run twice. Each run must resolve its own Session and
   // capture buffer — the second run's error must carry its own partial
-  // output, not inherit the first run's frozen capture or retired Session.
+  // output, not inherit the first run's frozen capture or scoped view.
   // onChunk deliveries discriminate: a run that inherited a frozen capture
   // buffer never tees, so it produces zero chunks (the frozen buffer still
   // *contains* run 1's text, which is why asserting on the error message