Skip to content

Commit fb8ae20

Browse files
committed
fix(desktop): refuse untaggable tmux runs, keep install ids durable, never reject a reconcile
- A tmux run whose pane cannot be tagged is closed at once and reported, so nothing the agent starts is ever beyond a later stop. - A pane tmux cannot be asked about is neither stopped nor given up on; only a confirmed absence or a mismatched tag releases a run. - A new install id is used only once it is saved. Registration retries instead of running under an id the next launch would not find. - A Sim without the executor routes is rechecked every 15 minutes, logged once, so an upgrade reaches an open app. - An inbox read never rejects: a failing notifier is logged, and the read settles. - A long completion message is shortened without splitting a character.
1 parent 7308f99 commit fb8ae20

11 files changed

Lines changed: 156 additions & 39 deletions

File tree

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,28 @@ describe('desktop executor client', () => {
2424
).rejects.toBeInstanceOf(UnsendableRequestError)
2525
expect(sent).toBe(0)
2626
})
27+
28+
it('never splits a character when it shortens a long completion message', async () => {
29+
const sent: string[] = []
30+
const client = createDesktopExecutorClient({
31+
origin: () => 'https://sim.test',
32+
fetch: async (_url, init) => {
33+
sent.push(String(init.body))
34+
return Response.json({ outcome: 'recorded', status: 'completed' })
35+
},
36+
deviceId: '00000000-0000-4000-8000-000000000000',
37+
})
38+
// An emoji straddles the cut point, so a cut by code units would leave half of it behind.
39+
const message = `${'a'.repeat(9_996)}😀${'b'.repeat(10)}`
40+
41+
await client.complete({
42+
toolCallId: 'call-1',
43+
executionToken: 'token-1',
44+
completion: { status: 'success', message },
45+
})
46+
47+
const sentMessage: string = JSON.parse(sent[0] ?? '{}').message
48+
const loneSurrogate = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/
49+
expect(loneSurrogate.test(sentMessage)).toBe(false)
50+
})
2751
})

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import { getErrorMessage } from '@sim/utils/errors'
77
import { parseRetryAfter } from '@sim/utils/retry'
8-
import { truncate } from '@sim/utils/string'
8+
import { truncateAtCodePoint } from '@sim/utils/string'
99
import {
1010
type ClaimedDesktopCall,
1111
COMPLETION_MESSAGE_MAX_CHARS,
@@ -197,7 +197,7 @@ export function createDesktopExecutorClient(
197197
executionToken,
198198
status: completion.status,
199199
// Leaves room for the ellipsis, so a cut message still fits Sim's limit.
200-
message: truncate(completion.message, COMPLETION_MESSAGE_MAX_CHARS - 3),
200+
message: truncateAtCodePoint(completion.message, COMPLETION_MESSAGE_MAX_CHARS - 3),
201201
...(completion.data !== undefined ? { data: completion.data } : {}),
202202
})
203203
)

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,23 @@ describe('registration', () => {
671671
expect(journal.entries.size).toBe(0)
672672
})
673673

674+
it('settles an inbox read whose notifier throws instead of rejecting', async () => {
675+
const sim = new FakeSim()
676+
const executor = new DesktopExecutor({
677+
client: sim.client,
678+
journal: new MemoryJournal(),
679+
runner: new FakeRunner(),
680+
leaseRenewMs: 60_000,
681+
retryBaseMs: 5,
682+
onUnregistered: () => {},
683+
onApprovals: () => {
684+
throw new Error('notifications are unavailable')
685+
},
686+
})
687+
688+
await expect(executor.reconcile()).resolves.toBeUndefined()
689+
})
690+
674691
it('raises no approval from an inbox read that Sim answers after sign-out', async () => {
675692
const { sim, executor, approvals } = setup()
676693
const answer = deferred<void>()

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,9 @@ export class DesktopExecutor {
196196
this.reconcileAgain = false
197197
await this.reconcileOnce()
198198
} while (this.reconcileAgain && !this.disposed)
199+
} catch (error) {
200+
// Callers fire and forget; one bad read must never become an unhandled rejection.
201+
logger.error('Desktop inbox read failed unexpectedly', { error: getErrorMessage(error) })
199202
} finally {
200203
this.reconciling = null
201204
}

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

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, mkdtemp } from 'node:fs/promises'
1+
import { chmod, mkdir, mkdtemp } from 'node:fs/promises'
22
import { tmpdir } from 'node:os'
33
import { join } from 'node:path'
44
import { sleep } from '@sim/utils/helpers'
@@ -101,16 +101,38 @@ describe('desktop executor registration', () => {
101101
expect(sim.requests.filter((request) => request.includes('/api/desktop/inbox'))).toEqual([])
102102
})
103103

