Skip to content

Commit 93a0f75

Browse files
committed
fix(desktop): hold refused results until the device registers again, and stay awake through recovery
- A result Sim refuses because it no longer recognizes the device is parked rather than retried every 30 s. The journal keeps it, and it is sent once the device registers again. The service is told once. - A dormant device's 15-minute registration recheck is no longer pushed out by every refused request. - A 408 is retried like any other timeout. - Results a previous app run left behind keep the executor busy until Sim has them, so the machine does not sleep with them undelivered. - A Sim that answers registration with another protocol version gets no new turns bound to this device. - Selecting an already granted folder again while a read runs keeps the read's answer. Only a revoked or different grant discards it.
1 parent c33803e commit 93a0f75

8 files changed

Lines changed: 149 additions & 19 deletions

File tree

‎apps/desktop/src/main/desktop-executor/client.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ export class DeviceRequestError extends Error {
3838
return this.status === 401
3939
}
4040

41-
/** Worth retrying unchanged: no answer, a server fault, or a rate limit. */
41+
/** Worth retrying unchanged: no answer, a timeout, a server fault, or a rate limit. */
4242
get transient(): boolean {
43-
return this.status === 0 || this.status === 429 || this.status >= 500
43+
return this.status === 0 || this.status === 408 || this.status === 429 || this.status >= 500
4444
}
4545
}
4646

‎apps/desktop/src/main/desktop-executor/executor.test.ts‎

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,16 +152,18 @@ function setup(options: { leaseRenewMs?: number; maxHeldCalls?: number } = {}) {
152152
const journal = new MemoryJournal()
153153
const runner = new FakeRunner()
154154
const onUnregistered = vi.fn()
155+
const busy: boolean[] = []
155156
const executor = new DesktopExecutor({
156157
client: sim.client,
157158
journal,
158159
runner,
159160
leaseRenewMs: options.leaseRenewMs ?? 60_000,
160161
retryBaseMs: 5,
161162
onUnregistered,
163+
onBusyChange: (value) => busy.push(value),
162164
...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}),
163165
})
164-
return { sim, journal, runner, executor, onUnregistered }
166+
return { sim, journal, runner, executor, onUnregistered, busy }
165167
}
166168

167169
describe('claiming', () => {
@@ -431,6 +433,79 @@ describe('restarting', () => {
431433
})
432434
})
433435

