From 77a4c3693cbf3da4170607273f3ccdb0abe800db Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 03:16:26 -0400 Subject: [PATCH] fix(browser): correct what a parked page reports and which one a page-less command means Round-two review of the merged listing. Four defects, all in what parking projects rather than in the reclamation itself: - A page-less command picked the most recently used parked page. Recency is not activeness: an explicit `--page b` makes b the most recently used while a is still the active tab, and a page-less command has always meant "the active tab". The retained active page now wins, with recency only breaking the tie. - Parking the active page promotes another live tab to active, which then parks claiming the flag too, so a listing could report two active tabs. Only the newest claim holds it, and the merge enforces at most one either way. - A parked page reported no load failure, showing a page that could not load as healthy at its address. The failure becomes unreadable once the guest is unregistered, so it is captured onto the record. Certificate failures are deliberately not carried: their challenge id dies with the renderer, so surfacing one would offer an action that cannot work. - Parking sniffed the committed address, which cannot tell an intentional navigation to about:blank from the blank page a failed load left behind. The record now follows did-navigate, so waking returns the page to where it actually is; chrome-error addresses are the failure, not a destination, and never replace the address that produced them. Each has a mutation-checked regression test. --- src/main/browser/browser-backend.ts | 11 +- .../headless-browser-tab-listing.test.ts | 13 ++ .../browser/headless-browser-tab-listing.ts | 12 +- .../offscreen-browser-backend-reclaim.test.ts | 122 ++++++++++++++++-- src/main/browser/offscreen-browser-backend.ts | 42 ++++-- .../browser/offscreen-browser-open-pages.ts | 28 +++- ...a-runtime-browser-parked-targeting.test.ts | 2 +- src/main/runtime/orca-runtime-browser.ts | 4 +- 8 files changed, 197 insertions(+), 37 deletions(-) diff --git a/src/main/browser/browser-backend.ts b/src/main/browser/browser-backend.ts index e55c8dc7a16..e06b7f54b2e 100644 --- a/src/main/browser/browser-backend.ts +++ b/src/main/browser/browser-backend.ts @@ -6,6 +6,8 @@ // WebContents uniformly regardless of how the page was created. This interface // isolates the only step that actually differs: tab creation and teardown. +import type { BrowserLoadError } from '../../shared/browser-workspace-types' + export type BrowserBackendCreateTab = { url: string worktreeId?: string @@ -22,6 +24,9 @@ export type ParkedBrowserPage = { title: string /** Whether the page was its worktree's active tab when it parked. */ active?: boolean + /** The failure the page last reported. Reclaiming a renderer does not make a + * page that failed to load healthy again. */ + loadError?: BrowserLoadError | null } export type BrowserBackend = { @@ -40,7 +45,7 @@ export type BrowserBackend = { wakeTab?(browserPageId: string): Promise /** Pages this backend still owns whose renderer is currently reclaimed. */ listParkedPages?(worktreeId?: string): ParkedBrowserPage[] - /** Most-recently-used parked page, for commands that target a worktree - * rather than a page id. */ - getMostRecentlyUsedParkedPageId?(worktreeId?: string): string | null + /** The parked page a page-less command should target: the one that was + * active when it parked, else the most recently used. */ + getParkedPageIdForImplicitTarget?(worktreeId?: string): string | null } diff --git a/src/main/browser/headless-browser-tab-listing.test.ts b/src/main/browser/headless-browser-tab-listing.test.ts index d5ee2a21216..d04580b2fbe 100644 --- a/src/main/browser/headless-browser-tab-listing.test.ts +++ b/src/main/browser/headless-browser-tab-listing.test.ts @@ -60,4 +60,17 @@ describe('mergeParkedBrowserTabs', () => { parked: true }) }) + + it('reports at most one active tab when several parked pages claim it', () => { + // Why: `active` is a single selection. Parking the active page promotes + // another live tab, which can later park claiming the flag as well. + const merged = mergeParkedBrowserTabs([], [parked('b', true), parked('c', true)]) + expect(merged.filter((tab) => tab.active).map((tab) => tab.browserPageId)).toEqual(['b']) + }) + + it('carries a parked page load failure into the listing', () => { + const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' } + const [tab] = mergeParkedBrowserTabs([], [{ ...parked('b'), loadError }]) + expect(tab.loadError).toEqual(loadError) + }) }) diff --git a/src/main/browser/headless-browser-tab-listing.ts b/src/main/browser/headless-browser-tab-listing.ts index 8c1012f4baa..e2b1037a0bf 100644 --- a/src/main/browser/headless-browser-tab-listing.ts +++ b/src/main/browser/headless-browser-tab-listing.ts @@ -16,17 +16,23 @@ export function mergeParkedBrowserTabs( } // Why: parking clears the bridge's active pointer, which then lands on a // surviving live tab. A parked page may only claim active when nothing live - // does, or a listing can report two active tabs. - const liveClaimsActive = live.some((tab) => tab.active) + // does, and at most one may — `active` is a single selection, so a listing + // that reports two is not something any caller can act on. + let activeClaimed = live.some((tab) => tab.active) const merged = [...live] for (const page of parked) { + const active = !activeClaimed && page.active === true + activeClaimed = activeClaimed || active merged.push({ browserPageId: page.browserPageId, index: merged.length, url: page.url, title: page.title, - active: !liveClaimsActive && page.active === true, + active, parked: true, + // Why: a page that failed to load is still failed while parked; hiding + // that would show a broken page as healthy at its address. + loadError: page.loadError ?? null, worktreeId: page.worktreeId ?? null, profileId: page.profileId ?? null }) diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index b56ff27247b..2c013482d1e 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -25,6 +25,8 @@ type FakeWindow = BrowserWindow & { __destroyed: boolean __url: string __destroyedListeners: (() => void)[] + __navigationListeners: ((e: unknown, url: string) => void)[] + navigateTo: (url: string) => void } let nextWebContentsId = 100 @@ -36,7 +38,14 @@ function makeWindow(): FakeWindow { __destroyed: false, __url: 'about:blank', __destroyedListeners: [] as (() => void)[], + __navigationListeners: [] as ((e: unknown, url: string) => void)[], isDestroyed: () => win.__destroyed, + navigateTo: (url: string) => { + win.__url = url + for (const listener of win.__navigationListeners) { + listener(null, url) + } + }, destroy: () => { win.__destroyed = true for (const listener of win.__destroyedListeners) { @@ -52,6 +61,11 @@ function makeWindow(): FakeWindow { if (event === 'destroyed') { win.__destroyedListeners.push(listener) } + }, + on: (event: string, listener: (e: unknown, url: string) => void) => { + if (event === 'did-navigate' || event === 'did-navigate-in-page') { + win.__navigationListeners.push(listener) + } } } } as unknown as FakeWindow @@ -66,9 +80,17 @@ type Harness = { registered: Map windows: FakeWindow[] clock: { value: number } + activePageId: string | undefined } -function createHarness(overrides: { pinned?: Set; activePageId?: string } = {}): Harness { +function createHarness( + overrides: { + pinned?: Set + activePageId?: string + loadError?: { code: number; description: string; validatedUrl: string } | null + } = {} +): Harness { + const state = { activePageId: overrides.activePageId } const order: string[] = [] const registered = new Map() const windows: FakeWindow[] = [] @@ -95,7 +117,8 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string order.push(`unregister:${browserPageId}`) registered.delete(browserPageId) }, - getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null + getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null, + getBrowserPageLoadError: () => overrides.loadError ?? null } as unknown as BrowserManager const bridge = { @@ -105,7 +128,7 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string onProcessSwap: vi.fn(async (browserPageId: string, webContentsId: number) => { order.push(`process-swap:${browserPageId}:${webContentsId}`) }), - isActiveBrowserPage: (browserPageId: string) => overrides.activePageId === browserPageId + isActiveBrowserPage: (browserPageId: string) => state.activePageId === browserPageId } as unknown as AgentBrowserBridge const backend = new OffscreenBrowserBackend(manager, { @@ -114,7 +137,18 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string now: () => clock.value }) - return { backend, manager, bridge, order, registered, windows, clock } + return { + backend, + manager, + bridge, + order, + registered, + windows, + clock, + set activePageId(value: string | undefined) { + state.activePageId = value + } + } } beforeEach(() => { @@ -145,7 +179,8 @@ describe('OffscreenBrowserBackend reclamation', () => { profileId: 'default', url: 'https://example.test/a', title: `title-${h.windows[0].webContents.id}`, - active: false + active: false, + loadError: null } ]) }) @@ -186,7 +221,7 @@ describe('OffscreenBrowserBackend reclamation', () => { const h = createHarness() await h.backend.createTab({ url: 'https://example.test/a' }) const [pageId] = [...h.registered.keys()] - h.windows[0].__url = 'https://example.test/moved' + h.windows[0].navigateTo('https://example.test/moved') h.clock.value += 120_000 await h.backend.reclaimIdlePages() @@ -204,11 +239,10 @@ describe('OffscreenBrowserBackend reclamation', () => { }) it('keeps the requested address when a page parks before committing one', async () => { - // Why: an uncommitted window still reports about:blank, and adopting that - // would send the wake to a blank page instead of the agent's URL. + // Why: a page whose load never committed must still wake to the address the + // agent asked for, not to the blank page the window started on. const h = createHarness() await h.backend.createTab({ url: 'https://example.test/slow', browserPageId: 'a' }) - h.windows[0].__url = 'about:blank' h.clock.value += 120_000 await h.backend.reclaimIdlePages() @@ -298,6 +332,72 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(h.backend.listParkedPages().map((page) => page.browserPageId)).toEqual(['b']) }) + it('keeps an intentional navigation to about:blank across a park', async () => { + // Why: an in-page `location.href = "about:blank"` is a real destination. + // Sniffing the address at park time could not tell it apart from the blank + // page a window starts on, so the record follows navigation instead. + const h = createHarness() + await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' }) + h.windows[0].navigateTo('about:blank') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.url).toBe('about:blank') + }) + + it('ignores a chrome-error address so a wake retries the real one', async () => { + const h = createHarness() + await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' }) + h.windows[0].navigateTo('chrome-error://chromewebdata/') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.url).toBe('https://example.test/a') + }) + + it('carries a load failure onto the parked record', async () => { + // Why: reclaiming a renderer does not make a page that failed to load + // healthy, and the failure is unreadable once the guest is unregistered. + const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' } + const h = createHarness({ loadError }) + await h.backend.createTab({ url: 'https://nope', browserPageId: 'a' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.loadError).toEqual(loadError) + }) + + it('lets only the newest claim hold the parked active flag', async () => { + // Why: parking the active page promotes another live tab to active, which + // then parks claiming the flag too. `active` is a single selection. + const h = createHarness({ activePageId: 'a' }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + // Simulate the promotion: b is now the active page and parks claiming it. + h.activePageId = 'b' + await h.backend.wakeTab('b') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + const claimed = h.backend.listParkedPages().filter((page) => page.active) + expect(claimed.map((page) => page.browserPageId)).toEqual(['b']) + }) + + it('targets the page that was active, not merely the most recently used', async () => { + // Why: an explicit `--page b` command makes b the most recently used while + // a is still the active tab, and a page-less command means "the active tab". + const h = createHarness({ activePageId: 'a' }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + h.clock.value += 1_000 + await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.getParkedPageIdForImplicitTarget('wt-1')).toBe('a') + }) + it('reports the most recently used parked page for implicit targeting', async () => { const h = createHarness() for (const id of ['a', 'b', 'c', 'd']) { @@ -307,7 +407,7 @@ describe('OffscreenBrowserBackend reclamation', () => { h.clock.value += 10_000 await h.backend.reclaimIdlePages() - expect(h.backend.getMostRecentlyUsedParkedPageId()).toBe('b') + expect(h.backend.getParkedPageIdForImplicitTarget()).toBe('b') }) it('restarts the reclaim clock when a resident page is used', async () => { @@ -395,7 +495,7 @@ describe('OffscreenBrowserBackend reclamation', () => { await h.backend.reclaimIdlePages() expect(h.backend.listParkedPages('wt-1').map((page) => page.browserPageId)).toEqual(['a']) - expect(h.backend.getMostRecentlyUsedParkedPageId('wt-2')).toBe('b') + expect(h.backend.getParkedPageIdForImplicitTarget('wt-2')).toBe('b') }) it('destroys every page it owns on shutdown', async () => { diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index 96904d61903..bc974860ecc 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -68,6 +68,7 @@ export class OffscreenBrowserBackend implements BrowserBackend { window: null, activeWhenParked: false, loading: false, + loadError: null, lastActivityAt: this.now() } this.pages.add(page) @@ -151,9 +152,9 @@ export class OffscreenBrowserBackend implements BrowserBackend { return this.pages.listParked(worktreeId) } - /** Most-recently-used parked page in a worktree, for implicit targeting. */ - getMostRecentlyUsedParkedPageId(worktreeId?: string): string | null { - return this.pages.mostRecentlyUsedParkedId(worktreeId) + /** The parked page a page-less command should target in this worktree. */ + getParkedPageIdForImplicitTarget(worktreeId?: string): string | null { + return this.pages.parkedIdForImplicitTarget(worktreeId) } destroyAll(): void { @@ -217,23 +218,25 @@ export class OffscreenBrowserBackend implements BrowserBackend { if (!page?.window || page.window.isDestroyed()) { return } - // Why: capture the committed address before teardown so a woken page - // returns to where the agent left it, not to its original create URL. A - // page that never committed still reports the empty or blank address the - // window started on; adopting that would send the wake to the wrong page. - const wc = page.window.webContents - const committed = wc.getURL() - const uncommitted = !committed || committed === 'about:blank' - if (!uncommitted && !committed.startsWith('chrome-error://')) { - page.url = committed - } - page.title = wc.getTitle() ?? page.title + // Why: the record's address is kept current by did-navigate, so parking + // never has to guess it from a WebContents that may be sitting on the blank + // page a failed load left behind. + page.title = page.window.webContents.getTitle() ?? page.title + // Why: a reclaimed renderer does not make a page that failed to load + // healthy, and the failure becomes unreadable once the guest is + // unregistered — so carry it onto the record. + page.loadError = this.browserManager.getBrowserPageLoadError(browserPageId) page.activeWhenParked = this.options .getAgentBrowserBridge?.() ?.isActiveBrowserPage(browserPageId, page.worktreeId) === true await this.releaseRenderer(page, browserPageId) page.window = null + if (page.activeWhenParked) { + // Why: teardown promotes another live tab to active, which then parks + // claiming the flag too. Only the newest claim may hold it. + this.pages.claimParkedActive(browserPageId, page.worktreeId) + } } // Why: teardown order matters — the bridge must destroy the helper session and @@ -281,6 +284,17 @@ export class OffscreenBrowserBackend implements BrowserBackend { // the throw would escape the 'destroyed' listener into the main process. // Capture the id while it is still safe to read. const webContentsId = win.webContents.id + // Why: the record's address must follow the page, not the create call — an + // agent that navigates with `goto` or in-page script has to be woken back + // to where it actually is. A chrome-error address is the failure, not a + // destination, so it never replaces the address that produced it. + const trackNavigation = (_event: unknown, url: string): void => { + if (page.window === win && url && !url.startsWith('chrome-error://')) { + page.url = url + } + } + win.webContents.on('did-navigate', trackNavigation) + win.webContents.on('did-navigate-in-page', trackNavigation) // Why: if the window is destroyed out from under us (crash, app teardown), // drop the page so commands fail cleanly instead of resolving a dead // WebContents. Parking destroys it deliberately, so it opts out here. diff --git a/src/main/browser/offscreen-browser-open-pages.ts b/src/main/browser/offscreen-browser-open-pages.ts index 5508ec66b90..5cf74760504 100644 --- a/src/main/browser/offscreen-browser-open-pages.ts +++ b/src/main/browser/offscreen-browser-open-pages.ts @@ -1,4 +1,5 @@ import type { BrowserWindow } from 'electron' +import type { BrowserLoadError } from '../../shared/browser-workspace-types' import type { ParkedBrowserPage } from './browser-backend' // Why (STA-4341): a headless browser page and the renderer behind it have @@ -20,6 +21,8 @@ export type OffscreenBrowserPage = { activeWhenParked: boolean /** True while the initial or post-wake navigation is still in flight. */ loading: boolean + /** The failure the page reported when its renderer was reclaimed. */ + loadError: BrowserLoadError | null lastActivityAt: number } @@ -77,18 +80,37 @@ export class OffscreenBrowserOpenPages { profileId: page.profileId, url: page.url, title: page.title, - active: page.activeWhenParked + active: page.activeWhenParked, + loadError: page.loadError })) } - /** Most-recently-used parked page, for commands that name a worktree. */ - mostRecentlyUsedParkedId(worktreeId?: string): string | null { + /** + * The parked page a page-less command should target. Activity order alone is + * wrong: an explicit `--page B` command makes B the most recently used while + * A is still the active tab, and a page-less command has always meant "the + * active tab". So the retained active page wins, and recency only breaks the + * tie when none is recorded. + */ + parkedIdForImplicitTarget(worktreeId?: string): string | null { let best: OffscreenBrowserPage | null = null for (const page of this.parked(worktreeId)) { + if (page.activeWhenParked) { + return page.browserPageId + } if (!best || page.lastActivityAt > best.lastActivityAt) { best = page } } return best?.browserPageId ?? null } + + /** Only one page per worktree may carry the active flag across a park. */ + claimParkedActive(browserPageId: string, worktreeId?: string): void { + for (const page of this.parked(worktreeId)) { + if (page.browserPageId !== browserPageId) { + page.activeWhenParked = false + } + } + } } 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 6a7914bc22a..e2ef723a1ee 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -109,7 +109,7 @@ function createFakeHeadlessHost(live: string[], parked: string[]): FakeHeadlessH url: `https://example.test/${browserPageId}`, title: browserPageId })), - getMostRecentlyUsedParkedPageId: () => state.parkedPageIds.at(-1) ?? null + getParkedPageIdForImplicitTarget: () => state.parkedPageIds.at(-1) ?? null } as unknown as BrowserBackend state.host = { diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index e02958b7d83..83e234b43a4 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -327,7 +327,7 @@ export class RuntimeBrowserCommands { // Read-only callers opt out — listing tabs must never cost a renderer. if (options.wakeParkedPage !== false && !this.host.getAvailableAuthoritativeWindow()) { const offscreen = this.host.getOffscreenBrowserBackend() - const parkedPageId = offscreen?.getMostRecentlyUsedParkedPageId?.(worktreeId) + const parkedPageId = offscreen?.getParkedPageIdForImplicitTarget?.(worktreeId) if (parkedPageId && (await offscreen?.wakeTab?.(parkedPageId))) { return } @@ -1768,7 +1768,7 @@ export class RuntimeBrowserCommands { const resolvedTabId = tabId ?? bridge.getActivePageId(worktreeId) ?? - offscreen.getMostRecentlyUsedParkedPageId?.(worktreeId) ?? + offscreen.getParkedPageIdForImplicitTarget?.(worktreeId) ?? null if (!resolvedTabId) { return { closed: false }