fix(browser): roll back a failed page create and stop closing by index from waking

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.
This commit is contained in:
Jinwoo-H
2026-08-19 19:07:11 -04:00
parent 77a4c3693c
commit e7d9e26c31
4 changed files with 86 additions and 9 deletions
@@ -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)
+32 -4
View File
@@ -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.
@@ -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'])
+12 -5
View File
@@ -675,7 +675,8 @@ export class RuntimeBrowserCommands {
*/
private async resolveIndexedParkedAwarePageId(
worktreeId: string | undefined,
index: number
index: number,
options: { wake?: boolean } = {}
): Promise<string | null> {
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) {