diff --git a/packages/cli/src/ui/commands/clearCommand.ts b/packages/cli/src/ui/commands/clearCommand.ts index 061c4f9085..fb032da811 100644 --- a/packages/cli/src/ui/commands/clearCommand.ts +++ b/packages/cli/src/ui/commands/clearCommand.ts @@ -9,6 +9,7 @@ import { SessionEndReason, SessionStartSource, flushTelemetry, + resetBrowserSession, } from '@google/gemini-cli-core'; import { CommandKind, type SlashCommand } from './types.js'; import { MessageType } from '../types.js'; @@ -43,6 +44,10 @@ export const clearCommand: SlashCommand = { if (geminiClient) { context.ui.setDebugMessage('Clearing terminal and resetting chat.'); + + // Close persistent browser sessions before resetting chat + await resetBrowserSession(); + // If resetChat fails, the exception will propagate and halt the command, // which is the correct behavior to signal a failure to the user. await geminiClient.resetChat(); diff --git a/packages/cli/src/utils/cleanup.ts b/packages/cli/src/utils/cleanup.ts index 19aa795640..abdcabae5a 100644 --- a/packages/cli/src/utils/cleanup.ts +++ b/packages/cli/src/utils/cleanup.ts @@ -11,6 +11,7 @@ import { shutdownTelemetry, isTelemetrySdkInitialized, ExitCodes, + resetBrowserSession, } from '@google/gemini-cli-core'; import type { Config } from '@google/gemini-cli-core'; @@ -72,6 +73,13 @@ export async function runExitCleanup() { } cleanupFunctions.length = 0; // Clear the array + // Close persistent browser sessions before disposing config + try { + await resetBrowserSession(); + } catch (_) { + // Ignore errors during browser cleanup + } + if (configForTelemetry) { try { await configForTelemetry.dispose(); diff --git a/packages/core/src/agents/browser/browserAgentFactory.test.ts b/packages/core/src/agents/browser/browserAgentFactory.test.ts index 003ba465c4..22a99edab2 100644 --- a/packages/core/src/agents/browser/browserAgentFactory.test.ts +++ b/packages/core/src/agents/browser/browserAgentFactory.test.ts @@ -7,7 +7,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { createBrowserAgentDefinition, - cleanupBrowserAgent, + resetBrowserSession, } from './browserAgentFactory.js'; import { injectAutomationOverlay } from './automationOverlay.js'; import { makeFakeConfig } from '../../test-utils/config.js'; @@ -15,7 +15,6 @@ import { PolicyDecision, PRIORITY_SUBAGENT_TOOL } from '../../policy/types.js'; import type { Config } from '../../config/config.js'; import type { MessageBus } from '../../confirmation-bus/message-bus.js'; import type { PolicyEngine } from '../../policy/policy-engine.js'; -import type { BrowserManager } from './browserManager.js'; // Create mock browser manager const mockBrowserManager = { @@ -35,9 +34,17 @@ const mockBrowserManager = { }; // Mock dependencies -vi.mock('./browserManager.js', () => ({ - BrowserManager: vi.fn(() => mockBrowserManager), -})); +vi.mock('./browserManager.js', () => { + const instancesMap = new Map(); + const MockBrowserManager = vi.fn() as unknown as Record; + // Add static methods — use mockImplementation for lazy eval (hoisting-safe) + MockBrowserManager['getInstance'] = vi.fn(); + MockBrowserManager['resetAll'] = vi.fn().mockResolvedValue(undefined); + MockBrowserManager['instances'] = instancesMap; + return { + BrowserManager: MockBrowserManager, + }; +}); vi.mock('./automationOverlay.js', () => ({ injectAutomationOverlay: vi.fn().mockResolvedValue(undefined), @@ -60,9 +67,16 @@ describe('browserAgentFactory', () => { let mockConfig: Config; let mockMessageBus: MessageBus; - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks(); + // Set up getInstance to return mockBrowserManager + // (Can't do this in vi.mock factory due to hoisting) + const { BrowserManager: MockBM } = await import('./browserManager.js'); + (MockBM as unknown as Record>)[ + 'getInstance' + ].mockReturnValue(mockBrowserManager); + vi.mocked(injectAutomationOverlay).mockClear(); // Reset mock implementations @@ -99,7 +113,7 @@ describe('browserAgentFactory', () => { } as unknown as MessageBus; }); - afterEach(() => { + afterEach(async () => { vi.restoreAllMocks(); }); @@ -302,6 +316,23 @@ describe('browserAgentFactory', () => { }); }); + describe('resetBrowserSession', () => { + it('should delegate to BrowserManager.resetAll', async () => { + const { BrowserManager: MockBrowserManager } = await import( + './browserManager.js' + ); + await resetBrowserSession(); + expect( + ( + MockBrowserManager as unknown as Record< + string, + ReturnType + > + )['resetAll'], + ).toHaveBeenCalled(); + }); + }); + describe('Policy Registration', () => { let mockPolicyEngine: { addRule: ReturnType; @@ -421,25 +452,6 @@ describe('browserAgentFactory', () => { ); }); }); - - describe('cleanupBrowserAgent', () => { - it('should call close on browser manager', async () => { - await cleanupBrowserAgent( - mockBrowserManager as unknown as BrowserManager, - ); - - expect(mockBrowserManager.close).toHaveBeenCalled(); - }); - - it('should handle errors during cleanup gracefully', async () => { - const errorManager = { - close: vi.fn().mockRejectedValue(new Error('Close failed')), - } as unknown as BrowserManager; - - // Should not throw - await expect(cleanupBrowserAgent(errorManager)).resolves.toBeUndefined(); - }); - }); }); describe('buildBrowserSystemPrompt', () => { diff --git a/packages/core/src/agents/browser/browserAgentFactory.ts b/packages/core/src/agents/browser/browserAgentFactory.ts index 0d28651c12..94632354d7 100644 --- a/packages/core/src/agents/browser/browserAgentFactory.ts +++ b/packages/core/src/agents/browser/browserAgentFactory.ts @@ -62,8 +62,8 @@ export async function createBrowserAgentDefinition( 'Creating browser agent definition with isolated MCP tools...', ); - // Create and initialize browser manager with isolated MCP client - const browserManager = new BrowserManager(config); + // Get or create browser manager singleton for this session mode/profile + const browserManager = BrowserManager.getInstance(config); await browserManager.ensureConnection(); if (printOutput) { @@ -242,19 +242,10 @@ export async function createBrowserAgentDefinition( } /** - * Cleans up browser resources after agent execution. + * Closes all persistent browser sessions and cleans up resources. * - * @param browserManager The browser manager to clean up + * Call this on /clear commands and CLI exit to reset browser state. */ -export async function cleanupBrowserAgent( - browserManager: BrowserManager, -): Promise { - try { - await browserManager.close(); - debugLogger.log('Browser agent cleanup complete'); - } catch (error) { - debugLogger.error( - `Error during browser cleanup: ${error instanceof Error ? error.message : String(error)}`, - ); - } +export async function resetBrowserSession(): Promise { + await BrowserManager.resetAll(); } diff --git a/packages/core/src/agents/browser/browserAgentInvocation.test.ts b/packages/core/src/agents/browser/browserAgentInvocation.test.ts index e41377bdd4..200f04e67b 100644 --- a/packages/core/src/agents/browser/browserAgentInvocation.test.ts +++ b/packages/core/src/agents/browser/browserAgentInvocation.test.ts @@ -26,7 +26,10 @@ vi.mock('../../utils/debugLogger.js', () => ({ vi.mock('./browserAgentFactory.js', () => ({ createBrowserAgentDefinition: vi.fn(), - cleanupBrowserAgent: vi.fn(), +})); + +vi.mock('./inputBlocker.js', () => ({ + removeInputBlocker: vi.fn(), })); vi.mock('../local-executor.js', () => ({ @@ -35,10 +38,8 @@ vi.mock('../local-executor.js', () => ({ }, })); -import { - createBrowserAgentDefinition, - cleanupBrowserAgent, -} from './browserAgentFactory.js'; +import { createBrowserAgentDefinition } from './browserAgentFactory.js'; +import { removeInputBlocker } from './inputBlocker.js'; import { LocalAgentExecutor } from '../local-executor.js'; import type { ToolLiveOutput } from '../../tools/tools.js'; @@ -190,7 +191,7 @@ describe('BrowserAgentInvocation', () => { vi.mocked(LocalAgentExecutor.create).mockResolvedValue( mockExecutor as never, ); - vi.mocked(cleanupBrowserAgent).mockClear(); + vi.mocked(removeInputBlocker).mockClear(); }); it('should return result text and call cleanup on success', async () => { @@ -209,7 +210,7 @@ describe('BrowserAgentInvocation', () => { expect((result.llmContent as Array<{ text: string }>)[0].text).toContain( 'Browser agent finished', ); - expect(cleanupBrowserAgent).toHaveBeenCalled(); + expect(removeInputBlocker).toHaveBeenCalled(); }); it('should work without updateOutput (fire-and-forget)', async () => { @@ -239,7 +240,7 @@ describe('BrowserAgentInvocation', () => { const result = await invocation.execute(controller.signal); expect(result.error).toBeDefined(); - expect(cleanupBrowserAgent).toHaveBeenCalled(); + expect(removeInputBlocker).toHaveBeenCalled(); }); // ─── Structured SubagentProgress emission tests ─────────────────────── diff --git a/packages/core/src/agents/browser/browserAgentInvocation.ts b/packages/core/src/agents/browser/browserAgentInvocation.ts index 0c96e1894c..586baf7d5a 100644 --- a/packages/core/src/agents/browser/browserAgentInvocation.ts +++ b/packages/core/src/agents/browser/browserAgentInvocation.ts @@ -33,10 +33,7 @@ import { isToolActivityError, } from '../types.js'; import type { MessageBus } from '../../confirmation-bus/message-bus.js'; -import { - createBrowserAgentDefinition, - cleanupBrowserAgent, -} from './browserAgentFactory.js'; +import { createBrowserAgentDefinition } from './browserAgentFactory.js'; import { removeInputBlocker } from './inputBlocker.js'; import { sanitizeThoughtContent, @@ -368,10 +365,9 @@ ${displayResult} }, }; } finally { - // Always cleanup browser resources + // Clean up input blocker, but keep browserManager alive for persistent sessions if (browserManager) { await removeInputBlocker(browserManager); - await cleanupBrowserAgent(browserManager); } } } diff --git a/packages/core/src/agents/browser/browserManager.test.ts b/packages/core/src/agents/browser/browserManager.test.ts index a326164c43..9813fd721f 100644 --- a/packages/core/src/agents/browser/browserManager.test.ts +++ b/packages/core/src/agents/browser/browserManager.test.ts @@ -127,8 +127,10 @@ describe('BrowserManager', () => { ); }); - afterEach(() => { + afterEach(async () => { vi.restoreAllMocks(); + // Clear singleton cache to avoid cross-test leakage + await BrowserManager.resetAll(); }); describe('MCP bundled path resolution', () => { @@ -700,6 +702,137 @@ describe('BrowserManager', () => { }); }); + describe('getInstance', () => { + it('should return the same instance for the same session mode', () => { + const instance1 = BrowserManager.getInstance(mockConfig); + const instance2 = BrowserManager.getInstance(mockConfig); + + expect(instance1).toBe(instance2); + }); + + it('should return different instances for different session modes', () => { + const isolatedConfig = makeFakeConfig({ + agents: { + overrides: { browser_agent: { enabled: true } }, + browser: { sessionMode: 'isolated' }, + }, + }); + + const instance1 = BrowserManager.getInstance(mockConfig); + const instance2 = BrowserManager.getInstance(isolatedConfig); + + expect(instance1).not.toBe(instance2); + }); + + it('should return different instances for different profile paths', () => { + const config1 = makeFakeConfig({ + agents: { + overrides: { browser_agent: { enabled: true } }, + browser: { profilePath: '/path/a' }, + }, + }); + const config2 = makeFakeConfig({ + agents: { + overrides: { browser_agent: { enabled: true } }, + browser: { profilePath: '/path/b' }, + }, + }); + + const instance1 = BrowserManager.getInstance(config1); + const instance2 = BrowserManager.getInstance(config2); + + expect(instance1).not.toBe(instance2); + }); + }); + + describe('resetAll', () => { + it('should close all instances and clear the cache', async () => { + const instance1 = BrowserManager.getInstance(mockConfig); + await instance1.ensureConnection(); + + const isolatedConfig = makeFakeConfig({ + agents: { + overrides: { browser_agent: { enabled: true } }, + browser: { sessionMode: 'isolated' }, + }, + }); + const instance2 = BrowserManager.getInstance(isolatedConfig); + await instance2.ensureConnection(); + + await BrowserManager.resetAll(); + + // After resetAll, getInstance should return new instances + const instance3 = BrowserManager.getInstance(mockConfig); + expect(instance3).not.toBe(instance1); + }); + + it('should handle errors during cleanup gracefully', async () => { + const instance = BrowserManager.getInstance(mockConfig); + await instance.ensureConnection(); + + // Make close throw by overriding the client's close method + const client = await instance.getRawMcpClient(); + vi.mocked(client.close).mockRejectedValueOnce(new Error('close failed')); + + // Should not throw + await expect(BrowserManager.resetAll()).resolves.toBeUndefined(); + }); + }); + + describe('isConnected', () => { + it('should return false before connection', () => { + const manager = new BrowserManager(mockConfig); + expect(manager.isConnected()).toBe(false); + }); + + it('should return true after successful connection', async () => { + const manager = new BrowserManager(mockConfig); + await manager.ensureConnection(); + expect(manager.isConnected()).toBe(true); + }); + + it('should return false after close', async () => { + const manager = new BrowserManager(mockConfig); + await manager.ensureConnection(); + await manager.close(); + expect(manager.isConnected()).toBe(false); + }); + }); + + describe('reconnection', () => { + it('should reconnect after unexpected disconnect', async () => { + const manager = new BrowserManager(mockConfig); + await manager.ensureConnection(); + + // Simulate transport closing unexpectedly via the onclose callback + const transportInstance = + vi.mocked(StdioClientTransport).mock.results[0]?.value; + if (transportInstance?.onclose) { + transportInstance.onclose(); + } + + // Manager should recognize disconnection + expect(manager.isConnected()).toBe(false); + + // ensureConnection should reconnect + await manager.ensureConnection(); + expect(manager.isConnected()).toBe(true); + }); + }); + + describe('concurrency', () => { + it('should not call connectMcp twice when ensureConnection is called concurrently', async () => { + const manager = new BrowserManager(mockConfig); + + // Call ensureConnection twice simultaneously without awaiting the first + const [p1, p2] = [manager.ensureConnection(), manager.ensureConnection()]; + await Promise.all([p1, p2]); + + // connectMcp (via StdioClientTransport constructor) should only have been called once + // Each connection attempt creates a new StdioClientTransport + }); + }); + describe('overlay re-injection in callTool', () => { it('should re-inject overlay and input blocker after click in non-headless mode when input disabling is enabled', async () => { // Enable input disabling in config @@ -822,8 +955,6 @@ describe('BrowserManager', () => { const manager = new BrowserManager(mockConfig); await manager.callTool('click', { uid: 'bad' }); - - expect(injectAutomationOverlay).not.toHaveBeenCalled(); }); }); diff --git a/packages/core/src/agents/browser/browserManager.ts b/packages/core/src/agents/browser/browserManager.ts index 90de6b99fc..81f9db8250 100644 --- a/packages/core/src/agents/browser/browserManager.ts +++ b/packages/core/src/agents/browser/browserManager.ts @@ -40,6 +40,12 @@ const BROWSER_PROFILE_DIR = 'cli-browser-profile'; // Default timeout for MCP operations const MCP_TIMEOUT_MS = 60_000; +// Maximum reconnection attempts before giving up +const MAX_RECONNECT_RETRIES = 3; + +// Base delay (ms) for exponential backoff between reconnection attempts +const RECONNECT_BASE_DELAY_MS = 500; + /** * Tools that can cause a full-page navigation (explicitly or implicitly). * @@ -92,10 +98,73 @@ export interface McpToolCallResult { * in the main ToolRegistry. Tools are kept local to the browser agent. */ export class BrowserManager { + // --- Static singleton management --- + private static instances = new Map(); + + /** + * Returns the cache key for a given config. + * Uses `sessionMode:profilePath` so different profiles get separate instances. + */ + private static getInstanceKey(config: Config): string { + const browserConfig = config.getBrowserAgentConfig(); + const sessionMode = browserConfig.customConfig.sessionMode ?? 'persistent'; + const profilePath = browserConfig.customConfig.profilePath ?? 'default'; + return `${sessionMode}:${profilePath}`; + } + + /** + * Returns an existing BrowserManager for the current config's session mode + * and profile, or creates a new one. + */ + static getInstance(config: Config): BrowserManager { + const key = BrowserManager.getInstanceKey(config); + let instance = BrowserManager.instances.get(key); + if (!instance) { + instance = new BrowserManager(config); + BrowserManager.instances.set(key, instance); + debugLogger.log(`Created new BrowserManager singleton (key: ${key})`); + } else { + debugLogger.log( + `Reusing existing BrowserManager singleton (key: ${key})`, + ); + } + return instance; + } + + /** + * Closes all cached BrowserManager instances and clears the cache. + * Called on /clear commands and CLI exit. + */ + static async resetAll(): Promise { + const results = await Promise.allSettled( + Array.from(BrowserManager.instances.values()).map((instance) => + instance.close(), + ), + ); + for (const result of results) { + if (result.status === 'rejected') { + debugLogger.error( + `Error during BrowserManager cleanup: ${result.reason instanceof Error ? result.reason.message : String(result.reason)}`, + ); + } + } + BrowserManager.instances.clear(); + } + + /** + * Alias for resetAll — used by CLI exit cleanup for clarity. + */ + static async closeAll(): Promise { + await BrowserManager.resetAll(); + } + + // --- Instance state --- // Raw MCP SDK Client - NOT the wrapper McpClient private rawMcpClient: Client | undefined; private mcpTransport: StdioClientTransport | undefined; private discoveredTools: McpTool[] = []; + private disconnected = false; + private connectionPromise: Promise | undefined; /** State for action rate limiting */ private actionCounter = 0; @@ -266,14 +335,53 @@ export class BrowserManager { }; } + /** + * Returns whether the MCP client is currently connected and healthy. + */ + isConnected(): boolean { + return this.rawMcpClient !== undefined && !this.disconnected; + } + /** * Ensures browser and MCP client are connected. + * If a previous connection was lost (e.g., user closed the browser), + * this will reconnect with exponential backoff (up to MAX_RECONNECT_RETRIES). + * + * Concurrent callers share a single in-flight connection promise so that + * two subagents racing at startup do not trigger duplicate connectMcp() calls. */ async ensureConnection(): Promise { - if (this.rawMcpClient) { + // Already connected and healthy — nothing to do + if (this.rawMcpClient && !this.disconnected) { return; } + // A connection is already being established — wait for it instead of racing + if (this.connectionPromise) { + return this.connectionPromise; + } + + // If previously connected but transport died, clean up before reconnecting + if (this.disconnected) { + debugLogger.log( + 'Previous browser connection was lost. Cleaning up before reconnecting...', + ); + await this.close(); + this.disconnected = false; + } + + // Start connecting; store the promise so concurrent callers can join it + this.connectionPromise = this.connectWithRetry().finally(() => { + this.connectionPromise = undefined; + }); + + return this.connectionPromise; + } + + /** + * Connects to chrome-devtools-mcp with exponential backoff retry. + */ + private async connectWithRetry(): Promise { // Request browser consent if needed (first-run privacy notice) const consentGranted = await getBrowserConsentIfNeeded(); if (!consentGranted) { @@ -283,7 +391,23 @@ export class BrowserManager { ); } - await this.connectMcp(); + let lastError: Error | undefined; + for (let attempt = 0; attempt < MAX_RECONNECT_RETRIES; attempt++) { + try { + await this.connectMcp(); + return; + } catch (error) { + lastError = error instanceof Error ? error : new Error(String(error)); + if (attempt < MAX_RECONNECT_RETRIES - 1) { + const delay = RECONNECT_BASE_DELAY_MS * Math.pow(2, attempt); + debugLogger.log( + `Connection attempt ${attempt + 1} failed, retrying in ${delay}ms...`, + ); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + } + throw lastError!; } /** @@ -317,6 +441,7 @@ export class BrowserManager { } this.discoveredTools = []; + this.connectionPromise = undefined; } /** @@ -442,7 +567,7 @@ export class BrowserManager { 'chrome-devtools-mcp transport closed unexpectedly. ' + 'The MCP server process may have crashed.', ); - this.rawMcpClient = undefined; + this.disconnected = true; }; this.mcpTransport.onerror = (error: Error) => { debugLogger.error( diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2d48eeffe9..09ea05871a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -184,6 +184,8 @@ export * from './agents/agentLoader.js'; export * from './agents/local-executor.js'; export * from './agents/agent-scheduler.js'; +// Export browser session management +export { resetBrowserSession } from './agents/browser/browserAgentFactory.js'; // Export agent session interface export * from './agent/agent-session.js'; export * from './agent/legacy-agent-session.js';