Skip to content

Commit 31a911d

Browse files
authored
improvement(desktop): ship the background executor without a flag, close its QA gaps (#8782)
* improvement(desktop): ship the background executor without a flag * fix(desktop): wait on the shell's own startup signal before refusing a run A background terminal run on a fresh shell waited a flat 8 s for the first prompt and then failed with NO_SHELL_INTEGRATION. A startup file that stops at a question (oh-my-zsh's update prompt) holds the prompt indefinitely, so the run failed with a message blaming the shell's integration. The generated startup files now send a SimStartup marker before any of the user's files run. A shell that never sends it within 8 s of spawning is not instrumented and is refused as before (an unsupported shell at once). One that did gets 30 s to reach its prompt, which tolerates slow startup files on a busy machine; past that the run is refused with the screen it is stuck on, so the agent can ask the user to answer it. Stop now ends the wait, and a shell that exits mid-wait reports SESSION_CLOSED. * test(desktop): durable E2E reports, hermetic shells, and a real network cut - background-executor and terminal-cancel record each check through one shared helper as it finishes. The background-executor report was written from module state in afterAll, so a worker restart after a failure wiped it and the report could read green. Reports from an earlier run are replaced, not appended to. check-report.spec.ts forces a worker restart in a nested Playwright run and asserts the failure stays. - The executor fixture launches the app with zsh and an empty ZDOTDIR, so a developer's .zshrc (an unanswered oh-my-zsh update prompt) cannot hold the first prompt and fail unrelated scenarios. - Scenario B now cuts the network at the socket level: every open connection, the doorbell stream included, drops and new ones are reset. It asserts the result lands exactly once, the doorbell reopens, and new work is picked up after reconnecting. * test(desktop): a call declined in a background chat is never claimed or run The fixture Sim can now decline a held call the way Sim settles it, and the executor suite checks the device never claims it and its command never runs after the approval notification. * test(desktop): run the live desktop suite with the background executor on The live suite's Sim has Redis, so with the executor shipped always-on every turn now binds to the app and runs in the background. The proxy answers the app's registration as an install without Redis would, so the chat-view tests keep covering that path, and the round trip asserts the app stays dormant. Two tests let Sim's own answer through: a call issued after the user switched chats runs on the desktop, and a result reported across a network cut (every connection dropped, the first report landing late) reaches the agent exactly once. * fix(desktop): measure a shell's startup bound from when its startup files began The 30 s bound ran from spawn, so a shell whose startup marker arrived late on a busy machine got less than the promised time for its startup files. The deadline now starts at the marker. * test(desktop): drop the outer runner's variables with the shared omit helper
1 parent 59e749e commit 31a911d

38 files changed

Lines changed: 881 additions & 279 deletions

‎.github/scripts/http-e2e.sh‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ case "$group" in
139139
desktop-inbox)
140140
export REDIS_URL=redis://127.0.0.1:6379
141141
export NEXT_PUBLIC_FORCE_HOSTED=false
142-
export MSHIP_DESKTOP_BACKGROUND_EXECUTOR=true
143142
export COPILOT_TOOL_PERMISSIONS_ENABLED=true
144143
export INTERNAL_API_SECRET=desktop-inbox-http-ci-local-secret-at-least-32-characters
145144
export DB_TX_TRIPWIRE=throw

‎.github/workflows/checks.yml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,8 +307,9 @@ jobs:
307307
key: ${{ steps.next-cache.outputs.key }}
308308
path: ./apps/sim/.next/dev
309309

310-
# Chat switches, Stop, sign-out, approval and the flag-off foreground round trip. The spec
311-
# runs the recording proxy (the app's public origin) and the stand-in worker.
310+
# Chat switches, Stop, sign-out, approval, the dormant-executor round trip, and background
311+
# runs across a chat switch and a network cut. The spec runs the recording proxy (the app's
312+
# public origin) and the stand-in worker.
312313
- name: Verify desktop tools in the Electron app against a local app
313314
env:
314315
NEXT_PUBLIC_APP_URL: http://127.0.0.1:3020

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

