Skip to content

Commit 72a5ba3

Browse files
committed
fix(desktop): settle Stop without the device, survive Redis errors, and only run offered calls
Stop on a call the background executor held never settled the turn's tool executions unless the device acknowledged: the executor's claim marked a Sim execution as started, and Stop does not settle that execution. A device that was asleep, offline or signed out left abortRun unsettled and the next turn's workbench pending. The executor's claim now takes only its owner token and lease; no Sim handler runs for it, so Sim-execution quiescence ignores it, as it already ignores the chat view's desktop claims. An overdue unclaimed call now settles from its row when presence cannot be read, and never fails early as offline on a failed read; each call in the cron's sweep settles in its own try/catch; presence writes are best effort, so a Redis error no longer fails a pull or a renewal. The executor takes only a call Sim offered it, within its pickup window, and the inbox lists unclaimed calls only while offered or waiting for the user. A call Sim never got to offer gets an implicit deadline one pickup window after it could first run, so the cron still settles it. A revoked install id stays revoked on re-registration, a claim racing the device binding answers "no longer waiting", Stop rings the device only after its chat is validated, the two device lookups are one, and the desktop inbox E2E runs in the http-e2e CI job against its own app with Redis.
1 parent d6d1947 commit 72a5ba3

13 files changed

Lines changed: 445 additions & 119 deletions

File tree

‎.github/workflows/test-build.yml‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,8 @@ jobs:
123123
http-e2e:
124124
name: End-to-end over real HTTP
125125
runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-8vcpu-ubuntu-2404' || 'ubuntu-latest' }}
126-
timeout-minutes: 20
126+
# Four suites, each starting its own dev app, run in sequence.
127+
timeout-minutes: 30
127128
services:
128129
postgres:
129130
image: pgvector/pgvector:pg17
@@ -138,6 +139,16 @@ jobs:
138139
--health-interval 5s
139140
--health-timeout 5s
140141
--health-retries 10
142+
# Only the desktop executor's app is given REDIS_URL: its doorbell and presence live there.
143+
redis:
144+
image: redis:7-alpine
145+
ports:
146+
- 6379:6379
147+
options: >-
148+
--health-cmd "redis-cli ping"
149+
--health-interval 5s
150+
--health-timeout 5s
151+
--health-retries 10
141152
env:
142153
DATABASE_URL: postgresql://postgres:postgres@127.0.0.1:5432/sim_test
143154
BETTER_AUTH_SECRET: http-e2e-ci-secret-at-least-32-characters
@@ -298,6 +309,54 @@ jobs:
298309
STOP_AFTER_E2E_REPORT_PATH="$report_dir/stop-after-http-report.json" \
299310
bun run test:workflow-stop-after:e2e
300311
312+
# The desktop background executor's protocol: device registration, the SSE doorbell over
313+
# Redis pub/sub, presence, leased claims, Stop and isolation, against its own app with Redis.
314+
- name: Verify the desktop background executor's inbox over real HTTP
315+
working-directory: apps/sim
316+
env:
317+
NEXT_PUBLIC_APP_URL: http://127.0.0.1:3019
318+
BETTER_AUTH_URL: http://127.0.0.1:3019
319+
REDIS_URL: redis://127.0.0.1:6379
320+
NEXT_PUBLIC_FORCE_HOSTED: 'false'
321+
MSHIP_DESKTOP_BACKGROUND_EXECUTOR: 'true'
322+
COPILOT_TOOL_PERMISSIONS_ENABLED: 'true'
323+
INTERNAL_API_SECRET: desktop-inbox-http-ci-local-secret-at-least-32-characters
324+
DB_TX_TRIPWIRE: throw
325+
DISABLE_TELEMETRY: 'true'
326+
NEXT_TELEMETRY_DISABLED: '1'
327+
READY_TIMEOUT_SECONDS: 300
328+
run: |
329+
report_dir="$RUNNER_TEMP/e2e"
330+
server_log="$report_dir/desktop-inbox-next.log"
331+
mkdir -p "$report_dir"
332+
node ../../node_modules/next/dist/bin/next dev --hostname 127.0.0.1 --port 3019 > "$server_log" 2>&1 &
333+
server_pid=$!
334+
finish() {
335+
kill "$server_pid" 2>/dev/null || true
336+
wait "$server_pid" 2>/dev/null || true
337+
awk '/^ (GET|POST|PUT|PATCH|DELETE|HEAD) \/api\// { print }' "$server_log" > "$report_dir/desktop-inbox-http-status.log"
338+
}
339+
trap finish EXIT
340+
fail_startup() {
341+
echo "::error::$1"
342+
tail -n 200 "$server_log"
343+
exit 1
344+
}
345+
started=$SECONDS
346+
until curl --fail --silent --max-time 10 http://127.0.0.1:3019/api/health > /dev/null; do
347+
kill -0 "$server_pid" 2>/dev/null || fail_startup 'Local desktop executor app exited during startup.'
348+
[ $((SECONDS - started)) -lt "$READY_TIMEOUT_SECONDS" ] ||
349+
fail_startup "Local desktop executor app did not become ready within $READY_TIMEOUT_SECONDS seconds."
350+
sleep 2
351+
done
352+
echo "Local desktop executor app ready after $((SECONDS - started))s"
353+
DESKTOP_INBOX_E2E_BASE_URL="$NEXT_PUBLIC_APP_URL" \
354+
DESKTOP_INBOX_E2E_DATABASE_URL="$DATABASE_URL" \
355+
DESKTOP_INBOX_E2E_REDIS_URL="$REDIS_URL" \
356+
DESKTOP_INBOX_E2E_AUTH_SECRET="$BETTER_AUTH_SECRET" \
357+
DESKTOP_INBOX_E2E_REPORT_PATH="$report_dir/desktop-inbox-http-report.json" \
358+
bun run test:desktop-inbox:e2e
359+
301360
- name: Upload end-to-end reports and server logs
302361
if: failure()
303362
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4

