Skip to content

Commit 40c5709

Browse files
committed
fix: isolate concurrent browser agent instances
When two browser_agent calls run concurrently, they previously shared a single BrowserManager singleton, causing one agent to navigate away from the other's page. Add acquire()/release() tracking to BrowserManager so getInstance() creates a new parallel instance when the primary is already in-use. Sequential calls still reuse the same browser (persistent session). Changes: - BrowserManager: add inUse field, acquire/release/isAcquired methods - BrowserManager.getInstance: skip in-use instances, create parallel instances with suffixed keys (e.g. persistent:default:1) - browserAgentFactory: call acquire() after getInstance() - browserAgentInvocation: call release() in finally block - Add 4 unit tests for concurrent isolation behavior - Add integration test for concurrent browser agents with persistent session mode
1 parent 68fef87 commit 40c5709

8 files changed

Lines changed: 180 additions & 2 deletions
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"I'll launch two browser agents concurrently to check both repositories."},{"functionCall":{"name":"browser_agent","args":{"task":"Navigate to https://example.com and get the page title"}}},{"functionCall":{"name":"browser_agent","args":{"task":"Navigate to https://example.com and get the page title"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":50,"totalTokenCount":150}}]}
2+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"navigate_page","args":{"url":"https://example.com"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":20,"totalTokenCount":120}}]}
3+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"navigate_page","args":{"url":"https://example.com"}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":100,"candidatesTokenCount":20,"totalTokenCount":120}}]}
4+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"take_snapshot","args":{}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":150,"candidatesTokenCount":15,"totalTokenCount":165}}]}
5+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"take_snapshot","args":{}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":150,"candidatesTokenCount":15,"totalTokenCount":165}}]}
6+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"complete_task","args":{"result":{"success":true,"summary":"Page title is Example Domain."}}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":200,"candidatesTokenCount":30,"totalTokenCount":230}}]}
7+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"complete_task","args":{"result":{"success":true,"summary":"Page title is Example Domain."}}}}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":200,"candidatesTokenCount":30,"totalTokenCount":230}}]}
8+
{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"Both browser agents completed successfully. Agent 1 and Agent 2 both navigated to their respective pages and confirmed the page titles."}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":300,"candidatesTokenCount":40,"totalTokenCount":340}}]}

integration-tests/browser-agent.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,4 +307,47 @@ describe.skipIf(!chromeAvailable)('browser-agent', () => {
307307

308308
await run.expectText('successfully written', 15000);
309309
});
310+
311+
it('should handle concurrent browser agents with persistent session mode', async () => {
312+
rig.setup('browser-concurrent', {
313+
fakeResponsesPath: join(__dirname, 'browser-agent.concurrent.responses'),
314+
settings: {
315+
agents: {
316+
overrides: {
317+
browser_agent: {
318+
enabled: true,
319+
},
320+
},
321+
browser: {
322+
headless: true,
323+
// Persistent mode is the default — the regression scenario.
324+
// Two concurrent calls should each get a separate browser.
325+
sessionMode: 'persistent',
326+
},
327+
},
328+
},
329+
});
330+
331+
const result = await rig.run({
332+
args: 'Launch two browser agents concurrently to check example.com',
333+
});
334+
335+
assertModelHasOutput(result);
336+
337+
const toolLogs = rig.readToolLogs();
338+
const browserCalls = toolLogs.filter(
339+
(t) => t.toolRequest.name === 'browser_agent',
340+
);
341+
342+
// Both browser_agent invocations should have been called
343+
expect(browserCalls.length).toBe(2);
344+
345+
// Both should complete successfully (no errors)
346+
for (const call of browserCalls) {
347+
expect(
348+
call.toolRequest.success,
349+
`browser_agent call failed: ${JSON.stringify(call.toolRequest)}`,
350+
).toBe(true);
351+
}
352+
});
310353
});

packages/core/src/agents/browser/browserAgentFactory.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ const mockBrowserManager = {
3838
]),
3939
callTool: vi.fn().mockResolvedValue({ content: [] }),
4040
close: vi.fn().mockResolvedValue(undefined),
41+
acquire: vi.fn(),
42+
release: vi.fn(),
4143
};
4244