436+
describe('delivery', () => {
437+
it('retries a result whose request timed out', async () => {
438+
const { sim, journal, executor } = setup()
439+
sim.completeErrors = [new DeviceRequestError(408, 'request timeout')]
440+
await journal.put({
441+
toolCallId: 'r-1',
442+
state: 'result',
443+
executionToken: 't-1',
444+
completion: DONE,
445+
})
446+
447+
await executor.recover()
448+
449+
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
450+
})
451+
452+
it('holds a result Sim refuses as unregistered until the device registers again', async () => {
453+
const { sim, journal, executor, onUnregistered } = setup()
454+
sim.completeErrors = [new DeviceRequestError(401, 'unregistered')]
455+
await journal.put({
456+
toolCallId: 'r-1',
457+
state: 'result',
458+
executionToken: 't-1',
459+
completion: DONE,
460+
})
461+
462+
await executor.recover()
463+
await vi.waitFor(() => expect(onUnregistered).toHaveBeenCalledTimes(1))
464+
await sleep(40)
465+
466+
expect(onUnregistered).toHaveBeenCalledTimes(1)
467+
expect(sim.completions).toHaveLength(0)
468+
expect(journal.entries.get('r-1')?.state).toBe('result')
469+
470+
executor.resumeParked()
471+
472+
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
473+
await vi.waitFor(() => expect(journal.entries.size).toBe(0))
474+
})
475+
})
476+
477+
describe('keeping the machine awake', () => {
478+
it('is busy from the claim until Sim has the result', async () => {
479+
const { sim, runner, executor, busy } = setup()
480+
sim.inbox = [callItem('call-1', 'chat-a')]
481+
await executor.reconcile()
482+
await vi.waitFor(() => expect(runner.started).toEqual(['call-1']))
483+
expect(busy).toEqual([true])
484+
485+
runner.finish('call-1')
486+
487+
await vi.waitFor(() => expect(busy).toEqual([true, false]))
488+
expect(sim.completions).toHaveLength(1)
489+
})
490+
491+
it('stays busy while a result a previous run left is still on its way to Sim', async () => {
492+
const { sim, journal, executor, busy } = setup()
493+
sim.completeErrors = [new DeviceRequestError(503, 'deploying')]
494+
await journal.put({
495+
toolCallId: 'result-1',
496+
state: 'result',
497+
executionToken: 't-result',
498+
completion: DONE,
499+
})
500+
501+
await executor.recover()
502+
expect(busy).toEqual([true])
503+
504+
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
505+
await vi.waitFor(() => expect(busy).toEqual([true, false]))
506+
})
507+
})
508+
434509
describe('registration', () => {
435510
it('asks to register again when Sim no longer recognizes the device', async () => {
436511
const { sim, executor, onUnregistered } = setup()

‎apps/desktop/src/main/desktop-executor/executor.ts‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,13 @@ export class DesktopExecutor {
8686
private paused = false
8787
private disposed = false
8888
private busy = false
89+
/** Results a previous app run left, still on their way to Sim. */
90+
private recoveredInFlight = 0
91+
/** Results Sim refused because it no longer recognized this device; sent once it registers again. */
92+
private readonly parked = new Map<
93+
string,
94+
{ executionToken: string; completion: DesktopToolCompletion }
95+
>()
8996

9097
constructor(private readonly options: DesktopExecutorOptions) {}
9198

@@ -131,7 +138,22 @@ export class DesktopExecutor {
131138
toolCallId: entry.toolCallId,
132139
state: entry.state,
133140
})
134-
void this.deliver(entry.toolCallId, entry.executionToken, completion)
141+
// Held awake like a running call: the result exists only on this machine until Sim has it.
142+
this.recoveredInFlight += 1
143+
this.updateBusy()
144+
void this.deliver(entry.toolCallId, entry.executionToken, completion).finally(() => {
145+
this.recoveredInFlight -= 1
146+
this.updateBusy()
147+
})
148+
}
149+
}
150+
151+
/** After registering again: sends the results parked while Sim did not recognize the device. */
152+
resumeParked(): void {
153+
const parked = [...this.parked]
154+
this.parked.clear()
155+
for (const [toolCallId, { executionToken, completion }] of parked) {
156+
void this.deliver(toolCallId, executionToken, completion)
135157
}
136158
}
137159

@@ -329,15 +351,21 @@ export class DesktopExecutor {
329351
break
330352
} catch (error) {
331353
if (!(error instanceof DeviceRequestError)) throw error
332-
if (error.unregistered) this.options.onUnregistered()
333-
else if (error.status === 413 && pending.data !== undefined) {
354+
if (error.unregistered) {
355+
// Retrying cannot help until the device registers again; the journal keeps the result.
356+
this.parked.set(toolCallId, { executionToken, completion: pending })
357+
this.options.onUnregistered()
358+
return
359+
}
360+
if (error.status === 413 && pending.data !== undefined) {
334361
pending = {
335362
status: pending.status,
336363
message: RESULT_TOO_LARGE,
337364
data: { error: RESULT_TOO_LARGE, resultOmitted: true },
338365
}
339366
continue
340-
} else if (!error.transient) {
367+
}
368+
if (!error.transient) {
341369
logger.warn('Sim refused a desktop call result; dropping it', {
342370
toolCallId,
343371
status: error.status,
@@ -413,7 +441,7 @@ export class DesktopExecutor {
413441
}
414442

415443
private updateBusy(): void {
416-
const busy = this.held.size > 0
444+
const busy = this.held.size > 0 || this.recoveredInFlight > 0
417445
if (busy === this.busy) return
418446
this.busy = busy
419447
this.options.onBusyChange?.(busy)

‎apps/desktop/src/main/desktop-executor/service.test.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ vi.mock('electron', () => import('@/test/electron-mock'))
1010
import { createDesktopExecutorService } from '@/main/desktop-executor/service'
1111

1212
/** Sim's device routes, with registration answers held until the test releases them. */
13-
function fakeSim() {
13+
function fakeSim(protocolVersion = 1) {
1414
const requests: string[] = []
1515
const registrations: Array<(enabled: boolean) => void> = []
1616
const fetch = vi.fn(async (url: string, init: RequestInit): Promise<Response> => {
@@ -20,7 +20,7 @@ function fakeSim() {
2020
const enabled = await new Promise<boolean>((resolve) => registrations.push(resolve))
2121
return Response.json({
2222
enabled,
23-
protocolVersion: 1,
23+
protocolVersion,
2424
leaseMs: 60_000,
2525
leaseRenewMs: 20_000,
2626
reconcileMs: 10_000,
@@ -34,8 +34,8 @@ function fakeSim() {
3434
return { fetch, requests, registrations }
3535
}
3636

37-
async function service() {
38-
const sim = fakeSim()
37+
async function service(protocolVersion = 1) {
38+
const sim = fakeSim(protocolVersion)
3939
const desktopExecutor = createDesktopExecutorService({
4040
userDataPath: await mkdtemp(join(tmpdir(), 'sim-executor-service-')),
4141
origin: () => 'https://sim.test',
@@ -72,4 +72,15 @@ describe('desktop executor registration', () => {
7272
expect(sim.requests).not.toContain('GET /api/desktop/inbox')
7373
expect(sim.requests).not.toContain('GET /api/desktop/inbox/stream')
7474
})
75+
76+
it('offers no binding to a Sim that speaks another protocol version', async () => {
77+
const { sim, desktopExecutor } = await service(2)
78+
desktopExecutor.start()
79+
await vi.waitFor(() => expect(sim.registrations).toHaveLength(1))
80+
81+
sim.registrations[0]?.(true)
82+
await vi.waitFor(() => expect(sim.requests).toContain('GET /api/desktop/inbox'))
83+
84+
expect(desktopExecutor.getDevice()).toBeNull()
85+
})
7586
})

‎apps/desktop/src/main/desktop-executor/service.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,8 @@ export function createDesktopExecutorService(
150150
return
151151
}
152152
stopLoops()
153-
scheduleRegistration(DORMANT_RECHECK_MS)
153+
// Every refused request lands here; re-arming each time would push the recheck out forever.
154+
if (!registrationTimer) scheduleRegistration(DORMANT_RECHECK_MS)
154155
}
155156

156157
function stopLoops(): void {
@@ -186,6 +187,8 @@ export function createDesktopExecutorService(
186187
await executor.recover()
187188
// Signed out while recovering: sign-out already disposed this executor.
188189
if (registrationGeneration !== generation || !executor) return
190+
} else {
191+
executor.resumeParked()
189192
}
190193
if (!doorbell) {
191194
doorbell = new InboxDoorbell({
@@ -224,9 +227,11 @@ export function createDesktopExecutorService(
224227
})
225228
if (registrationGeneration !== generation || id !== deviceId) return
226229
registrationAttempt = 0
227-
device = nextTiming.enabled
228-
? { deviceId: id, protocolVersion: DESKTOP_EXECUTOR_PROTOCOL_VERSION }
229-
: null
230+
// A Sim that speaks another protocol version gets no new turns bound to this device.
231+
device =
232+
nextTiming.enabled && nextTiming.protocolVersion === DESKTOP_EXECUTOR_PROTOCOL_VERSION
233+
? { deviceId: id, protocolVersion: DESKTOP_EXECUTOR_PROTOCOL_VERSION }
234+
: null
230235
logger.info('Desktop executor registered', { enabled: nextTiming.enabled })
231236
// Off for this user: a turn already bound to this device still finishes here, so the
232237
// executor keeps serving the inbox; no new turn binds while `device` is null.

‎apps/desktop/src/main/local-filesystem.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,16 @@ describe('LocalFilesystemService', () => {
247247
await expect(searching).resolves.toMatchObject({ ok: false, code: 'MOUNT_NOT_FOUND' })
248248
})
249249

250+
it('keeps a read whose folder the user selected again while it ran', async () => {
251+
const granted = await mount(service)
252+
253+
const reading = service.handle({ operation: 'read', uri: `${granted.uri}README.md` })
254+
const again = await mount(service)
255+
256+
expect(again.uri).toBe(granted.uri)
257+
await expect(reading).resolves.toMatchObject({ ok: true })
258+
})
259+
250260
it('rejects unknown mounts and symlinks that escape the selected directory', async () => {
251261
const granted = await mount(service)
252262
const outside = await mkdtemp(join(tmpdir(), 'sim-localfs-outside-'))

‎apps/desktop/src/main/local-filesystem.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -493,7 +493,7 @@ export class LocalFilesystemService {
493493
this.activeRequests.delete(requestId)
494494
}
495495
}
496-
if (grant && this.mounts.get(grant.id) !== grant) throw mountNotFound()
496+
if (grant && this.mounts.get(grant.id)?.rootPath !== grant.rootPath) throw mountNotFound()
497497
return { ok: true, data }
498498
} catch (error) {
499499
const safe = safeError(error)

‎apps/sim/lib/mothership/tools/client/local-filesystem.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment jsdom
33
*/
4+
import { sleep } from '@sim/utils/helpers'
45
import { beforeEach, describe, expect, it, vi } from 'vitest'
56

67
const { mockReportCompletion } = vi.hoisted(() => ({
@@ -260,12 +261,12 @@ describe('executeLocalFilesystemTool', () => {
260261
await vi.waitFor(() =>
261262
expect(localFilesystem).toHaveBeenCalledWith(expect.objectContaining({ operation: 'read' }))
262263
)
263-
await new Promise((resolve) => setTimeout(resolve, 10))
264+
await sleep(10)
264265
expect(resolved).toBe(false)
265266

266267
finishRead({ ok: true, data: { content: 'hello', totalLines: 1 } })
267268
await vi.waitFor(() => expect(mockReportCompletion).toHaveBeenCalled())
268-
await new Promise((resolve) => setTimeout(resolve, 10))
269+
await sleep(10)
269270
expect(resolved).toBe(false)
270271

271272
finishReport()

0 commit comments

Comments
 (0)