‎apps/sim/lib/desktop/application/executor.integration.ts‎

Lines changed: 79 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,14 +153,15 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
153153
}
154154

155155
/**
156-
* A desktop call the run persisted before forwarding its frame. `awaitingApproval` marks it held
157-
* for the user's decision, as pre-persist does for a gated call.
156+
* A desktop call the run persisted before forwarding its frame and offered to the device, as the
157+
* run's wait does. `awaitingApproval` instead holds it for the user's decision, as pre-persist
158+
* does for a gated call; `unoffered` leaves it as a run that died before offering it would.
158159
*/
159160
async function pendingCall(
160161
runId: string,
161162
toolName = 'browser_click',
162163
args: Record<string, unknown> = { ref: 'e1' },
163-
options: { awaitingApproval?: boolean } = {}
164+
options: { awaitingApproval?: boolean; unoffered?: boolean } = {}
164165
) {
165166
const toolCallId = generateId()
166167
await db.insert(copilotAsyncToolCalls).values({
@@ -169,6 +170,9 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
169170
toolName,
170171
args,
171172
...(options.awaitingApproval ? { permissionRequestedAt: new Date() } : {}),
173+
...(options.awaitingApproval || options.unoffered
174+
? {}
175+
: { pickupDeadlineAt: sql`now() + interval '1 minute'` }),
172176
})
173177
return toolCallId
174178
}
@@ -283,6 +287,28 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
283287
expect(stored).toEqual({ userId: owner.userId, sessionId: owner.sessionId })
284288
})
285289

290+
it('keeps a revoked install id revoked when the device registers it again', async () => {
291+
const desktop = await signedInDesktop()
292+
await db
293+
.update(desktopDevices)
294+
.set({ revokedAt: new Date() })
295+
.where(eq(desktopDevices.id, desktop.deviceId))
296+
297+
await expect(
298+
registerDesktopDevice.execute({
299+
principal: desktop.principal,
300+
input: {
301+
deviceId: desktop.deviceId,
302+
name: 'Fixture Mac',
303+
appVersion: '0.9.1',
304+
platform: 'darwin-arm64',
305+
capabilities: CAPABILITIES,
306+
},
307+
})
308+
).rejects.toThrow('was revoked')
309+
await expect(inbox(desktop)).rejects.toBeInstanceOf(DesktopDeviceUnrecognizedError)
310+
})
311+
286312
it('rebinds the device to its newest session and locks out the older one', async () => {
287313
const desktop = await signedInDesktop()
288314
const sessionId = generateId()
@@ -345,7 +371,8 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
345371
expect(stored.status).toBe('running')
346372
expect(stored.claimedBy).toBe('desktop-browser')
347373
expect(stored.executionOwnerToken).toBe(granted?.executionToken)
348-
expect(stored.executionStartedAt).not.toBeNull()
374+
/** The device runs it, not a Sim handler, so no Sim execution is marked as started. */
375+
expect(stored.executionStartedAt).toBeNull()
349376
expect(stored.executionLeaseExpiresAt?.getTime()).toBeGreaterThan(Date.now() + 50_000)
350377
})
351378

@@ -412,13 +439,30 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
412439
executor: { deviceId, ownerToken: generateId() },
413440
})
414441

415-
await expect(executorClaim(sameUserOtherMac.deviceId)).rejects.toThrow(
416-
'Tool execution ownership is unavailable'
417-
)
442+
await expect(executorClaim(sameUserOtherMac.deviceId)).resolves.toEqual({
443+
outcome: 'existing',
444+
})
418445
expect((await row(toolCallId)).status).toBe('pending')
419446
await expect(executorClaim(desktop.deviceId)).resolves.toEqual({ outcome: 'claimed' })
420447
})
421448

