From 9a891b8307431cb2bb58be3b66cba509df2877d9 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 20 Aug 2026 02:45:02 -0400 Subject: [PATCH] fix(browser): keep headless tab order, active flag and env knobs honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compat-lens findings from the fourth review round (1 P1, 4 P2): P1 — the merged listing ordered resident tabs before parked ones, so a background park renumbered indices that pre-reclaim moved only on create/close: an index the agent read minutes ago could close a different tab, and a paired client's tab bar reordered whenever a tab parked or woke. Listings now sort by the page book's creation order, which parking cannot change. P2 — closing the page that carried the scope's only active claim (the parked-active holder, or the last live tab) left a worktree whose every open page reported inactive, breaking the one-selected-tab assumption paired clients render on. closeTab now promotes the most recently used parked page when nothing live claims active. P2 — claimParkedActive treated an undefined worktree as 'every worktree', so a worktree-less page (a remote 'tab create' without --worktree) parking while active wiped every other worktree's parked-active flag host-wide. The claim now scopes strictly, undefined matching only undefined. P2 — readPositiveIntEnv used parseInt, which reads '1e9' as 1: an operator following the docs' 'set the idle window very high' advice silently got a 1ms window. Number() consumes the whole string, and trailing garbage now falls back instead of truncating. P2 — the docs' closed enumeration of park vetoes omitted the download and in-flight-wake vetoes; the download one is the guarantee an operator most needs stated, since parking cancels a download. Renderer construction moved to offscreen-browser-renderer-attachment.ts — the backend crossed the max-lines budget, and building + wiring a renderer to its record was a coherent seam to split. --- docs/reference/headless-linux-server.md | 7 +- src/main/browser/browser-backend.ts | 3 + .../headless-browser-tab-listing.test.ts | 27 +++++ .../browser/headless-browser-tab-listing.ts | 20 +++- .../offscreen-browser-backend-reclaim.test.ts | 47 +++++++- src/main/browser/offscreen-browser-backend.ts | 101 +++++++----------- .../browser/offscreen-browser-open-pages.ts | 41 ++++++- .../offscreen-browser-page-reclaim.test.ts | 12 +++ .../browser/offscreen-browser-page-reclaim.ts | 8 +- .../offscreen-browser-renderer-attachment.ts | 78 ++++++++++++++ ...a-runtime-browser-parked-targeting.test.ts | 30 +++++- src/main/runtime/orca-runtime-browser.ts | 4 +- src/main/runtime/orca-runtime.ts | 3 +- ...headless-serve-browser-tab-reclaim.spec.ts | 9 +- 14 files changed, 306 insertions(+), 84 deletions(-) create mode 100644 src/main/browser/offscreen-browser-renderer-attachment.ts diff --git a/docs/reference/headless-linux-server.md b/docs/reference/headless-linux-server.md index 0b7fce6881d..b6ab31c7e90 100644 --- a/docs/reference/headless-linux-server.md +++ b/docs/reference/headless-linux-server.md @@ -292,8 +292,9 @@ 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, while its -initial load is still running, or while it is waiting on a certificate -decision. A stream ending counts as use, so a viewer who closes the pane gets +initial load is still running, while it is waiting on a certificate decision, +while a wake is still rebuilding it, or while it is writing an active +download — parking cancels a download, so an active one always vetoes it. A stream ending counts as use, so a viewer who closes the pane gets the full idle window before the page parks, even if they only watched. A navigation that has still not finished by the load timeout is deliberately reclaimable — otherwise one stalled page per create could hold a renderer forever, which is the failure this exists to prevent; waking it simply @@ -882,7 +883,7 @@ refuse to run there and print the command to run on the machine you want. - Clients cannot connect: make sure `--pairing-address` is an address reachable from the client, and make sure firewalls allow the selected `--port`. - Journal shows `Another Orca instance is already running for this userData -profile` and the unit exits `3`: another process already owns the profile, so + profile` and the unit exits `3`: another process already owns the profile, so `RestartPreventExitStatus=3` leaves the unit `failed` on purpose. Find the owner with `systemctl status orca-serve` and `pgrep -af orca`. Stop it (or keep it and leave the unit down), then run diff --git a/src/main/browser/browser-backend.ts b/src/main/browser/browser-backend.ts index e06b7f54b2e..5d8ae3b91c7 100644 --- a/src/main/browser/browser-backend.ts +++ b/src/main/browser/browser-backend.ts @@ -45,6 +45,9 @@ export type BrowserBackend = { wakeTab?(browserPageId: string): Promise /** Pages this backend still owns whose renderer is currently reclaimed. */ listParkedPages?(worktreeId?: string): ParkedBrowserPage[] + /** Open page ids in creation order, resident or parked — listings sort by + * this so parking never renumbers a tab index the caller already read. */ + listOpenPageIds?(worktreeId?: string): string[] /** 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 d04580b2fbe..0b3e42c2152 100644 --- a/src/main/browser/headless-browser-tab-listing.test.ts +++ b/src/main/browser/headless-browser-tab-listing.test.ts @@ -68,6 +68,33 @@ describe('mergeParkedBrowserTabs', () => { expect(merged.filter((tab) => tab.active).map((tab) => tab.browserPageId)).toEqual(['b']) }) + it('orders by creation so parking never renumbers an index the caller read', () => { + // Why: the bridge lists by registration order, which a park/wake cycle + // mutates (a woken tab re-registers at the end). A tab created first must + // stay at index 0 whether or not its renderer is currently reclaimed. + const merged = mergeParkedBrowserTabs([live('b', 0, true)], [parked('a')], ['a', 'b']) + expect(merged.map((tab) => [tab.browserPageId, tab.index, tab.parked === true])).toEqual([ + ['a', 0, true], + ['b', 1, false] + ]) + }) + + it('reindexes an all-live listing when the creation order disagrees', () => { + const merged = mergeParkedBrowserTabs([live('b', 0), live('a', 1, true)], [], ['a', 'b']) + expect(merged.map((tab) => [tab.browserPageId, tab.index])).toEqual([ + ['a', 0], + ['b', 1] + ]) + }) + + it('keeps ids missing from the creation order at their merged position', () => { + const merged = mergeParkedBrowserTabs([live('x', 0, true)], [parked('a')], ['a']) + expect(merged.map((tab) => [tab.browserPageId, tab.index])).toEqual([ + ['a', 0], + ['x', 1] + ]) + }) + 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 }]) diff --git a/src/main/browser/headless-browser-tab-listing.ts b/src/main/browser/headless-browser-tab-listing.ts index e2b1037a0bf..4b7b6e60512 100644 --- a/src/main/browser/headless-browser-tab-listing.ts +++ b/src/main/browser/headless-browser-tab-listing.ts @@ -9,9 +9,14 @@ import type { ParkedBrowserPage } from './browser-backend' export function mergeParkedBrowserTabs( live: readonly BrowserTabInfo[], - parked: readonly ParkedBrowserPage[] + parked: readonly ParkedBrowserPage[], + // Why: the bridge lists by registration order, which parking and waking both + // mutate (a woken tab re-registers at the end). Ordering by the page book's + // creation order instead keeps every tab's index stable across a park — an + // index the caller read must never be renumbered by a background timer. + creationOrder?: readonly string[] ): BrowserTabInfo[] { - if (parked.length === 0) { + if (parked.length === 0 && !creationOrder) { return [...live] } // Why: parking clears the bridge's active pointer, which then lands on a @@ -37,5 +42,16 @@ export function mergeParkedBrowserTabs( profileId: page.profileId ?? null }) } + if (!creationOrder) { + return merged + } + const rank = new Map(creationOrder.map((browserPageId, index) => [browserPageId, index])) return merged + .map((tab, index) => ({ tab, index })) + .sort((left, right) => { + const leftRank = rank.get(left.tab.browserPageId) ?? creationOrder.length + left.index + const rightRank = rank.get(right.tab.browserPageId) ?? creationOrder.length + right.index + return leftRank - rightRank + }) + .map(({ tab }, index) => ({ ...tab, index })) } diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index e1bd24f83a6..d36f4ff2983 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -160,7 +160,8 @@ function createHarness( onProcessSwap: vi.fn(async (browserPageId: string, webContentsId: number) => { order.push(`process-swap:${browserPageId}:${webContentsId}`) }), - isActiveBrowserPage: (browserPageId: string) => state.activePageId === browserPageId + isActiveBrowserPage: (browserPageId: string) => state.activePageId === browserPageId, + getActivePageId: () => state.activePageId } as unknown as AgentBrowserBridge const backend = new OffscreenBrowserBackend(manager, { @@ -729,6 +730,50 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(claimed.map((page) => page.browserPageId)).toEqual(['b']) }) + it('hands the active flag to a survivor when the parked holder closes', async () => { + // Why: closing a parked page has no bridge teardown to promote a successor, + // so without this the worktree's every remaining tab reports inactive and a + // paired client loses its one-selected-tab assumption. + const h = createHarness({ activePageId: 'b' }) + 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() + h.activePageId = undefined + + await h.backend.closeTab('b') + + expect( + h.backend.listParkedPages('wt-1').map((page) => [page.browserPageId, page.active === true]) + ).toEqual([['a', true]]) + }) + + it('scopes the parked active claim to its worktree, undefined included', async () => { + // Why: a remote `tab create` without --worktree makes a worktree-less page; + // parking it while active must not wipe another worktree's parked flag. + const h = createHarness({ activePageId: 'w' }) + await h.backend.createTab({ url: 'https://w', browserPageId: 'w', worktreeId: 'wt-1' }) + await h.backend.createTab({ url: 'https://g', browserPageId: 'g' }) + h.clock.value += 120_000 + await h.backend.wakeTab('g') + await h.backend.reclaimIdlePages() + h.activePageId = 'g' + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect( + h.backend.listParkedPages('wt-1').map((page) => [page.browserPageId, page.active === true]) + ).toEqual([['w', true]]) + expect( + h.backend + .listParkedPages() + .filter((page) => page.active) + .map((page) => page.browserPageId) + .sort() + ).toEqual(['g', 'w']) + }) + 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". diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index a75d18519ff..f0c7e90e374 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -1,5 +1,4 @@ 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' @@ -11,8 +10,8 @@ import { OffscreenBrowserOpenPages, type OffscreenBrowserPage } from './offscreen-browser-open-pages' +import { materializeOffscreenBrowserRenderer } from './offscreen-browser-renderer-attachment' import { - createOffscreenBrowserWindow, loadOffscreenBrowserUrl, OFFSCREEN_BROWSER_WAKE_LOAD_BUDGET_MS } from './offscreen-browser-window' @@ -117,6 +116,13 @@ export class OffscreenBrowserBackend implements BrowserBackend { const page = this.pages.delete(browserPageId) ?? null this.waking.delete(browserPageId) await this.releaseRenderer(page, browserPageId) + // Why: the closed page may have carried the scope's only active claim — + // either the parked-active flag, or a live pointer whose teardown found no + // other live tab to promote. A worktree with open pages and no active tab + // breaks the one-selected-tab assumption every paired client renders on. + if (page && !this.options.getAgentBrowserBridge?.()?.getActivePageId(page.worktreeId)) { + this.pages.promoteParkedActive(page.worktreeId) + } // Why: closing changes both residency and rank, and with the last page gone // this arms nothing at all. this.reclaimer.reschedule() @@ -207,6 +213,11 @@ export class OffscreenBrowserBackend implements BrowserBackend { return this.pages.listParked(worktreeId) } + /** Open page ids in creation order — the stable order listings sort by. */ + listOpenPageIds(worktreeId?: string): string[] { + return this.pages.openIds(worktreeId) + } + /** The parked page a page-less command should target in this worktree. */ getParkedPageIdForImplicitTarget(worktreeId?: string): string | null { return this.pages.parkedIdForImplicitTarget(worktreeId) @@ -315,70 +326,30 @@ 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() + materializeOffscreenBrowserRenderer(page, { + isDeliberateTeardown: (browserPageId) => this.releasing.has(browserPageId), + onRendererLost: (webContentsId) => { + this.pages.delete(page.browserPageId) + // Why: an unprompted renderer loss must reclaim the helper session too, + // or a crash loop leaks one session, CDP proxy and listening port per + // page — the same leak the deliberate teardown path fixes. + void this.options + .getAgentBrowserBridge?.() + ?.onTabClosed(webContentsId) + .catch(() => {}) + this.browserManager.unregisterGuest(page.browserPageId) + this.reclaimer.reschedule() + }, + registerGuest: (webContentsId) => { + const profile = page.profileId ? browserSessionRegistry.getProfile(page.profileId) : null + this.browserManager.registerOffscreenGuest({ + browserPageId: page.browserPageId, + worktreeId: page.worktreeId, + sessionProfileId: page.profileId ?? null, + userAgentMode: profile?.userAgentMode, + webContentsId + }) } - 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. - // 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 recordAddress = (url: string): void => { - if (page.window === win && url && !url.startsWith('chrome-error://')) { - page.url = url - } - } - // 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. - win.webContents.once('destroyed', () => { - if (this.releasing.has(page.browserPageId) || page.window !== win) { - return - } - this.pages.delete(page.browserPageId) - // Why: an unprompted renderer loss must reclaim the helper session too, or - // a crash loop leaks one session, CDP proxy and listening port per page — - // the same leak the deliberate teardown path fixes. - void this.options - .getAgentBrowserBridge?.() - ?.onTabClosed(webContentsId) - .catch(() => {}) - this.browserManager.unregisterGuest(page.browserPageId) - this.reclaimer.reschedule() - }) - const profile = page.profileId ? browserSessionRegistry.getProfile(page.profileId) : null - this.browserManager.registerOffscreenGuest({ - browserPageId: page.browserPageId, - worktreeId: page.worktreeId, - sessionProfileId: page.profileId ?? null, - userAgentMode: profile?.userAgentMode, - webContentsId }) } diff --git a/src/main/browser/offscreen-browser-open-pages.ts b/src/main/browser/offscreen-browser-open-pages.ts index 5cf74760504..dd8f73c7a07 100644 --- a/src/main/browser/offscreen-browser-open-pages.ts +++ b/src/main/browser/offscreen-browser-open-pages.ts @@ -73,6 +73,17 @@ export class OffscreenBrowserOpenPages { ) } + /** + * Every open page's id in creation order, resident or parked. Listings order + * by this so a tab's position never moves because its renderer was reclaimed + * — an index read minutes ago must not be renumbered by a background timer. + */ + openIds(worktreeId?: string): string[] { + return this.all() + .filter((page) => !worktreeId || page.worktreeId === worktreeId) + .map((page) => page.browserPageId) + } + listParked(worktreeId?: string): ParkedBrowserPage[] { return this.parked(worktreeId).map((page) => ({ browserPageId: page.browserPageId, @@ -105,12 +116,36 @@ export class OffscreenBrowserOpenPages { return best?.browserPageId ?? null } - /** Only one page per worktree may carry the active flag across a park. */ + /** + * Only one page per worktree may carry the active flag across a park. + * Strict scope: a worktree-less page clears only other worktree-less pages — + * an undefined filter would wipe every worktree's flag host-wide. + */ claimParkedActive(browserPageId: string, worktreeId?: string): void { - for (const page of this.parked(worktreeId)) { - if (page.browserPageId !== browserPageId) { + for (const page of this.parked()) { + if (page.browserPageId !== browserPageId && page.worktreeId === worktreeId) { page.activeWhenParked = false } } } + + /** + * Hand the active flag to the most recently used parked page when nothing + * in the scope holds it. Closing the flag's holder (or the last live tab) + * must not leave a worktree whose every page reports inactive — a paired + * client assumes one selected browser tab per worktree. + */ + promoteParkedActive(worktreeId?: string): void { + const scoped = this.parked().filter((page) => page.worktreeId === worktreeId) + if (scoped.length === 0 || scoped.some((page) => page.activeWhenParked)) { + return + } + let best = scoped[0] + for (const page of scoped) { + if (page.lastActivityAt > best.lastActivityAt) { + best = page + } + } + best.activeWhenParked = true + } } diff --git a/src/main/browser/offscreen-browser-page-reclaim.test.ts b/src/main/browser/offscreen-browser-page-reclaim.test.ts index 9a85f47aa32..39c168416ec 100644 --- a/src/main/browser/offscreen-browser-page-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-page-reclaim.test.ts @@ -73,6 +73,18 @@ describe('readOffscreenBrowserRetentionBudget', () => { process.env.ORCA_HEADLESS_BROWSER_RESIDENT_LIMIT = 'lots' expect(readOffscreenBrowserRetentionBudget().limit).toBe(OFFSCREEN_BROWSER_RESIDENT_LIMIT) }) + + it('reads scientific notation whole, not truncated to its mantissa', () => { + // Why: parseInt('1e9') is 1 — an operator following the docs' "set the + // idle window very high" advice would get a 1ms window instead. + process.env.ORCA_HEADLESS_BROWSER_PARK_IDLE_MS = '1e9' + expect(readOffscreenBrowserRetentionBudget().idleMs).toBe(1_000_000_000) + }) + + it('falls back on trailing garbage instead of truncating it', () => { + process.env.ORCA_HEADLESS_BROWSER_PARK_IDLE_MS = '1234abc' + expect(readOffscreenBrowserRetentionBudget().idleMs).toBe(OFFSCREEN_BROWSER_IDLE_PARK_MS) + }) }) describe('nextOffscreenBrowserReclaimCheckAt', () => { diff --git a/src/main/browser/offscreen-browser-page-reclaim.ts b/src/main/browser/offscreen-browser-page-reclaim.ts index 8bd5d69a4f9..f14b11742d7 100644 --- a/src/main/browser/offscreen-browser-page-reclaim.ts +++ b/src/main/browser/offscreen-browser-page-reclaim.ts @@ -28,10 +28,14 @@ export const OFFSCREEN_BROWSER_PINNED_RECHECK_MS = OFFSCREEN_BROWSER_PARK_GRACE_ function readPositiveIntEnv(name: string, fallback: number): number { const raw = process.env[name] - if (raw === undefined) { + if (raw === undefined || raw.trim() === '') { return fallback } - const parsed = Number.parseInt(raw, 10) + // Why Number, not parseInt: parseInt('1e9') is 1, silently turning the + // documented "set the idle window very high" advice into a 1ms window. + // Number() consumes the whole string, so '1e9' parses fully and trailing + // garbage falls back instead of truncating. + const parsed = Number(raw) return Number.isInteger(parsed) && parsed >= 0 ? parsed : fallback } diff --git a/src/main/browser/offscreen-browser-renderer-attachment.ts b/src/main/browser/offscreen-browser-renderer-attachment.ts new file mode 100644 index 00000000000..458e204c795 --- /dev/null +++ b/src/main/browser/offscreen-browser-renderer-attachment.ts @@ -0,0 +1,78 @@ +import type { BrowserWindow } from 'electron' +import type { OffscreenBrowserPage } from './offscreen-browser-open-pages' +import { createOffscreenBrowserWindow } from './offscreen-browser-window' + +// Why (STA-4341): building a page's renderer and wiring it to the page record +// is one transaction — the window, the address tracking that keeps the record +// wakeable, the loss handler that cleans up a crash, and the guest +// registration. The backend owns what happens on those events; this module +// owns that they are attached to every renderer identically, whether it came +// from a create or a wake. + +export type OffscreenBrowserRendererHooks = { + /** A teardown the owner started on purpose — the loss handler stands down. */ + isDeliberateTeardown: (browserPageId: string) => boolean + /** The renderer died out from under the owner (crash, app teardown). */ + onRendererLost: (webContentsId: number) => void + /** Map the fresh WebContents into the guest registries. */ + registerGuest: (webContentsId: number) => void +} + +export function materializeOffscreenBrowserRenderer( + page: OffscreenBrowserPage, + hooks: OffscreenBrowserRendererHooks +): void { + const win = createOffscreenBrowserWindow(page.partition) + try { + attachWindow(page, win, hooks) + } 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 + } +} + +function attachWindow( + page: OffscreenBrowserPage, + win: BrowserWindow, + hooks: OffscreenBrowserRendererHooks +): 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. + // 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 recordAddress = (url: string): void => { + if (page.window === win && url && !url.startsWith('chrome-error://')) { + page.url = url + } + } + // 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), + // report the loss so commands fail cleanly instead of resolving a dead + // WebContents. Parking destroys it deliberately, so it opts out here. + win.webContents.once('destroyed', () => { + if (hooks.isDeliberateTeardown(page.browserPageId) || page.window !== win) { + return + } + hooks.onRendererLost(webContentsId) + }) + hooks.registerGuest(webContentsId) +} 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 cfef8428379..0951a26654c 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -50,7 +50,13 @@ type FakeHeadlessHost = { closedPageIds: string[] } -function createFakeHeadlessHost(live: string[], parked: string[]): FakeHeadlessHost { +function createFakeHeadlessHost( + live: string[], + parked: string[], + // Why a separate order: creation order is what the real page book carries; + // live-then-parked only coincides with it until something parks. + creationOrder: string[] = [...live, ...parked] +): FakeHeadlessHost { const state: FakeHeadlessHost = { host: null as unknown as RuntimeBrowserCommandHost, livePageIds: [...live], @@ -109,7 +115,11 @@ function createFakeHeadlessHost(live: string[], parked: string[]): FakeHeadlessH url: `https://example.test/${browserPageId}`, title: browserPageId })), - getParkedPageIdForImplicitTarget: () => state.parkedPageIds.at(-1) ?? null + getParkedPageIdForImplicitTarget: () => state.parkedPageIds.at(-1) ?? null, + listOpenPageIds: () => + creationOrder.filter( + (id) => state.livePageIds.includes(id) || state.parkedPageIds.includes(id) + ) } as unknown as BrowserBackend state.host = { @@ -145,6 +155,22 @@ describe('headless parked-page targeting', () => { expect(fake.wakeCalls).toEqual([]) }) + it('keeps a tab index stable when an older tab parks', async () => { + // Why: pre-reclaim, headless indices moved only on create/close. A listing + // ordered by residency would let a background park renumber an index the + // agent read minutes earlier, silently retargeting its next command. + const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') + const fake = createFakeHeadlessHost(['live-b'], ['parked-a'], ['parked-a', 'live-b']) + const commands = new RuntimeBrowserCommands(fake.host) + + const listed = await commands.browserTabList({ worktree: 'id:wt-1' }) + + expect(listed.tabs.map((tab) => [tab.browserPageId, tab.index, tab.parked === true])).toEqual([ + ['parked-a', 0, true], + ['live-b', 1, false] + ]) + }) + it('switches to the page the listed index named, not one an implicit wake promoted', async () => { // Why: with every page parked, resolving the worktree first wakes the most // recently used one, which makes it live and moves it to listing index 0. diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 763a745f34c..580ed826e1d 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -658,9 +658,11 @@ export class RuntimeBrowserCommands { private listBrowserTabsIncludingParked( worktreeId: string | undefined ): BrowserTabListResult['tabs'] { + const offscreen = this.host.getOffscreenBrowserBackend() const merged = mergeParkedBrowserTabs( this.requireAgentBrowserBridge().tabList(worktreeId).tabs, - this.host.getOffscreenBrowserBackend()?.listParkedPages?.(worktreeId) ?? [] + offscreen?.listParkedPages?.(worktreeId) ?? [], + offscreen?.listOpenPageIds?.(worktreeId) ) return merged.map((tab) => tab.parked diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 2d32fc02c73..4e303c9156e 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -33491,7 +33491,8 @@ export class OrcaRuntimeService { } return mergeParkedBrowserTabs( this.agentBrowserBridge.tabList(worktreeId).tabs, - this.offscreenBrowserBackend?.listParkedPages?.(worktreeId) ?? [] + this.offscreenBrowserBackend?.listParkedPages?.(worktreeId) ?? [], + this.offscreenBrowserBackend?.listOpenPageIds?.(worktreeId) ) } diff --git a/tests/e2e/headless-serve-browser-tab-reclaim.spec.ts b/tests/e2e/headless-serve-browser-tab-reclaim.spec.ts index 5b200e734e1..0888c45b1f9 100644 --- a/tests/e2e/headless-serve-browser-tab-reclaim.spec.ts +++ b/tests/e2e/headless-serve-browser-tab-reclaim.spec.ts @@ -238,11 +238,12 @@ test('resolves --index against the listing that includes parked tabs', async ({ const before = await listTabs(host, worktreeId) expect(before).toHaveLength(5) - // Why: parked pages sit after the resident ones, so the last index is the - // one the bridge's live-only index handling would never have reached. - const targetIndex = before.length - 1 + // Why: the listing is creation-ordered, so parked and resident tabs are + // interleaved. Target a parked one — an index the bridge's live-only index + // handling would either miss or resolve to a different (live) tab. + const targetIndex = before.findIndex((tab) => tab.parked === true) + expect(targetIndex).toBeGreaterThanOrEqual(0) const targeted = before[targetIndex] - expect(targeted.parked).toBe(true) await host.client.call('browser.tabClose', { index: targetIndex,