mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 00:03:52 +00:00
revert(browser): drop the park abort; keep the bounded download veto
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.
This commit is contained in:
@@ -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<typeof vi.fn> }
|
||||
bridge.onTabClosed.mockImplementationOnce(async () => {
|
||||
await new Promise<void>((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.
|
||||
|
||||
@@ -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<string, Promise<boolean>>()
|
||||
private readonly releasing = new Map<string, Promise<void>>()
|
||||
private readonly waking = new Map<string, Promise<boolean>>()
|
||||
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<boolean> {
|
||||
browserPageId: string
|
||||
): Promise<void> {
|
||||
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<boolean> {
|
||||
browserPageId: string
|
||||
): Promise<void> {
|
||||
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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user