449+
it('never hands over a call Sim did not offer, nor list it', async () => {
450+
const desktop = await signedInDesktop()
451+
const run = await boundRun(desktop)
452+
const toolCallId = await pendingCall(
453+
run.runId,
454+
'browser_click',
455+
{ ref: 'e1' },
456+
{ unoffered: true }
457+
)
458+
459+
expect((await inbox(desktop)).items).toEqual([])
460+
await expect(claim(desktop.principal, desktop.deviceId, toolCallId)).rejects.toThrow(
461+
'no longer waiting'
462+
)
463+
expect((await row(toolCallId)).status).toBe('pending')
464+
})
465+
422466
it('ignores runs that were never bound to a device', async () => {
423467
const desktop = await signedInDesktop()
424468
const unbound = await boundRun({ userId: desktop.userId, deviceId: null })
@@ -456,9 +500,14 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
456500
message: expect.stringContaining('not approved'),
457501
})
458502

503+
/** The user allows it, and the run's wait offers it to the device. */
459504
await db
460505
.update(copilotAsyncToolCalls)
461-
.set({ permissionDecision: 'allow', permissionDecidedAt: new Date() })
506+
.set({
507+
permissionDecision: 'allow',
508+
permissionDecidedAt: new Date(),
509+
pickupDeadlineAt: sql`now() + interval '1 minute'`,
510+
})
462511
.where(eq(copilotAsyncToolCalls.toolCallId, toolCallId))
463512
expect((await inbox(desktop)).items).toMatchObject([{ kind: 'call', toolCallId }])
464513
await expect(claim(desktop.principal, desktop.deviceId, toolCallId)).resolves.toMatchObject({
@@ -748,6 +797,28 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
748797
})
749798

750799
describe('presence and the inbox', () => {
800+
it('still serves pulls and renewals when presence cannot be written', async () => {
801+
const desktop = await signedInDesktop()
802+
const run = await boundRun(desktop)
803+
const toolCallId = await pendingCall(run.runId)
804+
const redis = getRedisClient()
805+
if (!redis) throw new Error('Redis is required')
806+
const writes = vi.spyOn(redis, 'set').mockRejectedValue(new Error('Redis unavailable'))
807+
try {
808+
expect((await inbox(desktop)).items).toMatchObject([{ kind: 'call', toolCallId }])
809+
const { executionToken } = await claim(desktop.principal, desktop.deviceId, toolCallId)
810+
await expect(
811+
renewDesktopToolLease.execute({
812+
principal: desktop.principal,
813+
input: { deviceId: desktop.deviceId, toolCallId, executionToken },
814+
})
815+
).resolves.toEqual({ renewed: true })
816+
expect(writes).toHaveBeenCalled()
817+
} finally {
818+
writes.mockRestore()
819+
}
820+
})
821+
751822
it('lists pending calls in persistence order even when their doorbell was never heard', async () => {
752823
const desktop = await signedInDesktop()
753824
const run = await boundRun(desktop)

‎apps/sim/lib/desktop/application/executor.ts‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import { classifyDesktopInbox, type DesktopInboxEntry } from '@/lib/desktop/exec
2626
import { markDesktopPresent } from '@/lib/desktop/executor/presence'
2727
import {
2828
acknowledgeDesktopCallResult,
29-
getBindableDesktopDevice,
3029
getBoundDesktopCall,
3130
getBoundDesktopDevice,
3231
listDesktopInboxRows,
@@ -79,11 +78,10 @@ export async function resolveTurnDesktopDevice(
7978
deviceId: string
8079
): Promise<string | null> {
8180
if (!(await isDesktopBackgroundExecutorEnabled(principal.userId))) return null
82-
const device = await getBindableDesktopDevice({
83-
deviceId,
84-
userId: principal.userId,
85-
sessionId: principal.sessionId,
86-
})
81+
const device = await getBoundDesktopDevice(
82+
{ deviceId, userId: principal.userId, sessionId: principal.sessionId },
83+
{ executor: true }
84+
)
8785
if (!device) {
8886
logger.warn('Turn not bound: its desktop is not registered to this session', {
8987
userId: principal.userId,
@@ -133,7 +131,7 @@ export const registerDesktopDevice = defineAuthorizedCredentialUserUseCase({
133131
if (!registered)
134132
throw new OrchestrationError(
135133
'conflict',
136-
'This device ID belongs to another account. Generate a new device ID and register again.'
134+
'This device ID belongs to another account or was revoked. Generate a new device ID and register again.'
137135
)
138136
logger.info('Desktop device registered', {
139137
userId: principal.userId,

0 commit comments

Comments
 (0)