From b215ea5ab16c3826e735f0dedddf186274cd2c35 Mon Sep 17 00:00:00 2001 From: Anderson Martinez Date: Tue, 11 Aug 2026 13:36:32 -0500 Subject: [PATCH 1/2] fix(agent): ensure bare calls receive isolated browsers and handle management --- src/lib/agent-client.ts | 58 ++--------- src/tools/agent.ts | 23 ++--- test/lib/agent-client.spec.ts | 185 ++++++++++++++++++++++++---------- test/tools/agent.spec.ts | 5 +- 4 files changed, 150 insertions(+), 121 deletions(-) diff --git a/src/lib/agent-client.ts b/src/lib/agent-client.ts index 4898797..1f7fc17 100644 --- a/src/lib/agent-client.ts +++ b/src/lib/agent-client.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import type { IncomingMessage } from 'node:http'; import WebSocket from 'ws'; import { z } from 'zod'; @@ -168,10 +169,6 @@ const pending = new Map>(); const DEFAULT_TIMEOUT = 60_000; const IDLE_TTL_MS = 15 * 60 * 1000; const MAX_SESSIONS = 500; -// How long an MCP session id must go unused before its browser is adoptable. -// Churn abandons the old id instantly; a live conversation re-uses it per turn. -const ADOPT_AFTER_IDLE_MS = 30_000; - // mcp session id -> last time a request arrived on it. `disconnect` is the // primary signal, but a client that abandons a transport never sends one. const mcpSeenAt = new Map(); @@ -644,45 +641,6 @@ const sendMessage = ( ws.send(JSON.stringify(msg)); }); -/** - * Re-key an orphaned browser onto the caller's new MCP session id. - */ -const adoptOrphan = ( - key: string, - mcpSessionId: string | undefined, - handle: string, - source: string | undefined, -): ActiveSession | undefined => { - if (!mcpSessionId || handle !== mcpSessionId) return; - - const marker = KEY_SEP + 'conv#'; - const at = key.indexOf(marker); - if (at === -1) return; - const prefix = key.slice(0, at + marker.length); - const suffix = key.slice(prefix.length + handle.length); - - const now = Date.now(); - const candidates = [...sessions.entries()].filter(([k, s]) => { - if (k === key || !k.startsWith(prefix) || !k.endsWith(suffix)) return false; - if (s.ws.readyState !== WebSocket.OPEN || s.source !== source) return false; - const owner = k.slice(prefix.length, k.length - suffix.length); - if (owner === mcpSessionId) return false; - const seen = mcpSeenAt.get(owner); - return !seen || now - seen > ADOPT_AFTER_IDLE_MS; - }); - - if (candidates.length !== 1) return; - - const [oldKey, session] = candidates[0]; - sessions.delete(oldKey); - session.handle = handle; - sessions.set(key, session); - console.error( - `[agent-client] adopted orphaned browser from mcp session ${oldKey.slice(prefix.length, oldKey.length - suffix.length)} into ${mcpSessionId}`, - ); - return session; -}; - export const getOrCreateSession = async ( mcpSessionId: string | undefined, apiUrl: string, @@ -696,9 +654,11 @@ export const getOrCreateSession = async ( echoedSessionId?: string, ): Promise => { sweepSessions(); - // Resolving up front keeps the key and the session's own handle identical - // (sessionHandle is idempotent once the handle is known). - const handle = sessionHandle(mcpSessionId, token, echoedSessionId); + // Reusing on a bare call guessed "same task" — but every concurrent task in a + // conversation shares the MCP session id, so the guess collided them onto one page. + const handle = + echoedSessionId ?? + (attachSessionId ? `attach:${attachSessionId}` : `s:${randomUUID()}`); const key = getSessionKey( mcpSessionId, token, @@ -709,11 +669,7 @@ export const getOrCreateSession = async ( handle, ); noteMcpSession(mcpSessionId); - const existing = - sessions.get(key) ?? - (echoedSessionId - ? undefined - : adoptOrphan(key, mcpSessionId, handle, source)); + const existing = sessions.get(key); if ( existing && diff --git a/src/tools/agent.ts b/src/tools/agent.ts index 092533b..61c57ce 100644 --- a/src/tools/agent.ts +++ b/src/tools/agent.ts @@ -432,17 +432,12 @@ const CLOSE_REMINDER = `When the task is done, send \`{ "method": "close" }\` as its own call — or, if ` + `the user may want to keep browsing, ask them before leaving it open.`; -// httpStream only: the handle has to travel through the conversation to pin the -// browser, while a stdio client's key is already stable for its process life. -const sessionLine = ( - session: { handle: string }, - transport: McpConfig['transport'], -): string => - transport === 'httpStream' - ? `sessionId: ${session.handle} — pass this back as \`sessionId\` on your next ` + - `browserless_agent call to keep driving THIS browser. Omitting it opens a blank one. ` + - CLOSE_REMINDER - : CLOSE_REMINDER; +// Both transports: the minted handle is the only way back, so stdio needs it too — +// its old process-wide key was what collided concurrent tasks. +const sessionLine = (session: { handle: string }): string => + `sessionId: ${session.handle} — pass this back as \`sessionId\` on your next ` + + `browserless_agent call to keep driving THIS browser. Omitting it opens a blank one. ` + + CLOSE_REMINDER; export function registerAgentTools( server: FastMCP, @@ -684,7 +679,7 @@ export function registerAgentTools( const text = createProfile ? `Profile-creation session "${createProfile.name}" is open (non-headless). Send commands to drive the login, then call saveProfile.` : 'Browser session is open. Send commands to drive it.'; - const line = sessionLine(opened, config.transport); + const line = sessionLine(opened); return [ { type: 'text' as const, text: line ? `${text}\n\n${line}` : text }, ]; @@ -872,7 +867,7 @@ export function registerAgentTools( throw new UserError( [ appendSkills(body, triggered, compliant), - fatal ? '' : sessionLine(agentSession, config.transport), + fatal ? '' : sessionLine(agentSession), ] .filter(Boolean) .join('\n\n'), @@ -989,7 +984,7 @@ export function registerAgentTools( const extraText = [ renderedSkills, siteNotice, - closedDuringBatch ? '' : sessionLine(agentSession, config.transport), + closedDuringBatch ? '' : sessionLine(agentSession), ] .filter(Boolean) .join('\n\n'); diff --git a/test/lib/agent-client.spec.ts b/test/lib/agent-client.spec.ts index 7bf09c7..2accadb 100644 --- a/test/lib/agent-client.spec.ts +++ b/test/lib/agent-client.spec.ts @@ -496,6 +496,103 @@ describe('agent-client connect (upgrade error handling)', () => { }); }); +describe('agent-client bare-call isolation', () => { + const bare = (sid: string | undefined, url: string) => + getOrCreateSession(sid, url, 'tok'); + const echo = (sid: string | undefined, url: string, handle: string) => + getOrCreateSession( + sid, + url, + 'tok', + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + handle, + ); + + // Regression: tasks in one conversation hashed to one key, so every task after + // the first landed on the same browser AND page, each goto clobbering the others. + it('gives every bare caller its own browser, sequential or concurrent', async () => { + const server = await makeAcceptingServer(); + try { + // Never timing-dependent: a browser idle between commands was reusable too. + const first = await bare('mcp-parallel', server.url); + const second = await bare('mcp-parallel', server.url); + expect(second.ws).to.not.equal(first.ws); + expect(second.handle).to.not.equal(first.handle); + + // Concurrent bare calls: no shared in-flight creation either. + const [a, b, c] = await Promise.all([ + bare('mcp-parallel', server.url), + bare('mcp-parallel', server.url), + bare('mcp-parallel', server.url), + ]); + const sockets = new Set([first.ws, second.ws, a.ws, b.ws, c.ws]); + expect(sockets.size).to.equal(5); + } finally { + await server.close(); + } + }); + + // stdio had no MCP session id, so its key was one process-wide slot — the worst + // case for parallel workers sharing a server process. + it('isolates bare callers on stdio, which has no MCP session id', async () => { + const server = await makeAcceptingServer(); + try { + const [a, b] = await Promise.all([ + bare(undefined, server.url), + bare(undefined, server.url), + ]); + expect(a.ws).to.not.equal(b.ws); + } finally { + await server.close(); + } + }); + + it('returns the same browser whenever its handle is echoed back', async () => { + const server = await makeAcceptingServer(); + try { + const opened = await bare('mcp-echo', server.url); + const resumed = await echo('mcp-echo', server.url, opened.handle); + expect(resumed.ws).to.equal(opened.ws); + + // Continuity follows the handle, not the MCP session id — remote clients + // mint a fresh id per turn, and stdio never had one. + const churned = await echo('mcp-echo-2', server.url, opened.handle); + expect(churned.ws).to.equal(opened.ws); + const onStdio = await echo(undefined, server.url, opened.handle); + expect(onStdio.ws).to.equal(opened.ws); + } finally { + await server.close(); + } + }); + + it('keeps an echoed handle scoped to its own token', async () => { + const server = await makeAcceptingServer(); + try { + const mine = await bare('mcp-tok', server.url); + const theirs = await getOrCreateSession( + 'mcp-tok', + server.url, + 'other-token', + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + mine.handle, + ); + expect(theirs.ws).to.not.equal(mine.ws); + } finally { + await server.close(); + } + }); +}); + describe('agent-client session-cache isolation', () => { it('keeps distinct sessions for the same mcpSessionId+token with different profiles', async () => { const server = await makeAcceptingServer(); @@ -521,13 +618,19 @@ describe('agent-client session-cache isolation', () => { expect(sessA.profile).to.equal('profile-a'); expect(sessB.profile).to.equal('profile-b'); - // Asking for the same (sid, profile) again returns the cached session. + // The handle carries the session, not the profile name: a bare re-ask would + // open a third browser (see bare-call isolation). const sessAAgain = await getOrCreateSession( sidA, server.url, 'tok', undefined, 'profile-a', + undefined, + undefined, + undefined, + undefined, + sessA.handle, ); expect(sessAAgain.ws).to.equal(sessA.ws); } finally { @@ -575,7 +678,9 @@ describe('agent-client session handle', () => { const server = await makeAcceptingServer(); try { const first = await getOrCreateSession('mcp-1', server.url, 'tok'); - expect(first.handle).to.equal('mcp-1'); + // The handle is minted per task, not derived from the MCP session id — + // that id is shared by every concurrent task in the conversation. + expect(first.handle).to.not.equal('mcp-1'); // The client re-initialized: new MCP session id, same conversation. const churned = await getOrCreateSession( @@ -600,64 +705,24 @@ describe('agent-client session handle', () => { } }); }); - -describe('agent-client orphan adoption', () => { - const open = async (sid: string, url: string) => +describe('agent-client mcp-session churn', () => { + const bare = (sid: string, url: string) => getOrCreateSession(sid, url, 'tok'); - it('adopts the orphaned browser when the previous MCP session went quiet', async () => { + // "Orphan adoption" let a bare call recover a browser whose MCP session went + // quiet — the same guess that collided tasks. The handle is now the only way back. + it("does not hand a quiet session's browser to the next bare caller", async () => { const server = await makeAcceptingServer(); try { - const first = await open('mcp-a', server.url); + const first = await bare('mcp-a', server.url); dropMcpSession('mcp-a'); - // Same conversation, re-initialized, model did NOT echo the handle. - const churned = await open('mcp-b', server.url); - expect(churned.ws).to.equal(first.ws); - expect(churned.handle).to.equal('mcp-b'); - } finally { - await server.close(); - } - }); - - it('leaves a live conversation alone', async () => { - const server = await makeAcceptingServer(); - try { - const live = await open('mcp-live', server.url); - // No dropMcpSession: mcp-live was just seen, so it is still driving. - const other = await open('mcp-new', server.url); - expect(other.ws).to.not.equal(live.ws); - } finally { - await server.close(); - } - }); - - it('refuses to guess when two orphans are candidates', async () => { - const server = await makeAcceptingServer(); - try { - const one = await open('mcp-1', server.url); - const two = await open('mcp-2', server.url); - dropMcpSession('mcp-1'); - dropMcpSession('mcp-2'); - - const third = await open('mcp-3', server.url); - expect(third.ws).to.not.equal(one.ws); - expect(third.ws).to.not.equal(two.ws); - } finally { - await server.close(); - } - }); - - it('prefers an echoed handle over adoption', async () => { - const server = await makeAcceptingServer(); - try { - const target = await open('mcp-x', server.url); - const decoy = await open('mcp-y', server.url); - dropMcpSession('mcp-x'); - dropMcpSession('mcp-y'); + const next = await bare('mcp-b', server.url); + expect(next.ws).to.not.equal(first.ws); + // The original browser is still reachable — by its handle. const resumed = await getOrCreateSession( - 'mcp-z', + 'mcp-b', server.url, 'tok', undefined, @@ -666,10 +731,20 @@ describe('agent-client orphan adoption', () => { undefined, false, undefined, - 'mcp-x', + first.handle, ); - expect(resumed.ws).to.equal(target.ws); - expect(resumed.ws).to.not.equal(decoy.ws); + expect(resumed.ws).to.equal(first.ws); + } finally { + await server.close(); + } + }); + + it("never lets a bare caller reach another conversation's live browser", async () => { + const server = await makeAcceptingServer(); + try { + const live = await bare('mcp-live', server.url); + const other = await bare('mcp-new', server.url); + expect(other.ws).to.not.equal(live.ws); } finally { await server.close(); } diff --git a/test/tools/agent.spec.ts b/test/tools/agent.spec.ts index ee6d762..bbf298e 100644 --- a/test/tools/agent.spec.ts +++ b/test/tools/agent.spec.ts @@ -1127,7 +1127,10 @@ describe('browserless_agent session handle on errors', () => { ); handle = /sessionId: (\S+)/.exec(msg)?.[1]; } - expect(handle).to.equal('err-handle-1'); + // Minted per task rather than taken from the MCP session id, which every + // concurrent task in the conversation shares. + expect(handle).to.match(/^s:/); + expect(handle).to.not.equal('err-handle-1'); // A churned MCP session echoing that handle must land on the same browser. const after = await execute( From 8dfd3d867463545207a8e55b94e78f2ba6a8a9b6 Mon Sep 17 00:00:00 2001 From: Anderson Martinez Date: Tue, 11 Aug 2026 13:43:07 -0500 Subject: [PATCH 2/2] fix(agent): simplify session line handling in registerAgentTools --- src/tools/agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/agent.ts b/src/tools/agent.ts index 3a8abc0..dcf7e46 100644 --- a/src/tools/agent.ts +++ b/src/tools/agent.ts @@ -886,7 +886,7 @@ export function registerAgentTools( message: `the page did not load — the browser is on ${(resp.result as { url?: string }).url ?? 'an error page'}`, recovery: navFailure.recovery, }), - sessionLine(agentSession, config.transport), + sessionLine(agentSession), ] .filter(Boolean) .join('\n\n'),