From e7563c63f1ff882a564322929d79ecd88e8565ab Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 6 Sep 2026 18:50:15 -0700 Subject: [PATCH] fix: fence browser recovery to attach inventory placements (#18910) * fix: fence browser recovery to attach inventory placements * refactor: name the attach-inventory fence and make its test deterministic Extract the placement check into isPlacedAsObservedAtAttach so the recovery filter stays a flat list of named predicates, and document that omitting pagePlacementsAtAttach recovers against unfenced live state. Replace the 30-microtask drain in the post-attach regression with the handler's own completion: attach only settles after recovery returns, so awaiting the dispatch orders the assertions instead of guessing at a microtask count. Verified by forcing the fence open: both regressions fail (the post-attach one in 60ms on a retired placement) and the other 27 still pass. * test: settle the attach handler even when the regression fails early The barrier ran inline, so a waitFor timeout or the placement guard left the attach handler parked on a promise nothing awaited. Hoist it into settleAttach and call it from a finally as well; cleanup is guarded and the dispatch promise is already settled, so the second call is a no-op. --- ...rowser-client-host-attach-adoption.test.ts | 46 +++++++++++++++++++ .../rpc/methods/browser-client-host.ts | 7 +++ ...ntime-browser-client-page-recovery.test.ts | 19 ++++++++ .../runtime-browser-client-page-recovery.ts | 20 ++++++++ 4 files changed, 92 insertions(+) diff --git a/src/main/runtime/rpc/methods/browser-client-host-attach-adoption.test.ts b/src/main/runtime/rpc/methods/browser-client-host-attach-adoption.test.ts index 51fa6304d2d..0d6bf3b80ef 100644 --- a/src/main/runtime/rpc/methods/browser-client-host-attach-adoption.test.ts +++ b/src/main/runtime/rpc/methods/browser-client-host-attach-adoption.test.ts @@ -183,6 +183,52 @@ describe('browser.clientHost.attach adoption', () => { await rig.dispatch }) + it('does not recover a page created after the attach inventory was captured', async () => { + let releaseAdoption!: (value: BrowserExecutionHostKeyResolution) => void + const route = new Promise((resolve) => { + releaseAdoption = resolve + }) + const resolveExecutionHostKey = vi.fn(() => route) + const rig = attachHost([orphanedPage()], { resolveExecutionHostKey }) + const settleAttach = async (): Promise => { + rig.cleanups.get(`browser-client-host:${HOST_CLIENT_ID}`)?.() + await rig.dispatch + } + try { + await vi.waitFor(() => expect(resolveExecutionHostKey).toHaveBeenCalled()) + const authority = getBrowserHostLeaseRegistry(rig.hostRuntime) + const pages = getRuntimeBrowserPageRegistry(rig.hostRuntime) + const placement = authority.placeClientPage('page-created-after-attach', HOST_CLIENT_ID) + if (placement.kind !== 'client') { + throw new Error('expected client placement') + } + pages.publishClientPage({ + browserPageId: 'page-created-after-attach', + workspaceId: WORKSPACE_ID, + browserProfileId: 'default', + executionHostKey: EXECUTION_HOST_KEY, + placement, + pairedDeviceId: 'device-a', + url: 'https://remote.internal/new', + loading: false, + active: true + }) + releaseAdoption({ status: 'resolved', executionHostKey: EXECUTION_HOST_KEY }) + await vi.waitFor(() => expect(rig.markClientHostedPagesReconciled).toHaveBeenCalled()) + // Attach only settles once recovery has returned, so this is the barrier the assertions need: + // draining microtasks would let a regression slip through as a not-yet-issued command. + await settleAttach() + + expect(authority.getPlacement('page-created-after-attach')).toEqual(placement) + expect(pages.getPage('page-created-after-attach')).toMatchObject({ placement, active: true }) + expect( + rig.commands().filter((event) => event.browserPageId === 'page-created-after-attach') + ).toEqual([]) + } finally { + await settleAttach() + } + }) + it('does not re-enter recovery for a page it just adopted', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) const rig = attachHost([orphanedPage({ browserPageId: 'page-d' })], { diff --git a/src/main/runtime/rpc/methods/browser-client-host.ts b/src/main/runtime/rpc/methods/browser-client-host.ts index 5126a870d24..525fd4fde96 100644 --- a/src/main/runtime/rpc/methods/browser-client-host.ts +++ b/src/main/runtime/rpc/methods/browser-client-host.ts @@ -39,6 +39,12 @@ export const BROWSER_CLIENT_HOST_METHODS: RpcAnyMethod[] = [ } const registry = getBrowserHostLeaseRegistry(runtime) + // Attach inventory cannot describe pages created or replaced after readiness is published. + const pagePlacementsAtAttach = new Map( + getRuntimeBrowserPageRegistry(runtime) + .listPages() + .map((page) => [page.browserPageId, page.placement]) + ) const handle = registry.attach({ browserHostClientId: params.browserHostClientId, connectionId, @@ -127,6 +133,7 @@ export const BROWSER_CLIENT_HOST_METHODS: RpcAnyMethod[] = [ lease: handle.lease, authority: registry, pages: getRuntimeBrowserPageRegistry(runtime), + pagePlacementsAtAttach, notifyWorkspace: (workspaceId) => runtime.notifyMobileSessionTabsChanged(workspaceId), releaseUnrecoverablePage: (page) => releaseRuntimeBrowserClientPageRecord(runtime, page.browserPageId, page.placement), diff --git a/src/main/runtime/runtime-browser-client-page-recovery.test.ts b/src/main/runtime/runtime-browser-client-page-recovery.test.ts index 4845493e1c0..133b02aaf66 100644 --- a/src/main/runtime/runtime-browser-client-page-recovery.test.ts +++ b/src/main/runtime/runtime-browser-client-page-recovery.test.ts @@ -43,6 +43,25 @@ describe('runtime browser client page recovery', () => { expect(notifyWorkspace).toHaveBeenCalledOnce() }) + it('does not apply an attach inventory to a replacement placed after capture', async () => { + const { authority, commands, notifyWorkspace, pages, placements } = harness() + const pagePlacementsAtAttach = new Map([['page-a', oldPlacement]]) + pages.replaceClientPagePlacement('page-a', oldPlacement, newPlacement) + placements.set('page-a', newPlacement) + await recoverUnavailableRuntimeBrowserClientPages({ + lease: lease([]), + authority, + pages, + notifyWorkspace, + pagePlacementsAtAttach + }) + expect(authority.getPlacement('page-a')).toEqual(newPlacement) + expect(pages.getPage('page-a')?.placement).toEqual(newPlacement) + expect(authority.createClientPage).not.toHaveBeenCalled() + expect(commands).toEqual([]) + expect(notifyWorkspace).not.toHaveBeenCalled() + }) + it('retains an exact active generation without commands or metadata churn', async () => { const { authority, commands, notifyWorkspace, pages } = harness() diff --git a/src/main/runtime/runtime-browser-client-page-recovery.ts b/src/main/runtime/runtime-browser-client-page-recovery.ts index 8390db8686a..ad0f062f03e 100644 --- a/src/main/runtime/runtime-browser-client-page-recovery.ts +++ b/src/main/runtime/runtime-browser-client-page-recovery.ts @@ -45,6 +45,8 @@ export async function recoverUnavailableRuntimeBrowserClientPages(options: { } authority: RecoveryAuthority pages: RuntimeBrowserPageRegistry + /** Placements as of the attach inventory. Omitting it recovers against unfenced live state. */ + pagePlacementsAtAttach?: ReadonlyMap notifyWorkspace(workspaceId: string): void /** Drops a page whose placement recovery destroyed without replacing it. */ releaseUnrecoverablePage?: (page: RuntimeBrowserClientPage) => void @@ -77,6 +79,7 @@ export async function recoverUnavailableRuntimeBrowserClientPages(options: { .listPages() .filter( (page) => + isPlacedAsObservedAtAttach(page, options.pagePlacementsAtAttach) && !options.adoptedPageIds?.has(page.browserPageId) && isRecoverableByLease(page, options.lease) && !isActiveExactPage(page, inventoryByPageId.get(page.browserPageId), options.lease) @@ -101,6 +104,23 @@ export async function recoverUnavailableRuntimeBrowserClientPages(options: { ) } +/** + * Whether the attach inventory can still speak for this page. + * + * Readiness is published before recovery runs, so the client can place a page the inventory predates + * -- and absence from the inventory means "recreate". Those are left to the attach that can see them. + */ +function isPlacedAsObservedAtAttach( + page: RuntimeBrowserClientPage, + pagePlacementsAtAttach: ReadonlyMap | undefined +): boolean { + if (!pagePlacementsAtAttach) { + return true + } + const observed = pagePlacementsAtAttach.get(page.browserPageId) + return observed !== undefined && sameRuntimeBrowserPlacement(observed, page.placement) +} + /** * Whether this lease is the one allowed to take a page back. *