improvement(desktop): ship the background executor without a flag, close its QA gaps

.github/scripts/http-e2e.sh

@@ -137,7 +137,6 @@ case "$group" in
   desktop-inbox)
     export REDIS_URL=redis://127.0.0.1:6379
     export NEXT_PUBLIC_FORCE_HOSTED=false
-    export MSHIP_DESKTOP_BACKGROUND_EXECUTOR=true
     export COPILOT_TOOL_PERMISSIONS_ENABLED=true
     export INTERNAL_API_SECRET=desktop-inbox-http-ci-local-secret-at-least-32-characters
     export DB_TX_TRIPWIRE=throw

.github/workflows/checks.yml

@@ -307,8 +307,9 @@ jobs:
           key: ${{ steps.next-cache.outputs.key }}
           path: ./apps/sim/.next/dev
 
-      # Chat switches, Stop, sign-out, approval and the flag-off foreground round trip. The spec
-      # runs the recording proxy (the app's public origin) and the stand-in worker.
+      # Chat switches, Stop, sign-out, approval, the dormant-executor round trip, and background
+      # runs across a chat switch and a network cut. The spec runs the recording proxy (the app's
+      # public origin) and the stand-in worker.
       - name: Verify desktop tools in the Electron app against a local app
         env:
           NEXT_PUBLIC_APP_URL: http://127.0.0.1:3020

apps/desktop/e2e/background-executor.spec.ts

@@ -5,6 +5,7 @@ import { type ElectronApplication, expect, test } from '@playwright/test'
 import type { SimDesktopApi } from '@sim/desktop-bridge'
 import { getErrorMessage } from '@sim/utils/errors'
 import { sleep } from '@sim/utils/helpers'
