diff --git a/docs/reference/headless-linux-server.md b/docs/reference/headless-linux-server.md index b6ab31c7e90..d234a6dc0f7 100644 --- a/docs/reference/headless-linux-server.md +++ b/docs/reference/headless-linux-server.md @@ -293,8 +293,10 @@ 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, 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 +or while a wake is still rebuilding it. An active download vetoes the park +decision — parking cancels downloads — but a download that begins during the +teardown itself is still cancelled, exactly as if the tab had been closed at +that instant. 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 diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index b08c2a043be..fca5112baa9 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -817,6 +817,35 @@ describe('OffscreenBrowserBackend reclamation', () => { .map((page) => page.browserPageId) .sort() ).toEqual(['g', 'w']) + // Why: with one flag per scope, a scope-less implicit command must target + // the page that held the MOST RECENT active claim — not the oldest scope's + // page just because it comes first in creation order. + expect(h.backend.getParkedPageIdForImplicitTarget()).toBe('g') + }) + + it('promotes for a scope even while another worktree has a resident page', async () => { + // Why: the promotion gate is per scope. A live tab elsewhere on the host + // says nothing about this worktree's selection. + const h = createHarness({ activePageId: 'holder' }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + h.clock.value += 1_000 + await h.backend.createTab({ + url: 'https://holder', + browserPageId: 'holder', + worktreeId: 'wt-1' + }) + h.clock.value += 1_000 + await h.backend.createTab({ url: 'https://other', browserPageId: 'other', worktreeId: 'wt-2' }) + h.clock.value += 120_000 + await h.backend.wakeTab('other') + await h.backend.reclaimIdlePages() + h.activePageId = undefined + + await h.backend.closeTab('holder') + + expect( + h.backend.listParkedPages('wt-1').map((page) => [page.browserPageId, page.active === true]) + ).toEqual([['a', true]]) }) it('targets the page that was active, not merely the most recently used', async () => { diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index f0c7e90e374..d1ff66a06f2 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -120,7 +120,12 @@ export class OffscreenBrowserBackend implements BrowserBackend { // 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)) { + // Gate on the book, not the bridge: getActivePageId resolves through + // resolveActiveTab, which reassigns the active pointers as a side effect, + // and its undefined scope means "anywhere" while the promotion is strict. + // A scope holding a resident page needs no promotion — the live listing + // marks one of its tabs active itself. + if (page && !this.pages.resident().some((open) => open.worktreeId === page.worktreeId)) { this.pages.promoteParkedActive(page.worktreeId) } // Why: closing changes both residency and rank, and with the last page gone diff --git a/src/main/browser/offscreen-browser-open-pages.ts b/src/main/browser/offscreen-browser-open-pages.ts index dd8f73c7a07..daf0af953c3 100644 --- a/src/main/browser/offscreen-browser-open-pages.ts +++ b/src/main/browser/offscreen-browser-open-pages.ts @@ -104,16 +104,22 @@ export class OffscreenBrowserOpenPages { * tie when none is recorded. */ parkedIdForImplicitTarget(worktreeId?: string): string | null { + // Why MRU among the flagged: the claim is strict per scope, so a scoped + // query sees at most one flag — but the scope-less query spans every + // worktree and can see one flag per scope. Recency picks the page that + // actually held the most recent active claim, matching the bridge's own + // global-pointer semantics; creation order would pick the oldest scope. + let flagged: OffscreenBrowserPage | null = null let best: OffscreenBrowserPage | null = null for (const page of this.parked(worktreeId)) { - if (page.activeWhenParked) { - return page.browserPageId + if (page.activeWhenParked && (!flagged || page.lastActivityAt > flagged.lastActivityAt)) { + flagged = page } if (!best || page.lastActivityAt > best.lastActivityAt) { best = page } } - return best?.browserPageId ?? null + return (flagged ?? best)?.browserPageId ?? null } /** diff --git a/src/main/browser/offscreen-browser-page-reclaim.ts b/src/main/browser/offscreen-browser-page-reclaim.ts index f14b11742d7..732cb944221 100644 --- a/src/main/browser/offscreen-browser-page-reclaim.ts +++ b/src/main/browser/offscreen-browser-page-reclaim.ts @@ -36,7 +36,13 @@ function readPositiveIntEnv(name: string, fallback: number): number { // 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 + if (!Number.isInteger(parsed) || parsed < 0) { + // Why the warning: parseInt used to truncate '60000ms' to 60000; falling + // back silently would change such a knob's behavior with no signal. + console.warn(`[offscreen-browser] ignoring invalid ${name}=${raw}`) + return fallback + } + return parsed } export function readOffscreenBrowserRetentionBudget(): BrowserRetentionBudget { diff --git a/src/main/browser/offscreen-browser-page-reclaimer-scheduling.test.ts b/src/main/browser/offscreen-browser-page-reclaimer-scheduling.test.ts index a9086fd4ab5..f94b38f7da2 100644 --- a/src/main/browser/offscreen-browser-page-reclaimer-scheduling.test.ts +++ b/src/main/browser/offscreen-browser-page-reclaimer-scheduling.test.ts @@ -38,6 +38,7 @@ function createReclaimer( pages?: OffscreenBrowserPage[] pinned?: readonly string[] park?: (browserPageId: string, live: Map) => Promise + budget?: BrowserRetentionBudget } = {} ) { const live = new Map( @@ -51,7 +52,7 @@ function createReclaimer( } as unknown as OffscreenBrowserOpenPages const reclaimer = new OffscreenBrowserPageReclaimer({ pages, - budget: BUDGET, + budget: args.budget ?? BUDGET, isReleasing: () => false, isWaking: () => false, hasCertificateChallenge: () => false, @@ -201,6 +202,23 @@ describe('OffscreenBrowserPageReclaimer scheduling', () => { expect(attempts).toBeLessThanOrEqual(11) }) + it('caps the armed delay below the setTimeout ceiling', () => { + // Why: Node clamps a delay past 2^31-1ms to 1ms — a documented "set the + // idle window very high" configuration would otherwise spin the sweep at + // 1ms forever on an idle host. + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') + const { reclaimer } = createReclaimer({ + pages: [page('a', 1_000_000)], + budget: { ...BUDGET, idleMs: 100_000_000_000 } + }) + reclaimer.reschedule() + + expect(reclaimer.isScheduled).toBe(true) + const delays = setTimeoutSpy.mock.calls.map((call) => call[1] as number) + expect(delays.length).toBeGreaterThan(0) + expect(Math.max(...delays)).toBeLessThanOrEqual(2_147_483_647) + }) + it('arms nothing when stop lands while a sweep is in flight', async () => { // Why: stop() clears sweepInFlight, so without a terminal flag the sweep's // own re-arm no longer sees itself in flight and schedules a fresh check diff --git a/src/main/browser/offscreen-browser-page-reclaimer.ts b/src/main/browser/offscreen-browser-page-reclaimer.ts index 4dc1cc9faf0..ed3569c6229 100644 --- a/src/main/browser/offscreen-browser-page-reclaimer.ts +++ b/src/main/browser/offscreen-browser-page-reclaimer.ts @@ -17,6 +17,9 @@ import { nextOffscreenBrowserReclaimCheckAt } from './offscreen-browser-page-rec // timer for the moment its own answer could next change. There is deliberately // no recurring sweep: a host doing nothing holds no timer at all. +/** Node's setTimeout ceiling; longer delays are clamped by Node to 1ms. */ +const MAX_RECLAIM_TIMER_DELAY_MS = 2_147_483_647 + export type OffscreenBrowserReclaimerDeps = { pages: OffscreenBrowserOpenPages budget: BrowserRetentionBudget @@ -79,8 +82,13 @@ export class OffscreenBrowserPageReclaimer { } // Why the floor: a deadline in the past that the last sweep could not act on // would re-arm at 0ms, and a timer that re-arms itself instantly is a pegged - // CPU on an otherwise idle server. - const delay = Math.max(0, at - now, this.backoffUntil - now) + // CPU on an otherwise idle server. Why the ceiling: Node clamps a setTimeout + // beyond 2^31-1ms to 1ms — the same spin, reachable by configuring the idle + // window past ~24.8 days. Re-checking at the horizon is harmless. + const delay = Math.min( + Math.max(0, at - now, this.backoffUntil - now), + MAX_RECLAIM_TIMER_DELAY_MS + ) this.timer = setTimeout(() => { this.timer = null this.runSweep() 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 0951a26654c..fac1d6fd8a5 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -171,6 +171,22 @@ describe('headless parked-page targeting', () => { ]) }) + it('reports listing indices, not bridge registration order, from show and switch', async () => { + // Why: a park+wake permutes the bridge's registration order. The index a + // caller gets back must name the same tab in the listing they read, or + // chaining it into --index addresses a different tab. + const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') + const fake = createFakeHeadlessHost(['c', 'a'], [], ['a', 'c']) + const commands = new RuntimeBrowserCommands(fake.host) + + const current = await commands.browserTabCurrent({ worktree: 'id:wt-1' }) + expect(current.tab.browserPageId).toBe('c') + expect(current.tab.index).toBe(1) + + const switched = await commands.browserTabSwitch({ page: 'c', worktree: 'id:wt-1' }) + expect(switched).toMatchObject({ browserPageId: 'c', switched: 1 }) + }) + 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 580ed826e1d..6ab018ad858 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -655,6 +655,28 @@ export class RuntimeBrowserCommands { // it into opening yet another one, which is the leak this fixes. Every // index-addressed command resolves through this same list so `tab list` // indices and `--index` never disagree. + /** Headless serve with the parked-aware, creation-ordered listing. */ + private isHeadlessOffscreenListing(): boolean { + return ( + this.host.getOffscreenBrowserBackend()?.listOpenPageIds !== undefined && + !this.host.getAvailableAuthoritativeWindow() + ) + } + + /** The page's position in the creation-ordered listing; null on desktop. */ + private headlessListedTabIndex( + worktreeId: string | undefined, + browserPageId: string + ): number | null { + if (!this.isHeadlessOffscreenListing()) { + return null + } + const index = this.listBrowserTabsIncludingParked(worktreeId).findIndex( + (tab) => tab.browserPageId === browserPageId + ) + return index === -1 ? null : index + } + private listBrowserTabsIncludingParked( worktreeId: string | undefined ): BrowserTabListResult['tabs'] { @@ -770,12 +792,13 @@ export class RuntimeBrowserCommands { 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 + // Why (STA-4341): the bridge computes `switched` over its registration + // order, which any park+wake permutes. Report the target's position in the + // creation-ordered listing instead — the same number `tab list` prints and + // `--index` resolves against — or a caller chaining the reported number + // into `--index` addresses a different tab than the one it switched to. + const listedIndex = this.headlessListedTabIndex(target.worktreeId, switched.browserPageId) + const result = listedIndex === null ? switched : { ...switched, switched: listedIndex } 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 = @@ -1874,17 +1897,30 @@ export class RuntimeBrowserCommands { explicitWorktreeId?: string ): BrowserTabListResult['tabs'][number] { const worktreeId = explicitWorktreeId ?? browserManager.getWorktreeIdForTab(browserPageId) - const tab = this.requireAgentBrowserBridge() - .tabList(worktreeId) - .tabs.find((entry) => entry.browserPageId === browserPageId) - if (!tab) { - const scope = worktreeId ? ' in this worktree' : '' - throw new BrowserError( - 'browser_tab_not_found', - `Browser page ${browserPageId} was not found${scope}` + // Why (STA-4341): the bridge numbers tabs by registration order, which any + // park+wake permutes. Every index a headless caller sees must come from the + // creation-ordered listing `tab list` prints, or `tab show`/`tab current` + // report a number that names a different tab in that listing. + if (this.isHeadlessOffscreenListing()) { + const tab = this.listBrowserTabsIncludingParked(worktreeId).find( + (entry) => entry.browserPageId === browserPageId ) + if (tab) { + return tab + } + } else { + const tab = this.requireAgentBrowserBridge() + .tabList(worktreeId) + .tabs.find((entry) => entry.browserPageId === browserPageId) + if (tab) { + return this.enrichBrowserTabInfo(tab) + } } - return this.enrichBrowserTabInfo(tab) + const scope = worktreeId ? ' in this worktree' : '' + throw new BrowserError( + 'browser_tab_not_found', + `Browser page ${browserPageId} was not found${scope}` + ) } // Why: headless serve path — the offscreen backend registers synchronously, so there is no webview-mount wait.