4345
// Mock dependencies

packages/core/src/agents/browser/browserAgentFactory.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export async function createBrowserAgentDefinition(
8181

8282
// Get or create browser manager singleton for this session mode/profile
8383
const browserManager = BrowserManager.getInstance(config);
84+
browserManager.acquire();
8485
await browserManager.ensureConnection();
8586

8687
debugLogger.log('Browser connected with isolated MCP client.');

packages/core/src/agents/browser/browserAgentInvocation.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,10 @@ describe('BrowserAgentInvocation', () => {
192192
promptConfig: { query: '', systemPrompt: '' },
193193
toolConfig: { tools: ['analyze_screenshot', 'click'] },
194194
},
195-
browserManager: {} as never,
195+
browserManager: {
196+
release: vi.fn(),
197+
callTool: vi.fn().mockResolvedValue({ content: [] }),
198+
} as never,
196199
visionEnabled: true,
197200
sessionMode: 'persistent',
198201
});
@@ -766,6 +769,7 @@ describe('BrowserAgentInvocation', () => {
766769
}
767770
return { isError: false };
768771
}),
772+
release: vi.fn(),
769773
};
770774

771775
vi.mocked(createBrowserAgentDefinition).mockResolvedValue({

packages/core/src/agents/browser/browserAgentInvocation.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,13 @@ import {
3434
isToolActivityError,
3535
} from '../types.js';
3636
import type { MessageBus } from '../../confirmation-bus/message-bus.js';
37-
import { createBrowserAgentDefinition } from './browserAgentFactory.js';
37+
import {
38+
createBrowserAgentDefinition,
39+
cleanupBrowserAgent,
40+
} from './browserAgentFactory.js';
3841
import { removeInputBlocker } from './inputBlocker.js';
3942
import { logBrowserAgentTaskOutcome } from '../../telemetry/loggers.js';
43+
import { recordBrowserAgentTaskOutcome } from '../../telemetry/metrics.js';
4044
import {
4145
sanitizeThoughtContent,
4246
sanitizeToolArgs,
@@ -405,6 +409,14 @@ ${output.result}`;
405409
duration_ms: Date.now() - invocationStartMs,
406410
});
407411

412+
recordBrowserAgentTaskOutcome(this.config, {
413+
success: taskSuccess,
414+
session_mode: sessionMode,
415+
vision_enabled: visionEnabled,
416+
headless: !!this.config.getBrowserAgentConfig().customConfig.headless,
417+
duration_ms: Date.now() - invocationStartMs,
418+
});
419+
408420
// Clean up input blocker, but keep browserManager alive for persistent sessions
409421
if (browserManager) {
410422
await removeInputBlocker(browserManager, signal);
@@ -441,6 +453,8 @@ ${output.result}`;
441453
} catch {
442454
// Ignore errors for removing the overlays.
443455
}
456+
await cleanupBrowserAgent(browserManager, this.config, sessionMode);
457+
browserManager.release();
444458
}
445459
}
446460
}

