diff --git a/src/main/browser/agent-browser-bridge-command-transport.test.ts b/src/main/browser/agent-browser-bridge-command-transport.test.ts index e8c5b786a9b..733cd966ce7 100644 --- a/src/main/browser/agent-browser-bridge-command-transport.test.ts +++ b/src/main/browser/agent-browser-bridge-command-transport.test.ts @@ -51,6 +51,7 @@ vi.mock('./cdp-bridge', () => ({ })) import { AgentBrowserBridge } from './agent-browser-bridge' +import { AGENT_BROWSER_IDLE_TIMEOUT_MS } from './agent-browser-process-environment' import { createSucceedWith, mockBrowserManager, @@ -338,7 +339,10 @@ describe('AgentBrowserBridge', () => { expect(args).toContain('wait') expect(args).toContain('#ready') expect(options.timeout).toBe(2200) - expect(options.env).toBe(process.env) + // Why not toBe(process.env): the bridge hands the daemon an idle-lifetime bound (#16367). + const env = options.env as NodeJS.ProcessEnv + expect(env.PATH).toBe(process.env.PATH) + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBe(String(AGENT_BROWSER_IDLE_TIMEOUT_MS)) }) it('returns browser_timeout for timed conditional waits without recycling the session', async () => { diff --git a/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts b/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts index 4ef3d24fa25..5eab202541c 100644 --- a/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts +++ b/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts @@ -64,6 +64,12 @@ overrideBridgeWebContentsLookup(AgentBrowserBridge.prototype, webContentsFromIdM const succeedWith = createSucceedWith(execFileMock, stdinWrites) +function closeCallCount(): number { + return execFileMock.mock.calls.filter((call: unknown[]) => + (call[1] as string[]).includes('close') + ).length +} + describe('AgentBrowserBridge', () => { let bridge: AgentBrowserBridge @@ -453,6 +459,52 @@ describe('AgentBrowserBridge', () => { ).toBe(0) }) + // Why: the daemon's own idle timer retires it between commands; a replacement still serves the + // page but has none of the session's network routes, so leaving them dropped is a silent wrong + // answer for the next request the caller expected to be stubbed (#16367). + it('replays intercept routes after the daemon idles out', async () => { + succeedWith({ ok: true }) + await bridge.interceptEnable(['https://api.example/**']) + + const sessions = (bridge as unknown as { sessions: Map }) + .sessions + const session = sessions.get('orca-tab-tab-1')! + session.lastCommandAt = Date.now() - 11 * 60 * 1000 + + const commandCalls: string[][] = [] + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { + commandCalls.push(args) + cb(null, JSON.stringify({ success: true, data: { snapshot: 'tree' } }), '') + } + ) + await bridge.snapshot() + + const routeCalls = commandCalls.filter( + (args) => args.includes('network') && args.includes('route') + ) + expect(routeCalls).toHaveLength(1) + expect(routeCalls[0]).toContain('https://api.example/**') + }) + + it('leaves a session alone while the daemon is still within its idle bound', async () => { + succeedWith({ ok: true }) + await bridge.interceptEnable(['https://api.example/**']) + + const commandCalls: string[][] = [] + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { + commandCalls.push(args) + cb(null, JSON.stringify({ success: true, data: { snapshot: 'tree' } }), '') + } + ) + await bridge.snapshot() + + expect( + commandCalls.filter((args) => args.includes('network') && args.includes('route')) + ).toHaveLength(0) + }) + // ── destroyAllSessions ── it('makes runtime-wide session destruction terminal', async () => { @@ -591,4 +643,47 @@ describe('AgentBrowserBridge', () => { expect(CdpWsProxyMock.instances).toHaveLength(1) expect(execFileMock).toHaveBeenCalledTimes(1) }) + + // Why: quit awaits destroyAllSessions inside a 20s barrier, so an unbounded close can hold the + // window up for the whole deadline when the daemon is wedged (#16367). + it('bounds every teardown close well inside the quit barrier', async () => { + succeedWith({ snapshot: 'tree' }) + await bridge.snapshot() + + succeedWith(null) + await bridge.destroyAllSessions() + + const closeCall = execFileMock.mock.calls.findLast((c: unknown[]) => + (c[1] as string[]).includes('close') + ) + expect((closeCall![2] as { timeout: number }).timeout).toBeLessThanOrEqual(5_000) + }) + + // Why: the daemon is already spawned by the time the name reaches pendingSessionCreation, so a + // quit that only walks `sessions` leaves exactly the orphan the barrier was added to prevent. + it('closes a session still being created when everything is torn down', async () => { + let releaseProxyStart: (() => void) | undefined + CdpWsProxyMock.mockImplementationOnce(function (this: Record) { + this.start = vi.fn( + () => + new Promise((resolve) => { + releaseProxyStart = () => resolve('ws://127.0.0.1:9222') + }) + ) + this.stop = vi.fn(async () => {}) + this.getPort = vi.fn(() => 9222) + }) + + succeedWith({ snapshot: 'tree' }) + const inFlight = bridge.snapshot() + await vi.waitFor(() => expect(releaseProxyStart).toBeDefined()) + + // Why the baseline: session creation already spawned a stale-session `close` of its own. + const closesBeforeTeardown = closeCallCount() + const teardown = bridge.destroyAllSessions() + releaseProxyStart!() + await Promise.allSettled([inFlight, teardown]) + + expect(closeCallCount()).toBeGreaterThan(closesBeforeTeardown) + }) }) diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 10a64dc16ce..54611bffa8f 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -52,12 +52,17 @@ import { normalizeBrowserNavigationUrl } from '../../shared/browser-url' import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' import { iterateBrowserTextInsertionChunks } from './browser-text-insertion' import { createAgentBrowserProcessEnvironment } from './agent-browser-process-environment' +import { + ORCA_TAB_SESSION_PREFIX, + sweepOrphanedAgentBrowserSessions +} from './agent-browser-orphan-sweep' // Why: must exceed agent-browser's internal timeouts (goto 30s, wait 60s) so the bridge never kills a command before its own timeout fires. const EXEC_TIMEOUT_MS = 90_000 const CONSECUTIVE_TIMEOUT_LIMIT = 3 const WAIT_PROCESS_TIMEOUT_GRACE_MS = 1_000 const STALE_SESSION_CLOSE_TIMEOUT_MS = 3_000 +// Why separate from EXEC_TIMEOUT_MS: a close is a member of the 20s will-quit barrier and must finish well inside it. const AGENT_BROWSER_CLEANUP_TIMEOUT_MS = 5_000 const AGENT_BROWSER_CLEANUP_CONCURRENCY = 4 const EMBEDDED_NAVIGATION_TIMEOUT_MS = 30_000 @@ -72,6 +77,8 @@ type SessionState = { // Why: track active interception patterns so they can be re-enabled after session restart activeInterceptPatterns: string[] activeCapture: boolean + // Why: the daemon retires itself once idle; the gap since the last command is how the bridge notices. + lastCommandAt: number // Why: verify the tab is alive at execution time, not just enqueue time — queue delay can destroy it in between. webContentsId: number activeProcess: ChildProcess | null @@ -348,7 +355,7 @@ function isTabClosedTransportError(message: string): boolean { } function pageUnavailableMessageForSession(sessionName: string): string { - const prefix = 'orca-tab-' + const prefix = ORCA_TAB_SESSION_PREFIX const browserPageId = sessionName.startsWith(prefix) ? sessionName.slice(prefix.length) : null return browserPageId ? `Browser page ${browserPageId} is no longer available` @@ -587,6 +594,9 @@ export class AgentBrowserBridge { private screenshotTurn: Promise = Promise.resolve() private readonly agentBrowserBin: string private readonly agentBrowserEnv: NodeJS.ProcessEnv + private readonly ownsAgentBrowserSocketDirectory: boolean + // Why: null when nothing bounds the daemon, so the bridge never guesses that one was replaced. + private readonly agentBrowserIdleTimeoutMs: number | null // Why: stash intercept patterns from a swap-destroyed session, keyed by name, so the next session restores them. private readonly pendingInterceptRestore = new Map() // Why: promise-lock so two concurrent ensureSession calls don't both create the session entry. @@ -601,11 +611,15 @@ export class AgentBrowserBridge { private readonly options: AgentBrowserBridgeOptions = {} ) { this.agentBrowserBin = resolveAgentBrowserBinary() - this.agentBrowserEnv = createAgentBrowserProcessEnvironment({ + const processEnvironment = createAgentBrowserProcessEnvironment({ inheritedEnv: process.env, platform: process.platform, userDataPath: app.getPath('userData') }) + this.agentBrowserEnv = processEnvironment.env + this.ownsAgentBrowserSocketDirectory = processEnvironment.ownsSocketDirectory + const idleTimeoutMs = Number(this.agentBrowserEnv.AGENT_BROWSER_IDLE_TIMEOUT_MS) + this.agentBrowserIdleTimeoutMs = idleTimeoutMs > 0 ? idleTimeoutMs : null } // ── Tab tracking ── @@ -691,9 +705,15 @@ export class AgentBrowserBridge { this.options.onTabsChanged?.(owningWorktreeId) } - /** Retire a helper by its stable page identity when WebContents mapping is gone. */ + /** + * Retire a page's daemon by page id. + * + * The headless offscreen backend owns pages by id and unregisters the guest + * itself, so `onTabClosed`'s webContentsId lookup can never resolve one — it + * has to say which page closed (#16367). + */ async onPageClosed(browserPageId: string): Promise { - const sessionName = `orca-tab-${browserPageId}` + const sessionName = `${ORCA_TAB_SESSION_PREFIX}${browserPageId}` await this.destroySession(sessionName) this.pendingInterceptRestore.delete(sessionName) } @@ -704,7 +724,7 @@ export class AgentBrowserBridge { previousWebContentsId?: number ): Promise { // Why: an Electron process swap keeps browserPageId but gives a new webContentsId — destroy the session so the next command recreates it. - const sessionName = `orca-tab-${browserPageId}` + const sessionName = `${ORCA_TAB_SESSION_PREFIX}${browserPageId}` const session = this.sessions.get(sessionName) const oldWebContentsId = previousWebContentsId ?? session?.webContentsId const owningWorktreeId = this.browserManager.getWorktreeIdForTab(browserPageId) @@ -898,7 +918,9 @@ export class AgentBrowserBridge { navigationTimeout = null } if (!this.getWebContents(target.webContentsId)) { - throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + throw this.createPageUnavailableError( + `${ORCA_TAB_SESSION_PREFIX}${target.browserPageId}` + ) } // Why: ERR_ABORTED also covers a page vetoing unload; that navigation did not succeed. if ( @@ -1619,7 +1641,9 @@ export class AgentBrowserBridge { throw error } if (!this.getWebContents(target.webContentsId)) { - throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + throw this.createPageUnavailableError( + `${ORCA_TAB_SESSION_PREFIX}${target.browserPageId}` + ) } throw new BrowserError( 'browser_error', @@ -2057,8 +2081,22 @@ export class AgentBrowserBridge { // ── Session lifecycle ── + // Why: a previous run that crashed or was SIGKILL'd left one daemon per open tab with + // nobody holding its name — closeStaleAgentBrowserSession only resets a name being reused. + async sweepOrphanedSessions(): Promise { + return sweepOrphanedAgentBrowserSessions({ + binaryPath: this.agentBrowserBin, + env: this.agentBrowserEnv, + ownsSocketDirectory: this.ownsAgentBrowserSocketDirectory, + isSessionLive: (sessionName) => + this.sessions.has(sessionName) || this.pendingSessionCreation.has(sessionName) + }) + } + async destroyAllSessions(options?: AgentBrowserCleanupOptions): Promise { this.shutdownStarted = true + // Why the union: a session still being created has already spawned its daemon but is not in + // `sessions` yet, so closing only `sessions` lets that daemon outlive the quit (#16367). const sessionNames = new Set([ ...this.sessions.keys(), ...this.pendingSessionCreation.keys(), @@ -2094,7 +2132,7 @@ export class AgentBrowserBridge { ): Promise { this.assertCommandAdmission() const target = this.resolveCommandTarget(worktreeId, browserPageId, options.requireScopedTarget) - const sessionName = `orca-tab-${target.browserPageId}` + const sessionName = `${ORCA_TAB_SESSION_PREFIX}${target.browserPageId}` if (options.ensureSession !== false) { await this.ensureSession(sessionName, target.browserPageId, target.webContentsId) @@ -2360,6 +2398,7 @@ export class AgentBrowserBridge { consecutiveTimeouts: 0, activeInterceptPatterns: [], activeCapture: false, + lastCommandAt: Date.now(), webContentsId, activeProcess: null }) @@ -2471,6 +2510,7 @@ export class AgentBrowserBridge { const destroy = (async (): Promise => { try { // Why: each tab has its own named session — close without --session leaves this tab's daemon running. + // Why bounded: this runs inside the 20s will-quit barrier, so it cannot inherit the 90s exec timeout. await this.runAgentBrowserRaw( sessionName, ['--session', sessionName, 'close'], @@ -2506,6 +2546,26 @@ export class AgentBrowserBridge { } } + /** + * Notice that the daemon retired itself between two commands. + * + * A replacement daemon still serves the page (every call reasserts `--cdp`) + * but carries none of the session's network routes, so without this the + * interception the caller configured is silently gone (#16367). + */ + private reinitializeIfDaemonIdledOut(sessionName: string, session: SessionState): void { + if ( + this.agentBrowserIdleTimeoutMs === null || + Date.now() - session.lastCommandAt < this.agentBrowserIdleTimeoutMs + ) { + return + } + session.initialized = false + if (session.activeInterceptPatterns.length > 0) { + this.pendingInterceptRestore.set(sessionName, [...session.activeInterceptPatterns]) + } + } + private assertCommandAdmission(): void { if (this.shutdownStarted) { throw new BrowserError('browser_owner_unavailable', 'Browser runtime is shutting down') @@ -2529,6 +2589,9 @@ export class AgentBrowserBridge { throw this.createPageUnavailableError(sessionName) } + this.reinitializeIfDaemonIdledOut(sessionName, session) + session.lastCommandAt = Date.now() + const args = ['--session', sessionName] const managesInterceptRoutes = commandArgs[0] === 'network' && (commandArgs[1] === 'route' || commandArgs[1] === 'unroute') @@ -2810,7 +2873,7 @@ export class AgentBrowserBridge { private requireTargetWebContents(target: ResolvedBrowserCommandTarget): WebContents { const wc = this.getWebContents(target.webContentsId) if (!wc || wc.isDestroyed()) { - throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) + throw this.createPageUnavailableError(`${ORCA_TAB_SESSION_PREFIX}${target.browserPageId}`) } return wc } diff --git a/src/main/browser/agent-browser-orphan-sweep.test.ts b/src/main/browser/agent-browser-orphan-sweep.test.ts new file mode 100644 index 00000000000..ce565e2fa6e --- /dev/null +++ b/src/main/browser/agent-browser-orphan-sweep.test.ts @@ -0,0 +1,190 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const runProcessMock = vi.fn() +vi.mock('../../shared/child-process/run-process', () => ({ + runProcess: (spec: unknown) => runProcessMock(spec) +})) + +import { sweepOrphanedAgentBrowserSessions } from './agent-browser-orphan-sweep' + +type Spec = { args?: readonly string[] } + +const BIN = '/opt/orca/agent-browser' +const SCOPED = { + env: { AGENT_BROWSER_SOCKET_DIR: '/tmp/orca-ab-0123456789abcdef' }, + ownsSocketDirectory: true +} + +function respond(sessions: string[]): void { + runProcessMock.mockImplementation((spec: Spec) => { + if (spec.args?.[0] === 'session') { + return Promise.resolve({ + code: 0, + signal: null, + stdout: JSON.stringify({ success: true, data: { sessions } }), + stderr: '', + timedOut: false + }) + } + return Promise.resolve({ code: 0, signal: null, stdout: '', stderr: '', timedOut: false }) + }) +} + +function closedArgs(): string[][] { + return runProcessMock.mock.calls + .map((call) => [...((call[0] as Spec).args ?? [])]) + .filter((args) => args.includes('close')) +} + +describe('agent-browser orphan sweep', () => { + beforeEach(() => { + runProcessMock.mockReset() + }) + + it('closes tab daemons left by a previous run', async () => { + respond(['orca-tab-aaa', 'orca-tab-bbb']) + + const closed = await sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + + expect(closed).toEqual(['orca-tab-aaa', 'orca-tab-bbb']) + expect(closedArgs()).toEqual([ + ['--session', 'orca-tab-aaa', 'close'], + ['--session', 'orca-tab-bbb', 'close'] + ]) + }) + + it('never closes a daemon outside Orca tab naming', async () => { + respond(['default', 'agent1', 'orca-orcad-deadbeef', 'orca-tab-aaa']) + + await sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + + expect(closedArgs()).toEqual([['--session', 'orca-tab-aaa', 'close']]) + }) + + it('leaves sessions this run already owns alone', async () => { + respond(['orca-tab-live', 'orca-tab-orphan']) + + await sweepOrphanedAgentBrowserSessions({ + binaryPath: BIN, + ...SCOPED, + isSessionLive: (name) => name === 'orca-tab-live' + }) + + expect(closedArgs()).toEqual([['--session', 'orca-tab-orphan', 'close']]) + }) + + // Why: without a socket dir Orca derived itself, `session list` can reach daemons another Orca + // profile owns (Windows named pipes, or an inherited AGENT_BROWSER_SOCKET_DIR). Idle timeout bounds those. + it.each([ + ['no socket directory at all', { PATH: 'C:\\Windows' }], + ['a socket directory Orca inherited', { AGENT_BROWSER_SOCKET_DIR: '/tmp/shared-ab' }] + ])('does not enumerate with %s', async (_label, env) => { + respond(['orca-tab-aaa']) + + const closed = await sweepOrphanedAgentBrowserSessions({ + binaryPath: BIN, + env, + ownsSocketDirectory: false + }) + + expect(closed).toEqual([]) + expect(runProcessMock).not.toHaveBeenCalled() + }) + + it('closes nothing when the listing is unusable', async () => { + runProcessMock.mockResolvedValue({ + code: 1, + signal: null, + stdout: 'not json', + stderr: 'boom', + timedOut: false + }) + + await expect( + sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + ).resolves.toEqual([]) + expect(closedArgs()).toEqual([]) + }) + + it('survives a listing that never returns', async () => { + runProcessMock.mockRejectedValue(new Error('ENOENT')) + + await expect( + sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + ).resolves.toEqual([]) + }) + + it('keeps sweeping after one close fails', async () => { + runProcessMock.mockImplementation((spec: Spec) => { + if (spec.args?.[0] === 'session') { + return Promise.resolve({ + code: 0, + signal: null, + stdout: JSON.stringify({ data: { sessions: ['orca-tab-aaa', 'orca-tab-bbb'] } }), + stderr: '', + timedOut: false + }) + } + if (spec.args?.[1] === 'orca-tab-aaa') { + return Promise.reject(new Error('spawn failed')) + } + return Promise.resolve({ code: 0, signal: null, stdout: '', stderr: '', timedOut: false }) + }) + + const closed = await sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + + expect(closed).toEqual(['orca-tab-bbb']) + }) + + it('bounds every child it starts', async () => { + respond(['orca-tab-aaa']) + + await sweepOrphanedAgentBrowserSessions({ binaryPath: BIN, ...SCOPED }) + + for (const call of runProcessMock.mock.calls) { + expect((call[0] as { timeoutMs?: number | null }).timeoutMs).toBeGreaterThan(0) + } + }) +}) + +describe('sweep kill switch', () => { + const previous = process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP + + afterEach(() => { + if (previous === undefined) { + delete process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP + } else { + process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP = previous + } + }) + + // Why: the idle bound is an env passthrough an operator can raise and the quit close is + // self-bounded, so the sweep is the only new behaviour whose failure would need a revert. + it('enumerates nothing when disabled, even when Orca owns the socket directory', async () => { + process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP = '1' + runProcessMock.mockClear() + + const closed = await sweepOrphanedAgentBrowserSessions({ + binaryPath: BIN, + env: {}, + ownsSocketDirectory: true + }) + + expect(closed).toEqual([]) + expect(runProcessMock).not.toHaveBeenCalled() + }) + + it('still sweeps when the flag holds any other value', async () => { + process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP = '0' + runProcessMock.mockClear() + runProcessMock.mockResolvedValue({ code: 0, stdout: '{"data":{"sessions":[]}}', stderr: '' }) + + await sweepOrphanedAgentBrowserSessions({ + binaryPath: BIN, + env: {}, + ownsSocketDirectory: true + }) + + expect(runProcessMock).toHaveBeenCalled() + }) +}) diff --git a/src/main/browser/agent-browser-orphan-sweep.ts b/src/main/browser/agent-browser-orphan-sweep.ts new file mode 100644 index 00000000000..062eb1235db --- /dev/null +++ b/src/main/browser/agent-browser-orphan-sweep.ts @@ -0,0 +1,91 @@ +import { runProcess } from '../../shared/child-process/run-process' + +/** Session-name namespace Orca gives one daemon per browser tab. */ +export const ORCA_TAB_SESSION_PREFIX = 'orca-tab-' + +const SWEEP_TIMEOUT_MS = 5_000 +const SWEEP_MAX_OUTPUT_BYTES = 256 * 1024 + +type SessionListEnvelope = { + data?: { sessions?: unknown } +} + +function parseSessionNames(stdout: string): string[] { + let envelope: SessionListEnvelope + try { + envelope = JSON.parse(stdout) as SessionListEnvelope + } catch { + return [] + } + const sessions = envelope?.data?.sessions + if (!Array.isArray(sessions)) { + return [] + } + return sessions.filter((name): name is string => typeof name === 'string' && name.length > 0) +} + +/** + * Close agent-browser daemons left behind by a previous Orca run. + * + * A crash (or SIGKILL) leaves one daemon per open tab with nobody holding its + * name; `closeStaleAgentBrowserSession` only resets the single name a new tab + * is about to reuse, so the rest persist. This closes them through + * agent-browser's own CLI rather than by walking pids. + * + * Scoping — this only runs when Orca derived the socket directory itself + * (`ownsSocketDirectory`), because that private per-profile directory is what + * proves the enumeration can only see this Orca profile's daemons. An inherited + * `AGENT_BROWSER_SOCKET_DIR` can be shared with a second Orca profile, and + * Windows gets none at all (named pipes make the directory moot); both cases + * skip the sweep rather than run a `session list` that could close a daemon Orca + * does not own, and stay bounded by `AGENT_BROWSER_IDLE_TIMEOUT_MS` instead. + * + * `ORCA_DISABLE_AGENT_BROWSER_SWEEP=1` turns it off in the field. The other two + * behaviours this PR adds are already recoverable without a build — the idle bound + * is an env passthrough an operator can raise, and the quit close is bounded by its + * own timeout — but a sweep that closes the wrong daemon, or spawns one process per + * stale name on a profile with hundreds, would otherwise need a revert. + */ +export async function sweepOrphanedAgentBrowserSessions(options: { + binaryPath: string + env: NodeJS.ProcessEnv + ownsSocketDirectory: boolean + isSessionLive?: (sessionName: string) => boolean +}): Promise { + if (!options.ownsSocketDirectory || process.env.ORCA_DISABLE_AGENT_BROWSER_SWEEP === '1') { + return [] + } + let listed: string[] + try { + const result = await runProcess({ + program: options.binaryPath, + args: ['session', 'list', '--json'], + env: options.env, + timeoutMs: SWEEP_TIMEOUT_MS, + maxOutputBytes: SWEEP_MAX_OUTPUT_BYTES + }) + listed = result.timedOut ? [] : parseSessionNames(result.stdout) + } catch { + return [] + } + + const closed: string[] = [] + for (const sessionName of listed) { + if (!sessionName.startsWith(ORCA_TAB_SESSION_PREFIX) || options.isSessionLive?.(sessionName)) { + continue + } + try { + await runProcess({ + program: options.binaryPath, + args: ['--session', sessionName, 'close'], + env: options.env, + timeoutMs: SWEEP_TIMEOUT_MS, + maxOutputBytes: SWEEP_MAX_OUTPUT_BYTES + }) + closed.push(sessionName) + } catch { + // A daemon that died mid-sweep needs no closing. + } + } + return closed +} diff --git a/src/main/browser/agent-browser-process-environment.test.ts b/src/main/browser/agent-browser-process-environment.test.ts index 739567e49f1..69606d1241d 100644 --- a/src/main/browser/agent-browser-process-environment.test.ts +++ b/src/main/browser/agent-browser-process-environment.test.ts @@ -1,40 +1,86 @@ import { describe, expect, it, vi } from 'vitest' -import { createAgentBrowserProcessEnvironment } from './agent-browser-process-environment' +import { + AGENT_BROWSER_IDLE_TIMEOUT_MS, + createAgentBrowserProcessEnvironment +} from './agent-browser-process-environment' vi.mock('node:fs', () => ({ mkdirSync: vi.fn(), chmodSync: vi.fn() })) describe('agent-browser process environment', () => { it('bounds Unix socket paths independently of a long profile path', () => { - const env = createAgentBrowserProcessEnvironment({ + const { env, ownsSocketDirectory } = createAgentBrowserProcessEnvironment({ inheritedEnv: { PATH: '/bin' }, platform: 'darwin', userDataPath: `/private/var/folders/${'long-profile-segment/'.repeat(12)}` }) const socketDirectory = env.AGENT_BROWSER_SOCKET_DIR + expect(ownsSocketDirectory).toBe(true) expect(socketDirectory).toMatch(/^\/tmp\/orca-ab-[0-9a-f]{16}$/) expect( `${socketDirectory}/orca-tab-00000000-0000-4000-8000-000000000000.sock`.length ).toBeLessThan(104) }) - it('preserves explicit overrides and leaves Windows unchanged', () => { - const configured = { AGENT_BROWSER_SOCKET_DIR: '/custom/socket-dir' } - expect( - createAgentBrowserProcessEnvironment({ - inheritedEnv: configured, - platform: 'linux', + // Why ownsSocketDirectory is false for both: a directory Orca did not derive can be shared with + // another Orca profile, so `session list` under it is no proof of ownership. + it('preserves explicit overrides and leaves Windows socket routing unchanged', () => { + const configured = createAgentBrowserProcessEnvironment({ + inheritedEnv: { AGENT_BROWSER_SOCKET_DIR: '/custom/socket-dir' }, + platform: 'linux', + userDataPath: '/profile' + }) + expect(configured.env.AGENT_BROWSER_SOCKET_DIR).toBe('/custom/socket-dir') + expect(configured.ownsSocketDirectory).toBe(false) + + const windows = createAgentBrowserProcessEnvironment({ + inheritedEnv: { PATH: 'C:\\Windows' }, + platform: 'win32', + userDataPath: 'C:\\Users\\Orca' + }) + expect(windows.env.AGENT_BROWSER_SOCKET_DIR).toBeUndefined() + expect(windows.env.PATH).toBe('C:\\Windows') + expect(windows.ownsSocketDirectory).toBe(false) + }) + + // Why: the only daemon bound that survives a SIGKILL'd Orca, so it must be set on every platform. + it.each(['darwin', 'linux', 'win32'])( + 'bounds daemon idle lifetime on %s', + (platform) => { + const { env } = createAgentBrowserProcessEnvironment({ + inheritedEnv: { PATH: '/bin' }, + platform, userDataPath: '/profile' }) - ).toBe(configured) + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBe(String(AGENT_BROWSER_IDLE_TIMEOUT_MS)) + } + ) - const windows = { PATH: 'C:\\Windows' } - expect( - createAgentBrowserProcessEnvironment({ - inheritedEnv: windows, - platform: 'win32', - userDataPath: 'C:\\Users\\Orca' - }) - ).toBe(windows) + it('never cuts a command short: idle timeout exceeds the bridge exec timeout', () => { + expect(AGENT_BROWSER_IDLE_TIMEOUT_MS).toBeGreaterThan(90_000) + }) + + it('honors an explicit idle timeout from the environment', () => { + const { env } = createAgentBrowserProcessEnvironment({ + inheritedEnv: { AGENT_BROWSER_IDLE_TIMEOUT_MS: '5000' }, + platform: 'darwin', + userDataPath: '/profile' + }) + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBe('5000') + }) + + it('still bounds the daemon when the socket directory cannot be created', async () => { + const fs = await import('node:fs') + vi.mocked(fs.mkdirSync).mockImplementationOnce(() => { + throw new Error('EACCES') + }) + const { env, ownsSocketDirectory } = createAgentBrowserProcessEnvironment({ + inheritedEnv: { PATH: '/bin' }, + platform: 'linux', + userDataPath: '/profile' + }) + expect(env.AGENT_BROWSER_SOCKET_DIR).toBeUndefined() + expect(ownsSocketDirectory).toBe(false) + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBe(String(AGENT_BROWSER_IDLE_TIMEOUT_MS)) }) }) diff --git a/src/main/browser/agent-browser-process-environment.ts b/src/main/browser/agent-browser-process-environment.ts index 9d25236b8dd..ae21104f8d3 100644 --- a/src/main/browser/agent-browser-process-environment.ts +++ b/src/main/browser/agent-browser-process-environment.ts @@ -4,13 +4,45 @@ import { join } from 'node:path' const AGENT_BROWSER_SOCKET_DIRECTORY_PREFIX = 'orca-ab-' +/** + * Lifetime bound for the agent-browser daemon. + * + * `agent-browser` is a client/daemon CLI: Orca only ever spawns the short-lived + * client, which forks a daemon Orca holds no handle on and that reparents to + * pid 1 immediately. Nothing in Orca can reap it — not teardown, not a pid walk + * (see `windows-pty-job.ts` for why walking your own orphans is guesswork) — + * and a SIGKILL'd Orca never runs teardown at all. The daemon's own idle timer + * is the only bound that survives every way Orca can die (#16367). + * + * 10 minutes: >6x `EXEC_TIMEOUT_MS` (90s) so no command, retry chain, or normal + * gap between two user commands can be cut short by it, while capping an + * abandoned daemon at minutes instead of days. Only per-tab helper daemons get + * this: see `externalChromiumAgentBrowserEnvironment` for why the daemon that + * owns a whole Chromium tree must not be idled out. + */ +export const AGENT_BROWSER_IDLE_TIMEOUT_MS = 10 * 60 * 1000 + +export type AgentBrowserProcessEnvironment = { + env: NodeJS.ProcessEnv + /** + * True only when Orca derived the socket directory itself. An inherited + * `AGENT_BROWSER_SOCKET_DIR` can be shared with another Orca profile, so it is + * no proof that `session list` under it sees only this profile's daemons. + */ + ownsSocketDirectory: boolean +} + export function createAgentBrowserProcessEnvironment(options: { inheritedEnv: NodeJS.ProcessEnv platform: NodeJS.Platform userDataPath: string -}): NodeJS.ProcessEnv { - if (options.platform === 'win32' || options.inheritedEnv.AGENT_BROWSER_SOCKET_DIR?.trim()) { - return options.inheritedEnv +}): AgentBrowserProcessEnvironment { + const env = { ...options.inheritedEnv } + if (!env.AGENT_BROWSER_IDLE_TIMEOUT_MS?.trim()) { + env.AGENT_BROWSER_IDLE_TIMEOUT_MS = String(AGENT_BROWSER_IDLE_TIMEOUT_MS) + } + if (options.platform === 'win32' || env.AGENT_BROWSER_SOCKET_DIR?.trim()) { + return { env, ownsSocketDirectory: false } } const profileKey = createHash('sha256').update(options.userDataPath).digest('hex').slice(0, 16) const socketDirectory = join('/tmp', `${AGENT_BROWSER_SOCKET_DIRECTORY_PREFIX}${profileKey}`) @@ -18,7 +50,8 @@ export function createAgentBrowserProcessEnvironment(options: { mkdirSync(socketDirectory, { recursive: true, mode: 0o700 }) chmodSync(socketDirectory, 0o700) } catch { - return options.inheritedEnv + return { env, ownsSocketDirectory: false } } - return { ...options.inheritedEnv, AGENT_BROWSER_SOCKET_DIR: socketDirectory } + env.AGENT_BROWSER_SOCKET_DIR = socketDirectory + return { env, ownsSocketDirectory: true } } diff --git a/src/main/browser/offscreen-browser-backend-lifecycle.test.ts b/src/main/browser/offscreen-browser-backend-lifecycle.test.ts index 52a555c9f5f..da393fa273d 100644 --- a/src/main/browser/offscreen-browser-backend-lifecycle.test.ts +++ b/src/main/browser/offscreen-browser-backend-lifecycle.test.ts @@ -282,4 +282,19 @@ describe('OffscreenBrowserBackend lifecycle', () => { expect(peakRetirements).toBe(4) }) + + it('closes the page even when daemon retirement throws', async () => { + const browserManager = { registerOffscreenGuest: vi.fn(), unregisterGuest: vi.fn() } + const backend = new OffscreenBrowserBackend(browserManager as never, { + getAgentBrowserBridge: () => ({ + onPageClosed: vi.fn(async () => { + throw new Error('daemon gone') + }) + }) + }) + + await backend.createTab({ browserPageId: 'page-1', url: 'about:blank', worktreeId: 'wt' }) + await expect(backend.closeTab('page-1')).resolves.toBeUndefined() + expect(browserManager.unregisterGuest).toHaveBeenCalledWith('page-1') + }) }) diff --git a/src/main/index.ts b/src/main/index.ts index 249700bcd16..a142fc6cf7d 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -3035,6 +3035,8 @@ void app.whenReady().then(async () => { onTabsChanged: (worktreeId) => runtimeService.notifyMobileSessionTabsChanged(worktreeId) }) runtimeService.setAgentBrowserBridge(agentBrowserBridge) + // Why: daemons a crashed or SIGKILL'd previous run left behind answer to nobody; nothing else reclaims them. + void agentBrowserBridge.sweepOrphanedSessions() const browserClientAutomationDispatcher = new RpcDispatcher({ runtime: runtimeService }) configureBrowserClientPageAutomationRuntime({ browserManager, @@ -3534,7 +3536,10 @@ app.on('will-quit', (e) => { // Why: cancels relay restart/reinstall timers and kills wsl.exe children deterministically, not via stdio-pipe teardown. wslHookRelayManager.disposeAll() const statsFlush = stats?.flushAsync() ?? Promise.resolve() - // Why: retire headless page owners first, then sweep residual helper sessions without duplicate close fanout. + // Why: agent-browser daemon processes would otherwise linger after quit, holding ports and stale session state on disk. + // Why the barrier below: each session's close is its own agent-browser child taking hundreds of ms, + // so an unawaited call reaches app.quit() first and every open tab's daemon survives the quit (#16367). + // Why retire headless page owners first: it closes those helpers without a duplicate close fanout. const browserShutdown = (async (): Promise => { await runtime?.getOffscreenBrowserBackend()?.destroyAll?.() await runtime?.getAgentBrowserBridge()?.destroyAllSessions() diff --git a/src/main/orcad/external-chromium-browser-session.test.ts b/src/main/orcad/external-chromium-browser-session.test.ts new file mode 100644 index 00000000000..730a3d36e2d --- /dev/null +++ b/src/main/orcad/external-chromium-browser-session.test.ts @@ -0,0 +1,130 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const runProcessMock = vi.fn() +vi.mock('../../shared/child-process/run-process', () => ({ + runProcess: (spec: unknown) => runProcessMock(spec) +})) +vi.mock('node:fs/promises', () => ({ + mkdir: vi.fn(async () => undefined), + readFile: vi.fn(async () => Buffer.from('')), + rm: vi.fn(async () => undefined) +})) + +import { + ExternalChromiumBrowserSession, + externalChromiumAgentBrowserEnvironment +} from './external-chromium-browser-session' + +const BASE = { + executablePath: '/opt/orca/chromium', + profilePath: '/state/browser-chromium', + sessionName: 'orca-orcad-0123456789abcdef' +} + +type Spec = { args?: readonly string[]; env?: NodeJS.ProcessEnv } + +function commands(): string[][] { + return runProcessMock.mock.calls.map((call) => [...((call[0] as Spec).args ?? [])]) +} + +describe('orcad external-chromium agent-browser environment', () => { + beforeEach(() => { + runProcessMock.mockReset() + }) + + // Why: this daemon owns the user's remote Chromium, so an idle bound would close a live browser. + it('never bounds the daemon that owns the Chromium tree', () => { + const env = externalChromiumAgentBrowserEnvironment({ inheritedEnv: {}, ...BASE }) + + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBeUndefined() + expect(env.AGENT_BROWSER_EXECUTABLE_PATH).toBe(BASE.executablePath) + expect(env.AGENT_BROWSER_SESSION).toBe(BASE.sessionName) + }) + + it('passes an operator-set idle timeout through untouched', () => { + const env = externalChromiumAgentBrowserEnvironment({ + inheritedEnv: { AGENT_BROWSER_IDLE_TIMEOUT_MS: '1234' }, + ...BASE + }) + + expect(env.AGENT_BROWSER_IDLE_TIMEOUT_MS).toBe('1234') + }) + + it('keeps launch arguments joined for the daemon', () => { + const env = externalChromiumAgentBrowserEnvironment({ + inheritedEnv: {}, + ...BASE, + browserArgs: ['--headless=new', '--no-sandbox'] + }) + + expect(env.AGENT_BROWSER_ARGS).toBe('--headless=new\n--no-sandbox') + }) + + // Why (#16367): orcad is the backend a remote user's Chromium hangs off, and this session + // never passes --cdp, so a daemon an earlier orcad left behind is not bound to the dead + // process. Closing it on every start would take their browser and every tab with it. + it("reuses a surviving session instead of closing the user's browser", async () => { + runProcessMock.mockImplementation((spec: Spec) => { + const data = spec.args?.includes('tab') + ? { tabs: [{ active: true, tabId: 'tab-live', title: 'x', url: 'https://example.test' }] } + : {} + return Promise.resolve({ + code: 0, + signal: null, + stdout: JSON.stringify({ success: true, data }), + stderr: '', + timedOut: false + }) + }) + + const session = new ExternalChromiumBrowserSession( + '/opt/orca/agent-browser', + { executablePath: BASE.executablePath, provider: 'chromium' }, + '/state' + ) + await expect(session.start()).resolves.toBe('tab-live') + + const issued = commands().map((args) => args.filter((arg) => !arg.startsWith('-'))) + expect(issued.some((args) => args.includes('close'))).toBe(false) + expect(issued.some((args) => args.includes('open'))).toBe(false) + }) + + // Why: a name that answers nothing is wedged or half-dead, so reclaiming it is correct — + // that is the killed-orcad case the stable session name exists to recover. + it('reclaims a session that answers nothing, then opens', async () => { + let listed = 0 + runProcessMock.mockImplementation((spec: Spec) => { + if (spec.args?.includes('tab')) { + listed += 1 + // First probe finds nothing; after `open` the page exists. + const tabs = + listed === 1 ? [] : [{ active: true, tabId: 'tab-1', title: 'x', url: 'about:blank' }] + return Promise.resolve({ + code: 0, + signal: null, + stdout: JSON.stringify({ success: true, data: { tabs } }), + stderr: '', + timedOut: false + }) + } + return Promise.resolve({ + code: 0, + signal: null, + stdout: JSON.stringify({ success: true, data: {} }), + stderr: '', + timedOut: false + }) + }) + + const session = new ExternalChromiumBrowserSession( + '/opt/orca/agent-browser', + { executablePath: BASE.executablePath, provider: 'chromium' }, + '/state' + ) + await expect(session.start()).resolves.toBe('tab-1') + + const issued = commands().map((args) => args.filter((arg) => !arg.startsWith('-'))) + expect(issued.some((args) => args.includes('close'))).toBe(true) + expect(issued.some((args) => args.includes('open'))).toBe(true) + }) +}) diff --git a/src/main/orcad/external-chromium-browser-session.ts b/src/main/orcad/external-chromium-browser-session.ts index a8aadac3ccd..de72ae8a061 100644 --- a/src/main/orcad/external-chromium-browser-session.ts +++ b/src/main/orcad/external-chromium-browser-session.ts @@ -51,6 +51,25 @@ function classifyAgentBrowserError(message: string): string { return 'browser_error' } +// Why no AGENT_BROWSER_IDLE_TIMEOUT_MS here: unlike a per-tab helper daemon, this one owns the +// user's remote Chromium tree, so idling it out would close their live browser and every tab in it. +// The stable session name plus the `close` in start() is what reclaims a killed orcad's tree (#16367). +export function externalChromiumAgentBrowserEnvironment(options: { + inheritedEnv: NodeJS.ProcessEnv + executablePath: string + profilePath: string + sessionName: string + browserArgs?: readonly string[] +}): NodeJS.ProcessEnv { + return { + ...options.inheritedEnv, + AGENT_BROWSER_EXECUTABLE_PATH: options.executablePath, + AGENT_BROWSER_PROFILE: options.profilePath, + AGENT_BROWSER_SESSION: options.sessionName, + AGENT_BROWSER_ARGS: options.browserArgs?.join('\n') ?? '' + } +} + export class ExternalChromiumBrowserSession { private readonly profilePath: string private readonly sessionName: string @@ -70,16 +89,34 @@ export class ExternalChromiumBrowserSession { async start(): Promise { await mkdir(this.profilePath, { recursive: true }) + // Why: the session name is stable across runs, so a daemon an earlier orcad left behind is + // still driving the user's Chromium. Unlike the pane bridge this session never passes --cdp, + // so nothing binds it to the old process — a surviving one is reusable as-is, and closing it + // would take the remote user's browser and every tab with it (#16367). + const reusable = await this.readActiveTabId() + if (reusable) { + return reusable + } + // Nothing answered, so anything under this name is wedged or half-dead; reclaim it. + await this.stop() await this.run(['open', 'about:blank']) - const tabs = await this.readTabs() - const active = tabs.find((tab) => tab.active) ?? tabs[0] - if (!active) { + const opened = await this.readActiveTabId() + if (!opened) { throw new BrowserError( BROWSER_UNAVAILABLE_ERROR_CODE, 'The browser launched without an automation target.' ) } - return active.tabId + return opened + } + + private async readActiveTabId(): Promise { + try { + const tabs = await this.readTabs() + return (tabs.find((tab) => tab.active) ?? tabs[0])?.tabId ?? null + } catch { + return null + } } async stop(): Promise { @@ -121,13 +158,13 @@ export class ExternalChromiumBrowserSession { args.push('--args', this.launch.browserArgs.join('\n')) } args.push(...command, '--json') - const env = { - ...process.env, - AGENT_BROWSER_EXECUTABLE_PATH: this.launch.executablePath, - AGENT_BROWSER_PROFILE: this.profilePath, - AGENT_BROWSER_SESSION: this.sessionName, - AGENT_BROWSER_ARGS: this.launch.browserArgs?.join('\n') ?? '' - } + const env = externalChromiumAgentBrowserEnvironment({ + inheritedEnv: process.env, + executablePath: this.launch.executablePath, + profilePath: this.profilePath, + sessionName: this.sessionName, + browserArgs: this.launch.browserArgs + }) const result = await runProcess({ program: this.agentBrowserPath, args, diff --git a/src/main/quit-teardown-agent-browser-daemons.test.ts b/src/main/quit-teardown-agent-browser-daemons.test.ts new file mode 100644 index 00000000000..cc2c7d7a26e --- /dev/null +++ b/src/main/quit-teardown-agent-browser-daemons.test.ts @@ -0,0 +1,33 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * agent-browser forks a daemon per browser tab that Orca holds no handle on, and + * `destroyAllSessions` closes each one by spawning another agent-browser child — + * hundreds of ms apiece. Left off the will-quit barrier, `app.quit()` fired first and + * every open tab's daemon outlived the app (#16367). + */ +const source = readFileSync(join(__dirname, 'index.ts'), 'utf8') + +function teardownBarrierMembers(): string { + const start = source.indexOf('settleTeardownWithinDeadline([') + expect(start).toBeGreaterThanOrEqual(0) + const end = source.indexOf('])', start) + expect(end).toBeGreaterThan(start) + return source.slice(start, end) +} + +describe('quit teardown of agent-browser daemons', () => { + it('joins the will-quit teardown barrier', () => { + expect(teardownBarrierMembers()).toContain("{ name: 'browser', promise: browserShutdown }") + }) + + it('captures the destroyAllSessions promise instead of firing and forgetting', () => { + expect(source).toMatch( + /const browserShutdown = \(async \(\): Promise => \{[\s\S]*?await runtime\?\.getAgentBrowserBridge\(\)\?\.destroyAllSessions\(\)\s+\}\)\(\)/ + ) + // Why: a second, uncaptured call site is the pre-fix shape — it loses the race to app.quit(). + expect(source.match(/getAgentBrowserBridge\(\)\?\.destroyAllSessions\(\)/g)).toHaveLength(1) + }) +})