Skip to content

Commit 7e3fe3a

Browse files
committed
fix(desktop): keep a restarted user's tmux run only when the model has, or will get, its pane
- A run is marked handed back when Sim takes the result that hands it back (recorded, or a duplicate of one it recorded), not when the journal is written: without OS encryption the journal write saves nothing. - At launch, for the same user, a run is also kept when the journal holds its real result for recovery to send, so a crash between the journal write and Sim's answer no longer kills a pane the model is about to get. An unreadable journal counts as holding none. - A run whose record could not be saved has its freshly tagged pane closed at once, rather than left for the start gate to time out. - When a terminal closes, a finished run's files go only after its pane check, which an untracked run needs them for.
1 parent 6e5104c commit 7e3fe3a

11 files changed

Lines changed: 109 additions & 76 deletions

File tree

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

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,8 @@ function setup(
168168
const runner = new FakeRunner()
169169
const onUnregistered = vi.fn()
170170
const busy: boolean[] = []
171-
/** Calls whose result the executor reported as journaled, in order. */
172-
const journaled: string[] = []
171+
/** Calls whose result the executor reported as reaching the model, in order. */
172+
const delivered: string[] = []
173173
const executor = new DesktopExecutor({
174174
client: sim.client,
175175
journal,
@@ -179,13 +179,13 @@ function setup(
179179
onUnregistered,
180180
onBusyChange: (value) => busy.push(value),
181181
onApprovals: (items) => approvals.push(items),
182-
onResultRecorded: (toolCallId) => journaled.push(toolCallId),
182+
onResultDelivered: (toolCallId) => delivered.push(toolCallId),
183183
...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}),
184184
...(options.deliveryAwakeLimitMs !== undefined
185185
? { deliveryAwakeLimitMs: options.deliveryAwakeLimitMs }
186186
: {}),
187187
})
188-
return { sim, journal, runner, executor, onUnregistered, busy, approvals, journaled }
188+
return { sim, journal, runner, executor, onUnregistered, busy, approvals, delivered }
189189
}
190190

