diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index 6b1a4b3ed50..f3cb1f3686a 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -267,6 +267,92 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(await h.backend.reclaimIdlePages()).toEqual(['a']) }) + it('does not park a page that was used while an earlier park was in flight', async () => { + // Why: parking awaits helper-session teardown, so a page later in the + // selection can be woken and driven before its turn comes. The selection is + // a proposal, not a licence. + let releaseFirstTeardown = (): void => {} + const h = createHarness() + for (const id of ['a', 'b', 'c', 'd']) { + await h.backend.createTab({ url: `https://${id}`, browserPageId: id }) + h.clock.value += 1_000 + } + h.clock.value += 10_000 + const bridge = h.bridge as unknown as { onTabClosed: ReturnType } + bridge.onTabClosed.mockImplementationOnce( + async () => new Promise((resolve) => (releaseFirstTeardown = resolve)) + ) + + const sweep = h.backend.reclaimIdlePages() + await Promise.resolve() + // b is next in line to park; a command lands on it while a is tearing down. + await h.backend.wakeTab('b') + releaseFirstTeardown() + + expect(await sweep).toEqual(['a']) + expect(h.backend.listParkedPages().map((page) => page.browserPageId)).toEqual(['a']) + }) + + it('does not close a parked page that was woken while an earlier close was in flight', async () => { + let releaseFirstTeardown = (): void => {} + process.env.ORCA_HEADLESS_BROWSER_MAX_RETAINED_PAGES = '2' + // Why: pinning the two newer pages keeps the park loop empty, so the sweep + // that blocks is the retention close — the path under test. + const h = createHarness({ pinned: new Set(['c', 'd']) }) + const bridge = h.bridge as unknown as { onTabClosed: ReturnType } + + for (const id of ['a', 'b']) { + await h.backend.createTab({ url: `https://${id}`, browserPageId: id }) + h.clock.value += 1_000 + } + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + expect(h.backend.listParkedPages().map((page) => page.browserPageId)).toEqual(['a', 'b']) + + // Two more push the retention budget over, dooming the two oldest parked. + for (const id of ['c', 'd']) { + await h.backend.createTab({ url: `https://${id}`, browserPageId: id }) + h.clock.value += 1_000 + } + h.clock.value += 120_000 + bridge.onTabClosed.mockImplementationOnce( + async () => new Promise((resolve) => (releaseFirstTeardown = resolve)) + ) + + const sweep = h.backend.reclaimIdlePages() + await Promise.resolve() + await h.backend.wakeTab('b') + releaseFirstTeardown() + await sweep + + // a was closed; b survived the stale close list and is resident again. + expect(await h.backend.wakeTab('a')).toBe(false) + expect(await h.backend.wakeTab('b')).toBe(true) + }) + + it('does not park a page whose wake is still rebuilding it', async () => { + // Why: a wake is resident but not yet loading while it awaits the process + // swap, so without an explicit pin the sweep can destroy it mid-rebuild. + let releaseSwap = (): void => {} + const h = createHarness() + await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + const bridge = h.bridge as unknown as { onProcessSwap: ReturnType } + bridge.onProcessSwap.mockImplementationOnce( + async () => new Promise((resolve) => (releaseSwap = resolve)) + ) + const wake = h.backend.wakeTab('a') + await Promise.resolve() + h.clock.value += 120_000 + + expect(await h.backend.reclaimIdlePages()).toEqual([]) + + releaseSwap() + expect(await wake).toBe(true) + }) + 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 b8bca8cfc76..a5403e12ed1 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -185,41 +185,45 @@ export class OffscreenBrowserBackend implements BrowserBackend { /** Park every page the policy no longer wants resident. Exposed for tests. */ async reclaimIdlePages(): Promise { - const now = this.now() const resident = this.pages.resident().filter((page) => !this.releasing.has(page.browserPageId)) const doomed = selectOffscreenBrowserPagesToPark( - resident.map((page) => ({ - browserPageId: page.browserPageId, - lastActivityAt: page.lastActivityAt, - // Why: a page still committing its first navigation has nothing worth - // keeping yet and would be woken straight back to the same address. - pinned: page.loading || this.options.isPagePinned?.(page.browserPageId) === true - })), - now, + resident.map((page) => this.toReclaimCandidate(page)), + this.now(), this.policy ) + const parked: string[] = [] for (const browserPageId of doomed) { + // Why: parking awaits the helper session's teardown, so a page later in + // this list can be woken and driven while an earlier one is still being + // torn down. The selection is a proposal, not a licence — re-check each + // page against the live state before destroying its renderer. + if (!this.isSafeToReclaim(browserPageId)) { + continue + } await this.parkPage(browserPageId) + parked.push(browserPageId) } await this.closeOverRetainedPages() - return doomed + return parked } // Why: parking bounds renderer processes, not the records behind them. An // agent that opens pages forever and closes none would still grow the backend // without limit, so the oldest parked pages are eventually closed outright. private async closeOverRetainedPages(): Promise { - const parked = this.pages.parked() const doomed = selectOffscreenBrowserPagesToClose( - parked.map((page) => ({ - browserPageId: page.browserPageId, - lastActivityAt: page.lastActivityAt, - pinned: this.options.isPagePinned?.(page.browserPageId) === true - })), + this.pages.parked().map((page) => this.toReclaimCandidate(page)), this.pages.size, this.policy ) for (const browserPageId of doomed) { + // Why: closing is destructive and awaits teardown, so a page selected + // here can be woken before its turn comes. Only close one that is still + // parked and still unwanted. + const page = this.pages.get(browserPageId) + if (!page || page.window || !this.isSafeToReclaim(browserPageId)) { + continue + } console.warn( `[offscreen-browser] closing parked page ${browserPageId}: more than ${this.policy.retainedPageLimit} pages are open` ) @@ -227,6 +231,38 @@ export class OffscreenBrowserBackend implements BrowserBackend { } } + private toReclaimCandidate(page: OffscreenBrowserPage): { + browserPageId: string + lastActivityAt: number + pinned: boolean + } { + return { + browserPageId: page.browserPageId, + lastActivityAt: page.lastActivityAt, + pinned: this.isPagePinned(page) + } + } + + // Why: a page is off limits while anything is depending on its renderer — + // a client streaming it, a command in flight, its first navigation still + // committing, or a wake still rebuilding it. The wake case matters because a + // wake is resident but not yet loading while it awaits the process swap. + private isPagePinned(page: OffscreenBrowserPage): boolean { + return ( + page.loading || + this.waking.has(page.browserPageId) || + this.options.isPagePinned?.(page.browserPageId) === true + ) + } + + private isSafeToReclaim(browserPageId: string): boolean { + const page = this.pages.get(browserPageId) + if (!page || this.releasing.has(browserPageId)) { + return false + } + return !this.isPagePinned(page) && this.now() - page.lastActivityAt >= this.policy.parkGraceMs + } + private async parkPage(browserPageId: string): Promise { const page = this.pages.get(browserPageId) if (!page?.window || page.window.isDestroyed()) { 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 971f97bdb3e..45cfbe2c7b5 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -184,6 +184,18 @@ describe('headless parked-page targeting', () => { expect(fake.closedPageIds).toEqual(['parked-c']) }) + it('counts `tab current` as using the page it names', async () => { + // Why: without this an agent could poll `tab current` continuously and + // still have that very page reclaimed under it. + const { RuntimeBrowserCommands } = await import('./orca-runtime-browser') + const fake = createFakeHeadlessHost(['live-a'], []) + const commands = new RuntimeBrowserCommands(fake.host) + + await commands.browserTabCurrent({ worktree: 'id:wt-1' }) + + expect(fake.wakeCalls).toEqual(['live-a']) + }) + 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. diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 0731799c32e..2fe3bac8a42 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -737,6 +737,10 @@ export class RuntimeBrowserCommands { if (!browserPageId) { throw new BrowserError('browser_no_tab', 'No browser tab open in this worktree') } + // Why (STA-4341): this names a page like any other page-targeting command, + // so it has to count as using it. Without this an agent could poll `tab + // current` continuously and still have that very page reclaimed under it. + await this.markHeadlessBrowserPageUsed(browserPageId) return { tab: this.describeBrowserTab(browserPageId, worktreeId) } }