Skip to content

Commit 9025afb

Browse files
committed
fix(desktop): stop a run the model never got the result of
1 parent 8760ca7 commit 9025afb

8 files changed

Lines changed: 132 additions & 11 deletions

File tree

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

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,8 @@ function setup(
170170
const busy: boolean[] = []
171171
/** Calls whose result the executor reported as reaching the model, in order. */
172172
const delivered: string[] = []
173+
/** Calls Sim finished with without their real result reaching the model, in order. */
174+
const notDelivered: string[] = []
173175
const executor = new DesktopExecutor({
174176
client: sim.client,
175177
journal,
@@ -180,12 +182,23 @@ function setup(
180182
onBusyChange: (value) => busy.push(value),
181183
onApprovals: (items) => approvals.push(items),
182184
onResultDelivered: (toolCallId) => delivered.push(toolCallId),
185+
onResultNotDelivered: (toolCallId) => notDelivered.push(toolCallId),
183186
...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}),
184187
...(options.deliveryAwakeLimitMs !== undefined
185188
? { deliveryAwakeLimitMs: options.deliveryAwakeLimitMs }
186189
: {}),
187190
})
188-
return { sim, journal, runner, executor, onUnregistered, busy, approvals, delivered }
191+
return {
192+
sim,
193+
journal,
194+
runner,
195+
executor,
196+
onUnregistered,
197+
busy,
198+
approvals,
199+
delivered,
200+
notDelivered,
201+
}
189202
}
190203

