From 88ecc739e3ee7a261e978f28dd0bb129b832ca1c Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:49:31 -0700 Subject: [PATCH] fix(browser): bound the agent-browser daemon's life instead of hoping teardown runs (#16367) (#16588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): bound the agent-browser daemon lifetime (#16367) `agent-browser` is a client/daemon CLI. Orca only ever spawns the short-lived client; that client forks a daemon Orca holds no handle on, which reparents to pid 1 immediately. Nothing in Orca reclaimed it, so a crashed or SIGKILL'd run left one daemon per browser tab alive forever — two of them at ~25.6 GiB and ~7.2 GiB RSS saturated a 64 GiB cgroup under headless `orca serve`. Three fixes, in order of how much they cover: 1. Set `AGENT_BROWSER_IDLE_TIMEOUT_MS` on both spawn paths (the bundled-binary bridge and the orcad external-Chromium provider). This is the only bound that survives every way Orca can die, including SIGKILL, where no teardown code ever runs. 10 minutes: >6x the bridge's 90s `EXEC_TIMEOUT_MS`, so it can never cut a command, a retry chain, or an ordinary gap between two user commands, while capping an abandoned daemon at minutes instead of days. Verified against agent-browser 0.27.0: an idle daemon exits and takes its Chromium tree and socket sidecar files with it, and a daemon attached over `--cdp` (the bridge's case) leaves the attached browser running, so an Orca tab is never closed by its daemon idling out. 2. Await `destroyAllSessions()` in the will-quit teardown barrier. It was fire-and-forget and the only browser member missing from `settleTeardownWithinDeadline`; each session's close is its own agent-browser child taking hundreds of ms, so `app.quit()` won. 3. Sweep daemons a previous run left behind, using agent-browser's own `session list` / `close` rather than a pid walk (see `windows-pty-job.ts` for why walking your own orphans is guesswork). `closeStaleAgentBrowserSession` only ever reset the one name a new tab was about to reuse. The sweep runs only when `AGENT_BROWSER_SOCKET_DIR` is set, because that private per-profile directory is what proves the enumeration can only see this Orca profile's daemons; it is never set on Windows, so Windows gets no enumeration rather than a machine-wide sweep that could close a daemon Orca does not own. Windows stays bounded by the idle timeout, which needs no ownership proof. orcad's session name is stable across runs, so it closes that one name at start instead — a killed orcad's daemon would otherwise be reused while still holding the previous run's Chromium on a dead serve port. Where the 25 GiB went is inference from code, not a measurement: `captureStart` sets `activeCapture` and only an explicit `captureStop` ends it, so a HAR capture in a daemon living for days is unbounded. Not claimed as proven; the idle bound caps it either way. Not re-landed: the queue bounds from #10179 (reverted by #10255) bound Orca's own main-process heap, not the daemon's RSS, so they do not address this report. Also true but left alone: the 3-strike breaker's `destroySession` is an unawaited call whose `close` is `catch {}`-swallowed, and it only fires while a command is in flight — an idle-but-bloated daemon is never noticed. The idle timeout now bounds that case. `getOffscreenBrowserBackend()?.destroyAll?.()` is declared `void` and fully synchronous, so unlike `destroyAllSessions` it has no promise to lose and needs no barrier entry. * fix(browser): scope the daemon idle bound and close every daemon Orca owns Review follow-ups on the agent-browser orphan fix. - Never idle-bound the orcad external-Chromium daemon: it owns the user's remote browser, so the 10-minute bound closed a live session and every tab in it. Per-tab helper daemons keep the bound; the stable session name plus the `close` in start() is what reclaims a killed orcad's Chromium tree. - Retire a page's daemon from the headless offscreen backend, which is the only place `orca serve` closes a page and never reached the bridge. Credit to @Jinwoo-H (#16564) for identifying this owner-boundary gap. - Bound the teardown close at 5s so the will-quit barrier member cannot inherit the 90s exec timeout, and close sessions still being created. - Gate the startup sweep on a socket directory Orca derived itself; an inherited AGENT_BROWSER_SOCKET_DIR is no proof of per-profile ownership. - Replay a session's network routes when the daemon idled out between two commands, instead of silently serving unstubbed requests. * fix(browser): give the orphan sweep a kill switch Of the three behaviours this PR adds, two are already recoverable in the field without a build: the idle bound is an env passthrough an operator can raise, and the quit close is bounded by its own timeout inside the teardown deadline. The startup sweep was the exception — it fires unconditionally, and if it closes a daemon it should not, or spawns one process per stale name on a profile holding hundreds, the only remedy was a revert. ORCA_DISABLE_AGENT_BROWSER_SWEEP=1 turns it off, matching the existing ORCA_DISABLE_CODEX_TRUST_RPC / ORCA_DISABLE_HTTP2 convention. Note for anyone reaching for it on macOS: a Finder-launched Orca does not see shell env, so it needs launchctl setenv or a terminal launch. * fix(orcad): reuse a surviving browser session instead of closing the user's start() closed the daemon before every open, killing the Chromium tree with it. That runs on every provider start, not just after a crash — so an `orca serve` restart took the remote user's browser and every tab in it. The justification was borrowed from the pane bridge, which passes --cdp and so really does hold a port that dies with its Orca. This session passes only --session and --profile: nothing binds it to the old process, and the daemon owns its Chromium independently. A survivor is reusable as-is. start() now probes for an active tab first and returns it untouched. Only a name that answers nothing gets closed and reopened — which is still the killed-orcad case the stable session name exists to recover. This matters more as orcad becomes the backend the remote host runs on: the browser it manages belongs to a user, not to the process that happens to be driving it this minute. It is also the same principle that already exempts this path from AGENT_BROWSER_IDLE_TIMEOUT_MS. --- ...t-browser-bridge-command-transport.test.ts | 6 +- ...t-browser-bridge-session-lifecycle.test.ts | 95 +++++++++ src/main/browser/agent-browser-bridge.ts | 81 +++++++- .../agent-browser-orphan-sweep.test.ts | 190 ++++++++++++++++++ .../browser/agent-browser-orphan-sweep.ts | 91 +++++++++ .../agent-browser-process-environment.test.ts | 80 ++++++-- .../agent-browser-process-environment.ts | 43 +++- ...ffscreen-browser-backend-lifecycle.test.ts | 15 ++ src/main/index.ts | 7 +- .../external-chromium-browser-session.test.ts | 130 ++++++++++++ .../external-chromium-browser-session.ts | 59 +++++- ...uit-teardown-agent-browser-daemons.test.ts | 33 +++ 12 files changed, 786 insertions(+), 44 deletions(-) create mode 100644 src/main/browser/agent-browser-orphan-sweep.test.ts create mode 100644 src/main/browser/agent-browser-orphan-sweep.ts create mode 100644 src/main/orcad/external-chromium-browser-session.test.ts create mode 100644 src/main/quit-teardown-agent-browser-daemons.test.ts 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) + }) +})