+import { recordCheck } from './check-report'
 import {
   type FixtureCall,
   FixtureSim,
@@ -24,37 +25,32 @@ import {
  * device protocol (register, inbox, doorbell, claim, lease, complete, import) the way Sim's
  * routes do.
  * The window navigates, reloads and leaves the chats while their calls run: nothing in this
- * suite depends on a chat view, which is the point. Each scenario's checks land in a JSON
- * report at BACKGROUND_EXECUTOR_REPORT_PATH.
+ * suite depends on a chat view, which is the point. Each check lands in a JSON report at
+ * BACKGROUND_EXECUTOR_REPORT_PATH as it finishes.
  */
 
 const CHAT_A = 'chat-browser-a'
 const CHAT_B = 'chat-terminal-b'
 const CHAT_C = 'chat-idle-c'
 
-interface ReportCheck {
-  name: string
-  status: 'passed' | 'failed'
-  durationMs: number
-  error?: string
-}
-
-const report: ReportCheck[] = []
-
 const sim = new FixtureSim()
 
+/** Runs one check and adds its outcome to the report as soon as it is known. */
 async function check(name: string, body: () => Promise<void>): Promise<void> {
   const startedAt = Date.now()
-  try {
-    await body()
-    report.push({ name, status: 'passed', durationMs: Date.now() - startedAt })
-  } catch (error) {
-    report.push({
+  const record = (status: 'passed' | 'failed', error?: unknown) =>
+    recordCheck(process.env.BACKGROUND_EXECUTOR_REPORT_PATH, 'background-executor', {
       name,
-      status: 'failed',
+      status,
       durationMs: Date.now() - startedAt,
-      error: getErrorMessage(error),
+      retry: test.info().retry,
+      ...(error === undefined ? {} : { error: getErrorMessage(error) }),
     })
+  try {
+    await body()
+    record('passed')
+  } catch (error) {
+    record('failed', error)
     throw error
   }
 }
@@ -74,13 +70,6 @@ test.describe('background executor', () => {
 
   test.afterAll(async () => {
     await sim.stop()
-    const reportPath = process.env.BACKGROUND_EXECUTOR_REPORT_PATH
-    if (reportPath) {
-      writeFileSync(
-        reportPath,
-        JSON.stringify({ suite: 'background-executor', checks: report }, null, 2)
-      )
-    }
   })
 
   test('A: two chats run browser and terminal work while the user is elsewhere and reloads', async () => {
@@ -168,7 +157,7 @@ test.describe('background executor', () => {
     })
   })
 
-  test('B: a result produced while offline is delivered once after reconnecting', async () => {
+  test('B: a result produced while the network is cut is delivered once after reconnecting', async () => {
     const userData = mkdtempSync(join(tmpdir(), 'sim-executor-b-'))
     app = (await launch(sim, userData)).app
     const deviceId = await registeredDevice(sim)
@@ -178,21 +167,33 @@ test.describe('background executor', () => {
       args: { command: 'sleep 2; echo offline-done', waitSeconds: 30 },
     })
     await expect.poll(() => sim.requireCall(run).claims).toBe(1)
-    sim.offline = true
-    await sleep(6_000)
+    sim.disconnect()
 
-    await check('B: nothing reached Sim while offline', async () => {
-      expect(sim.requireCall(run).completions).toHaveLength(0)
-      expect(sim.droppedWhileOffline).toBeGreaterThan(0)
-    })
-    sim.offline = false
+    await check(
+      'B: the cut drops the doorbell and nothing reaches Sim while it lasts',
+      async () => {
+        await expect.poll(() => sim.streams.get(deviceId)?.size ?? 0).toBe(0)
+        await expect.poll(() => sim.droppedWhileOffline, { timeout: 15_000 }).toBeGreaterThan(0)
+        await sleep(4_000)
+        expect(sim.requireCall(run).completions).toHaveLength(0)
+      }
+    )
+    sim.reconnect()
 
     await check('B: the result arrives once after reconnecting', async () => {
       const completion = await settled(sim, run, 60_000)
-      expect(completion.status).toBe('success')
+      expect(completion.status, completion.message).toBe('success')
       expect(JSON.stringify(completion.data)).toContain('offline-done')
       await sleep(3_000)
       expect(sim.requireCall(run).completions).toHaveLength(1)
+      expect(sim.requireCall(run).claims).toBe(1)
+    })
+
+    await check('B: the device reopens its doorbell and picks up new work', async () => {
+      await expect.poll(() => sim.streams.get(deviceId)?.size ?? 0, { timeout: 45_000 }).toBe(1)
+      const after = sim.issue(deviceId, CHAT_B, 'browser_list_tabs', {})
+      const completion = await settled(sim, after)
+      expect(completion.status, completion.message).toBe('success')
     })
   })
 
@@ -376,6 +377,34 @@ test.describe('background executor', () => {
     })
   })
 
+  test('F: a call the user declines in a background chat never runs', async () => {
+    const userData = mkdtempSync(join(tmpdir(), 'sim-executor-f-declined-'))
+    const marker = join(userData, 'declined-marker.txt')
+    app = (await launch(sim, userData)).app
+    const deviceId = await registeredDevice(sim)
+    const pulls = () =>
+      sim.requests.filter((request) => request.startsWith('GET /api/desktop/inbox')).length
+    const pullsBefore = pulls()
+
+    const gated = sim.issue(
+      deviceId,
+      CHAT_B,
+      'terminal',
+      { operation: 'run', args: { command: `echo ran >> '${marker}'`, waitSeconds: 30 } },
+      'awaiting_approval'
+    )
+    // The device has pulled its inbox and seen the call waiting for approval.
+    await expect.poll(pulls).toBeGreaterThan(pullsBefore)
+    sim.decline(gated)
+
+    await check('F: the declined call is never claimed, and its command never runs', async () => {
+      await sleep(RECONCILE_MS * 2)
+      expect(sim.requireCall(gated).claims).toBe(0)
+      expect(sim.requireCall(gated).completions).toEqual([])
+      expect(readFileSafe(marker)).toBe('')
+    })
+  })
+
   test('G: only the device a turn is bound to claims its calls', async () => {
     const first = await launch(sim, mkdtempSync(join(tmpdir(), 'sim-executor-g1-')))
     const firstDevice = await registeredDevice(sim)

apps/desktop/e2e/check-report.spec.ts

@@ -0,0 +1,49 @@
+import { spawnSync } from 'node:child_process'
+import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'
+import { createRequire } from 'node:module'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { fileURLToPath } from 'node:url'
+import { expect, test } from '@playwright/test'
+import { omit } from '@sim/utils/object'
+import type { ReportCheck } from './check-report'
+
+/**
+ * The suites' JSON reports against a real worker restart: a nested Playwright run of a fixture
+ * whose first test fails, so the second runs in a fresh worker with fresh module state. The report
+ * must keep the failure, and must not carry checks from an earlier run.
+ */
+const CLI = createRequire(import.meta.url).resolve('@playwright/test/cli')
+const CONFIG = fileURLToPath(new URL('./fixtures/worker-restart.config.ts', import.meta.url))
+
+test('a check that failed before the worker restarted stays in the report', () => {
+  const reportPath = join(mkdtempSync(join(tmpdir(), 'sim-check-report-')), 'report.json')
+  writeFileSync(
+    reportPath,
+    JSON.stringify({
+      suite: 'worker-restart',
+      run: -1,
+      checks: [{ name: 'left by an earlier run', status: 'passed', durationMs: 0 }],
+    })
+  )
+  // The outer run's worker variables would make the nested runner think it is a worker.
+  const env = omit(
+    process.env,
+    Object.keys(process.env).filter((key) => /^(TEST_|PW_)/.test(key))
+  )
+
+  const run = spawnSync(process.execPath, [CLI, 'test', '--config', CONFIG], {
+    env: { ...env, WORKER_RESTART_REPORT_PATH: reportPath },
+    encoding: 'utf8',
+    timeout: 60_000,
+  })
+
+  expect(run.status, run.stdout + run.stderr).toBe(1)
+  const checks = (JSON.parse(readFileSync(reportPath, 'utf8')) as { checks: ReportCheck[] }).checks
+  expect(checks.map(({ name, status }) => ({ name, status }))).toEqual([
+    { name: 'fails first', status: 'failed' },
+    { name: 'passes after the worker restarts', status: 'passed' },
+  ])
+  // Proof the second check ran in a replacement worker, not the one that failed.
+  expect(checks[0]?.error).not.toBe(checks[1]?.error)
+})

apps/desktop/e2e/check-report.ts

@@ -0,0 +1,44 @@
+import { readFileSync, writeFileSync } from 'node:fs'
+
+/** One check's outcome in a suite's JSON report. */
+export interface ReportCheck {
+  name: string
+  status: string
+  durationMs: number
+  error?: string
+  retry?: number
+}
+
+interface Report {
+  suite: string
+  /** The Playwright run that wrote it: its workers' parent process. */
+  run: number
+  checks: ReportCheck[]
+}
+
+/**
+ * Adds a check to the suite's report at `reportPath` as soon as its outcome is known, keeping the
+ * checks already there from the same run.
+ *
+ * Playwright replaces a worker after a failure, and the new one starts with empty module state, so
+ * a report kept in memory and written at the end would drop everything before the failure, the
+ * failure included, and could read green. The file holds it instead. Every worker of one run is a
+ * child of the same runner, so a report left by an earlier run is replaced rather than added to.
+ */
+export function recordCheck(
+  reportPath: string | undefined,
+  suite: string,
+  check: ReportCheck
+): void {
+  if (!reportPath) return
+  const run = process.ppid
+  let checks: ReportCheck[] = []
+  try {
+    const existing = JSON.parse(readFileSync(reportPath, 'utf8')) as Report
+    if (existing.suite === suite && existing.run === run) checks = existing.checks
+  } catch {
+    // No report yet from this run.
+  }
+  const report: Report = { suite, run, checks: [...checks, check] }
+  writeFileSync(reportPath, JSON.stringify(report, null, 2))
+}