mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 00:03:52 +00:00
fix(browser): stop a stale sweep decision from reclaiming a page back in use
Round two, lifecycle. The sweep picks its victims once and then parks them one at a time, awaiting each helper-session teardown. That await is long enough for a page later in the list to be woken and driven, and the selection was being treated as a licence rather than a proposal: - A page used while an earlier park was in flight was parked anyway, destroying the renderer and session underneath a running command. - The retention close had the same shape and was worse: it ignored pins entirely, so a page woken mid-sweep could be deleted outright. - A wake is resident but not yet loading while it awaits the process swap, so it was neither pinned nor loading — a concurrent sweep could park a page in the middle of rebuilding it. Each page is now re-checked against live state immediately before its renderer goes, and a wake in progress pins the page for its duration. Also makes `tab current` count as using the page it names. Every other page-targeting command refreshes the reclaim clock; this one resolved the active id directly and skipped it, so an agent polling `tab current` could have that very page reclaimed under it. All four mutation-checked.
This commit is contained in:
@@ -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<typeof vi.fn> }
|
||||
bridge.onTabClosed.mockImplementationOnce(
|
||||
async () => new Promise<void>((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<typeof vi.fn> }
|
||||
|
||||
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<void>((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<typeof vi.fn> }
|
||||
bridge.onProcessSwap.mockImplementationOnce(
|
||||
async () => new Promise<void>((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' })
|
||||
|
||||
@@ -185,41 +185,45 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
|
||||
/** Park every page the policy no longer wants resident. Exposed for tests. */
|
||||
async reclaimIdlePages(): Promise<string[]> {
|
||||
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<void> {
|
||||
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<void> {
|
||||
const page = this.pages.get(browserPageId)
|
||||
if (!page?.window || page.window.isDestroyed()) {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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) }
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user