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

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

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