191191
describe('claiming', () => {
@@ -206,20 +206,22 @@ describe('claiming', () => {
206206
expect(executor.heldCallCount()).toBe(0)
207207
})
208208

209-
it('reports a result as durable only once the journal holds it', async () => {
210-
const { sim, journal, runner, executor, journaled } = setup()
209+
it('reports a result as reaching the model only once Sim takes it as the call own', async () => {
210+
const { sim, journal, runner, executor, delivered } = setup()
211211
runner.immediate = DONE
212-
journal.failOn = 'result'
213-
sim.inbox = [callItem('call-unjournaled', 'chat-a')]
212+
// Sim settled this call first: the result never reached the model.
213+
sim.completionOutcome = 'superseded'
214+
sim.inbox = [callItem('call-superseded', 'chat-a')]
214215
await executor.reconcile()
215216
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
216-
expect(journaled).toEqual([])
217+
expect(delivered).toEqual([])
217218

218-
journal.failOn = null
219-
sim.inbox = [callItem('call-journaled', 'chat-a')]
219+
// Taken by Sim, even with nothing written locally (no OS encryption, say).
220+
sim.completionOutcome = 'recorded'
221+
journal.failOn = 'result'
222+
sim.inbox = [callItem('call-recorded', 'chat-a')]
220223
await executor.reconcile()
221-
await vi.waitFor(() => expect(sim.completions).toHaveLength(2))
222-
expect(journaled).toEqual(['call-journaled'])
224+
await vi.waitFor(() => expect(delivered).toEqual(['call-recorded']))
223225
})
224226

225227
it('claims a whole backlog at once, before any of it runs', async () => {

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@ export interface DesktopExecutorOptions {
6868
/** Called whenever the number of held calls changes between zero and more. */
6969
onBusyChange?: (busy: boolean) => void
7070
/**
71-
* Called once a call's result is in the journal: from then on it reaches Sim, now or after a
72-
* restart, so anything it hands back (a pane still running) can be counted on.
71+
* Called once Sim has taken a call's result as the call's own (recorded, or a duplicate of one
72+
* it recorded): the model has it, so anything it hands back (a pane still running) is in use.
7373
*/
74-
onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void
74+
onResultDelivered?: (toolCallId: string, completion: DesktopToolCompletion) => void
7575
maxHeldCalls?: number
7676
/** First delivery retry delay; tests shorten it. */
7777
retryBaseMs?: number
@@ -380,9 +380,7 @@ export class DesktopExecutor {
380380
): Promise<void> {
381381
if (this.disposed) return
382382
// Best effort: unrecorded, a crash reports the call from its `started` entry as outcome unknown.
383-
if (await this.record({ toolCallId, state: 'result', executionToken, completion })) {
384-
this.options.onResultRecorded?.(toolCallId, completion)
385-
}
383+
await this.record({ toolCallId, state: 'result', executionToken, completion })
386384
const sendingSince = Date.now()
387385
try {
388386
await this.sendResult(toolCallId, executionToken, completion, sendingSince)
@@ -408,6 +406,8 @@ export class DesktopExecutor {
408406
completion: pending,
409407
})
410408
logger.info('Desktop call result acknowledged', { toolCallId, outcome })
409+
// Superseded: Sim settled the call first, so this result never reached the model.
410+
if (outcome !== 'superseded') this.options.onResultDelivered?.(toolCallId, pending)
411411
break
412412
} catch (error) {
413413
// Encoding failed on this machine, so nothing was sent; the same data would fail again.

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

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,8 @@ async function service(protocolVersion = 1, userDataPath?: string) {
9696
return { sim, desktopExecutor, busy }
9797
}
9898

99-
describe('calls never handed back with a result', () => {
100-
it('names claimed and started calls, and results that are not started or unknown', async () => {
99+
describe('results recovery will hand to the model', () => {
100+
it('names the calls whose real result the journal holds for recovery to send', async () => {
101101
const userData = await mkdtemp(join(tmpdir(), 'sim-executor-service-'))
102102
const journal = createExecutorJournal(join(userData, 'desktop-executor-journal.json'))
103103
await journal.put({ toolCallId: 'claimed', state: 'claimed', executionToken: 't1' })
@@ -122,12 +122,7 @@ describe('calls never handed back with a result', () => {
122122
})
123123
const { desktopExecutor } = await service(1, userData)
124124

125-
expect([...((await desktopExecutor.unresolvedCalls()) ?? [])].sort()).toEqual([
126-
'claimed',
127-
'not-started',
128-
'started',
129-
'unknown',
130-
])
125+
expect([...(await desktopExecutor.pendingResults())]).toEqual(['handed-back'])
131126
})
132127
})
133128

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

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ export interface DesktopExecutorServiceDeps {
6262
onApprovals?: (items: DesktopApprovalItem[]) => void
6363
/** Whether any chat has desktop work claimed on this machine changed. */
6464
onBusyChange?: (busy: boolean) => void
65-
/** A call's result is in the journal, so it reaches Sim even across a restart. */
66-
onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void
65+
/** Sim has taken a call's result as the call's own, so the model has it. */
66+
onResultDelivered?: (toolCallId: string, completion: DesktopToolCompletion) => void
6767
}
6868

6969
export interface DesktopExecutorService {
@@ -72,11 +72,11 @@ export interface DesktopExecutorService {
7272
refreshRegistration(): void
7373
getDevice(): DesktopExecutorDevice | null
7474
/**
75-
* The calls the journal shows as never handed back with a real result: claimed or started with
76-
* none, or settled as not started or outcome unknown. Read before recovery reports them; null
77-
* when the journal cannot be read.
75+
* The calls whose real result (not one reported as not started or outcome unknown) is in the
76+
* journal and not yet acknowledged, so recovery will hand it to the model. Read before recovery
77+
* changes the journal; empty when it cannot be read.
7878
*/
79-
unresolvedCalls(): Promise<Set<string> | null>
79+
pendingResults(): Promise<Set<string>>
8080
/** Stores one entry of a claimed import, as this device's registered session. */
8181
importEntry(
8282
request: DesktopImportEntryRequest,
@@ -248,7 +248,7 @@ export function createDesktopExecutorService(
248248
onUnregistered: handleUnrecognized,
249249
...(deps.onApprovals ? { onApprovals: deps.onApprovals } : {}),
250250
...(deps.onBusyChange ? { onBusyChange: deps.onBusyChange } : {}),
251-
...(deps.onResultRecorded ? { onResultRecorded: deps.onResultRecorded } : {}),
251+
...(deps.onResultDelivered ? { onResultDelivered: deps.onResultDelivered } : {}),
252252
})
253253
await executor.recover()
254254
// Signed out while recovering: sign-out already disposed this executor.
@@ -453,23 +453,21 @@ export function createDesktopExecutorService(
453453
getDevice() {
454454
return device
455455
},
456-
async unresolvedCalls() {
456+
async pendingResults() {
457+
const pending = new Set<string>()
457458
try {
458-
const unresolved = new Set<string>()
459459
for (const entry of await journal.load()) {
460-
const data = entry.state === 'result' ? entry.completion.data : undefined
461-
if (
462-
entry.state !== 'result' ||
463-
data?.outcomeUnknown === true ||
464-
data?.notStarted === true
465-
) {
466-
unresolved.add(entry.toolCallId)
467-
}
460+
if (entry.state !== 'result') continue
461+
const data = entry.completion.data
462+
if (data?.outcomeUnknown === true || data?.notStarted === true) continue
463+
pending.add(entry.toolCallId)
468464
}
469-
return unresolved
470-
} catch {
471-
return null
465+
} catch (error) {
466+
logger.warn('Could not read the executor journal for pending results', {
467+
error: getErrorMessage(error),
468+
})
472469
}
470+
return pending
473471
},
474472
importEntry(request, signal) {
475473
if (!client) throw new Error('The Sim desktop app is not signed in to Sim.')

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -607,9 +607,9 @@ function main(): void {
607607
accountDataAvailable,
608608
onApprovals: (items) => approvalNotifier.update(items),
609609
onBusyChange: (busy) => sleepBlocker.setBusy(busy),
610-
// A result that will reach the model (not one reported as not started or outcome unknown)
611-
// makes a tmux run it handed back as still going collectable across a restart.
612-
onResultRecorded: (toolCallId, completion) => {
610+
// A result the model has (not one reported as not started or outcome unknown) makes a tmux
611+
// run it handed back as still going collectable across a restart.
612+
onResultDelivered: (toolCallId, completion) => {
613613
if (completion.data?.outcomeUnknown !== true && completion.data?.notStarted !== true) {
614614
terminal.markRunDelivered(toolCallId)
615615
}
@@ -822,8 +822,8 @@ function main(): void {
822822
// it does, so it is stopped, while its pane still carries its tag. A run already handed back as
823823
// still going, with its pane, is left to the model, which may come back to it. Read before the
824824
// executor starts, since its recovery rewrites the journal.
825-
const unresolvedCalls = desktopExecutor.unresolvedCalls()
826-
void unresolvedCalls.then((unresolved) => terminal.stopUncollectableRuns(unresolved))
825+
const pendingResults = desktopExecutor.pendingResults()
826+
void pendingResults.then((pending) => terminal.stopUncollectableRuns(pending))
827827

828828
if (!accountDataAvailable()) {
829829
logger.warn(
@@ -974,7 +974,7 @@ function main(): void {
974974
ensureAppSession().cookies.on('changed', (_event, cookie, _cause, removed) => {
975975
if (!removed && isSessionCookieName(cookie.name)) desktopExecutor.refreshRegistration()
976976
})
977-
await unresolvedCalls
977+
await pendingResults
978978
desktopExecutor.start()
979979
}
980980
await ensureMainWindow()

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -538,13 +538,16 @@ export class TerminalService {
538538
if (!pending) return
539539
for (const handle of pending) {
540540
// A finished run's pane may still be open (`remain-on-exit`): it is closed, while still the
541-
// run's, before the record goes. Without the shell's environment the record stays, and the
542-
// next sweep closes it. An untracked run is never stopped, so it is not kept either.
541+
// run's, before the record goes, and its files go only after that check, which an untracked
542+
// run needs them for. Without the shell's environment the record stays, and the next sweep
543+
// closes it. An untracked run is never stopped, so it is not kept either.
543544
if (isRunComplete(handle) && env) {
544-
void closeRunPane(handle, env).then(async () => {
545-
// A pane tmux could not answer for keeps its record, for the next sweep to close.
546-
if ((await runPaneState(handle, env)) === 'gone') this.forgetRun(handle)
547-
})
545+
void closeRunPane(handle, env)
546+
.then(async () => {
547+
if ((await runPaneState(handle, env)) === 'gone') this.forgetRun(handle)
548+
})
549+
.finally(() => this.releaseRun(handle))
550+
continue
548551
}
549552
if (env && handle.runId !== null && !isRunComplete(handle)) this.orphanedRuns.set(handle, env)
550553
this.releaseRun(handle)
@@ -1351,8 +1354,8 @@ export class TerminalService {
13511354
handle.dispose()
13521355
}
13531356
// Still going, it stays tracked, and nothing polls the status file again: `read` captures
1354-
// the pane instead. Its record is marked handed back only once that result is durable
1355-
// (the executor's journal); see `TerminalRegistry.markRunDelivered`.
1357+
// the pane instead. Its record is marked handed back only once that result reaches the model;
1358+
// see `TerminalRegistry.markRunDelivered`.
13561359

13571360
const { text, truncated } = elideOutput(outcome.output)
13581361
return {

‎apps/desktop/src/main/terminal/registry-run-ledger.test.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,23 +69,29 @@ describe('stopping recorded tmux runs', () => {
6969
it("at launch, leaves the same user's runs the model can come back to, and stops the rest", async () => {
7070
const dir = ledgerDir()
7171
const previous = createRunLedger(dir)
72+
// Sim took its result: the model has the pane.
7273
previous.record(run('handed-back', '%1', true))
7374
previous.record(run('never-handed-back', '%2'))
74-
// Handed back, but the call's journal still shows no result: the model never got it.
75-
previous.record(run('lost-on-the-way', '%3', true))
75+
// Its result is in the journal, unacknowledged: recovery will hand the pane to the model.
76+
previous.record(run('on-its-way', '%3'))
7677
previous.record(run('handed-back-and-gone', '%4', true))
7778
panes.set('handed-back-and-gone', 'gone')
7879
const ledger = createRunLedger(dir)
79-
const unresolved = new Set(['call-lost-on-the-way'])
8080

81-
await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns(unresolved)
81+
await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns(
82+
new Set(['call-on-its-way'])
83+
)
8284

8385
expect(Object.fromEntries(panes)).toEqual({
8486
'never-handed-back': 'stopped',
85-
'lost-on-the-way': 'stopped',
8687
'handed-back-and-gone': 'gone',
8788
})
88-
expect(ledger.list().map((record) => record.runId)).toEqual(['handed-back'])
89+
expect(
90+
ledger
91+
.list()
92+
.map((record) => record.runId)
93+
.sort()
94+
).toEqual(['handed-back', 'on-its-way'])
8995
})
9096

9197
it('keeps meaning to stop a run a stop for everything could not confirm', async () => {

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -388,21 +388,21 @@ export class TerminalRegistry {
388388
}
389389

390390
/**
391-
* At launch, for the same user: stops a previous process's tmux run whose call never handed back
392-
* its result, or whose result the executor's journal (`unresolvedCalls`, null when unreadable)
393-
* shows as never reaching the model. A run handed back as still going, with its pane, is left
394-
* to the model, which may come back to it.
391+
* At launch, for the same user: leaves a previous process's tmux run going only when the model
392+
* has, or will get, the result that handed it back as still going: Sim acknowledged it
393+
* (`delivered`), or the executor's journal holds it for recovery to send (`pendingResults`).
394+
* Every other run is stopped, as is one a stop for everything could not confirm.
395395
*/
396-
stopUncollectableRuns(unresolvedCalls: ReadonlySet<string> | null): Promise<void> {
396+
stopUncollectableRuns(pendingResults: ReadonlySet<string>): Promise<void> {
397397
return this.stopRecordedRuns({
398398
excludeLive: true,
399-
keep: (run) => run.delivered && !run.mustStop && !unresolvedCalls?.has(run.callId),
399+
keep: (run) => !run.mustStop && (run.delivered || pendingResults.has(run.callId)),
400400
})
401401
}
402402

403403
/**
404-
* Notes that a call's result is durable (in the executor's journal), so a tmux run it handed
405-
* back as still going may be left to the model across a restart.
404+
* Notes that a call's result reached the model, so a tmux run it handed back as still going may
405+
* be left to the model across a restart.
406406
*/
407407
markRunDelivered(callId: string): void {
408408
const ledger = this.runLedger

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,10 @@ vi.mock('@/main/terminal/tmux', async () => {
8484
closeRunPane: async (...args: Parameters<typeof actual.closeRunPane>) => {
8585
if (!tmuxFake.on) return actual.closeRunPane(...args)
8686
if (tmuxFake.unanswered) return
87+
// An untracked run's pane is proven its own only from the run's files, read after tmux
88+
// has answered, as the real check does.
89+
await sleep(10)
90+
if (args[0].runId === null && !existsSync(args[0].statusPath)) return
8791
tmuxFake.open.delete(args[0].pane)
8892
tmuxFake.gone.add(args[0].pane)
8993
},
@@ -755,6 +759,27 @@ describe('agent commands in tmux', () => {
755759
}
756760
})
757761

762+
it("closes a finished untracked run's pane when its terminal closes", async () => {
763+
tmuxFake.on = true
764+
tmuxFake.untracked = true
765+
tmuxFake.statusPaths.clear()
766+
tmuxFake.open.clear()
767+
try {
768+
const terminal = new TerminalService({ loadCwd: () => '/tmp' })
769+
const { activeTerminalId } = terminal.start({ cols: 80, rows: 24 })
770+
await terminal.executeTool('call-old-tmux', 'run', { command: 'make build', waitSeconds: 1 })
771+
const [[pane = '', statusPath = ''] = []] = [...tmuxFake.statusPaths]
772+
writeFileSync(statusPath, '0')
773+
774+
terminal.closeTerminal(activeTerminalId as string)
775+
776+
await vi.waitFor(() => expect(tmuxFake.open.has(pane)).toBe(false))
777+
} finally {
778+
tmuxFake.on = false
779+
tmuxFake.untracked = false
780+
}
781+
})
782+
758783
it('keeps the record of a finished run whose pane tmux could not confirm closing', async () => {
759784
tmuxFake.on = true
760785
tmuxFake.statusPaths.clear()

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,8 @@ describe('stopping a run another process started, from its record', () => {
391391
})
392392

393393
expect(result).toMatchObject({ error: expect.stringContaining('was not run') })
394+
// The pane it opened and tagged is closed with it.
395+
expect(tmux.read().panes).toEqual({})
394396
await sleep(1_500)
395397
expect(existsSync(marker)).toBe(false)
396398
})

0 commit comments

Comments
 (0)