From 6d88ec63ece9e2d0bb0b5ed95f903efea442b8ef Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 2 Nov 2026 12:13:43 +0000 Subject: [PATCH] fix(automate): concurrent fetch_page calls returned another call's page Each engine drives one shared tab (navigate, then read). Two fetches in flight interleaved, so every caller got the last navigated page's text while the status line still named its own URL. Seen live on the hosted relay: three concurrent fetches (Wikipedia, Salesforce, nichedb) all got nichedb's page, and pit.world summaries came out identical. Navigate+read is now one critical section per engine instance. Regression test with a one-tab fake fails without the lock. Co-Authored-By: Claude Opus 5.5 --- packages/sdk/src/mcp/automate.test.ts | 40 ++++++++++++++++++++++++++- packages/sdk/src/mcp/automate.ts | 22 +++++++++++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/packages/sdk/src/mcp/automate.test.ts b/packages/sdk/src/mcp/automate.test.ts index 1723a4a..b0289ee 100644 --- a/packages/sdk/src/mcp/automate.test.ts +++ b/packages/sdk/src/mcp/automate.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; import type { AgentSnapshot } from '@tronbrowser/browser-core'; -import { automateTools, fetchPage, markdownExpression, parseFetchArgs, wallReason, engineStatus } from './automate.js'; +import { automateTools, exclusive, fetchPage, markdownExpression, parseFetchArgs, wallReason, engineStatus } from './automate.js'; import { ObscuraClient, type ObscuraProcess } from './obscura.js'; import { McpBrowserSession, type McpPage } from './session.js'; import { createMcpServer } from './server.js'; @@ -110,6 +110,44 @@ describe('wallReason', () => { }); }); +describe('fetchPage concurrency', () => { + it('concurrent fetches each get their own page, never the tab another fetch navigated to', async () => { + // One shared tab, like the real Obscura: navigate sets it after a delay, markdown reads whatever it is now. + let tab = ''; + const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + const obscura = { + available: () => true, + call: async (name: string, a: Record) => { + if (name === 'browser_navigate') { + await sleep(String(a.url).includes('slow') ? 30 : 5); + tab = String(a.url); + return { content: [{ type: 'text', text: 'ok' }] }; + } + await sleep(10); + return { content: [{ type: 'text', text: `# Page at ${tab}\n\n${LONG}` }] }; + }, + } as unknown as ObscuraClient; + const { session } = fakePage(); + const urls = ['https://a.example/slow', 'https://b.example/', 'https://c.example/']; + const out = await Promise.all(urls.map((url) => fetchPage({ obscura, session }, parseFetchArgs({ url })))); + out.forEach((r, i) => { + expect(r.outcome.engine).toBe('obscura'); + expect(r.text).toContain(`# Page at ${urls[i]}`); + }); + }); + + it('a failed fetch does not block the next one', async () => { + const order: string[] = []; + await expect(exclusive(fakePage().session, async () => { order.push('a'); throw new Error('boom'); })).rejects.toThrow('boom'); + expect(await exclusive(fakePage().session, async () => (order.push('b'), 'ok'))).toBe('ok'); + const key = {}; + const first = exclusive(key, async () => { throw new Error('x'); }); + const second = exclusive(key, async () => 'after'); + await expect(first).rejects.toThrow('x'); + expect(await second).toBe('after'); + }); +}); + describe('fetchPage routing', () => { it('uses Obscura when it answers with real content', async () => { const { client, calls } = fakeObscura({ browser_markdown: { text: '# Example\n\n' + LONG } }); diff --git a/packages/sdk/src/mcp/automate.ts b/packages/sdk/src/mcp/automate.ts index 98926af..c7325b0 100644 --- a/packages/sdk/src/mcp/automate.ts +++ b/packages/sdk/src/mcp/automate.ts @@ -154,7 +154,26 @@ export function markdownExpression(): string { })()`; } +/** + * One fetch at a time per browser. Each engine drives ONE shared tab: navigate, then read. Two + * fetches in flight interleave (A navigates, B navigates, A reads B's page), and every caller got + * the last page's text while the status line still named its own URL. So navigate+read is one + * critical section per engine, keyed by the browser object. Obscura and Chromium are separate + * locks, so an Obscura read and a Chromium fallback can still overlap. + */ +const tails = new WeakMap>(); +export function exclusive(key: object, run: () => Promise): Promise { + const before = tails.get(key) ?? Promise.resolve(); + const mine = before.then(run, run); + tails.set(key, mine.catch(() => undefined)); + return mine; +} + async function viaObscura(obscura: ObscuraClient, args: FetchArgs): Promise { + return exclusive(obscura, () => obscuraOnce(obscura, args)); +} + +async function obscuraOnce(obscura: ObscuraClient, args: FetchArgs): Promise { const nav = await obscura.call('browser_navigate', { url: args.url, waitUntil: 'load' }, args.budgetMs); if (nav.isError) throw new Error(resultText(nav) || 'navigate failed'); let result; @@ -231,8 +250,7 @@ export async function fetchPage(deps: AutomateDeps, args: FetchArgs): Promise<{ if (args.engine === 'obscura') throw new Error(fellBack); } - const page = await deps.session.getPage(); - const text = await viaChromium(page, args); + const text = await exclusive(deps.session, async () => viaChromium(await deps.session.getPage(), args)); return finish('chromium', text, fellBack); }