191204
describe('claiming', () => {
@@ -207,13 +220,13 @@ describe('claiming', () => {
207220
})
208221

209222
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()
223+
const { sim, journal, runner, executor, delivered, notDelivered } = setup()
211224
runner.immediate = DONE
212225
// Sim settled this call first: the result never reached the model.
213226
sim.completionOutcome = 'superseded'
214227
sim.inbox = [callItem('call-superseded', 'chat-a')]
215228
await executor.reconcile()
216-
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
229+
await vi.waitFor(() => expect(notDelivered).toEqual(['call-superseded']))
217230
expect(delivered).toEqual([])
218231

219232
// Taken by Sim, even with nothing written locally (no OS encryption, say).
@@ -222,10 +235,40 @@ describe('claiming', () => {
222235
sim.inbox = [callItem('call-recorded', 'chat-a')]
223236
await executor.reconcile()
224237
await vi.waitFor(() => expect(delivered).toEqual(['call-recorded']))
238+
expect(notDelivered).toEqual(['call-superseded'])
239+
})
240+
241+
it('reports a result Sim settled first, while a failed send waited to retry, as never reaching the model', async () => {
242+
const { sim, runner, executor, delivered, notDelivered } = setup()
243+
// The terminal handed back a command still going, and the first send of that result fails.
244+
runner.immediate = { status: 'success', message: 'running', data: { status: 'running' } }
245+
sim.completeErrors = [new DeviceRequestError(503, 'unavailable')]
246+
sim.inbox = [callItem('call-1', 'chat-a', 'terminal')]
247+
await executor.reconcile()
248+
await vi.waitFor(() => expect(runner.started).toEqual(['call-1']))
249+
250+
// The user pressed Stop meanwhile, so Sim settled the call before the retry reached it.
251+
sim.completionOutcome = 'superseded'
252+
sim.inbox = [{ kind: 'cancel', toolCallId: 'call-1' }]
253+
await executor.reconcile()
254+
255+
await vi.waitFor(() => expect(notDelivered).toEqual(['call-1']))
256+
expect(delivered).toEqual([])
257+
})
258+
259+
it('reports a result Sim refused as never reaching the model', async () => {
260+
const { sim, runner, executor, notDelivered } = setup()
261+
runner.immediate = DONE
262+
sim.completeErrors = [new DeviceRequestError(403, 'forbidden')]
263+
sim.inbox = [callItem('call-refused', 'chat-a')]
264+
265+
await executor.reconcile()
266+
267+
await vi.waitFor(() => expect(notDelivered).toEqual(['call-refused']))
225268
})
226269

227270
it('reports no result that stands in for what the action produced as reaching the model', async () => {
228-
const { sim, runner, executor, delivered } = setup()
271+
const { sim, runner, executor, delivered, notDelivered } = setup()
229272
const standIns: DesktopToolCompletion[] = [
230273
{ status: 'cancelled', message: 'Stopped.' },
231274
{ status: 'error', message: 'Too large.', data: { resultOmitted: true } },
@@ -243,6 +286,7 @@ describe('claiming', () => {
243286
sim.inbox = [callItem('real', 'chat-a')]
244287
await executor.reconcile()
245288
await vi.waitFor(() => expect(delivered).toEqual(['real']))
289+
expect(notDelivered).toEqual(['stand-in-0', 'stand-in-1', 'stand-in-2', 'stand-in-3'])
246290
})
247291

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

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,12 @@ export interface DesktopExecutorOptions {
8787
* pane still running) is in use.
8888
*/
8989
onResultDelivered?: (toolCallId: string) => void
90+
/**
91+
* Called once Sim is done with a call without taking its real result: it settled the call
92+
* first, refused the result, or was sent one that stands in for it. The model never learns of
93+
* anything the action handed back as still going, so nothing will come back to it.
94+
*/
95+
onResultNotDelivered?: (toolCallId: string) => void
9096
maxHeldCalls?: number
9197
/** First delivery retry delay; tests shorten it. */
9298
retryBaseMs?: number
@@ -413,6 +419,7 @@ export class DesktopExecutor {
413419
sendingSince: number
414420
): Promise<void> {
415421
let pending = completion
422+
let delivered = false
416423
for (let attempt = 1; !this.disposed; attempt++) {
417424
try {
418425
const outcome = await this.options.client.complete({
@@ -422,9 +429,8 @@ export class DesktopExecutor {
422429
})
423430
logger.info('Desktop call result acknowledged', { toolCallId, outcome })
424431
// Superseded: Sim settled the call first, so this result never reached the model.
425-
if (outcome !== 'superseded' && isDeliveredResult(pending)) {
426-
this.options.onResultDelivered?.(toolCallId)
427-
}
432+
delivered = outcome !== 'superseded' && isDeliveredResult(pending)
433+
if (delivered) this.options.onResultDelivered?.(toolCallId)
428434
break
429435
} catch (error) {
430436
// Encoding failed on this machine, so nothing was sent; the same data would fail again.
@@ -481,7 +487,9 @@ export class DesktopExecutor {
481487
)
482488
}
483489
}
484-
if (!this.disposed) await this.forget(toolCallId)
490+
if (this.disposed) return
491+
if (!delivered) this.options.onResultNotDelivered?.(toolCallId)
492+
await this.forget(toolCallId)
485493
}
486494

487495
private async renew(entry: HeldCall): Promise<void> {

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ export interface DesktopExecutorServiceDeps {
6464
onBusyChange?: (busy: boolean) => void
6565
/** Sim has taken a call's real result as the call's own, so the model has it. */
6666
onResultDelivered?: (toolCallId: string) => void
67+
/** Sim is done with a call whose real result the model never got. */
68+
onResultNotDelivered?: (toolCallId: string) => void
6769
}
6870

6971
export interface DesktopExecutorService {
@@ -275,6 +277,7 @@ export function createDesktopExecutorService(
275277
...(deps.onApprovals ? { onApprovals: deps.onApprovals } : {}),
276278
...(deps.onBusyChange ? { onBusyChange: deps.onBusyChange } : {}),
277279
...(deps.onResultDelivered ? { onResultDelivered: deps.onResultDelivered } : {}),
280+
...(deps.onResultNotDelivered ? { onResultNotDelivered: deps.onResultNotDelivered } : {}),
278281
})
279282
// Recovery rewrites the journal, so what it held before is read first.
280283
await pendingResultsSnapshot()

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,9 @@ function main(): void {
610610
// A result the model has makes a tmux run it handed back as still going collectable across a
611611
// restart.
612612
onResultDelivered: (toolCallId) => terminal.markRunDelivered(toolCallId),
613+
// One the model never got leaves a run it handed back as still going with no one to come back
614+
// to it, the rule the launch sweep applies to a previous process's runs.
615+
onResultNotDelivered: (toolCallId) => void terminal.stopUndeliveredRun(toolCallId),
613616
runner: createDesktopToolRunner({
614617
preferences: () => desktopSettings.getPreferences(),
615618
accountDataAvailable,

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -938,6 +938,13 @@ export class TerminalService {
938938
await Promise.allSettled(stops)
939939
}
940940

941+
/** Stops the command a plain shell's `run` handed back as still going, if it still is. */
942+
async stopAgentCommand(toolCallId: string): Promise<void> {
943+
await Promise.allSettled(
944+
[...this.sessions.values()].map((session) => this.stopCommand(session, toolCallId))
945+
)
946+
}
947+
941948
/** Waits for the command a run started to end, up to `ms`. */
942949
private async commandEnds(
943950
session: TerminalSession,

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,23 @@ describe('stopping recorded tmux runs', () => {
119119
expect(ledger.list()).toEqual([])
120120
})
121121

122+
it("stops only the run of a call whose result never reached the model, this process's or a previous one's", async () => {
123+
const dir = ledgerDir()
124+
createRunLedger(dir).record(run('previous', '%1'))
125+
const ledger = createRunLedger(dir)
126+
ledger.record(run('current', '%2'))
127+
ledger.record(run('other', '%3'))
128+
const registry = new TerminalRegistry(undefined, undefined, ledger)
129+
130+
await registry.stopUndeliveredRun('call-current')
131+
await registry.stopUndeliveredRun('call-previous')
132+
133+
expect(panes.get('current')).toBe('stopped')
134+
expect(panes.get('previous')).toBe('stopped')
135+
expect(panes.get('other')).toBeUndefined()
136+
expect(ledger.list().map((record) => record.runId)).toEqual(['other'])
137+
})
138+
122139
it('notes the run of a call whose result reached the model as handed back', () => {
123140
const dir = ledgerDir()
124141
const ledger = createRunLedger(dir)

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -410,23 +410,39 @@ export class TerminalRegistry {
410410
if (runId) this.runLedger?.advance(runId, 'delivered')
411411
}
412412

413+
/**
414+
* Stops what a call's `run` handed back as still going once Sim is done with the call without
415+
* its result reaching the model: a plain shell's command, or a tmux run, this process's or a
416+
* previous one's.
417+
*/
418+
async stopUndeliveredRun(callId: string): Promise<void> {
419+
await Promise.allSettled(
420+
[...this.entries.values()].map((entry) => entry.service.stopAgentCommand(callId))
421+
)
422+
await this.stopRecordedRuns({ callId })
423+
}
424+
413425
/**
414426
* Stops the recorded tmux runs, each only while its pane still carries its tag, and drops the
415427
* records with nothing left to stop. `excludeLive` skips the runs this process has started;
416-
* `keep` names runs to leave going, such as a previous process's runs whose results the model
417-
* already has and may come back to.
428+
* `callId` limits it to the run that call started; `keep` names runs to leave going, such as a
429+
* previous process's runs whose results the model already has and may come back to.
418430
*/
419431
async stopRecordedRuns(
420432
options: {
421433
excludeLive?: boolean
434+
callId?: string
422435
/** Runs to leave going; their records are only dropped once their panes are gone. */
423436
keep?: (run: RunRecord) => boolean
424437
} = {}
425438
): Promise<void> {
426439
const ledger = this.runLedger
427440
if (!ledger) return
441+
const runs = ledger
442+
.list({ excludeLive: options.excludeLive })
443+
.filter((run) => options.callId === undefined || run.callId === options.callId)
428444
await Promise.allSettled(
429-
ledger.list({ excludeLive: options.excludeLive }).map(async (run) => {
445+
runs.map(async (run) => {
430446
let keeping = options.keep?.(run) ?? false
431447
let state = keeping ? await recordedRunState(run, process.env) : 'ours'
432448
// A kept run whose command already ended leaves only its dead pane (`remain-on-exit`).

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ const { stubSessions } = vi.hoisted(() => ({
106106
setInterruptible(interruptible: boolean): void
107107
/** Ends the running command the way its process exiting would. */
108108
finishRun(exitCode: number): void
109+
/** Returns the run as still going, its command left running, as a wait that ran out does. */
110+
handBack(): void
109111
kill: ReturnType<typeof vi.fn>
110112
readonly runningToolCallId: string | null
111113
}
@@ -158,6 +160,11 @@ vi.mock('@/main/terminal/session', async () => {
158160
state.interruptible = interruptible
159161
},
160162
finishRun,
163+
handBack: () => {
164+
const resolve = state.resolveRun
165+
state.resolveRun = null
166+
resolve?.({ status: 'running', exitCode: null, terminalId })
167+
},
161168
runCommand: (_command: string, toolCallId: string) =>
162169
new Promise((resolve) => {
163170
state.busy = true
@@ -572,6 +579,22 @@ describe('stopping a tool call', () => {
572579
await expect(running).resolves.toMatchObject({ ok: true, result: { exitCode: 130 } })
573580
})
574581

582+
it('stops the command a run handed back as still going, once asked for that call', async () => {
583+
const { terminal, session } = cancellableService()
584+
const { running } = await startRun(terminal, session, 'call-handed-back')
585+
session.handBack()
586+
await expect(running).resolves.toMatchObject({ ok: true, result: { status: 'running' } })
587+
// The call is over, so a Stop for it in flight finds nothing.
588+
await expect(terminal.cancelTool('call-handed-back')).resolves.toBe(false)
589+
590+
await terminal.stopAgentCommand('call-other')
591+
expect(session.kill).not.toHaveBeenCalled()
592+
593+
await terminal.stopAgentCommand('call-handed-back')
594+
expect(session.kill).toHaveBeenCalledWith('SIGINT')
595+
expect(session.runningToolCallId).toBeNull()
596+
})
597+
575598
it('leaves a command the user started alone at sign-out', async () => {
576599
const { terminal, session, processGroups } = cancellableService()
577600
session.setBusy(true)

0 commit comments

Comments
 (0)