Lines changed: 63 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { type ElectronApplication, expect, test } from '@playwright/test'
55
import type { SimDesktopApi } from '@sim/desktop-bridge'
66
import { getErrorMessage } from '@sim/utils/errors'
77
import { sleep } from '@sim/utils/helpers'
8+
import { recordCheck } from './check-report'
89
import {
910
type FixtureCall,
1011
FixtureSim,
@@ -24,37 +25,32 @@ import {
2425
* device protocol (register, inbox, doorbell, claim, lease, complete, import) the way Sim's
2526
* routes do.
2627
* The window navigates, reloads and leaves the chats while their calls run: nothing in this
27-
* suite depends on a chat view, which is the point. Each scenario's checks land in a JSON
28-
* report at BACKGROUND_EXECUTOR_REPORT_PATH.
28+
* suite depends on a chat view, which is the point. Each check lands in a JSON report at
29+
* BACKGROUND_EXECUTOR_REPORT_PATH as it finishes.
2930
*/
3031

3132
const CHAT_A = 'chat-browser-a'
3233
const CHAT_B = 'chat-terminal-b'
3334
const CHAT_C = 'chat-idle-c'
3435

35-
interface ReportCheck {
36-
name: string
37-
status: 'passed' | 'failed'
38-
durationMs: number
39-
error?: string
40-
}
41-
42-
const report: ReportCheck[] = []
43-
4436
const sim = new FixtureSim()
4537

38+
/** Runs one check and adds its outcome to the report as soon as it is known. */
4639
async function check(name: string, body: () => Promise<void>): Promise<void> {
4740
const startedAt = Date.now()
48-
try {
49-
await body()
50-
report.push({ name, status: 'passed', durationMs: Date.now() - startedAt })
51-
} catch (error) {
52-
report.push({
41+
const record = (status: 'passed' | 'failed', error?: unknown) =>
42+
recordCheck(process.env.BACKGROUND_EXECUTOR_REPORT_PATH, 'background-executor', {
5343
name,
54-
status: 'failed',
44+
status,
5545
durationMs: Date.now() - startedAt,
56-
error: getErrorMessage(error),
46+
retry: test.info().retry,
47+
...(error === undefined ? {} : { error: getErrorMessage(error) }),
5748
})
49+
try {
50+
await body()
51+
record('passed')
52+
} catch (error) {
53+
record('failed', error)
5854
throw error
5955
}
6056
}
@@ -74,13 +70,6 @@ test.describe('background executor', () => {
7470

7571
test.afterAll(async () => {
7672
await sim.stop()
77-
const reportPath = process.env.BACKGROUND_EXECUTOR_REPORT_PATH
78-
if (reportPath) {
79-
writeFileSync(
80-
reportPath,
81-
JSON.stringify({ suite: 'background-executor', checks: report }, null, 2)
82-
)
83-
}
8473
})
8574

8675
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', () => {
168157
})
169158
})
170159

171-
test('B: a result produced while offline is delivered once after reconnecting', async () => {
160+
test('B: a result produced while the network is cut is delivered once after reconnecting', async () => {
172161
const userData = mkdtempSync(join(tmpdir(), 'sim-executor-b-'))
173162
app = (await launch(sim, userData)).app
174163
const deviceId = await registeredDevice(sim)
@@ -178,21 +167,33 @@ test.describe('background executor', () => {
178167
args: { command: 'sleep 2; echo offline-done', waitSeconds: 30 },
179168
})
180169
await expect.poll(() => sim.requireCall(run).claims).toBe(1)
181-
sim.offline = true
182-
await sleep(6_000)
170+
sim.disconnect()
183171

184-
await check('B: nothing reached Sim while offline', async () => {
185-
expect(sim.requireCall(run).completions).toHaveLength(0)
186-
expect(sim.droppedWhileOffline).toBeGreaterThan(0)
187-
})
188-
sim.offline = false
172+
await check(
173+
'B: the cut drops the doorbell and nothing reaches Sim while it lasts',
174+
async () => {
175+
await expect.poll(() => sim.streams.get(deviceId)?.size ?? 0).toBe(0)
176+
await expect.poll(() => sim.droppedWhileOffline, { timeout: 15_000 }).toBeGreaterThan(0)
177+
await sleep(4_000)
178+
expect(sim.requireCall(run).completions).toHaveLength(0)
179+
}
180+
)
181+
sim.reconnect()
189182

190183
await check('B: the result arrives once after reconnecting', async () => {
191184
const completion = await settled(sim, run, 60_000)
192-
expect(completion.status).toBe('success')
185+
expect(completion.status, completion.message).toBe('success')
193186
expect(JSON.stringify(completion.data)).toContain('offline-done')
194187
await sleep(3_000)
195188
expect(sim.requireCall(run).completions).toHaveLength(1)
189+
expect(sim.requireCall(run).claims).toBe(1)
190+
})
191+
192+
await check('B: the device reopens its doorbell and picks up new work', async () => {
193+
await expect.poll(() => sim.streams.get(deviceId)?.size ?? 0, { timeout: 45_000 }).toBe(1)
194+
const after = sim.issue(deviceId, CHAT_B, 'browser_list_tabs', {})
195+
const completion = await settled(sim, after)
196+
expect(completion.status, completion.message).toBe('success')
196197
})
197198
})
198199

@@ -376,6 +377,34 @@ test.describe('background executor', () => {
376377
})
377378
})
378379

380+
test('F: a call the user declines in a background chat never runs', async () => {
381+
const userData = mkdtempSync(join(tmpdir(), 'sim-executor-f-declined-'))
382+
const marker = join(userData, 'declined-marker.txt')
383+
app = (await launch(sim, userData)).app
384+
const deviceId = await registeredDevice(sim)
385+
const pulls = () =>
386+
sim.requests.filter((request) => request.startsWith('GET /api/desktop/inbox')).length
387+
const pullsBefore = pulls()
388+
389+
const gated = sim.issue(
390+
deviceId,
391+
CHAT_B,
392+
'terminal',
393+
{ operation: 'run', args: { command: `echo ran >> '${marker}'`, waitSeconds: 30 } },
394+
'awaiting_approval'
395+
)
396+
// The device has pulled its inbox and seen the call waiting for approval.
397+
await expect.poll(pulls).toBeGreaterThan(pullsBefore)
398+
sim.decline(gated)
399+
400+
await check('F: the declined call is never claimed, and its command never runs', async () => {
401+
await sleep(RECONCILE_MS * 2)
402+
expect(sim.requireCall(gated).claims).toBe(0)
403+
expect(sim.requireCall(gated).completions).toEqual([])
404+
expect(readFileSafe(marker)).toBe('')
405+
})
406+
})
407+
379408
test('G: only the device a turn is bound to claims its calls', async () => {
380409
const first = await launch(sim, mkdtempSync(join(tmpdir(), 'sim-executor-g1-')))
381410
const firstDevice = await registeredDevice(sim)
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { spawnSync } from 'node:child_process'
2+
import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'
3+
import { createRequire } from 'node:module'
4+
import { tmpdir } from 'node:os'
5+
import { join } from 'node:path'
6+
import { fileURLToPath } from 'node:url'
7+
import { expect, test } from '@playwright/test'
8+
import { omit } from '@sim/utils/object'
9+
import type { ReportCheck } from './check-report'
10+
11+
/**
12+
* The suites' JSON reports against a real worker restart: a nested Playwright run of a fixture
13+
* whose first test fails, so the second runs in a fresh worker with fresh module state. The report
14+
* must keep the failure, and must not carry checks from an earlier run.
15+
*/
16+
const CLI = createRequire(import.meta.url).resolve('@playwright/test/cli')
17+
const CONFIG = fileURLToPath(new URL('./fixtures/worker-restart.config.ts', import.meta.url))
18+
19+
test('a check that failed before the worker restarted stays in the report', () => {
20+
const reportPath = join(mkdtempSync(join(tmpdir(), 'sim-check-report-')), 'report.json')
21+
writeFileSync(
22+
reportPath,
23+
JSON.stringify({
24+
suite: 'worker-restart',
25+
run: -1,
26+
checks: [{ name: 'left by an earlier run', status: 'passed', durationMs: 0 }],
27+
})
28+
)
29+
// The outer run's worker variables would make the nested runner think it is a worker.
30+
const env = omit(
31+
process.env,
32+
Object.keys(process.env).filter((key) => /^(TEST_|PW_)/.test(key))
33+
)
34+
35+
const run = spawnSync(process.execPath, [CLI, 'test', '--config', CONFIG], {
36+
env: { ...env, WORKER_RESTART_REPORT_PATH: reportPath },
37+
encoding: 'utf8',
38+
timeout: 60_000,
39+
})
40+
41+
expect(run.status, run.stdout + run.stderr).toBe(1)
42+
const checks = (JSON.parse(readFileSync(reportPath, 'utf8')) as { checks: ReportCheck[] }).checks
43+
expect(checks.map(({ name, status }) => ({ name, status }))).toEqual([
44+
{ name: 'fails first', status: 'failed' },
45+
{ name: 'passes after the worker restarts', status: 'passed' },
46+
])
47+
// Proof the second check ran in a replacement worker, not the one that failed.
48+
expect(checks[0]?.error).not.toBe(checks[1]?.error)
49+
})

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { readFileSync, writeFileSync } from 'node:fs'
2+
3+
/** One check's outcome in a suite's JSON report. */
4+
export interface ReportCheck {
5+
name: string
6+
status: string
7+
durationMs: number
8+
error?: string
9+
retry?: number
10+
}
11+
12+
interface Report {
13+
suite: string
14+
/** The Playwright run that wrote it: its workers' parent process. */
15+
run: number
16+
checks: ReportCheck[]
17+
}
18+
19+
/**
20+
* Adds a check to the suite's report at `reportPath` as soon as its outcome is known, keeping the
21+
* checks already there from the same run.
22+
*
23+
* Playwright replaces a worker after a failure, and the new one starts with empty module state, so
24+
* a report kept in memory and written at the end would drop everything before the failure, the
25+
* failure included, and could read green. The file holds it instead. Every worker of one run is a
26+
* child of the same runner, so a report left by an earlier run is replaced rather than added to.
27+
*/
28+
export function recordCheck(
29+
reportPath: string | undefined,
30+
suite: string,
31+
check: ReportCheck
32+
): void {
33+
if (!reportPath) return
34+
const run = process.ppid
35+
let checks: ReportCheck[] = []
36+
try {
37+
const existing = JSON.parse(readFileSync(reportPath, 'utf8')) as Report
38+
if (existing.suite === suite && existing.run === run) checks = existing.checks
39+
} catch {
40+
// No report yet from this run.
41+
}
42+
const report: Report = { suite, run, checks: [...checks, check] }
43+
writeFileSync(reportPath, JSON.stringify(report, null, 2))
44+
}

0 commit comments

Comments
 (0)