packages/core/src/agents/browser/browserManager.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -873,6 +873,55 @@ describe('BrowserManager', () => {
873873

874874
expect(instance1).not.toBe(instance2);
875875
});
876+
877+
it('should return a different instance when the primary is acquired', () => {
878+
const instance1 = BrowserManager.getInstance(mockConfig);
879+
instance1.acquire();
880+
881+
const instance2 = BrowserManager.getInstance(mockConfig);
882+
883+
expect(instance2).not.toBe(instance1);
884+
expect(instance1.isAcquired()).toBe(true);
885+
expect(instance2.isAcquired()).toBe(false);
886+
});
887+
888+
it('should reuse the primary when it has been released', () => {
889+
const instance1 = BrowserManager.getInstance(mockConfig);
890+
instance1.acquire();
891+
instance1.release();
892+
893+
const instance2 = BrowserManager.getInstance(mockConfig);
894+
895+
expect(instance2).toBe(instance1);
896+
expect(instance1.isAcquired()).toBe(false);
897+
});
898+
899+
it('should reuse a released parallel instance', () => {
900+
const instance1 = BrowserManager.getInstance(mockConfig);
901+
instance1.acquire();
902+
903+
const instance2 = BrowserManager.getInstance(mockConfig);
904+
instance2.acquire();
905+
instance2.release();
906+
907+
// Primary is still acquired, parallel is released — should reuse parallel
908+
const instance3 = BrowserManager.getInstance(mockConfig);
909+
expect(instance3).toBe(instance2);
910+
});
911+
912+
it('should create multiple parallel instances for multiple concurrent calls', () => {
913+
const instance1 = BrowserManager.getInstance(mockConfig);
914+
instance1.acquire();
915+
916+
const instance2 = BrowserManager.getInstance(mockConfig);
917+
instance2.acquire();
918+
919+
const instance3 = BrowserManager.getInstance(mockConfig);
920+
921+
expect(instance1).not.toBe(instance2);
922+
expect(instance2).not.toBe(instance3);
923+
expect(instance1).not.toBe(instance3);
924+
});
876925
});
877926

878927
describe('resetAll', () => {

packages/core/src/agents/browser/browserManager.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@ export class BrowserManager {
128128
/**
129129
* Returns an existing BrowserManager for the current config's session mode
130130
* and profile, or creates a new one.
131+
*
132+
* When an instance is currently acquired (in-use by an active invocation),
133+
* a new instance is created with a suffixed key so concurrent browser_agent
134+
* calls each get their own browser.
131135
*/
132136
static getInstance(config: Config): BrowserManager {
133137
const key = BrowserManager.getInstanceKey(config);
@@ -136,6 +140,29 @@ export class BrowserManager {
136140
instance = new BrowserManager(config);
137141
BrowserManager.instances.set(key, instance);
138142
debugLogger.log(`Created new BrowserManager singleton (key: ${key})`);
143+
} else if (instance.inUse) {
144+
// The primary instance is in-use by another concurrent invocation.
145+
// Find or create a parallel instance with a suffixed key.
146+
let suffix = 1;
147+
let parallelKey = `${key}:${suffix}`;
148+
let parallel = BrowserManager.instances.get(parallelKey);
149+
while (parallel?.inUse) {
150+
suffix++;
151+
parallelKey = `${key}:${suffix}`;
152+
parallel = BrowserManager.instances.get(parallelKey);
153+
}
154+
if (!parallel) {
155+
parallel = new BrowserManager(config);
156+
BrowserManager.instances.set(parallelKey, parallel);
157+
debugLogger.log(
158+
`Created parallel BrowserManager (key: ${parallelKey})`,
159+
);
160+
} else {
161+
debugLogger.log(
162+
`Reusing released parallel BrowserManager (key: ${parallelKey})`,
163+
);
164+
}
165+
instance = parallel;
139166
} else {
140167
debugLogger.log(
141168
`Reusing existing BrowserManager singleton (key: ${key})`,
@@ -180,6 +207,36 @@ export class BrowserManager {
180207
private isClosing = false;
181208
private connectionPromise: Promise<void> | undefined;
182209

210+
/**
211+
* Whether this instance is currently acquired by an active invocation.
212+
* Used by getInstance() to avoid handing the same browser to concurrent
213+
* browser_agent calls.
214+
*/
215+
private inUse = false;
216+
217+
/**
218+
* Marks this instance as in-use. Call this when starting a browser agent
219+
* invocation so concurrent calls get a separate instance.
220+
*/
221+
acquire(): void {
222+
this.inUse = true;
223+
}
224+
225+
/**
226+
* Marks this instance as available for reuse. Call this in the finally
227+
* block of a browser agent invocation.
228+
*/
229+
release(): void {
230+
this.inUse = false;
231+
}
232+
233+
/**
234+
* Returns whether this instance is currently acquired by an active invocation.
235+
*/
236+
isAcquired(): boolean {
237+
return this.inUse;
238+
}
239+
183240
/** State for action rate limiting */
184241
private actionCounter = 0;
185242
private readonly maxActionsPerTask: number;

0 commit comments

Comments
 (0)