From de4fef9c5a92ed6526dbb3f36e1dd83aefc02845 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 03:54:53 -0400 Subject: [PATCH] fix(browser): close the round-three parked-page reporting and reuse gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reusing a page id while its close was still tearing down let the new renderer register and the old close then unregister it, leaving a page the runtime owned but no command could resolve. A create now waits out an in-flight release for that id, which orders the whole reuse. - Closing a parked page told nobody: it has no WebContents teardown for the session-tabs publication to piggyback on, so a paired client kept showing a ghost tab until an operation against it failed. - A page waiting on a certificate decision was parkable. Its challenge id dies with the renderer, so parking discarded both the warning and the ability to approve it. Such a page is work waiting on an answer and stays resident. - The load helper resolves on its own 30s timeout, so `loading` stopped covering a navigation that was merely slow. The renderer is asked directly. - An iframe changing its own hash fires did-navigate-in-page too, and its address was being recorded as the tab's — waking would have landed the page on the iframe's document. Only main-frame navigations move the record. - `tab switch --index` reported the position the wake had renumbered the target to, which described neither the request nor the listing the caller read. Each mutation-checked. An ownership guard added alongside the reuse fix turned out to be unreachable once creates are ordered, so it is not here. --- docs/reference/headless-linux-server.md | 4 +- .../offscreen-browser-backend-reclaim.test.ts | 123 ++++++++++++++++-- src/main/browser/offscreen-browser-backend.ts | 45 ++++++- src/main/index.ts | 3 +- ...a-runtime-browser-parked-targeting.test.ts | 13 ++ src/main/runtime/orca-runtime-browser.ts | 8 +- src/main/runtime/orca-runtime.ts | 8 ++ 7 files changed, 186 insertions(+), 18 deletions(-) diff --git a/docs/reference/headless-linux-server.md b/docs/reference/headless-linux-server.md index 29eaae988e8..62af6ffaac7 100644 --- a/docs/reference/headless-linux-server.md +++ b/docs/reference/headless-linux-server.md @@ -289,8 +289,8 @@ targets it transparently rebuilds the renderer and reloads the address. What a park does _not_ preserve is in-page JavaScript state — cookies and local storage live in the profile partition and survive, but an unsubmitted form or an SPA's in-memory state does not. A page is never parked while a paired -client is streaming it, while a command against it is in flight, or while its -navigation has not committed. +client is streaming it, while a command against it is in flight, while it is +still navigating, or while it is waiting on a certificate decision. Two evictors decide what stays resident, mirroring terminal pane parking. The cap is the primary one: diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index f3cb1f3686a..6bf12e15ea4 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -25,8 +25,9 @@ type FakeWindow = BrowserWindow & { __destroyed: boolean __url: string __destroyedListeners: (() => void)[] - __navigationListeners: ((e: unknown, url: string) => void)[] - navigateTo: (url: string) => void + __navigationListeners: ((e: unknown, url: string, isMainFrame?: boolean) => void)[] + navigateTo: (url: string, isMainFrame?: boolean) => void + __loading: boolean } let nextWebContentsId = 100 @@ -38,12 +39,15 @@ function makeWindow(): FakeWindow { __destroyed: false, __url: 'about:blank', __destroyedListeners: [] as (() => void)[], - __navigationListeners: [] as ((e: unknown, url: string) => void)[], + __navigationListeners: [] as ((e: unknown, url: string, isMainFrame?: boolean) => void)[], + __loading: false, isDestroyed: () => win.__destroyed, - navigateTo: (url: string) => { - win.__url = url + navigateTo: (url: string, isMainFrame = true) => { + if (isMainFrame) { + win.__url = url + } for (const listener of win.__navigationListeners) { - listener(null, url) + listener(null, url, isMainFrame) } }, destroy: () => { @@ -56,14 +60,22 @@ function makeWindow(): FakeWindow { id, isDestroyed: () => win.__destroyed, getURL: () => win.__url, + isLoading: () => win.__loading, getTitle: () => `title-${id}`, once: (event: string, listener: () => void) => { 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') { + on: (event: string, listener: (e: unknown, url: string, isMainFrame?: boolean) => void) => { + if (event === 'did-navigate') { + win.__navigationListeners.push((e, url, isMainFrame) => { + if (isMainFrame !== false) { + listener(e, url) + } + }) + } + if (event === 'did-navigate-in-page') { win.__navigationListeners.push(listener) } } @@ -81,6 +93,8 @@ type Harness = { windows: FakeWindow[] clock: { value: number } activePageId: string | undefined + pagesChanged: (string | undefined)[] + certificateFailurePageIds: Set } function createHarness( @@ -88,9 +102,14 @@ function createHarness( pinned?: Set activePageId?: string loadError?: { code: number; description: string; validatedUrl: string } | null + certificateFailurePageIds?: string[] } = {} ): Harness { - const state = { activePageId: overrides.activePageId } + const state = { + activePageId: overrides.activePageId, + certificateFailurePageIds: new Set(overrides.certificateFailurePageIds ?? []), + pagesChanged: [] as (string | undefined)[] + } const order: string[] = [] const registered = new Map() const windows: FakeWindow[] = [] @@ -118,7 +137,9 @@ function createHarness( registered.delete(browserPageId) }, getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null, - getBrowserPageLoadError: () => overrides.loadError ?? null + getBrowserPageLoadError: () => overrides.loadError ?? null, + getBrowserPageCertificateFailure: (browserPageId: string) => + state.certificateFailurePageIds.has(browserPageId) ? { challengeId: 'c' } : null } as unknown as BrowserManager const bridge = { @@ -134,6 +155,7 @@ function createHarness( const backend = new OffscreenBrowserBackend(manager, { getAgentBrowserBridge: () => bridge, isPagePinned: (id) => overrides.pinned?.has(id) === true, + onPagesChanged: (worktreeId) => state.pagesChanged.push(worktreeId), now: () => clock.value }) @@ -147,7 +169,11 @@ function createHarness( clock, set activePageId(value: string | undefined) { state.activePageId = value - } + }, + get pagesChanged(): (string | undefined)[] { + return state.pagesChanged + }, + certificateFailurePageIds: state.certificateFailurePageIds } } @@ -353,6 +379,81 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(await wake).toBe(true) }) + it('does not let a slow close unregister a page that reused its id', async () => { + // Why: closing drops the record before teardown finishes, so a caller + // reusing the id could register a renderer the old close then unregisters, + // leaving the new page unreachable by every command. + let releaseTeardown = (): void => {} + const h = createHarness() + await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) + const bridge = h.bridge as unknown as { onTabClosed: ReturnType } + bridge.onTabClosed.mockImplementationOnce( + async () => new Promise((resolve) => (releaseTeardown = resolve)) + ) + + const close = h.backend.closeTab('a') + await Promise.resolve() + const recreate = h.backend.createTab({ url: 'https://a2', browserPageId: 'a' }) + releaseTeardown() + await close + await recreate + + expect(h.registered.get('a')).toBe(h.windows[1].webContents.id) + expect(h.windows[0].isDestroyed()).toBe(true) + expect(h.windows[1].isDestroyed()).toBe(false) + }) + + it('tells the session snapshot when a parked page is closed', async () => { + // Why: a parked close has no WebContents teardown to piggyback on, so + // nothing else would tell paired clients the tab is gone. + const h = createHarness() + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + h.pagesChanged.length = 0 + + await h.backend.closeTab('a') + + expect(h.pagesChanged).toEqual(['wt-1']) + }) + + it('does not park a page waiting on a certificate decision', async () => { + // Why: the challenge id dies with the renderer, so parking would discard + // both the warning and the ability to approve it. + const h = createHarness({ certificateFailurePageIds: ['a'] }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) + h.clock.value += 120_000 + + expect(await h.backend.reclaimIdlePages()).toEqual([]) + + h.certificateFailurePageIds.delete('a') + expect(await h.backend.reclaimIdlePages()).toEqual(['a']) + }) + + it('does not park a renderer whose navigation is still running after the load timeout', async () => { + // Why: the load helper resolves on its own timeout, so `loading` stops + // covering a navigation that is merely slow rather than finished. + const h = createHarness() + await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) + h.windows[0].__loading = true + h.clock.value += 120_000 + + expect(await h.backend.reclaimIdlePages()).toEqual([]) + + h.windows[0].__loading = false + expect(await h.backend.reclaimIdlePages()).toEqual(['a']) + }) + + it('ignores a subframe in-page navigation when recording the address', async () => { + const h = createHarness() + await h.backend.createTab({ url: 'https://host.test/', browserPageId: 'a' }) + h.windows[0].navigateTo('https://frame.test/#x', false) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.url).toBe('https://host.test/') + }) + it('coalesces concurrent wakes into one renderer', async () => { const h = createHarness() await h.backend.createTab({ url: 'https://example.test/a' }) diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index a5403e12ed1..206dcc0eadf 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -27,6 +27,8 @@ export type OffscreenBrowserBackendOptions = { getAgentBrowserBridge?: () => AgentBrowserBridge | null /** Pages a client is streaming or that have a command in flight. */ isPagePinned?: (browserPageId: string) => boolean + /** Called when the set of open pages changes, so paired clients republish. */ + onPagesChanged?: (worktreeId: string | undefined) => void now?: () => number } @@ -50,6 +52,10 @@ export class OffscreenBrowserBackend implements BrowserBackend { async createTab(params: BrowserBackendCreateTab): Promise<{ browserPageId: string }> { const browserPageId = params.browserPageId ?? randomUUID() + // Why: closing drops the record before its teardown finishes, so a caller + // reusing the id can register a new renderer that the old close then + // unregisters. Let the release finish before claiming the id again. + await this.releasing.get(browserPageId) if (this.pages.has(browserPageId)) { throw new Error(`Browser page ${browserPageId} already exists`) } @@ -104,6 +110,12 @@ export class OffscreenBrowserBackend implements BrowserBackend { if (this.pages.size === 0) { this.stopSweep() } + // Why: closing a parked page has no WebContents teardown to piggyback on, + // so nothing else tells paired clients the tab is gone — they would keep + // showing a ghost until an operation against it failed. + if (page) { + this.options.onPagesChanged?.(page.worktreeId) + } } /** @@ -250,11 +262,30 @@ export class OffscreenBrowserBackend implements BrowserBackend { private isPagePinned(page: OffscreenBrowserPage): boolean { return ( page.loading || + this.isNavigating(page) || this.waking.has(page.browserPageId) || + // Why: a page blocked on a certificate decision is work waiting on an + // answer. Its challenge id dies with the renderer, so parking it would + // discard both the warning and the ability to approve it. + this.browserManager.getBrowserPageCertificateFailure(page.browserPageId) !== null || this.options.isPagePinned?.(page.browserPageId) === true ) } + // Why: the load helper resolves on its own timeout, so `loading` stops + // covering a navigation that is slow rather than finished. Ask the renderer. + private isNavigating(page: OffscreenBrowserPage): boolean { + const win = page.window + if (!win || win.isDestroyed()) { + return false + } + try { + return win.webContents.isLoading() + } catch { + return false + } + } + private isSafeToReclaim(browserPageId: string): boolean { const page = this.pages.get(browserPageId) if (!page || this.releasing.has(browserPageId)) { @@ -352,13 +383,21 @@ export class OffscreenBrowserBackend implements BrowserBackend { // 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 => { + const recordAddress = (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) + // did-navigate is main-frame only; did-frame-navigate covers subframes. + win.webContents.on('did-navigate', (_event, url) => recordAddress(url)) + // Why: an iframe changing its own hash also fires did-navigate-in-page. A + // subframe navigation is not a navigation of the tab, and adopting its + // address would wake the page onto the iframe's document. + win.webContents.on('did-navigate-in-page', (_event, url, isMainFrame) => { + if (isMainFrame) { + recordAddress(url) + } + }) // 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/index.ts b/src/main/index.ts index ea46575d239..382f85d9d26 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -3168,7 +3168,8 @@ void app.whenReady().then(async () => { runtime.setOffscreenBrowserBackend( new OffscreenBrowserBackend(browserManager, { getAgentBrowserBridge: () => runtime?.getAgentBrowserBridge() ?? null, - isPagePinned: (browserPageId) => runtime?.isBrowserPagePinned(browserPageId) === true + isPagePinned: (browserPageId) => runtime?.isBrowserPagePinned(browserPageId) === true, + onPagesChanged: (worktreeId) => runtime?.notifyHeadlessBrowserPagesChanged(worktreeId) }) ) } 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 45cfbe2c7b5..6ec6001bd1d 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -163,6 +163,19 @@ describe('headless parked-page targeting', () => { ]) }) + it('reports back the index the caller passed, not the renumbered one', async () => { + // Why: waking the target makes it live, which renumbers the bridge listing. + // Reporting that position would describe neither the request nor the + // listing the caller read. + const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') + const fake = createFakeHeadlessHost(['live-a'], ['parked-b', 'parked-c']) + const commands = new RuntimeBrowserCommands(fake.host) + + const result = await commands.browserTabSwitch({ index: 2, worktree: 'id:wt-1' }) + + expect(result).toMatchObject({ switched: 2, browserPageId: 'parked-c' }) + }) + it('counts an indexed switch of a resident page as using it', async () => { const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') const fake = createFakeHeadlessHost(['live-a', 'live-b'], ['parked-c']) diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 2fe3bac8a42..84bf8389b9f 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -757,11 +757,17 @@ export class RuntimeBrowserCommands { // or `tab switch --index 0` selects a different tab than the one it showed. const indexed = await this.resolveIndexedTargetBeforeWake(params) const target = indexed ?? (await this.resolveBrowserCommandTarget(params)) - const result = await bridge.tabSwitch( + const switched = await bridge.tabSwitch( indexed ? undefined : params.index, target.worktreeId, target.browserPageId ) + // Why (STA-4341): waking the target makes it live, which renumbers the + // bridge's own listing. Report the index the caller passed — the one the + // listing they read actually names — rather than a position that describes + // neither their request nor what they were looking at. + const result = + indexed && params.index !== undefined ? { ...switched, switched: params.index } : switched if (params.focus) { // Why: scope focus to the tab's owning worktree; the renderer never yanks the user across worktrees on this signal (see focusBrowserTabInWorktree). const worktreeId = diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index ec9de89347d..2d32fc02c73 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -6608,6 +6608,14 @@ export class OrcaRuntimeService { return this.offscreenBrowserBackend } + // Why (STA-4341): closing a parked page has no WebContents teardown to + // piggyback on, so the session snapshot only learns about it through here. + notifyHeadlessBrowserPagesChanged(worktreeId: string | undefined): void { + if (worktreeId) { + this.notifyMobileSessionTabsChanged(worktreeId) + } + } + // Why (STA-4341): the headless reclaimer asks the runtime before parking a // page, so a streamed or mid-command page keeps its renderer. isBrowserPagePinned(browserPageId: string): boolean {