From 3d50a886c70a3dc8bd8bbe1cb854c4da92a097ea Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 19:00:36 -0400 Subject: [PATCH] refactor(browser): match the desktop guest budget instead of inventing policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Headless reclamation now has exactly the policy surface the desktop guest budget has, and nothing more. Dropped the retained-page limit and its outright close. Desktop destroys guests but never removes a tab, so closing a page past a count had no precedent here and was the one genuinely novel product decision in this change — it permanently destroyed a page id an agent still held. Parked records are ~200 bytes against the 220-480 MB renderers being reclaimed, so retaining them indefinitely is the right trade and matches what desktop already does. Dropped the download veto's progress window with it. Desktop treats an active download as work in progress for as long as it stays active, and headless now does the same. That accepts the same unbounded-veto shape desktop has: a download that never completes keeps its page resident. Deliberate, and the alternative was a threshold desktop does not have. What stays is the part with precedent: a resident cap of 4 (the desktop limit), LRU eviction, and the idle clock headless needs because it has no notion of a visible worktree. --- docs/reference/headless-linux-server.md | 16 ++-- src/main/browser/browser-manager.ts | 25 ++---- .../offscreen-browser-backend-reclaim.test.ts | 79 +------------------ src/main/browser/offscreen-browser-backend.ts | 4 +- .../offscreen-browser-page-reclaim.test.ts | 25 +----- .../browser/offscreen-browser-page-reclaim.ts | 44 ----------- .../offscreen-browser-page-reclaimer.ts | 46 ++--------- 7 files changed, 29 insertions(+), 210 deletions(-) diff --git a/docs/reference/headless-linux-server.md b/docs/reference/headless-linux-server.md index 3eed97535c1..67db9309469 100644 --- a/docs/reference/headless-linux-server.md +++ b/docs/reference/headless-linux-server.md @@ -294,18 +294,18 @@ initial load is still running, or while it is waiting on a certificate decision. 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 -retries the address. +retries the address. Parking never closes a page: the page stays open until +something closes it, exactly as on the desktop. Two evictors decide what stays resident, mirroring terminal pane parking. The cap is the primary one: -| Variable | Default | Meaning | -| ------------------------------------------ | -------- | ---------------------------------------------------------------------- | -| `ORCA_HEADLESS_BROWSER_RESIDENT_LIMIT` | `4` | Renderers kept resident. Least-recently-used pages park past this. | -| `ORCA_HEADLESS_BROWSER_PARK_IDLE_MS` | `300000` | A page parks after this long untouched, even under the cap. | -| `ORCA_HEADLESS_BROWSER_PARK_GRACE_MS` | `30000` | A page is never parked this soon after its last command. | -| `ORCA_HEADLESS_BROWSER_PARK_SWEEP_MS` | `15000` | How often the server re-evaluates residency. | -| `ORCA_HEADLESS_BROWSER_MAX_RETAINED_PAGES` | `100` | Open pages retained before the oldest parked ones are closed outright. | +| Variable | Default | Meaning | +| -------------------------------------- | -------- | ------------------------------------------------------------------ | +| `ORCA_HEADLESS_BROWSER_RESIDENT_LIMIT` | `4` | Renderers kept resident. Least-recently-used pages park past this. | +| `ORCA_HEADLESS_BROWSER_PARK_IDLE_MS` | `300000` | A page parks after this long untouched, even under the cap. | +| `ORCA_HEADLESS_BROWSER_PARK_GRACE_MS` | `30000` | A page is never parked this soon after its last command. | +| `ORCA_HEADLESS_BROWSER_PARK_SWEEP_MS` | `15000` | How often the server re-evaluates residency. | Raise the limit on a host with memory to spare, or set both the limit and the idle window very high to keep every page resident — at the cost this diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 0932ead11ac..427358cbe59 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -201,8 +201,6 @@ type ActiveDownload = { savePath: string reservationKey: string | null receivedBytes: number - /** Last time Electron reported progress; the liveness signal a reclaimer needs. */ - lastProgressAt: number transientState: BrowserDownloadProgressEvent['state'] terminalEvent: BrowserDownloadFinishedEvent | null startedSent: boolean @@ -1474,23 +1472,18 @@ export class BrowserManager { : (this.loadErrorsByGuestId.get(webContentsId) ?? null) } - /** - * When this page's in-flight downloads last made progress, or null when it has - * none. Why (STA-4341): unregisterGuest cancels a tab's in-flight downloads, so - * a reclaimer must not take a page that is still writing one — the desktop guest - * budget vetoes eviction for the same reason. A timestamp rather than a boolean - * because a download that stalls forever must not pin a renderer forever; the - * caller owns that bound. - */ - getBrowserPageActiveDownloadProgressAt(browserPageId: string): number | null { - let latest: number | null = null + // Why (STA-4341): unregisterGuest cancels a tab's in-flight downloads, so a + // reclaimer must not take a page that is still writing one. Mirrors the + // desktop guest budget's veto (browser-page-download-activity.ts) exactly, + // including its unbounded nature: a download is treated as work in progress + // for as long as it stays active. + hasActiveBrowserPageDownload(browserPageId: string): boolean { for (const download of this.downloadsById.values()) { if (download.browserTabId === browserPageId && !download.terminalEvent) { - latest = - latest === null ? download.lastProgressAt : Math.max(latest, download.lastProgressAt) + return true } } - return latest + return false } getBrowserPageCertificateFailure(browserPageId: string): BrowserCertificateFailure | null { @@ -1638,7 +1631,6 @@ export class BrowserManager { savePath: fallbackSavePath, reservationKey: destination?.reservationKey ?? null, receivedBytes: 0, - lastProgressAt: Date.now(), transientState: null, terminalEvent: null, startedSent: false, @@ -1680,7 +1672,6 @@ export class BrowserManager { const updatedHandler = (_event: Electron.Event, state: 'progressing' | 'interrupted'): void => { download.receivedBytes = this.getDownloadReceivedBytes(download.item) - download.lastProgressAt = Date.now() download.transientState = state this.sendDownloadProgress(download.browserTabId, { browserPageId: download.browserTabId ?? undefined, diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index 920dd7135fa..d90a61e097c 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -96,7 +96,6 @@ type Harness = { pagesChanged: (string | undefined)[] certificateFailurePageIds: Set downloadingPageIds: Set - downloadProgressAt: number } function createHarness( @@ -112,7 +111,6 @@ function createHarness( activePageId: overrides.activePageId, certificateFailurePageIds: new Set(overrides.certificateFailurePageIds ?? []), downloadingPageIds: new Set(overrides.downloadingPageIds ?? []), - downloadProgressAt: 1_000_000, pagesChanged: [] as (string | undefined)[] } const order: string[] = [] @@ -145,8 +143,8 @@ function createHarness( getBrowserPageLoadError: () => overrides.loadError ?? null, getBrowserPageCertificateFailure: (browserPageId: string) => state.certificateFailurePageIds.has(browserPageId) ? { challengeId: 'c' } : null, - getBrowserPageActiveDownloadProgressAt: (browserPageId: string) => - state.downloadingPageIds.has(browserPageId) ? state.downloadProgressAt : null + hasActiveBrowserPageDownload: (browserPageId: string) => + state.downloadingPageIds.has(browserPageId) } as unknown as BrowserManager const bridge = { @@ -181,10 +179,7 @@ function createHarness( return state.pagesChanged }, certificateFailurePageIds: state.certificateFailurePageIds, - downloadingPageIds: state.downloadingPageIds, - set downloadProgressAt(value: number) { - state.downloadProgressAt = value - } + downloadingPageIds: state.downloadingPageIds } } @@ -195,7 +190,6 @@ beforeEach(() => { process.env.ORCA_HEADLESS_BROWSER_PARK_IDLE_MS = '60000' process.env.ORCA_HEADLESS_BROWSER_PARK_GRACE_MS = '5000' process.env.ORCA_HEADLESS_BROWSER_PARK_SWEEP_MS = '100000' - process.env.ORCA_HEADLESS_BROWSER_MAX_RETAINED_PAGES = '100' }) describe('OffscreenBrowserBackend reclamation', () => { @@ -330,43 +324,6 @@ describe('OffscreenBrowserBackend reclamation', () => { 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. @@ -428,14 +385,13 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(h.pagesChanged).toEqual(['wt-1']) }) - it('does not park a page whose download is still advancing', async () => { + it('does not park a page that is still writing a download', async () => { // Why: releasing a renderer unregisters its guest, and browser-manager // cancels that page's in-flight downloads on unregister. The desktop guest // budget vetoes eviction for the same reason. const h = createHarness({ downloadingPageIds: ['a'] }) await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) h.clock.value += 120_000 - h.downloadProgressAt = h.clock.value expect(await h.backend.reclaimIdlePages()).toEqual([]) @@ -443,18 +399,6 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(await h.backend.reclaimIdlePages()).toEqual(['a']) }) - it('parks a page whose download has stalled so the cap cannot be defeated', async () => { - // Why: a download that never progresses again would otherwise pin its - // renderer forever, and one stalled download per page would defeat the - // resident cap outright — the same shape as the load-pin bug before it. - const h = createHarness({ downloadingPageIds: ['a'] }) - await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) - h.downloadProgressAt = h.clock.value - h.clock.value += 120_000 - - expect(await h.backend.reclaimIdlePages()).toEqual(['a']) - }) - 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. @@ -824,21 +768,6 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(await h.backend.wakeTab('a')).toBe(false) }) - it('closes the oldest parked pages once too many records are retained', async () => { - // Why: parking bounds renderer processes, not the page records behind them. - process.env.ORCA_HEADLESS_BROWSER_MAX_RETAINED_PAGES = '2' - const h = createHarness() - for (const id of ['a', 'b', 'c', 'd']) { - await h.backend.createTab({ url: `https://example.test/${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(['c', 'd']) - expect(h.windows.every((win) => win.isDestroyed())).toBe(true) - }) - it('scopes parked listings and implicit targeting to a worktree', async () => { const h = createHarness() await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index d7e1359aa11..7527879714c 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -48,8 +48,8 @@ export class OffscreenBrowserBackend implements BrowserBackend { isWaking: (browserPageId) => this.waking.has(browserPageId), hasCertificateChallenge: (browserPageId) => this.browserManager.getBrowserPageCertificateFailure(browserPageId) !== null, - activeDownloadProgressAt: (browserPageId) => - this.browserManager.getBrowserPageActiveDownloadProgressAt(browserPageId), + hasActiveDownload: (browserPageId) => + this.browserManager.hasActiveBrowserPageDownload(browserPageId), isHostPinned: (browserPageId) => this.options.isPagePinned?.(browserPageId) === true, park: (browserPageId) => this.parkPage(browserPageId), close: (browserPageId) => this.closeTab(browserPageId), diff --git a/src/main/browser/offscreen-browser-page-reclaim.test.ts b/src/main/browser/offscreen-browser-page-reclaim.test.ts index 110357290bb..17a83da58f5 100644 --- a/src/main/browser/offscreen-browser-page-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-page-reclaim.test.ts @@ -1,6 +1,5 @@ import { describe, expect, it } from 'vitest' import { - selectOffscreenBrowserPagesToClose, selectOffscreenBrowserPagesToPark, type OffscreenBrowserReclaimCandidate, type OffscreenBrowserReclaimPolicy @@ -12,8 +11,7 @@ const POLICY: OffscreenBrowserReclaimPolicy = { residentLimit: 2, idleParkMs: 60_000, parkGraceMs: 5_000, - sweepIntervalMs: 1_000, - retainedPageLimit: 100 + sweepIntervalMs: 1_000 } function page( @@ -98,24 +96,3 @@ describe('selectOffscreenBrowserPagesToPark', () => { ).toEqual([]) }) }) - -describe('selectOffscreenBrowserPagesToClose', () => { - it('retains every parked page under the limit', () => { - expect(selectOffscreenBrowserPagesToClose([page('a', 1), page('b', 2)], 2, POLICY)).toEqual([]) - }) - - it('closes the least recently used parked pages down to the limit', () => { - // Why: parking bounds renderer processes, not the records behind them. - const parked = [page('new', 1_000), page('old', 90_000), page('older', 99_000)] - expect( - selectOffscreenBrowserPagesToClose(parked, 5, { ...POLICY, retainedPageLimit: 3 }) - ).toEqual(['older', 'old']) - }) - - it('never closes a pinned page', () => { - const parked = [page('pinned', 99_000, true), page('idle', 90_000)] - expect( - selectOffscreenBrowserPagesToClose(parked, 4, { ...POLICY, retainedPageLimit: 2 }) - ).toEqual(['idle']) - }) -}) diff --git a/src/main/browser/offscreen-browser-page-reclaim.ts b/src/main/browser/offscreen-browser-page-reclaim.ts index 77b7891c43f..3dd69682139 100644 --- a/src/main/browser/offscreen-browser-page-reclaim.ts +++ b/src/main/browser/offscreen-browser-page-reclaim.ts @@ -16,22 +16,12 @@ export const OFFSCREEN_BROWSER_PARK_GRACE_MS = 30_000 /** How often the backend re-evaluates which pages should be resident. */ export const OFFSCREEN_BROWSER_SWEEP_INTERVAL_MS = 15_000 -/** Parked page records retained before the oldest are closed outright. */ -export const OFFSCREEN_BROWSER_RETAINED_PAGE_LIMIT = 100 -/** - * How long a download keeps its page resident after its last progress. Parking - * cancels a page's in-flight downloads, so an advancing one must veto — but a - * download that stalls forever must not, or one stalled download per page would - * defeat the resident cap outright. - */ -export const OFFSCREEN_BROWSER_DOWNLOAD_VETO_MS = 60_000 export type OffscreenBrowserReclaimPolicy = { residentLimit: number idleParkMs: number parkGraceMs: number sweepIntervalMs: number - retainedPageLimit: number } export type OffscreenBrowserReclaimCandidate = { @@ -67,13 +57,6 @@ export function readOffscreenBrowserReclaimPolicy(): OffscreenBrowserReclaimPoli sweepIntervalMs: Math.max( 100, readPositiveIntEnv('ORCA_HEADLESS_BROWSER_PARK_SWEEP_MS', OFFSCREEN_BROWSER_SWEEP_INTERVAL_MS) - ), - retainedPageLimit: Math.max( - 1, - readPositiveIntEnv( - 'ORCA_HEADLESS_BROWSER_MAX_RETAINED_PAGES', - OFFSCREEN_BROWSER_RETAINED_PAGE_LIMIT - ) ) } } @@ -123,30 +106,3 @@ export function selectOffscreenBrowserPagesToPark( return parkable.map((page) => page.browserPageId).filter((id) => parked.has(id)) } - -/** - * Pick parked pages to close outright. Parking bounds renderer processes but - * not the records behind them, so an agent that opens pages forever and closes - * none would still grow the backend without limit. Only parked pages are - * eligible and only the least recently used go, so this can never take a page - * an agent is still working with. - */ -export function selectOffscreenBrowserPagesToClose( - parkedPages: readonly OffscreenBrowserReclaimCandidate[], - totalPageCount: number, - policy: OffscreenBrowserReclaimPolicy -): string[] { - const excess = totalPageCount - policy.retainedPageLimit - if (excess <= 0) { - return [] - } - return parkedPages - .filter((page) => !page.pinned) - .sort( - (left, right) => - left.lastActivityAt - right.lastActivityAt || - left.browserPageId.localeCompare(right.browserPageId) - ) - .slice(0, excess) - .map((page) => page.browserPageId) -} diff --git a/src/main/browser/offscreen-browser-page-reclaimer.ts b/src/main/browser/offscreen-browser-page-reclaimer.ts index 581a5fb81c6..73a2cb44861 100644 --- a/src/main/browser/offscreen-browser-page-reclaimer.ts +++ b/src/main/browser/offscreen-browser-page-reclaimer.ts @@ -3,8 +3,6 @@ import type { OffscreenBrowserPage } from './offscreen-browser-open-pages' import { - OFFSCREEN_BROWSER_DOWNLOAD_VETO_MS, - selectOffscreenBrowserPagesToClose, selectOffscreenBrowserPagesToPark, type OffscreenBrowserReclaimPolicy } from './offscreen-browser-page-reclaim' @@ -26,8 +24,8 @@ export type OffscreenBrowserReclaimerDeps = { isWaking: (browserPageId: string) => boolean /** A certificate challenge the page is blocked on. */ hasCertificateChallenge: (browserPageId: string) => boolean - /** When the page's in-flight downloads last advanced, null when it has none. */ - activeDownloadProgressAt: (browserPageId: string) => number | null + /** A download the page is still writing. */ + hasActiveDownload: (browserPageId: string) => boolean /** A client streaming the page, or a command in flight against it. */ isHostPinned: (browserPageId: string) => boolean park: (browserPageId: string) => Promise @@ -63,7 +61,6 @@ export class OffscreenBrowserPageReclaimer { await this.deps.park(browserPageId) parked.push(browserPageId) } - await this.closeOverRetainedPages() return parked } @@ -100,30 +97,6 @@ export class OffscreenBrowserPageReclaimer { this.sweepInFlight = null } - // 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 doomed = selectOffscreenBrowserPagesToClose( - this.deps.pages.parked().map((page) => this.toCandidate(page)), - this.deps.pages.size, - this.deps.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.deps.pages.get(browserPageId) - if (!page || page.window || !this.isSafeToReclaim(browserPageId)) { - continue - } - console.warn( - `[offscreen-browser] closing parked page ${browserPageId}: more than ${this.deps.policy.retainedPageLimit} pages are open` - ) - await this.deps.close(browserPageId) - } - } - // Why: a page is off limits while anything depends on its renderer — a client // streaming it, a command in flight, its first navigation still committing, a // wake still rebuilding it, a certificate decision it is blocked on, or a @@ -137,21 +110,14 @@ export class OffscreenBrowserPageReclaimer { // Why: a challenge id dies with the renderer, so parking would discard // both the warning and the ability to approve it. this.deps.hasCertificateChallenge(page.browserPageId) || - this.hasAdvancingDownload(page.browserPageId) || + // Why: releasing a renderer unregisters its guest, and that cancels the + // page's in-flight downloads. Mirrors the desktop guest budget's veto + // (browser-guest-worktree-retention.ts). + this.deps.hasActiveDownload(page.browserPageId) || this.deps.isHostPinned(page.browserPageId) ) } - // Why: releasing a renderer unregisters its guest, and that cancels the page's - // in-flight downloads — the desktop guest budget vetoes eviction for the same - // reason (browser-guest-worktree-retention.ts). Bounded by progress, because a - // download that stalls forever would otherwise pin its renderer forever, and - // one stalled download per page would defeat the resident cap outright. - private hasAdvancingDownload(browserPageId: string): boolean { - const progressAt = this.deps.activeDownloadProgressAt(browserPageId) - return progressAt !== null && this.deps.now() - progressAt < OFFSCREEN_BROWSER_DOWNLOAD_VETO_MS - } - private toCandidate(page: OffscreenBrowserPage): { browserPageId: string lastActivityAt: number