From ba9238d9eeb6ff662180db80df37f5689e0ffa8a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 12 Oct 2026 13:10:52 -0700 Subject: [PATCH 1/3] fix(desktop-e2e): remove root causes of four retry-masked desktop flakes - packaged-smoke: remove the profile only after every process that can write it (the app's process group, Crashpad's handler) is gone; SIGKILLing main left helpers creating files during rmSync (ENOTEMPTY) - background-executor: answer shell dialogs by waiting for their window to close, since the dialog destroys itself before the click is acknowledged - background-executor A: replace the 1.5 s p95 wall-clock threshold with a gated check that every call is claimed while chat B's first command still holds its queue, within Sim's 15 s pickup window, and that held leases renew - driver: a click whose tab closes itself mid-dispatch (popup Allow -> window.close()) is a dispatched click, not a failed one, whichever of the close and the input acknowledgement lands first - page-dialogs / browser-focus / browser-tools: print the tool error and the shown dialog id when they fail, to root-cause the remaining flakes --- apps/desktop/e2e/background-executor.spec.ts | 61 +++++++++++------ apps/desktop/e2e/browser-focus.spec.ts | 2 +- apps/desktop/e2e/browser-page-dialogs.spec.ts | 9 ++- apps/desktop/e2e/browser-tools.spec.ts | 3 +- apps/desktop/e2e/executor-sim.ts | 20 ++++++ apps/desktop/e2e/packaged-smoke.spec.ts | 67 +++++++++++++++---- apps/desktop/src/main/browser-agent/driver.ts | 54 +++++++++++---- 7 files changed, 164 insertions(+), 52 deletions(-) diff --git a/apps/desktop/e2e/background-executor.spec.ts b/apps/desktop/e2e/background-executor.spec.ts index 6b01639f9de..d91b1bd8fdc 100644 --- a/apps/desktop/e2e/background-executor.spec.ts +++ b/apps/desktop/e2e/background-executor.spec.ts @@ -15,6 +15,7 @@ import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { recordCheck } from './check-report' import { + answerShellDialog, type FixtureCall, FixtureSim, launch, @@ -37,6 +38,9 @@ import { * BACKGROUND_EXECUTOR_REPORT_PATH as it finishes. */ +/** Sim's window for a device to claim an offered call (DESKTOP_TOOL_PICKUP_GRACE_MS in Sim). */ +const SIM_PICKUP_GRACE_MS = 15_000 + const CHAT_A = 'chat-browser-a' const CHAT_B = 'chat-terminal-b' const CHAT_C = 'chat-idle-c' @@ -89,6 +93,7 @@ test.describe('background executor', () => { await window.goto(`${sim.origin}/workspace/${WORKSPACE}/chat/${CHAT_C}`) const marker = join(userData, 'terminal-marker.txt') + const releaseCommands = join(userData, 'release-commands') const readable = join(userData, 'notes.txt') writeFileSync(readable, 'background read fixture') @@ -109,17 +114,41 @@ test.describe('background executor', () => { const terminalRuns = [1, 2, 3].map((n) => sim.issue(deviceId, CHAT_B, 'terminal', { operation: 'run', - args: { command: `sleep 1; echo B-${n} >> '${marker}'`, waitSeconds: 30 }, + args: { + command: `until [ -e '${releaseCommands}' ]; do sleep 0.1; done; echo B-${n} >> '${marker}'`, + waitSeconds: 30, + }, }) ) const readConsent = launched.app.waitForEvent('window') const localRead = sim.issue(deviceId, CHAT_B, 'read_local_file', { path: readable }) - await (await readConsent).getByRole('button', { name: 'Allow folder', exact: true }).click() + await answerShellDialog(await readConsent, 'Allow folder') await window.goto(`${sim.origin}/workspace/ws-other/home`) await window.reload() await window.goto(`${sim.origin}/workspace/${WORKSPACE}/chat/${CHAT_C}`) + // Chat B's first command holds its queue until released, so a claim that waited for the + // calls ahead of it to run could never happen in time. + await check("A: every call is claimed while chat B's first command still runs", async () => { + await expect + .poll( + () => + [...sim.calls.values()] + .filter((call) => call.claimedAt === undefined) + .map((call) => call.toolCallId), + { timeout: SIM_PICKUP_GRACE_MS } + ) + .toEqual([]) + }) + + await check('A: the lease is renewed while calls wait and run', async () => { + await expect + .poll(() => terminalRuns.filter((id) => sim.requireCall(id).renewals === 0)) + .toEqual([]) + }) + writeFileSync(releaseCommands, '') + await check('A: every call completes exactly once with its own token', async () => { for (const id of [...clicks, ...terminalRuns, localRead]) { const completion = await settled(sim, id, 90_000) @@ -153,18 +182,6 @@ test.describe('background executor', () => { sim.requests.filter((request) => request.includes('/api/desktop/tool/authorize')) ).toEqual([]) }) - - await check('A: calls are picked up within 1.5 s at p95', async () => { - const latencies = [...sim.calls.values()] - .map((call) => (call.claimedAt ?? Number.POSITIVE_INFINITY) - call.issuedAt) - .sort((a, b) => a - b) - const p95 = latencies[Math.ceil(latencies.length * 0.95) - 1] ?? Number.POSITIVE_INFINITY - expect(p95).toBeLessThan(1_500) - }) - - await check('A: the lease is renewed while calls wait and run', async () => { - expect(Math.max(...terminalRuns.map((id) => sim.requireCall(id).renewals))).toBeGreaterThan(0) - }) }) test('background file reads require consent and Stop cancels pending permission', async () => { @@ -181,7 +198,7 @@ test.describe('background executor', () => { const prompt = await shown await expect(prompt.getByRole('button', { name: 'Allow folder', exact: true })).toBeVisible() expect(sim.requireCall(call).completions).toHaveLength(0) - await prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + await answerShellDialog(prompt, "Don't allow") const completion = await settled(sim, call) expect(completion.status).toBe('error') expect(JSON.stringify(completion)).not.toContain('background consent fixture') @@ -202,7 +219,7 @@ test.describe('background executor', () => { const shown = launched.app.waitForEvent('window', { timeout: 10_000 }) const call = sim.issue(deviceId, CHAT_B, 'read_local_file', { path: readable }) const prompt = await shown - await prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() + await answerShellDialog(prompt, 'Allow folder') expect((await settled(sim, call)).status).toBe('success') const next = sim.issue(deviceId, CHAT_A, 'read_local_file', { path: readable }) expect(JSON.stringify((await settled(sim, next)).data)).toContain( @@ -225,7 +242,7 @@ test.describe('background executor', () => { const prompt = await shown await expect(prompt.getByRole('button', { name: 'Allow folder', exact: true })).toBeVisible() sim.disconnect() - await prompt.getByRole('button', { name: 'Allow folder', exact: true }).click() + await answerShellDialog(prompt, 'Allow folder') await expect .poll(() => launched.app.evaluate(({ safeStorage }, toolCallId) => { @@ -245,7 +262,7 @@ test.describe('background executor', () => { expect(JSON.stringify(completion)).not.toContain('offline consent fixture') const nextPrompt = launched.app.waitForEvent('window') const next = sim.issue(deviceId, CHAT_B, 'read_local_file', { path: readable }) - await (await nextPrompt).getByRole('button', { name: "Don't allow", exact: true }).click() + await answerShellDialog(await nextPrompt, "Don't allow") expect((await settled(sim, next)).status).toBe('error') }) }) @@ -270,7 +287,7 @@ test.describe('background executor', () => { BrowserWindow.getAllWindows().map((window) => window.getParentWindow() === null) ) ).toEqual([true]) - await prompt.getByRole('button', { name: "Don't allow", exact: true }).click() + await answerShellDialog(prompt, "Don't allow") expect((await settled(sim, call)).status).toBe('error') }) }) @@ -284,7 +301,7 @@ test.describe('background executor', () => { writeFileSync(readable, 'cleanup consent fixture') const shown = launched.app.waitForEvent('window') const call = sim.issue(deviceId, CHAT_B, 'read_local_file', { path: readable }) - await (await shown).getByRole('button', { name: 'Allow folder', exact: true }).click() + await answerShellDialog(await shown, 'Allow folder') expect((await settled(sim, call)).status).toBe('success') const grants = join(userData, 'local-filesystem-grants.json') expect(existsSync(grants)).toBe(true) @@ -321,7 +338,7 @@ test.describe('background executor', () => { item.click() }) const failure = await failedSignOut - await failure.getByRole('button', { name: 'OK', exact: true }).click() + await answerShellDialog(failure, 'OK') expect(existsSync(join(userData, 'account-data-teardown-required.json'))).toBe(true) await expect.poll(() => existsSync(grants)).toBe(false) } @@ -419,7 +436,7 @@ test.describe('background executor', () => { folderId: 'folder-e2e', }) - await (await importConsent).getByRole('button', { name: 'Allow folder', exact: true }).click() + await answerShellDialog(await importConsent, 'Allow folder') await check('D: the import completes with every entry it stored', async () => { const completion = await settled(sim, call, 60_000) diff --git a/apps/desktop/e2e/browser-focus.spec.ts b/apps/desktop/e2e/browser-focus.spec.ts index 60bd25a345e..86e13ee3726 100644 --- a/apps/desktop/e2e/browser-focus.spec.ts +++ b/apps/desktop/e2e/browser-focus.spec.ts @@ -122,7 +122,7 @@ test('browser focus and shortcuts stay with the surface the user is using', asyn (globalThis as Bridge).simDesktop.browserAgent.executeTool(callId, tool, args, scope), { callId, tool, args, scope: SCOPE } ) - expect(result.ok).toBe(true) + expect(result.ok, result.error).toBe(true) return result.ok ? result.result : undefined } const panelAction = (action: Record) => diff --git a/apps/desktop/e2e/browser-page-dialogs.spec.ts b/apps/desktop/e2e/browser-page-dialogs.spec.ts index e66012fbbaa..667a7ae74f8 100644 --- a/apps/desktop/e2e/browser-page-dialogs.spec.ts +++ b/apps/desktop/e2e/browser-page-dialogs.spec.ts @@ -212,7 +212,7 @@ createRoot(document.getElementById('root')).render(createElement(Fixture));`, (globalThis as Bridge).simDesktop.browserAgent.executeTool(callId, tool, args, scope), { callId, tool, args, scope: SCOPE } ) - expect(result.ok).toBe(true) + expect(result.ok, result.error).toBe(true) return result.ok ? result.result : undefined } /** Browser-chrome actions are user gestures, so each follows a real click in Sim. */ @@ -439,8 +439,13 @@ createRoot(document.getElementById('root')).render(createElement(Fixture));`, caret: 'initial', }) await expect(shell.getByRole('button', { name: 'Stay', exact: true })).toBeFocused() + const shown = await pageDialog() await shell.keyboard.press('Escape') - await expect.poll(pageDialog).toBeNull() + await expect + .poll(pageDialog, { + message: `dialog ${shown?.requestId} was showing when Escape was pressed`, + }) + .toBeNull() await expect .poll(() => shellApp.evaluate(({ webContents }) => webContents.getFocusedWebContents()?.getURL()) diff --git a/apps/desktop/e2e/browser-tools.spec.ts b/apps/desktop/e2e/browser-tools.spec.ts index ab5f150f2e4..3b573ad9df3 100644 --- a/apps/desktop/e2e/browser-tools.spec.ts +++ b/apps/desktop/e2e/browser-tools.spec.ts @@ -615,7 +615,8 @@ test.describe('browser tools', () => { /button "Allow" \[ref=(\d+)\]/ )?.[1] expect(allow, JSON.stringify(popup.result)).toBeTruthy() - expect((await execute('browser_click', { elementId: Number(allow) })).ok).toBe(true) + const allowed = await execute('browser_click', { elementId: Number(allow) }) + expect(allowed.ok, allowed.error).toBe(true) await expect.poll(dataset).toMatchObject({ connected: 'granted' }) await expect diff --git a/apps/desktop/e2e/executor-sim.ts b/apps/desktop/e2e/executor-sim.ts index f2b8e381042..fbb462b98aa 100644 --- a/apps/desktop/e2e/executor-sim.ts +++ b/apps/desktop/e2e/executor-sim.ts @@ -12,6 +12,7 @@ import { expect, type Page, } from '@playwright/test' +import { getErrorMessage } from '@sim/utils/errors' /** * A fixture Sim that speaks the background executor's device protocol (register, inbox, @@ -497,6 +498,25 @@ export async function settled( return sim.requireCall(toolCallId).completions[0] as Completion } +/** + * Answers a shell dialog with one of its buttons. The dialog destroys its window as soon as the + * answer reaches the main process, which can happen before Chromium acknowledges the click to + * Playwright; the click then rejects with a closed target although the answer went through. The + * window closing is the proof the answer landed, so that is what this waits for. + */ +export async function answerShellDialog(dialog: Page, button: string): Promise { + const closed = dialog.waitForEvent('close') + await dialog + .getByRole('button', { name: button, exact: true }) + .click() + .catch((error: unknown) => { + if (!getErrorMessage(error).includes('Target page, context or browser has been closed')) { + throw error + } + }) + await closed +} + export function refFor(outline: string, name: string): number { const match = outline .split('\n') diff --git a/apps/desktop/e2e/packaged-smoke.spec.ts b/apps/desktop/e2e/packaged-smoke.spec.ts index 84cefa543bc..559db75d3ea 100644 --- a/apps/desktop/e2e/packaged-smoke.spec.ts +++ b/apps/desktop/e2e/packaged-smoke.spec.ts @@ -1,6 +1,6 @@ -import { spawn } from 'node:child_process' +import { type ChildProcess, execFileSync, spawn } from 'node:child_process' import { once } from 'node:events' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, realpathSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { FuseV1Options, FuseVersion, getCurrentFuseWire } from '@electron/fuses' @@ -27,6 +27,53 @@ test.skip( 'Packaged smoke runs only after the desktop executable has been built' ) +/** + * The processes of a packaged run that can still write its profile. Chromium's helpers stay in the + * app's process group (it is spawned detached, so the group is its own), while Crashpad's handler + * starts its own session but names the profile's Crashpad database on its command line. + */ +function profileWriters(appGroup: number, profilePaths: readonly string[]): number[] { + return execFileSync('ps', ['-axww', '-o', 'pid=,pgid=,command='], { encoding: 'utf8' }) + .split('\n') + .flatMap((line) => { + const match = /^\s*(\d+)\s+(\d+)\s+(.*)$/.exec(line) + if (!match) return [] + const [, pid, group, command] = match + return Number(group) === appGroup || profilePaths.some((path) => command.includes(path)) + ? [Number(pid)] + : [] + }) +} + +/** + * Removes a profile only once nothing can write into it. Killing the main process leaves its + * helpers and Crashpad's handler running for a moment, and a file one of them creates while the + * directory is being removed fails the removal with ENOTEMPTY. + */ +async function stopAndRemoveProfile(child: ChildProcess, userDataPath: string): Promise { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit') + child.kill('SIGKILL') + await exited + } + const appGroup = child.pid + if (appGroup !== undefined) { + const profilePaths = [userDataPath, realpathSync(userDataPath)] + await expect + .poll(() => { + const writers = profileWriters(appGroup, profilePaths) + for (const pid of writers) { + try { + process.kill(pid, 'SIGKILL') + } catch {} + } + return writers + }) + .toEqual([]) + } + rmSync(userDataPath, { recursive: true, force: true }) +} + test('packaged Electron binary has the production fuse policy', async () => { const executablePath = process.env.SIM_DESKTOP_EXECUTABLE if (!executablePath) throw new Error('SIM_DESKTOP_EXECUTABLE is required') @@ -48,6 +95,7 @@ test('packaged main process starts and records launch telemetry', async () => { if (!executablePath) throw new Error('SIM_DESKTOP_EXECUTABLE is required') const userDataPath = mkdtempSync(join(tmpdir(), 'sim-desktop-packaged-e2e-')) const child = spawn(executablePath, [], { + detached: true, env: { ...process.env, SIM_DESKTOP_ORIGIN: 'http://127.0.0.1:1', @@ -74,12 +122,7 @@ test('packaged main process starts and records launch telemetry', async () => { ) .toBe(true) } finally { - if (child.exitCode === null && child.signalCode === null) { - const exited = once(child, 'exit') - child.kill('SIGKILL') - await exited - } - rmSync(userDataPath, { recursive: true, force: true }) + await stopAndRemoveProfile(child, userDataPath) } }) @@ -97,6 +140,7 @@ test('packaged shell renders the bundled offline page', async () => { // run on a machine that already has the real app's item) would block on a // Keychain prompt on its main thread, and the debugging endpoint with it. const child = spawn(executablePath, ['--remote-debugging-port=0', '--use-mock-keychain'], { + detached: true, env: { ...process.env, SIM_DESKTOP_ORIGIN: 'http://127.0.0.1:1', @@ -150,11 +194,6 @@ test('packaged shell renders the bundled offline page', async () => { await expect(picker.getByLabel('Server URL')).toHaveValue('http://127.0.0.1:1') } finally { await browser?.close().catch(() => {}) - if (child.exitCode === null && child.signalCode === null) { - const exited = once(child, 'exit') - child.kill('SIGKILL') - await exited - } - rmSync(userDataPath, { recursive: true, force: true }) + await stopAndRemoveProfile(child, userDataPath) } }) diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index a2f98b763ce..376000125d8 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -1802,6 +1802,36 @@ function navigationRescue( } } +/** + * A page that closes itself from the click's own handler (a sign-in popup's Allow that calls + * window.close()) can destroy its tab before Chromium acknowledges the release, and Electron then + * rejects the pending input command with "target closed while handling command" although the press + * landed. The same click otherwise reads as a failed dispatch whenever the close wins that race. + */ +function closedTabRescue( + contents: WebContents, + error: unknown, + base: { trusted: boolean; activation: string } +): Record | null { + if (!contents.isDestroyed() && !getErrorMessage(error).includes('target closed')) return null + const activeTab = session.automationTab() + const nowActive = activeTab && activeTab.view.webContents !== contents ? activeTab : null + return { + dispatched: true, + trusted: base.trusted, + activation: base.activation, + effectObserved: true, + possibleEffectObserved: true, + closedDuringDispatch: true, + effect: { tabChanged: true }, + dialogs: [], + ...(nowActive + ? { activeTab: { tabId: nowActive.id, url: nowActive.view.webContents.getURL() } } + : {}), + note: 'The tab closed while the click was being delivered, which is how a page that closes itself in response (a sign-in or permission popup) ends; that is why no postconditions are reported. Take a fresh browser_snapshot of the tab that is now active.', + } +} + function assertActiveContents(contents: WebContents, expectedNavigationEpoch?: number): void { const active = session.automationTab() if ( @@ -3296,10 +3326,10 @@ async function executeToolInner( trusted = true activation = 'native-pointer' } catch (error) { - const rescued = navigationRescue(contents, epochAtDispatch, urlAtDispatch, { - trusted: true, - activation: 'native-pointer', - }) + const nativePointer = { trusted: true, activation: 'native-pointer' } + const rescued = + navigationRescue(contents, epochAtDispatch, urlAtDispatch, nativePointer) ?? + closedTabRescue(contents, error, nativePointer) if (rescued) return rescued throw new ToolError( `Native click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` @@ -3338,10 +3368,10 @@ async function executeToolInner( activation = 'native-pointer' prepared = finalSurface } catch (error) { - const rescued = navigationRescue(contents, epochAtDispatch, urlAtDispatch, { - trusted: true, - activation: 'native-pointer', - }) + const nativePointer = { trusted: true, activation: 'native-pointer' } + const rescued = + navigationRescue(contents, epochAtDispatch, urlAtDispatch, nativePointer) ?? + closedTabRescue(contents, error, nativePointer) if (rescued) return rescued throw new ToolError( `Native framed click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` @@ -4763,10 +4793,10 @@ async function executeToolInner( try { await cdp.clickAt(contents, x, y, true, click, signal) } catch (error) { - const rescued = navigationRescue(contents, clickNavigationEpoch, urlAtDispatch, { - trusted: true, - activation: 'native-pointer', - }) + const nativePointer = { trusted: true, activation: 'native-pointer' } + const rescued = + navigationRescue(contents, clickNavigationEpoch, urlAtDispatch, nativePointer) ?? + closedTabRescue(contents, error, nativePointer) if (rescued) return rescued throw new ToolError( `Native click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` From 23f493e30c3806056d0da7775f178f93bd6e4147 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 12 Oct 2026 13:17:26 -0700 Subject: [PATCH 2/3] fix(desktop-e2e): rescue only a click whose final release hit a tab that then closed - cdp.clickAt raises TabClosedOnReleaseError only when the final mouseReleased was sent to a live tab and the tab is destroyed when it rejects; the driver rescues only that error (no string match, no pre-input/press/hold cases), with a note that does not claim the page closed itself and doNotRetry - background-executor A: assert each call is claimed within Sim's 15 s pickup window measured from issue, and always release the gated commands --- apps/desktop/e2e/background-executor.spec.ts | 59 +++++++++++-------- apps/desktop/src/main/browser-agent/cdp.ts | 35 ++++++++--- apps/desktop/src/main/browser-agent/driver.ts | 24 ++++---- 3 files changed, 71 insertions(+), 47 deletions(-) diff --git a/apps/desktop/e2e/background-executor.spec.ts b/apps/desktop/e2e/background-executor.spec.ts index d91b1bd8fdc..5849198fbc5 100644 --- a/apps/desktop/e2e/background-executor.spec.ts +++ b/apps/desktop/e2e/background-executor.spec.ts @@ -122,32 +122,41 @@ test.describe('background executor', () => { ) const readConsent = launched.app.waitForEvent('window') const localRead = sim.issue(deviceId, CHAT_B, 'read_local_file', { path: readable }) - await answerShellDialog(await readConsent, 'Allow folder') - - await window.goto(`${sim.origin}/workspace/ws-other/home`) - await window.reload() - await window.goto(`${sim.origin}/workspace/${WORKSPACE}/chat/${CHAT_C}`) - - // Chat B's first command holds its queue until released, so a claim that waited for the - // calls ahead of it to run could never happen in time. - await check("A: every call is claimed while chat B's first command still runs", async () => { - await expect - .poll( - () => - [...sim.calls.values()] - .filter((call) => call.claimedAt === undefined) - .map((call) => call.toolCallId), - { timeout: SIM_PICKUP_GRACE_MS } - ) - .toEqual([]) - }) + try { + await answerShellDialog(await readConsent, 'Allow folder') + + await window.goto(`${sim.origin}/workspace/ws-other/home`) + await window.reload() + await window.goto(`${sim.origin}/workspace/${WORKSPACE}/chat/${CHAT_C}`) + + // Chat B's first command holds its queue until released, so a claim that waited for the + // calls ahead of it to run could never happen in time. + await check("A: every call is claimed while chat B's first command still runs", async () => { + await expect + .poll( + () => + [...sim.calls.values()] + .filter((call) => call.claimedAt === undefined) + .map((call) => call.toolCallId), + { timeout: SIM_PICKUP_GRACE_MS } + ) + .toEqual([]) + for (const call of sim.calls.values()) { + expect( + (call.claimedAt ?? Number.POSITIVE_INFINITY) - call.issuedAt, + call.toolCallId + ).toBeLessThan(SIM_PICKUP_GRACE_MS) + } + }) - await check('A: the lease is renewed while calls wait and run', async () => { - await expect - .poll(() => terminalRuns.filter((id) => sim.requireCall(id).renewals === 0)) - .toEqual([]) - }) - writeFileSync(releaseCommands, '') + await check('A: the lease is renewed while calls wait and run', async () => { + await expect + .poll(() => terminalRuns.filter((id) => sim.requireCall(id).renewals === 0)) + .toEqual([]) + }) + } finally { + writeFileSync(releaseCommands, '') + } await check('A: every call completes exactly once with its own token', async () => { for (const id of [...clicks, ...terminalRuns, localRead]) { diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index f7c77d69e74..4e691e4f026 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -1093,6 +1093,17 @@ export function clearAgentContextMenu(contents: WebContents): void { agentContextClicks.delete(contents) } +/** + * The final release of a click was sent to a live tab, and the tab was gone when that release + * was rejected. The press and release both reached the page; only the acknowledgement was lost. + */ +export class TabClosedOnReleaseError extends Error { + constructor() { + super('The tab closed as the click was released.') + this.name = 'TabClosedOnReleaseError' + } +} + /** * Clicks at viewport coordinates. An already-aborted `signal` rejects before any input is sent. * During a press-and-hold it ends the hold early: the click rejects with the abort reason and the @@ -1140,15 +1151,21 @@ export async function clickAt( } signal?.throwIfAborted() } - await sendInput(contents, 'Input.dispatchMouseEvent', { - type: 'mouseReleased', - x, - y, - button, - buttons: 0, - modifiers, - clickCount: count, - }) + const finalReleaseToLiveTab = count === clickCount && !contents.isDestroyed() + try { + await sendInput(contents, 'Input.dispatchMouseEvent', { + type: 'mouseReleased', + x, + y, + button, + buttons: 0, + modifiers, + clickCount: count, + }) + } catch (error) { + if (finalReleaseToLiveTab && contents.isDestroyed()) throw new TabClosedOnReleaseError() + throw error + } } pressed = false } finally { diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 376000125d8..7dfe3890fc5 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -1803,19 +1803,16 @@ function navigationRescue( } /** - * A page that closes itself from the click's own handler (a sign-in popup's Allow that calls - * window.close()) can destroy its tab before Chromium acknowledges the release, and Electron then - * rejects the pending input command with "target closed while handling command" although the press - * landed. The same click otherwise reads as a failed dispatch whenever the close wins that race. + * The click's final release was sent to a live tab that was destroyed before Chromium + * acknowledged it (cdp.clickAt raises TabClosedOnReleaseError only then), so the press and + * release reached the page. Any other dispatch failure keeps its own error. */ function closedTabRescue( - contents: WebContents, error: unknown, base: { trusted: boolean; activation: string } ): Record | null { - if (!contents.isDestroyed() && !getErrorMessage(error).includes('target closed')) return null + if (!(error instanceof cdp.TabClosedOnReleaseError)) return null const activeTab = session.automationTab() - const nowActive = activeTab && activeTab.view.webContents !== contents ? activeTab : null return { dispatched: true, trusted: base.trusted, @@ -1823,12 +1820,13 @@ function closedTabRescue( effectObserved: true, possibleEffectObserved: true, closedDuringDispatch: true, + doNotRetry: true, effect: { tabChanged: true }, dialogs: [], - ...(nowActive - ? { activeTab: { tabId: nowActive.id, url: nowActive.view.webContents.getURL() } } + ...(activeTab + ? { activeTab: { tabId: activeTab.id, url: activeTab.view.webContents.getURL() } } : {}), - note: 'The tab closed while the click was being delivered, which is how a page that closes itself in response (a sign-in or permission popup) ends; that is why no postconditions are reported. Take a fresh browser_snapshot of the tab that is now active.', + note: 'The tab closed as the click was released, so no postconditions could be read. Do not repeat the click. If this page was not expected to close itself, verify the outcome with browser_snapshot before relying on it.', } } @@ -3329,7 +3327,7 @@ async function executeToolInner( const nativePointer = { trusted: true, activation: 'native-pointer' } const rescued = navigationRescue(contents, epochAtDispatch, urlAtDispatch, nativePointer) ?? - closedTabRescue(contents, error, nativePointer) + closedTabRescue(error, nativePointer) if (rescued) return rescued throw new ToolError( `Native click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` @@ -3371,7 +3369,7 @@ async function executeToolInner( const nativePointer = { trusted: true, activation: 'native-pointer' } const rescued = navigationRescue(contents, epochAtDispatch, urlAtDispatch, nativePointer) ?? - closedTabRescue(contents, error, nativePointer) + closedTabRescue(error, nativePointer) if (rescued) return rescued throw new ToolError( `Native framed click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` @@ -4796,7 +4794,7 @@ async function executeToolInner( const nativePointer = { trusted: true, activation: 'native-pointer' } const rescued = navigationRescue(contents, clickNavigationEpoch, urlAtDispatch, nativePointer) ?? - closedTabRescue(contents, error, nativePointer) + closedTabRescue(error, nativePointer) if (rescued) return rescued throw new ToolError( `Native click dispatch failed (${getErrorMessage(error)}). The action was not retried because a partial pointer press may already have reached the page. Take a fresh snapshot before continuing.` From 4a7ede1017b774cf0ec466da5e6bc58afbb1a65b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 12 Oct 2026 13:40:20 -0700 Subject: [PATCH 3/3] fix(desktop): report a click whose tab closed mid-release as unconfirmed The tab being destroyed proves teardown, not that Chromium processed the release, so the rescued result no longer claims effectObserved and its note says the outcome could not be confirmed. Adds clickAt tests for each TabClosedOnReleaseError guard and a driver test for the rescued shape. --- .../src/main/browser-agent/cdp.test.ts | 73 +++++++++++++++++++ apps/desktop/src/main/browser-agent/cdp.ts | 5 +- .../src/main/browser-agent/driver.test.ts | 44 +++++++++++ apps/desktop/src/main/browser-agent/driver.ts | 10 +-- 4 files changed, 125 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index a62595d7ee8..727f69563c8 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -22,6 +22,7 @@ import { releaseFileInput, resolveFileInput, setFileInputFiles, + TabClosedOnReleaseError, } from '@/main/browser-agent/cdp' function createOopifFrameFixture() { @@ -252,6 +253,78 @@ describe('browser-agent CDP instrumentation', () => { ]) }) + describe('a tab that closes during a click', () => { + /** + * Rejects the mouse event matching `type` and `clickCount` with `failure`, first marking the + * tab destroyed when `destroy` is `'at-rejection'`. `'before-release'` instead marks it + * destroyed as the press resolves, so the release is sent to an already-dead tab. + */ + function failMouseEvent( + contents: WebContents, + fail: { type: string; clickCount: number }, + failure: Error, + destroy: 'at-rejection' | 'before-release' | 'never' + ) { + let destroyed = false + vi.mocked(contents.isDestroyed).mockImplementation(() => destroyed) + vi.mocked(contents.debugger.sendCommand).mockImplementation(async (_method, params) => { + const { type, clickCount } = toRecord(params) + if (destroy === 'before-release' && type === 'mousePressed') destroyed = true + if (type === fail.type && clickCount === fail.clickCount) { + if (destroy === 'at-rejection') destroyed = true + throw failure + } + return {} + }) + } + + it('reports a final release rejected because the tab closed as TabClosedOnReleaseError', async () => { + const contents = new WebContentsView().webContents + failMouseEvent( + contents, + { type: 'mouseReleased', clickCount: 1 }, + new Error('Target closed'), + 'at-rejection' + ) + + await expect(clickAt(contents, 12, 24)).rejects.toBeInstanceOf(TabClosedOnReleaseError) + }) + + it('keeps the original error when the release fails on a tab that is still alive', async () => { + const contents = new WebContentsView().webContents + const failure = new Error('release rejected') + failMouseEvent(contents, { type: 'mouseReleased', clickCount: 1 }, failure, 'never') + + await expect(clickAt(contents, 12, 24)).rejects.toBe(failure) + }) + + it('keeps the original error when the tab was already gone before the release was sent', async () => { + const contents = new WebContentsView().webContents + const failure = new Error('Target closed') + failMouseEvent(contents, { type: 'mouseReleased', clickCount: 1 }, failure, 'before-release') + + await expect(clickAt(contents, 12, 24)).rejects.toBe(failure) + }) + + it('keeps the original error when the press fails as the tab closes', async () => { + const contents = new WebContentsView().webContents + const failure = new Error('Target closed') + failMouseEvent(contents, { type: 'mousePressed', clickCount: 1 }, failure, 'at-rejection') + + await expect(clickAt(contents, 12, 24)).rejects.toBe(failure) + }) + + it('keeps the original error when a non-final release of a multi-click fails as the tab closes', async () => { + const contents = new WebContentsView().webContents + const failure = new Error('Target closed') + failMouseEvent(contents, { type: 'mouseReleased', clickCount: 1 }, failure, 'at-rejection') + + await expect( + clickAt(contents, 12, 24, true, { ...PRIMARY_CLICK, clickCount: 2 }) + ).rejects.toBe(failure) + }) + }) + it('times out a hung press and sends cleanup before the tool watchdog can release', async () => { vi.useFakeTimers() try { diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index 4e691e4f026..59e570ad8ec 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -1095,11 +1095,12 @@ export function clearAgentContextMenu(contents: WebContents): void { /** * The final release of a click was sent to a live tab, and the tab was gone when that release - * was rejected. The press and release both reached the page; only the acknowledgement was lost. + * was rejected: the release was in flight when the tab closed. Whether it reached the page is + * unknown, so this proves neither delivery nor failure, only that the click must not be repeated. */ export class TabClosedOnReleaseError extends Error { constructor() { - super('The tab closed as the click was released.') + super('The tab closed while the click release was in flight.') this.name = 'TabClosedOnReleaseError' } } diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index a01264688a5..03ec50d8d57 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -2344,6 +2344,50 @@ describe('credential protection', () => { }) }) + it('reports a click whose tab closed during its release as unconfirmed, not failed', async () => { + const contents = await openPage() + let destroyed = false + vi.mocked(contents.isDestroyed).mockImplementation(() => destroyed) + vi.mocked(contents.executeJavaScript).mockImplementation((expression: string) => { + if (isPageCall(expression, 'describePointTarget')) { + return Promise.resolve({ found: true, element: 'Close', cursor: 'pointer' }) + } + if (isPageCall(expression, 'readActiveElementState')) return Promise.resolve({}) + if (isPageCall(expression, 'readPageActionState')) { + return Promise.resolve({ + url: 'https://example.com/popup', + title: 'Popup', + focus: 'body', + mutationRevision: 0, + dialogs: [], + popups: [], + scroll: [0], + }) + } + return Promise.resolve(undefined) + }) + vi.mocked(contents.debugger.sendCommand).mockImplementation(async (method, params) => { + if (method === 'Input.dispatchMouseEvent' && toRecord(params).type === 'mouseReleased') { + destroyed = true + throw new Error('Target closed') + } + return {} + }) + + const result = await driver.executeTool('chat-test', 'browser_click_at', { x: 100, y: 200 }) + + expect(result).toMatchObject({ + ok: true, + result: { + dispatched: true, + closedDuringDispatch: true, + doNotRetry: true, + possibleEffectObserved: true, + }, + }) + expect(toRecord(result.result)).not.toHaveProperty('effectObserved') + }) + it('reports navigation obstructed by a dialog that opened with it', async () => { const contents = await openPage() let actionReads = 0 diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 7dfe3890fc5..595d7369c05 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -1803,9 +1803,10 @@ function navigationRescue( } /** - * The click's final release was sent to a live tab that was destroyed before Chromium - * acknowledged it (cdp.clickAt raises TabClosedOnReleaseError only then), so the press and - * release reached the page. Any other dispatch failure keeps its own error. + * The tab closed while the click's final release was in flight (cdp.clickAt raises + * TabClosedOnReleaseError only then). Whether the release reached the page cannot be confirmed, + * so the result reports the tab change but not the click's effect, and forbids a repeat. Any other + * dispatch failure keeps its own error. */ function closedTabRescue( error: unknown, @@ -1817,7 +1818,6 @@ function closedTabRescue( dispatched: true, trusted: base.trusted, activation: base.activation, - effectObserved: true, possibleEffectObserved: true, closedDuringDispatch: true, doNotRetry: true, @@ -1826,7 +1826,7 @@ function closedTabRescue( ...(activeTab ? { activeTab: { tabId: activeTab.id, url: activeTab.view.webContents.getURL() } } : {}), - note: 'The tab closed as the click was released, so no postconditions could be read. Do not repeat the click. If this page was not expected to close itself, verify the outcome with browser_snapshot before relying on it.', + note: "The tab closed while the click's release was in flight, so the click's outcome could not be confirmed. Do not repeat the click; verify the outcome with browser_snapshot before relying on it.", } }