104-
it('stays dormant against a Sim without the executor routes, without retrying', async () => {
104+
it('stays dormant against a Sim without the executor routes, checking back only slowly', async () => {
105105
const { sim, desktopExecutor } = await service()
106-
desktopExecutor.start()
107-
await vi.waitFor(() => expect(sim.registrations).toHaveLength(1))
106+
vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval'] })
107+
try {
108+
desktopExecutor.start()
109+
await vi.waitFor(() => expect(sim.registrations).toHaveLength(1))
110+
111+
sim.registrations[0]?.(404)
112+
await vi.advanceTimersByTimeAsync(60_000)
113+
expect(sim.registrations).toHaveLength(1)
114+
expect(sim.requests.filter((request) => request.includes('/api/desktop/inbox'))).toEqual([])
115+
116+
await vi.advanceTimersByTimeAsync(15 * 60_000)
117+
expect(sim.registrations).toHaveLength(2)
118+
} finally {
119+
vi.useRealTimers()
120+
}
121+
})
108122

109-
sim.registrations[0]?.(404)
110-
await sleep(2_500)
123+
it('registers with no install id it could not save', async () => {
124+
const userData = await mkdtemp(join(tmpdir(), 'sim-executor-service-'))
125+
await chmod(userData, 0o500)
126+
try {
127+
const { sim, desktopExecutor } = await service(1, userData)
111128

112-
expect(sim.registrations).toHaveLength(1)
113-
expect(sim.requests.filter((request) => request.includes('/api/desktop/inbox'))).toEqual([])
129+
desktopExecutor.start()
130+
await sleep(200)
131+
132+
expect(sim.requests.filter((request) => request.includes('/api/desktop/devices'))).toEqual([])
133+
} finally {
134+
await chmod(userData, 0o700)
135+
}
114136
})
115137

116138
it('keeps its install id through a read failure instead of minting a new one', async () => {

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

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,8 @@ export function createDesktopExecutorService(
110110
let registering = false
111111
let registerAgain = false
112112
let registrationAttempt = 0
113+
/** Logged once per run of missing routes, not on every recheck. */
114+
let routesMissingNoted = false
113115
let suspended = false
114116
let started = false
115117
/** Bumped on sign-out, so work started for the previous session cannot resume it. */
@@ -122,12 +124,23 @@ export function createDesktopExecutorService(
122124
return deviceId
123125
}
124126

127+
/**
128+
* Mints a new install id and makes it durable before anything uses it: an id the next launch
129+
* would not find again could never resume the calls bound to it. Fails if it cannot be saved.
130+
*/
125131
async function rotateInstallId(): Promise<string> {
126-
deviceId = generateId()
127-
await writeJsonFileAtomically(identityPath, { deviceId }).catch((error) =>
128-
logger.warn('Could not save the desktop install id', { error: getErrorMessage(error) })
132+
const next = generateId()
133+
deviceId = null
134+
await writeJsonFileAtomically(identityPath, { deviceId: next })
135+
deviceId = next
136+
return next
137+
}
138+
139+
/** Rotates where a failure must not stop the caller; the next registration tries again. */
140+
async function retireInstallId(): Promise<void> {
141+
await rotateInstallId().catch((error) =>
142+
logger.warn('Could not save a new desktop install id', { error: getErrorMessage(error) })
129143
)
130-
return deviceId
131144
}
132145

133146
function fetchWithAppSession(url: string, init: RequestInit): Promise<Response> {
@@ -251,6 +264,7 @@ export function createDesktopExecutorService(
251264
})
252265
if (registrationGeneration !== generation || id !== deviceId) return
253266
registrationAttempt = 0
267+
routesMissingNoted = false
254268
// A Sim that speaks another protocol version gets no new turns bound to this device.
255269
device =
256270
nextTiming.enabled && nextTiming.protocolVersion === DESKTOP_EXECUTOR_PROTOCOL_VERSION
@@ -274,7 +288,7 @@ export function createDesktopExecutorService(
274288
// The id belongs to another account (a copied profile); this install takes a new one.
275289
logger.warn('Desktop install id is registered to another account; minting a new one')
276290
await resetExecutor()
277-
await rotateInstallId()
291+
await retireInstallId()
278292
scheduleRegistration(0)
279293
return
280294
}
@@ -284,10 +298,14 @@ export function createDesktopExecutorService(
284298
return
285299
}
286300
if (error instanceof DeviceRequestError && (error.status === 404 || error.status === 405)) {
287-
// A Sim without the executor routes (older, or self-hosted): nothing to retry against. The
288-
// next launch, session change or settings toggle asks again.
301+
// A Sim without the executor routes (older, or self-hosted): dormant, with only the slow
302+
// recheck, so an upgrade of Sim reaches this device without a relaunch.
289303
stopLoops()
290-
logger.info('Sim does not offer the desktop background executor; staying dormant')
304+
if (!routesMissingNoted) {
305+
routesMissingNoted = true
306+
logger.info('Sim does not offer the desktop background executor; staying dormant')
307+
}
308+
scheduleRegistration(DORMANT_RECHECK_MS)
291309
return
292310
}
293311
registrationAttempt += 1
@@ -384,7 +402,7 @@ export function createDesktopExecutorService(
384402
registrationTimer = null
385403
await resetExecutor()
386404
await journal.clear()
387-
await rotateInstallId()
405+
await retireInstallId()
388406
},
389407
}
390408
}

‎apps/desktop/src/main/terminal/index.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ import {
4545
capturePane,
4646
closeRunPane,
4747
isRunComplete,
48-
isRunPaneOurs,
4948
isTmuxUnavailable,
5049
killPane,
5150
listPanes,
5251
pollRun,
5352
resolveAttachment,
53+
runPaneState,
5454
sendKey,
5555
sendText,
5656
startRun,
@@ -485,7 +485,7 @@ export class TerminalService {
485485
private async reapFinishedRuns(terminalId: string, env: NodeJS.ProcessEnv): Promise<void> {
486486
for (const handle of this.pendingRuns.get(terminalId) ?? []) {
487487
if (this.awaitedRuns.has(handle)) continue
488-
if (isRunComplete(handle) || !(await isRunPaneOurs(handle, env))) {
488+
if (isRunComplete(handle) || (await runPaneState(handle, env)) === 'gone') {
489489
this.untrackRun(terminalId, handle)
490490
handle.dispose()
491491
}

‎apps/desktop/src/main/terminal/pending-runs.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ vi.mock('@/main/terminal/tmux', () => ({
2222
capturePane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
2323
closeRunPane: vi.fn(async () => undefined),
2424
isRunComplete: vi.fn((handle: { window: string }) => tmuxStub.complete.has(handle.window)),
25-
isRunPaneOurs: vi.fn(async () => true),
25+
runPaneState: vi.fn(async () => 'ours'),
2626
isTmuxUnavailable: vi.fn(() => false),
2727
killPane: vi.fn(async () => ({ ok: true, stdout: '', stderr: '' })),
2828
listPanes: vi.fn(async () => []),

‎apps/desktop/src/main/terminal/service.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ vi.mock('@/main/terminal/tmux', async () => {
4646
dispose: () => rmSync(dir, { recursive: true, force: true }),
4747
}
4848
},
49-
isRunPaneOurs: async (...args: Parameters<typeof actual.isRunPaneOurs>) => {
50-
if (!tmuxFake.on) return actual.isRunPaneOurs(...args)
51-
return !tmuxFake.gone.has(args[0].pane)
49+
runPaneState: async (...args: Parameters<typeof actual.runPaneState>) => {
50+
if (!tmuxFake.on) return actual.runPaneState(...args)
51+
return tmuxFake.gone.has(args[0].pane) ? 'gone' : 'ours'
5252
},
5353
stopRun: async (...args: Parameters<typeof actual.stopRun>) => {
5454
if (!tmuxFake.on) return actual.stopRun(...args)

‎apps/desktop/src/main/terminal/tmux.test.ts‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import { afterEach, describe, expect, it } from 'vitest'
55
import {
66
awaitRun,
77
isDescendantOf,
8-
isRunPaneOurs,
98
parseFormatLines,
109
pollRun,
10+
runPaneState,
1111
startRun,
1212
stopRun,
1313
type TmuxRunHandle,
@@ -101,6 +101,8 @@ interface FakeTmuxState {
101101
panes: Record<string, { window: string; options: Record<string, string> }>
102102
/** Every command that reached a pane: `send-keys %1 C-c`, `kill-pane %1`. */
103103
log: string[]
104+
/** Commands the fake fails, with the error tmux would print. */
105+
fail?: Record<string, string>
104106
}
105107

106108
const FAKE_TMUX = `
@@ -111,6 +113,7 @@ const args = process.argv.slice(2)
111113
const save = () => fs.writeFileSync(file, JSON.stringify(state))
112114
const target = () => args[args.indexOf('-t') + 1]
113115
const fail = (message) => { process.stderr.write(message); process.exit(1) }
116+
if (state.fail && state.fail[args[0]]) fail(state.fail[args[0]])
114117
switch (args[0]) {
115118
case 'new-window': {
116119
const window = '@' + state.nextWindow++
@@ -210,7 +213,7 @@ describe('stopping a tmux run touches only its own pane', () => {
210213
state.panes[run.pane] = { window: run.window, options: {} }
211214
tmux.write(state)
212215

213-
expect(await isRunPaneOurs(run, tmux.env)).toBe(false)
216+
expect(await runPaneState(run, tmux.env)).toBe('gone')
214217
await stopRun(run, tmux.env, 0)
215218

216219
expect(tmux.read().log).toEqual([])
@@ -224,8 +227,31 @@ describe('stopping a tmux run touches only its own pane', () => {
224227
delete state.panes[run.pane]
225228
tmux.write(state)
226229

227-
expect(await isRunPaneOurs(run, tmux.env)).toBe(false)
230+
expect(await runPaneState(run, tmux.env)).toBe('gone')
228231
await stopRun(run, tmux.env, 0)
229232
expect(tmux.read().log).toEqual([])
230233
})
234+
235+
it('closes a run it could not tag instead of leaving an unstoppable command', async () => {
236+
const tmux = fakeTmux()
237+
dirs.push(tmux.dir)
238+
tmux.write({ ...tmux.read(), fail: { 'set-option': 'invalid option: @sim-run-id' } })
239+
240+
const started = await startRun('agent', 'sleep 600', null, tmux.env)
241+
242+
expect('error' in started).toBe(true)
243+
expect(tmux.read().log).toEqual(['kill-pane %0'])
244+
expect(tmux.read().panes).toEqual({})
245+
})
246+
247+
it('neither stops nor gives up on a run while tmux cannot be asked', async () => {
248+
const tmux = fakeTmux()
249+
const run = await started(tmux)
250+
tmux.write({ ...tmux.read(), fail: { 'display-message': 'server exited unexpectedly' } })
251+
252+
expect(await runPaneState(run, tmux.env)).toBe('unknown')
253+
await stopRun(run, tmux.env, 0)
254+
255+
expect(tmux.read().log).toEqual([])
256+
})
231257
})

0 commit comments

Comments
 (0)