diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index 2c013482d1e..6b1a4b3ed50 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -283,6 +283,35 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(h.windows).toHaveLength(2) }) + it('owns no page and no renderer when materialization fails', async () => { + // Why: a create that never produced a usable renderer must not occupy the + // retention budget, be listed as parked, or block a retry with the same id. + const h = createHarness() + h.manager.registerOffscreenGuest = () => { + throw new Error('register failed') + } + + await expect(h.backend.createTab({ url: 'https://a', browserPageId: 'a' })).rejects.toThrow( + 'register failed' + ) + + expect(h.backend.listParkedPages()).toEqual([]) + expect(h.windows.every((win) => win.isDestroyed())).toBe(true) + // The id is free again, so a retry is not rejected as already existing. + h.manager.registerOffscreenGuest = (({ + browserPageId, + webContentsId + }: { + browserPageId: string + webContentsId: number + }) => { + h.registered.set(browserPageId, webContentsId) + }) as typeof h.manager.registerOffscreenGuest + await expect(h.backend.createTab({ url: 'https://a', browserPageId: 'a' })).resolves.toEqual({ + browserPageId: 'a' + }) + }) + it('reports false when waking a page it does not own', async () => { const h = createHarness() expect(await h.backend.wakeTab('nope')).toBe(false) diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index bc974860ecc..b8bca8cfc76 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -1,4 +1,5 @@ import { randomUUID } from 'node:crypto' +import type { BrowserWindow } from 'electron' import { ORCA_BROWSER_PARTITION } from '../../shared/constants' import type { AgentBrowserBridge } from './agent-browser-bridge' import type { BrowserBackend, BrowserBackendCreateTab, ParkedBrowserPage } from './browser-backend' @@ -72,10 +73,20 @@ export class OffscreenBrowserBackend implements BrowserBackend { lastActivityAt: this.now() } this.pages.add(page) - // Why: register the guest and return immediately so the new tab appears - // without waiting for the page to finish loading. A failed load leaves the - // (usable) tab open, matching how a normal browser tab survives one. - this.materialize(page) + try { + // Why: register the guest and return immediately so the new tab appears + // without waiting for the page to finish loading. A failed load leaves + // the (usable) tab open, matching how a normal browser tab survives one. + this.materialize(page) + } catch (error) { + // Why: a create that never produced a usable renderer must not become an + // owned page. Left in place it would occupy the retention budget, be + // listed as parked, and make a retry with the same id fail as "already + // exists" — while never having installed the crash handler that would + // have cleaned it up. + this.pages.delete(browserPageId) + throw error + } void this.loadPage(page, url).catch((error) => { console.warn( '[offscreen-browser] page load failed:', @@ -157,6 +168,9 @@ export class OffscreenBrowserBackend implements BrowserBackend { return this.pages.parkedIdForImplicitTarget(worktreeId) } + // Why: quit only. The bridge's own destroyAllSessions() runs immediately + // before this in the shutdown chain, so routing each page through the bridge + // again would only re-close sessions that are already gone. destroyAll(): void { this.stopSweep() for (const page of this.pages.all()) { @@ -279,6 +293,20 @@ export class OffscreenBrowserBackend implements BrowserBackend { private materialize(page: OffscreenBrowserPage): void { const win = createOffscreenBrowserWindow(page.partition) + try { + this.attachWindow(page, win) + } catch (error) { + // Why: the window exists before anything that can fail. Abandoning it + // here would leak a hidden renderer nothing owns or can reach. + page.window = null + if (!win.isDestroyed()) { + win.destroy() + } + throw error + } + } + + private attachWindow(page: OffscreenBrowserPage, win: BrowserWindow): void { page.window = win // Why: reading win.webContents once the contents are destroyed throws, and // the throw would escape the 'destroyed' listener into the main process. diff --git a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts index e2ef723a1ee..971f97bdb3e 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -184,6 +184,19 @@ describe('headless parked-page targeting', () => { expect(fake.closedPageIds).toEqual(['parked-c']) }) + it('closes a parked page by index without rebuilding its renderer', async () => { + // Why: waking a page only to destroy it is pure churn, and a wake that + // fails would turn a reclaimable page into an unclosable one. + const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') + const fake = createFakeHeadlessHost(['live-a'], ['parked-b']) + const commands = new RuntimeBrowserCommands(fake.host) + + await commands.browserTabClose({ index: 1, worktree: 'id:wt-1' }) + + expect(fake.closedPageIds).toEqual(['parked-b']) + expect(fake.wakeCalls).toEqual([]) + }) + it('reports a listed-index overrun against the merged listing', async () => { const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') const fake = createFakeHeadlessHost(['live-a'], ['parked-b']) diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 83e234b43a4..0731799c32e 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -675,7 +675,8 @@ export class RuntimeBrowserCommands { */ private async resolveIndexedParkedAwarePageId( worktreeId: string | undefined, - index: number + index: number, + options: { wake?: boolean } = {} ): Promise { const offscreen = this.host.getOffscreenBrowserBackend() if (!offscreen?.listParkedPages || this.host.getAvailableAuthoritativeWindow()) { @@ -689,9 +690,13 @@ export class RuntimeBrowserCommands { `Tab index ${index} out of range (0-${tabs.length - 1})` ) } - // Why: waking unconditionally also restarts the reclaim clock, so selecting - // a resident page counts as using it rather than leaving it up for eviction. - await offscreen.wakeTab?.(chosen.browserPageId) + // Why: waking also restarts the reclaim clock, so selecting a resident page + // counts as using it rather than leaving it up for eviction. A close opts + // out — rebuilding a renderer only to destroy it is pure churn, and a wake + // that fails would turn a reclaimable page into an unclosable one. + if (options.wake !== false) { + await offscreen.wakeTab?.(chosen.browserPageId) + } return chosen.browserPageId } @@ -1742,7 +1747,9 @@ export class RuntimeBrowserCommands { // Why (STA-4341): a headless listing includes parked pages, so an index // must be resolved against that same listing or `tab close --index` can // destroy a different tab than the one the caller read. - tabId = await this.resolveIndexedParkedAwarePageId(worktreeId, params.index) + tabId = await this.resolveIndexedParkedAwarePageId(worktreeId, params.index, { + wake: false + }) if (tabId === null) { const entries = [...bridge.getRegisteredTabs(worktreeId).entries()] if (params.index < 0 || params.index >= entries.length) {