From e7d9e26c314f848df58df091e1d2ba7910ddc78b Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 03:33:27 -0400 Subject: [PATCH] fix(browser): roll back a failed page create and stop closing by index from waking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two, resources. Two defects: - createTab added the page record before materializing it, so a window that failed to construct or register left the record behind — occupying the retention budget, listed as parked, and making a retry with the same id fail as "already exists" — without ever installing the crash handler that would have cleaned it up. The handler lives inside materialize, after the operations that can throw, so nothing else would have. materialize now destroys a window it cannot finish attaching rather than orphaning a hidden renderer, and createTab drops the record. - `tab close --index` on a parked page went through the shared index resolver, which wakes unconditionally so that switching counts as using a page. For a close that is pure churn: it rebuilds a renderer, tears down and recreates the helper session, starts a navigation, and then destroys all of it — and a wake that failed would turn a reclaimable page into an unclosable one. The resolver takes a no-wake mode and close uses it. Both mutation-checked. --- .../offscreen-browser-backend-reclaim.test.ts | 29 +++++++++++++++ src/main/browser/offscreen-browser-backend.ts | 36 ++++++++++++++++--- ...a-runtime-browser-parked-targeting.test.ts | 13 +++++++ src/main/runtime/orca-runtime-browser.ts | 17 ++++++--- 4 files changed, 86 insertions(+), 9 deletions(-) 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) {