fix(desktop-e2e): remove root causes of four retry-masked desktop flakes (#8940)

* 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

* 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

* 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.

Author: waleedlatif1

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,16 +114,49 @@ 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()
+    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 window.goto(`${sim.origin}/workspace/ws-other/home`)
-    await window.reload()
-    await window.goto(`${sim.origin}/workspace/${WORKSPACE}/chat/${CHAT_C}`)
+      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]) {
@@ -153,18 +191,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 +207,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 +228,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 +251,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 +271,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 +296,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 +310,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 +347,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 +445,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)

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<string, unknown>) =>

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())

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

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<void> {
+  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')

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<void> {
+  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)
   }
 })