From 2b309dfb5adb8dd26fb8afb0578bd0da0ecb4f05 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 17:50:49 -0400 Subject: [PATCH] revert(browser): drop the park abort; keep the bounded download veto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round eight. Both reviewers independently found the same defect in the abort I added last commit, and chasing it showed the abort was the wrong trade. releaseRenderer's pending branch discarded the boolean, so a `tab close` that joined an aborted park reported success while the renderer was deliberately left alive — orphaning the window, its guest mapping and its download, and arming a stale destroyed-handler that could unregister a page reusing the id. Two more followed from aborting after a destructive step: onTabClosed has already destroyed the helper session (losing the page's intercept patterns) and already moved the worktree's active pointer to a fallback, and the abort restored neither. So an aborted park left a resident page with reset automation state that implicit commands no longer routed to. All three exist only because the abort backs out halfway through a teardown that is not reversible. The veto is re-checked immediately before teardown starts; what remains is the narrow window during the helper-session await, where a download that begins is cancelled exactly as it would be if the tab were closed at that instant. That is the same decision-time-only guarantee the desktop guest budget gives, and it is now stated in the code. The bounded progress veto — the actual round-seven fix — stays. --- .../offscreen-browser-backend-reclaim.test.ts | 25 ----------- src/main/browser/offscreen-browser-backend.ts | 42 +++++++------------ .../offscreen-browser-page-reclaimer.ts | 4 +- 3 files changed, 18 insertions(+), 53 deletions(-) diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index 23a0199fcdc..920dd7135fa 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -455,31 +455,6 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(await h.backend.reclaimIdlePages()).toEqual(['a']) }) - it('gives up a park when the page starts a download during teardown', async () => { - // Why: teardown awaits the helper session while the page keeps running, and - // the unregister that follows would cancel a download begun in that window. - let releaseTeardown = (): void => {} - const h = createHarness() - await h.backend.createTab({ url: 'https://a', browserPageId: 'a' }) - const bridge = h.bridge as unknown as { onTabClosed: ReturnType } - bridge.onTabClosed.mockImplementationOnce(async () => { - await new Promise((resolve) => (releaseTeardown = resolve)) - // The page began downloading while its helper session was torn down. - h.downloadingPageIds.add('a') - h.downloadProgressAt = h.clock.value - }) - - h.clock.value += 120_000 - const sweep = h.backend.reclaimIdlePages() - releaseTeardown() - await sweep - - // The page kept its renderer and its registration rather than losing the write. - expect(h.backend.listParkedPages()).toEqual([]) - expect(h.windows[0].isDestroyed()).toBe(false) - expect(h.registered.get('a')).toBe(h.windows[0].webContents.id) - }) - 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. diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index 94e5d1e274d..d7e1359aa11 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -33,7 +33,7 @@ export class OffscreenBrowserBackend implements BrowserBackend { /** Renderer teardowns this backend initiated, keyed by page. Park and close * both destroy the window on purpose, so the crash handler stands down for * them — and a wake waits on one rather than racing it. */ - private readonly releasing = new Map>() + private readonly releasing = new Map>() private readonly waking = new Map>() private readonly reclaimer: OffscreenBrowserPageReclaimer @@ -244,17 +244,14 @@ export class OffscreenBrowserBackend implements BrowserBackend { this.options .getAgentBrowserBridge?.() ?.isActiveBrowserPage(browserPageId, page.worktreeId) === true - // Why the abort: teardown awaits the helper session, and the page keeps - // running throughout. A download started inside that window would be - // cancelled by the unregister that follows, so give up the park instead. - // The session is already gone, but the next command recreates it. - const released = await this.releaseRenderer(page, browserPageId, () => - this.reclaimer.hasAdvancingDownload(browserPageId) - ) - if (!released) { - page.activeWhenParked = false - return - } + // Why no abort once this starts: the veto is re-checked immediately before + // this call, but teardown then awaits the helper session while the page + // keeps running, so a download begun inside that window is still cancelled + // by the unregister below — exactly as it would be if the tab were closed + // at that instant. Backing out mid-teardown is worse: onTabClosed has + // already destroyed the helper session and moved the worktree's active + // pointer, so an abort leaves a resident page with reset automation state. + await this.releaseRenderer(page, browserPageId) page.window = null if (page.activeWhenParked) { // Why: teardown promotes another live tab to active, which then parks @@ -266,21 +263,19 @@ export class OffscreenBrowserBackend implements BrowserBackend { // Why: teardown order matters — the bridge must destroy the helper session and // detach its debugger while the WebContents is still alive and still mapped, // or the session, its CDP proxy and its listening port outlive the page. - /** Resolves false when abortAfterSessionTeardown vetoed the release. */ private async releaseRenderer( page: OffscreenBrowserPage | null, - browserPageId: string, - abortAfterSessionTeardown?: () => boolean - ): Promise { + browserPageId: string + ): Promise { const pending = this.releasing.get(browserPageId) if (pending) { await pending - return true + return } - const release = this.runReleaseRenderer(page, browserPageId, abortAfterSessionTeardown) + const release = this.runReleaseRenderer(page, browserPageId) this.releasing.set(browserPageId, release) try { - return await release + await release } finally { if (this.releasing.get(browserPageId) === release) { this.releasing.delete(browserPageId) @@ -290,22 +285,17 @@ export class OffscreenBrowserBackend implements BrowserBackend { private async runReleaseRenderer( page: OffscreenBrowserPage | null, - browserPageId: string, - abortAfterSessionTeardown?: () => boolean - ): Promise { + browserPageId: string + ): Promise { const bridge = this.options.getAgentBrowserBridge?.() ?? null const webContentsId = this.browserManager.getGuestWebContentsId(browserPageId) if (bridge && webContentsId != null) { await bridge.onTabClosed(webContentsId) } - if (abortAfterSessionTeardown?.() === true) { - return false - } this.browserManager.unregisterGuest(browserPageId) if (page?.window && !page.window.isDestroyed()) { page.window.destroy() } - return true } private materialize(page: OffscreenBrowserPage): void { diff --git a/src/main/browser/offscreen-browser-page-reclaimer.ts b/src/main/browser/offscreen-browser-page-reclaimer.ts index abc023c7f20..581a5fb81c6 100644 --- a/src/main/browser/offscreen-browser-page-reclaimer.ts +++ b/src/main/browser/offscreen-browser-page-reclaimer.ts @@ -130,7 +130,7 @@ export class OffscreenBrowserPageReclaimer { // download it is still writing. `loading` is bounded by the load helper's own // timeout, so it cannot hold a renderer forever; a navigation still pending // past that timeout is deliberately parkable, and waking retries the address. - isPinned(page: OffscreenBrowserPage): boolean { + private isPinned(page: OffscreenBrowserPage): boolean { return ( page.loading || this.deps.isWaking(page.browserPageId) || @@ -147,7 +147,7 @@ export class OffscreenBrowserPageReclaimer { // 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. - hasAdvancingDownload(browserPageId: string): boolean { + private hasAdvancingDownload(browserPageId: string): boolean { const progressAt = this.deps.activeDownloadProgressAt(browserPageId) return progressAt !== null && this.deps.now() - progressAt < OFFSCREEN_BROWSER_DOWNLOAD_VETO_MS }