From b32bc633466fdca29c008d6e4b34ca6c916b9283 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 21:13:41 -0700 Subject: [PATCH 1/6] fix(desktop): stop the agent's tmux runs a previous process left going A tmux run outlives the app, but only the process that started it knew about it: after a crash, a quit, or a crash mid-sign-out, the next launch had nothing to stop, and a chat put away took its runs out of reach of sign-out too. - Each tagged run is recorded in userData (outside account data): its tag, its pane and its tmux server's socket, nothing else. The record goes once the run has ended, its pane is gone, or Sim has stopped it. - Sign-out, switching Terminal off and the launch-time account recovery stop every recorded run; every launch stops the runs a previous process left going. A pane is only acted on, on the run's own server, while it still carries the run's tag. - Restart semantics follow the executor's journal: a call claimed before the crash is settled as outcome unknown and nothing reattaches to its command, so a run still going belongs to a call no one can collect; a run that finished is only cleaned up. --- apps/desktop/src/main/index.ts | 27 +++-- apps/desktop/src/main/terminal/index.ts | 22 +++- .../main/terminal/registry-run-ledger.test.ts | 66 +++++++++++ apps/desktop/src/main/terminal/registry.ts | 33 +++++- .../src/main/terminal/run-ledger.test.ts | 63 +++++++++++ apps/desktop/src/main/terminal/run-ledger.ts | 105 ++++++++++++++++++ .../desktop/src/main/terminal/service.test.ts | 34 +++++- apps/desktop/src/main/terminal/tmux.test.ts | 81 +++++++++++++- apps/desktop/src/main/terminal/tmux.ts | 64 ++++++++++- 9 files changed, 480 insertions(+), 15 deletions(-) create mode 100644 apps/desktop/src/main/terminal/registry-run-ledger.test.ts create mode 100644 apps/desktop/src/main/terminal/run-ledger.test.ts create mode 100644 apps/desktop/src/main/terminal/run-ledger.ts diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 40d44cf2ce5..57b18bec8ad 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -101,6 +101,7 @@ import { import { setShellTheme } from '@/main/shell-theme' import { attachTelemetryPolicy } from '@/main/telemetry-policy' import { TerminalRegistry } from '@/main/terminal/registry' +import { createRunLedger } from '@/main/terminal/run-ledger' import { installTray, type TrayHandle } from '@/main/tray' import { checkForUpdatesInteractive, initUpdater, type UpdaterHandle } from '@/main/updater' import { installBrowserUserAgent } from '@/main/user-agent' @@ -167,15 +168,20 @@ function main(): void { ), }) const scopeEvents = new ScopedEventRouter() - const terminal = new TerminalRegistry({ - load: (scopeId) => desktopChatSessions.getTerminal(processOrigin, scopeId) ?? undefined, - save: (scopeId, snapshot) => desktopChatSessions.setTerminal(processOrigin, scopeId, snapshot), - migrate: (fromScopeId, toScopeId) => - desktopChatSessions.migrateTerminal(processOrigin, fromScopeId, toScopeId), - disposeScope: (scopeId) => { - desktopChatSessions.deleteScope(processOrigin, scopeId) + const terminal = new TerminalRegistry( + { + load: (scopeId) => desktopChatSessions.getTerminal(processOrigin, scopeId) ?? undefined, + save: (scopeId, snapshot) => + desktopChatSessions.setTerminal(processOrigin, scopeId, snapshot), + migrate: (fromScopeId, toScopeId) => + desktopChatSessions.migrateTerminal(processOrigin, fromScopeId, toScopeId), + disposeScope: (scopeId) => { + desktopChatSessions.deleteScope(processOrigin, scopeId) + }, }, - }) + undefined, + createRunLedger(join(userDataPath, 'terminal-runs')) + ) const preloadPath = join(__dirname, 'preload.cjs') const windows = new Set() @@ -804,6 +810,11 @@ function main(): void { } } + // A tmux run the previous process left going belongs to a call it can no longer report (its + // journal settles it as outcome unknown) or to a chat view that is gone: nothing will collect + // what it does, so it is stopped, while its pane still carries its tag. + void terminal.stopRecordedRuns({ excludeLive: true }) + if (!accountDataAvailable()) { logger.warn( 'Account-bearing browser, terminal, and local filesystem APIs are unavailable until local recovery succeeds' diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index c8de703d177..c56bed043df 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -38,6 +38,7 @@ import { resourceTabTargetIndex, } from '@/main/resource-shortcuts' import { readForegroundProcessGroup, signalProcessGroup } from '@/main/terminal/process-group' +import type { RunLedger } from '@/main/terminal/run-ledger' import { elide, TerminalSession } from '@/main/terminal/session' import { activePane, @@ -156,6 +157,8 @@ export interface TerminalServiceOptions { canSpawn?(): boolean /** Reads and signals a terminal's foreground process group; the OS's by default. */ processGroups?: TerminalProcessGroups + /** Where tagged tmux runs are recorded, so a later process can still stop them. */ + runLedger?: RunLedger } interface TerminalProcessGroups { @@ -490,7 +493,10 @@ export class TerminalService { private async reapFinishedRuns(terminalId: string, env: NodeJS.ProcessEnv): Promise { // A closed tab's run whose pane has since gone (its command ended) needs no stopping. for (const [handle, orphanEnv] of this.orphanedRuns) { - if ((await runPaneState(handle, orphanEnv)) === 'gone') this.orphanedRuns.delete(handle) + if ((await runPaneState(handle, orphanEnv)) === 'gone') { + this.orphanedRuns.delete(handle) + this.forgetRun(handle) + } } for (const handle of this.pendingRuns.get(terminalId) ?? []) { if (this.awaitedRuns.has(handle)) continue @@ -499,11 +505,17 @@ export class TerminalService { // A pane kept open after its command ended (`remain-on-exit`) closes with its run. if (complete) await closeRunPane(handle, env) this.untrackRun(terminalId, handle) + this.forgetRun(handle) handle.dispose() } } } + /** Drops a run's record once nothing of it is left for any process to stop. */ + private forgetRun(handle: TmuxRunHandle): void { + if (handle.runId) this.options.runLedger?.forget(handle.runId) + } + /** Removes a run's files now, or once the call still reading them is done with them. */ private releaseRun(handle: TmuxRunHandle): void { if (this.awaitedRuns.has(handle)) this.releasedAwaitedRuns.add(handle) @@ -1282,6 +1294,13 @@ export class TerminalService { await this.reapFinishedRuns(terminal.terminalId, terminal.env) const handle = await startRun(session, command, terminal.currentCwd, terminal.env) if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error) + if (handle.runId && handle.socket) { + this.options.runLedger?.record({ + runId: handle.runId, + pane: handle.pane, + socket: handle.socket, + }) + } // Tracked from the moment its window exists, so sign-out can stop it even mid-wait. const pending = this.pendingRuns.get(terminal.terminalId) if (pending) pending.push(handle) @@ -1316,6 +1335,7 @@ export class TerminalService { if (outcome.done) { await closeRunPane(handle, terminal.env) this.untrackRun(terminal.terminalId, handle) + this.forgetRun(handle) handle.dispose() } // Still going, it stays tracked, and nothing polls the status file again: `read` captures diff --git a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts new file mode 100644 index 00000000000..55ce31dcea6 --- /dev/null +++ b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts @@ -0,0 +1,66 @@ +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => import('@/test/electron-mock')) + +/** How each recorded run's pane answers when stopped: still tracked by tmux or not. */ +const { panes } = vi.hoisted(() => ({ + panes: new Map(), +})) + +vi.mock('@/main/terminal/tmux', async () => { + const actual = + await vi.importActual('@/main/terminal/tmux') + return { + ...actual, + stopRecordedRun: async (run: { runId: string }) => panes.get(run.runId) ?? 'gone', + } +}) + +import { TerminalRegistry } from '@/main/terminal/registry' +import { createRunLedger } from '@/main/terminal/run-ledger' + +const dirs: string[] = [] + +function ledgerDir(): string { + const dir = mkdtempSync(join(tmpdir(), 'sim-registry-ledger-')) + dirs.push(dir) + return join(dir, 'terminal-runs') +} + +function run(runId: string, pane: string) { + return { runId, pane, socket: '/tmp/tmux-501/default' } +} + +afterEach(() => { + panes.clear() + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) +}) + +describe('stopping recorded tmux runs', () => { + it('forgets each run once nothing of it is left, and keeps one tmux could not answer for', async () => { + const dir = ledgerDir() + const previous = createRunLedger(dir) + previous.record(run('stopped', '%1')) + previous.record(run('unanswered', '%2')) + panes.set('unanswered', 'unknown') + const ledger = createRunLedger(dir) + + await new TerminalRegistry(undefined, undefined, ledger).stopAgentCommands() + + expect(ledger.list().map((record) => record.runId)).toEqual(['unanswered']) + }) + + it("at launch, stops the previous process's runs and none this one has started", async () => { + const dir = ledgerDir() + createRunLedger(dir).record(run('previous', '%1')) + const ledger = createRunLedger(dir) + ledger.record(run('current', '%2')) + + await new TerminalRegistry(undefined, undefined, ledger).stopRecordedRuns({ excludeLive: true }) + + expect(ledger.list().map((record) => record.runId)).toEqual(['current']) + }) +}) diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 0bdf7548537..3b5e2f22c44 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -19,6 +19,11 @@ import { type TerminalServiceOptions, type TerminalSink, } from '@/main/terminal' +import type { RunLedger } from '@/main/terminal/run-ledger' +import { stopRecordedRun } from '@/main/terminal/tmux' + +/** How long a recorded run gets to end on Ctrl-C before its pane is closed. */ +const RECORDED_RUN_GRACE_MS = 2_000 /** Native PTYs and their headless xterm buffers are process-wide resources. */ export const MAX_TERMINALS_PER_PROCESS = 48 @@ -106,7 +111,8 @@ export class TerminalRegistry { constructor( private readonly persistence?: TerminalScopePersistence, - private readonly serviceFactory: TerminalServiceFactory = createTerminalService + private readonly serviceFactory: TerminalServiceFactory = createTerminalService, + private readonly runLedger?: RunLedger ) {} setSink(sink: ScopedTerminalSink | null): void { @@ -369,11 +375,33 @@ export class TerminalRegistry { return true } - /** Stops every command the agent started in any chat's terminals; the user's own are untouched. */ + /** + * Stops every command the agent started in any chat's terminals; the user's own are untouched. + * That includes tmux runs no live terminal holds any more: a chat put away, or a previous + * process that quit or crashed while they ran. + */ async stopAgentCommands(): Promise { await Promise.allSettled( [...this.entries.values()].map((entry) => entry.service.stopAgentCommands()) ) + await this.stopRecordedRuns() + } + + /** + * Stops the recorded tmux runs, each only while its pane still carries its tag, and drops the + * records with nothing left to stop. At launch it skips the runs this process has started since: + * every other run belongs to a call the previous process can no longer report, which its + * journal settles as outcome unknown, so nothing is left to collect what it does. + */ + async stopRecordedRuns(options: { excludeLive?: boolean } = {}): Promise { + const ledger = this.runLedger + if (!ledger) return + await Promise.allSettled( + ledger.list(options).map(async (run) => { + const state = await stopRecordedRun(run, process.env, RECORDED_RUN_GRACE_MS) + if (state === 'gone') ledger.forget(run.runId) + }) + ) } /** Tears down every shell owned by every chat scope. */ @@ -405,6 +433,7 @@ export class TerminalRegistry { service: this.serviceFactory(scope, { loadCwd: () => this.entries.get(scope)?.persisted?.tabs[0]?.cwd, canSpawn: () => this.liveTerminalCount() < MAX_TERMINALS_PER_PROCESS, + runLedger: this.runLedger, }), persisted, restoreApplied: false, diff --git a/apps/desktop/src/main/terminal/run-ledger.test.ts b/apps/desktop/src/main/terminal/run-ledger.test.ts new file mode 100644 index 00000000000..b8ac8fad2b9 --- /dev/null +++ b/apps/desktop/src/main/terminal/run-ledger.test.ts @@ -0,0 +1,63 @@ +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { createRunLedger } from '@/main/terminal/run-ledger' + +const dirs: string[] = [] + +function scratch(): string { + const dir = mkdtempSync(join(tmpdir(), 'sim-run-ledger-')) + dirs.push(dir) + return join(dir, 'terminal-runs') +} + +const RUN = { runId: 'run-1', pane: '%3', socket: '/tmp/tmux-501/default' } + +afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) +}) + +describe('the tmux run ledger', () => { + it('keeps a run recorded until it is forgotten, for the next process too', () => { + const dir = scratch() + createRunLedger(dir).record(RUN) + + const nextProcess = createRunLedger(dir) + expect(nextProcess.list()).toEqual([RUN]) + + nextProcess.forget(RUN.runId) + expect(createRunLedger(dir).list()).toEqual([]) + }) + + it("leaves out this process's own runs when asked", () => { + const dir = scratch() + createRunLedger(dir).record(RUN) + const ledger = createRunLedger(dir) + ledger.record({ ...RUN, runId: 'run-2', pane: '%4' }) + + expect(ledger.list({ excludeLive: true })).toEqual([RUN]) + expect(ledger.list()).toHaveLength(2) + }) + + it('drops files nothing could act on safely', () => { + const dir = scratch() + const ledger = createRunLedger(dir) + ledger.record(RUN) + writeFileSync(join(dir, 'garbled.json'), '{not json') + writeFileSync(join(dir, 'run-9.json'), JSON.stringify({ ...RUN, socket: 'relative.sock' })) + // A record under another run's name could stop the wrong run. + writeFileSync(join(dir, 'run-8.json'), JSON.stringify({ ...RUN, runId: 'run-7' })) + + expect(ledger.list()).toEqual([RUN]) + expect(readdirSync(dir)).toEqual(['run-1.json']) + }) + + it('records nothing for a run tag that is not a plain id', () => { + const dir = scratch() + const ledger = createRunLedger(dir) + ledger.record({ ...RUN, runId: '../escape' }) + + expect(ledger.list()).toEqual([]) + }) +}) diff --git a/apps/desktop/src/main/terminal/run-ledger.ts b/apps/desktop/src/main/terminal/run-ledger.ts new file mode 100644 index 00000000000..e01444a34bd --- /dev/null +++ b/apps/desktop/src/main/terminal/run-ledger.ts @@ -0,0 +1,105 @@ +/** + * A durable record of the agent's tagged tmux runs, so a later process can still stop them. + * + * A tmux run outlives the app: quitting, crashing or signing out mid-teardown leaves its command + * going in the user's tmux server, and the next launch would otherwise know nothing about it. + * Each record names the run's tag, its pane and its tmux server's socket, and nothing else: no + * command line and no output, since it lives outside the account's encrypted data. A record is + * written once the pane is tagged and removed once the run has ended, its pane is gone, or Sim has + * stopped it. + */ + +import { mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' + +const logger = createLogger('DesktopTerminalRunLedger') + +/** One recorded run: what it takes to find its pane again, on the server it ran on. */ +export interface RunRecord { + /** The tag on the run's pane; only a pane carrying it is ever acted on. */ + runId: string + pane: string + /** The tmux server's socket, so a different server is never asked about this pane. */ + socket: string +} + +export interface RunLedger { + record(run: RunRecord): void + forget(runId: string): void + /** Every recorded run; `excludeLive` leaves out runs this process recorded. */ + list(options?: { excludeLive?: boolean }): RunRecord[] +} + +/** Run tags are generated ids; anything else in the directory is not a record. */ +const RUN_ID = /^[A-Za-z0-9_-]{1,128}$/ + +function parseRecord(text: string): RunRecord | null { + try { + const parsed = JSON.parse(text) as Partial + if ( + typeof parsed.runId === 'string' && + RUN_ID.test(parsed.runId) && + typeof parsed.pane === 'string' && + /^%\d+$/.test(parsed.pane) && + typeof parsed.socket === 'string' && + parsed.socket.startsWith('/') + ) { + return { runId: parsed.runId, pane: parsed.pane, socket: parsed.socket } + } + } catch { + // Unreadable: treated as no record below. + } + return null +} + +export function createRunLedger(dir: string): RunLedger { + /** Runs recorded by this process, still going as far as it knows. */ + const live = new Set() + const pathFor = (runId: string) => join(dir, `${runId}.json`) + + return { + record(run) { + if (!RUN_ID.test(run.runId)) return + try { + mkdirSync(dir, { recursive: true, mode: 0o700 }) + writeFileSync(pathFor(run.runId), JSON.stringify(run), { mode: 0o600 }) + live.add(run.runId) + } catch (error) { + logger.warn('Could not record a tmux run', { error: getErrorMessage(error) }) + } + }, + forget(runId) { + live.delete(runId) + if (!RUN_ID.test(runId)) return + rmSync(pathFor(runId), { force: true }) + }, + list(options = {}) { + let names: string[] + try { + names = readdirSync(dir) + } catch { + return [] + } + const runs: RunRecord[] = [] + for (const name of names) { + if (!name.endsWith('.json')) continue + let record: RunRecord | null = null + try { + record = parseRecord(readFileSync(join(dir, name), 'utf8')) + } catch { + record = null + } + if (!record || `${record.runId}.json` !== name) { + // Nothing could act on it safely; it only takes up space. + rmSync(join(dir, name), { force: true }) + continue + } + if (options.excludeLive && live.has(record.runId)) continue + runs.push(record) + } + return runs + }, + } +} diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 55204d4ce8e..5cdafc2d222 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { TerminalService } from '@/main/terminal' +import { createRunLedger } from '@/main/terminal/run-ledger' /** * A tmux attachment the service sees only when a test turns it on. Runs get real status files; @@ -45,7 +46,8 @@ vi.mock('@/main/terminal/tmux', async () => { return { window: `@${pane.slice(1)}`, pane, - runId: tmuxFake.untracked ? null : `run-${pane}`, + runId: tmuxFake.untracked ? null : `run-${pane.slice(1)}`, + socket: tmuxFake.untracked ? null : '/tmp/tmux-fake/default', outPath: join(dir, 'out'), statusPath, dispose: () => rmSync(dir, { recursive: true, force: true }), @@ -623,6 +625,36 @@ describe('agent commands in tmux', () => { } }) + it('keeps a record of a tagged run exactly as long as the run goes on', async () => { + tmuxFake.on = true + tmuxFake.statusPaths.clear() + const ledgerDir = join(mkdtempSync(join(tmpdir(), 'sim-ledger-')), 'terminal-runs') + const ledger = createRunLedger(ledgerDir) + try { + const terminal = new TerminalService({ loadCwd: () => '/tmp', runLedger: ledger }) + terminal.start({ cols: 80, rows: 24 }) + await terminal.executeTool('call-long', 'run', { command: 'make build', waitSeconds: 1 }) + const [[pane = '', statusPath = ''] = []] = [...tmuxFake.statusPaths] + + // Still going after its call returned: a later process must be able to find it. + expect(createRunLedger(ledgerDir).list()).toEqual([ + { runId: `run-${pane.slice(1)}`, pane, socket: '/tmp/tmux-fake/default' }, + ]) + + writeFileSync(statusPath, '0') + await terminal.executeTool('call-next', 'run', { command: 'ls', waitSeconds: 1 }) + + // The first finished and is forgotten; the one still going is recorded in its place. + expect( + createRunLedger(ledgerDir) + .list() + .map((run) => run.pane) + ).toEqual([[...tmuxFake.statusPaths.keys()][1]]) + } finally { + tmuxFake.on = false + } + }) + it("closes a run's pane when a later run reaps it after it finished", async () => { tmuxFake.on = true tmuxFake.statusPaths.clear() diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 88c1ccb1942..0b4785f9eb0 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -12,6 +12,7 @@ import { resolveAttachment, runPaneState, startRun, + stopRecordedRun, stopRun, type TmuxRunHandle, } from '@/main/terminal/tmux' @@ -71,6 +72,7 @@ describe('run status files', () => { window: '@1', pane: '%1', runId: 'run-1', + socket: null, outPath: join(dir, 'out'), statusPath: join(dir, 'status'), dispose: () => {}, @@ -119,6 +121,8 @@ describe('run status files', () => { * reshapes the server directly (a split, a closed window, a restart) by rewriting that file. */ interface FakeTmuxState { + /** The server's socket; `-S` naming any other reaches no server. */ + socket: string nextWindow: number nextPane: number panes: Record; command?: string }> @@ -138,8 +142,16 @@ const FAKE_TMUX = ` const fs = require('node:fs') const file = process.env.FAKE_TMUX_STATE const state = JSON.parse(fs.readFileSync(file, 'utf8')) -const args = process.argv.slice(2) +let args = process.argv.slice(2) const save = () => fs.writeFileSync(file, JSON.stringify(state)) +// \`-S socket\` names the server; any server but this one does not exist. +if (args[0] === '-S') { + if (args[1] !== state.socket) { + process.stderr.write('no server running on ' + args[1]) + process.exit(1) + } + args = args.slice(2) +} const target = () => args[args.indexOf('-t') + 1] const fail = (message) => { process.stderr.write(message); process.exit(1) } // Prints a format's output as tmux 3.4 and 3.5 do: a backslash doubled, and every other control @@ -187,6 +199,8 @@ switch (args[0]) { ? '' : name === 'pane_id' ? target() + : name === 'socket_path' + ? state.socket : name === 'pane_start_command' ? (pane.command ?? '') : (pane.options[name] ?? '') @@ -224,7 +238,7 @@ function fakeTmux(options: { exec?: boolean } = {}) { writeFileSync(binary, `#!${process.execPath}\n${FAKE_TMUX}`) chmodSync(binary, 0o755) const write = (state: FakeTmuxState) => writeFileSync(stateFile, JSON.stringify(state)) - write({ nextWindow: 0, nextPane: 0, panes: {}, log: [] }) + write({ nextWindow: 0, nextPane: 0, panes: {}, log: [], socket: join(dir, 'server.sock') }) const read = (): FakeTmuxState => JSON.parse(readFileSync(stateFile, 'utf8')) return { dir, @@ -274,6 +288,69 @@ describe('finding the tmux session a shell runs', () => { }) }) +describe('stopping a run another process started, from its record', () => { + const dirs: string[] = [] + + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) + }) + + async function recorded(tmux: ReturnType) { + dirs.push(tmux.dir) + const run = await startRun('agent', 'sleep 600', null, tmux.env) + if ('error' in run) throw new Error(run.error) + return { run, record: { runId: run.runId ?? '', pane: run.pane, socket: run.socket ?? '' } } + } + + it('records the server a tagged run runs on', async () => { + const tmux = fakeTmux() + const { run } = await recorded(tmux) + + expect(run.socket).toBe(tmux.read().socket) + }) + + it('interrupts and then closes the pane while it still carries the run tag', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + + expect(await stopRecordedRun(record, tmux.env, 0)).toBe('gone') + expect(tmux.read().log).toEqual([`send-keys ${record.pane} C-c`, `kill-pane ${record.pane}`]) + expect(tmux.read().panes).toEqual({}) + }) + + it('never touches a pane that took the recorded id after tmux restarted', async () => { + const tmux = fakeTmux() + const { run, record } = await recorded(tmux) + tmux.restart() + const state = tmux.read() + state.panes[run.pane] = { window: run.window, options: {}, command: 'zsh' } + tmux.write(state) + + expect(await stopRecordedRun(record, tmux.env, 0)).toBe('gone') + expect(tmux.read().log).toEqual([]) + expect(Object.keys(tmux.read().panes)).toEqual([run.pane]) + }) + + it('never asks a different tmux server about the pane', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + + const elsewhere = { ...record, socket: join(tmux.dir, 'another.sock') } + expect(await stopRecordedRun(elsewhere, tmux.env, 0)).toBe('gone') + expect(tmux.read().log).toEqual([]) + expect(Object.keys(tmux.read().panes)).toEqual([record.pane]) + }) + + it('leaves the record for later while tmux cannot be asked', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + tmux.write({ ...tmux.read(), fail: { 'display-message': 'server exited unexpectedly' } }) + + expect(await stopRecordedRun(record, tmux.env, 0)).toBe('unknown') + expect(tmux.read().log).toEqual([]) + }) +}) + describe('stopping a tmux run touches only its own pane', () => { const dirs: string[] = [] diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 6fb40d82ba3..a91d24597e2 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -317,6 +317,8 @@ export interface TmuxRunHandle { * nothing ever stops it, since nothing could tell its pane from one of the user's. */ runId: string | null + /** The tmux server's socket, for a record a later process can act on; null if unknown. */ + socket: string | null outPath: string statusPath: string dispose(): void @@ -439,6 +441,11 @@ export async function startRun( }) } const runId = tagged.ok ? tag : null + // Which server the pane lives on, so a later process stops it there and nowhere else. + const shown = runId + ? await runTmux(['display-message', '-p', '-t', pane, '#{socket_path}'], env) + : null + const socket = shown?.ok && shown.stdout.trim().startsWith('/') ? shown.stdout.trim() : null try { writeFileSync(goPath, '') } catch (error) { @@ -446,7 +453,7 @@ export async function startRun( return { error: `The command could not be started: ${getErrorMessage(error)}` } } - return { window, pane, runId, outPath, statusPath, dispose } + return { window, pane, runId, socket, outPath, statusPath, dispose } } /** @@ -489,6 +496,61 @@ export async function stopRun( if (!isRunComplete(handle)) await closeRunPane(handle, env) } +/** A recorded run as a later process finds it: its pane, its tag and its server. */ +export interface RecordedRun { + runId: string + pane: string + socket: string +} + +/** + * Whether a recorded run's pane still carries its tag, asked of the run's own server only: `gone` + * when that server or pane no longer exists or the pane is not tagged as this run's, `unknown` + * when tmux could not be asked. + */ +async function recordedRunState( + run: RecordedRun, + env: NodeJS.ProcessEnv +): Promise<'ours' | 'gone' | 'unknown'> { + const shown = await runTmux( + ['-S', run.socket, 'display-message', '-p', '-t', run.pane, `#{${RUN_ID_OPTION}}`], + env + ) + if (shown.ok) return shown.stdout.trim() === run.runId ? 'ours' : 'gone' + return /can't find|no server running|error connecting|no such file/i.test(shown.stderr) + ? 'gone' + : 'unknown' +} + +/** + * Stops a run another process started, from its record: Ctrl-C in its pane, then closing that pane + * if the command ignored it. Every step first checks, on the run's own server, that the pane still + * carries the run's tag, so a pane that took its id after a tmux restart is never touched. + * Resolves `gone` once nothing of the run is left to stop, and `unknown` when tmux could not say. + */ +export async function stopRecordedRun( + run: RecordedRun, + env: NodeJS.ProcessEnv, + graceMs: number +): Promise<'gone' | 'unknown'> { + const before = await recordedRunState(run, env) + if (before !== 'ours') return before + await runTmux(['-S', run.socket, 'send-keys', '-t', run.pane, 'C-c'], env) + const deadline = Date.now() + graceMs + let state: 'ours' | 'gone' | 'unknown' = 'ours' + while (Date.now() < deadline) { + await sleep(100) + state = await recordedRunState(run, env) + if (state !== 'ours') break + } + if (state === 'ours') { + if ((await recordedRunState(run, env)) !== 'ours') return 'gone' + await runTmux(['-S', run.socket, 'kill-pane', '-t', run.pane], env) + state = await recordedRunState(run, env) + } + return state === 'gone' ? 'gone' : 'unknown' +} + function readIfPresent(path: string): string | null { try { return readFileSync(path, 'utf8') From 84ea78837fff5c24a3e94b1251d7d11650164201 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 21:46:26 -0700 Subject: [PATCH 2/6] fix(desktop): record a tmux run before its command may start, and never lose a record tmux could not answer for - A tagged run's record is saved before its start gate opens; a run whose record could not be saved (no socket, or the write failed) never starts. - Records are written atomically; an unfinished write's temporary file is cleaned up, and a record that cannot be removed is logged and skipped rather than failing the sweep or the call. - Stopping a recorded run reports `unknown` whenever tmux could not answer, so its record stays for the next sweep; its pane is closed only while it is confirmed the run's. - A service rebuilt after a failed restore records its runs too, and a run that finished before its terminal closed is forgotten then. --- apps/desktop/src/main/terminal/index.ts | 16 ++--- .../src/main/terminal/registry.test.ts | 29 ++++++++ apps/desktop/src/main/terminal/registry.ts | 1 + .../src/main/terminal/run-ledger.test.ts | 33 ++++++++- apps/desktop/src/main/terminal/run-ledger.ts | 56 ++++++++------- .../desktop/src/main/terminal/service.test.ts | 69 +++++++++++++++++-- apps/desktop/src/main/terminal/tmux.test.ts | 49 +++++++++++++ apps/desktop/src/main/terminal/tmux.ts | 22 +++++- 8 files changed, 234 insertions(+), 41 deletions(-) diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index c56bed043df..d3c38183556 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -50,6 +50,7 @@ import { killPane, listPanes, pollRun, + type RecordedRun, resolveAttachment, runPaneState, sendKey, @@ -536,7 +537,8 @@ export class TerminalService { const pending = this.pendingRuns.get(terminalId) if (!pending) return for (const handle of pending) { - // An untracked run is never stopped, so there is nothing to keep it for. + // A finished run needs no record; an untracked one is never stopped, so is not kept either. + if (isRunComplete(handle)) this.forgetRun(handle) if (env && handle.runId !== null && !isRunComplete(handle)) this.orphanedRuns.set(handle, env) this.releaseRun(handle) } @@ -1292,15 +1294,11 @@ export class TerminalService { const started = Date.now() await this.reapFinishedRuns(terminal.terminalId, terminal.env) - const handle = await startRun(session, command, terminal.currentCwd, terminal.env) + const ledger = this.options.runLedger + const handle = await startRun(session, command, terminal.currentCwd, terminal.env, { + ...(ledger ? { beforeStart: (run: RecordedRun) => ledger.record(run) } : {}), + }) if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error) - if (handle.runId && handle.socket) { - this.options.runLedger?.record({ - runId: handle.runId, - pane: handle.pane, - socket: handle.socket, - }) - } // Tracked from the moment its window exists, so sign-out can stop it even mid-wait. const pending = this.pendingRuns.get(terminal.terminalId) if (pending) pending.push(handle) diff --git a/apps/desktop/src/main/terminal/registry.test.ts b/apps/desktop/src/main/terminal/registry.test.ts index 3926b3cfd79..5f723173378 100644 --- a/apps/desktop/src/main/terminal/registry.test.ts +++ b/apps/desktop/src/main/terminal/registry.test.ts @@ -1,4 +1,5 @@ import { tmpdir } from 'node:os' +import { join } from 'node:path' import type { TerminalCommandEvent } from '@sim/terminal-protocol' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -89,11 +90,13 @@ vi.mock('@/main/terminal/session', () => ({ }, })) +import { TerminalService, type TerminalServiceOptions } from '@/main/terminal' import { type ScopedTerminalSink, TerminalRegistry, type TerminalScopePersistence, } from '@/main/terminal/registry' +import { createRunLedger } from '@/main/terminal/run-ledger' function registry(): TerminalRegistry { return new TerminalRegistry() @@ -186,6 +189,32 @@ describe('TerminalRegistry', () => { terminals.dispose() }) + it('gives a service rebuilt after a failed restore the run ledger too', () => { + const ledger = createRunLedger(join(tmpdir(), `sim-registry-ledger-${process.pid}`)) + const built: Array = [] + const persistence: TerminalScopePersistence = { + load: () => ({ v: 1 as const, tabs: [{ cwd: tmpdir() }, { cwd: tmpdir() }], activeIndex: 0 }), + save: () => true, + migrate: () => true, + disposeScope: () => {}, + } + const terminals = new TerminalRegistry( + persistence, + (_scope, options) => { + built.push(options.runLedger) + return new TerminalService(options) + }, + ledger + ) + createControl.failAt = 2 + + expect(() => terminals.restoreScope('chat-A')).toThrow('PTY spawn failed') + + // The service that failed to restore, and the one built in its place. + expect(built).toHaveLength(2) + expect(built.every((runLedger) => runLedger === ledger)).toBe(true) + }) + it('rolls back a partial restore before retrying the complete descriptor', () => { const persistedTabs = [{ cwd: tmpdir() }, { cwd: process.cwd() }, { cwd: tmpdir() }] const persistence: TerminalScopePersistence = { diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 3b5e2f22c44..2122eb639d4 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -486,6 +486,7 @@ export class TerminalRegistry { replacement = this.serviceFactory(entry.scope, { loadCwd: () => entry.persisted?.tabs[0]?.cwd, canSpawn: () => this.liveTerminalCount() < MAX_TERMINALS_PER_PROCESS, + runLedger: this.runLedger, }) } catch { this.entries.delete(entry.scope) diff --git a/apps/desktop/src/main/terminal/run-ledger.test.ts b/apps/desktop/src/main/terminal/run-ledger.test.ts index b8ac8fad2b9..f7120c66a46 100644 --- a/apps/desktop/src/main/terminal/run-ledger.test.ts +++ b/apps/desktop/src/main/terminal/run-ledger.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' @@ -53,6 +53,37 @@ describe('the tmux run ledger', () => { expect(readdirSync(dir)).toEqual(['run-1.json']) }) + it('cleans up after a write that never finished, keeping the saved record', () => { + const dir = scratch() + const ledger = createRunLedger(dir) + ledger.record(RUN) + writeFileSync(join(dir, 'run-2.json.4242.1.tmp'), '{"runId":"run-2","pa') + + expect(ledger.list()).toEqual([RUN]) + expect(readdirSync(dir)).toEqual(['run-1.json']) + }) + + it('keeps sweeping, and lets a call finish, when an entry cannot be removed', () => { + const dir = scratch() + const ledger = createRunLedger(dir) + ledger.record(RUN) + // Not a file, so removing it fails. + mkdirSync(join(dir, 'run-3.json')) + mkdirSync(join(dir, 'run-4.json')) + + expect(ledger.list()).toEqual([RUN]) + expect(() => ledger.forget('run-4')).not.toThrow() + }) + + it('reports a record it could not save, so the run is not started', () => { + const dir = scratch() + // A file where the directory should be: nothing can be saved under it. + writeFileSync(join(dir, '..', 'blocked'), '') + const ledger = createRunLedger(join(dir, '..', 'blocked')) + + expect(ledger.record(RUN)).toBe(false) + }) + it('records nothing for a run tag that is not a plain id', () => { const dir = scratch() const ledger = createRunLedger(dir) diff --git a/apps/desktop/src/main/terminal/run-ledger.ts b/apps/desktop/src/main/terminal/run-ledger.ts index e01444a34bd..7e739a9892e 100644 --- a/apps/desktop/src/main/terminal/run-ledger.ts +++ b/apps/desktop/src/main/terminal/run-ledger.ts @@ -5,39 +5,33 @@ * going in the user's tmux server, and the next launch would otherwise know nothing about it. * Each record names the run's tag, its pane and its tmux server's socket, and nothing else: no * command line and no output, since it lives outside the account's encrypted data. A record is - * written once the pane is tagged and removed once the run has ended, its pane is gone, or Sim has - * stopped it. + * saved before the run's command may start, and removed once the run has ended, its pane is gone, + * or Sim has stopped it. */ -import { mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { readdirSync, readFileSync, rmSync } from 'node:fs' import { join } from 'node:path' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' +import { writeJsonFileAtomicallySync } from '@/main/atomic-json-file' +import type { RecordedRun } from '@/main/terminal/tmux' const logger = createLogger('DesktopTerminalRunLedger') -/** One recorded run: what it takes to find its pane again, on the server it ran on. */ -export interface RunRecord { - /** The tag on the run's pane; only a pane carrying it is ever acted on. */ - runId: string - pane: string - /** The tmux server's socket, so a different server is never asked about this pane. */ - socket: string -} - export interface RunLedger { - record(run: RunRecord): void + /** Saves a run's record; false when it could not be saved, so the run must not start. */ + record(run: RecordedRun): boolean forget(runId: string): void /** Every recorded run; `excludeLive` leaves out runs this process recorded. */ - list(options?: { excludeLive?: boolean }): RunRecord[] + list(options?: { excludeLive?: boolean }): RecordedRun[] } /** Run tags are generated ids; anything else in the directory is not a record. */ const RUN_ID = /^[A-Za-z0-9_-]{1,128}$/ -function parseRecord(text: string): RunRecord | null { +function parseRecord(text: string): RecordedRun | null { try { - const parsed = JSON.parse(text) as Partial + const parsed = JSON.parse(text) as Partial if ( typeof parsed.runId === 'string' && RUN_ID.test(parsed.runId) && @@ -54,6 +48,15 @@ function parseRecord(text: string): RunRecord | null { return null } +/** Removes a file the ledger no longer needs; a failure is logged, never thrown at a caller. */ +function remove(path: string): void { + try { + rmSync(path, { force: true }) + } catch (error) { + logger.warn('Could not remove a tmux run record', { error: getErrorMessage(error) }) + } +} + export function createRunLedger(dir: string): RunLedger { /** Runs recorded by this process, still going as far as it knows. */ const live = new Set() @@ -61,19 +64,19 @@ export function createRunLedger(dir: string): RunLedger { return { record(run) { - if (!RUN_ID.test(run.runId)) return + if (!RUN_ID.test(run.runId)) return false try { - mkdirSync(dir, { recursive: true, mode: 0o700 }) - writeFileSync(pathFor(run.runId), JSON.stringify(run), { mode: 0o600 }) + writeJsonFileAtomicallySync(pathFor(run.runId), run) live.add(run.runId) + return true } catch (error) { logger.warn('Could not record a tmux run', { error: getErrorMessage(error) }) + return false } }, forget(runId) { live.delete(runId) - if (!RUN_ID.test(runId)) return - rmSync(pathFor(runId), { force: true }) + if (RUN_ID.test(runId)) remove(pathFor(runId)) }, list(options = {}) { let names: string[] @@ -82,10 +85,15 @@ export function createRunLedger(dir: string): RunLedger { } catch { return [] } - const runs: RunRecord[] = [] + const runs: RecordedRun[] = [] for (const name of names) { + // A write that never finished leaves only its temporary file behind. + if (name.endsWith('.tmp')) { + remove(join(dir, name)) + continue + } if (!name.endsWith('.json')) continue - let record: RunRecord | null = null + let record: RecordedRun | null = null try { record = parseRecord(readFileSync(join(dir, name), 'utf8')) } catch { @@ -93,7 +101,7 @@ export function createRunLedger(dir: string): RunLedger { } if (!record || `${record.runId}.json` !== name) { // Nothing could act on it safely; it only takes up space. - rmSync(join(dir, name), { force: true }) + remove(join(dir, name)) continue } if (options.excludeLive && live.has(record.runId)) continue diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 5cdafc2d222..4bb9a6893be 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -37,8 +37,20 @@ vi.mock('@/main/terminal/tmux', async () => { : actual.resolveAttachment(pid, env), startRun: async (...args: Parameters) => { if (!tmuxFake.on) return actual.startRun(...args) - const dir = mkdtempSync(join(tmpdir(), 'sim-tmux-fake-')) + const options = args[4] const pane = `%${nextPane++}` + const runId = tmuxFake.untracked ? null : `run-${pane.slice(1)}` + const socket = tmuxFake.untracked ? null : '/tmp/tmux-fake/default' + // Like the real one, a tagged run starts only once its record is saved. + if ( + runId && + socket && + options?.beforeStart && + !options.beforeStart({ runId, pane, socket }) + ) { + return { error: 'The command could not be recorded for a later stop, so it was not run.' } + } + const dir = mkdtempSync(join(tmpdir(), 'sim-tmux-fake-')) const statusPath = join(dir, 'status') writeFileSync(join(dir, 'out'), 'partial output') tmuxFake.statusPaths.set(pane, statusPath) @@ -46,8 +58,8 @@ vi.mock('@/main/terminal/tmux', async () => { return { window: `@${pane.slice(1)}`, pane, - runId: tmuxFake.untracked ? null : `run-${pane.slice(1)}`, - socket: tmuxFake.untracked ? null : '/tmp/tmux-fake/default', + runId, + socket, outPath: join(dir, 'out'), statusPath, dispose: () => rmSync(dir, { recursive: true, force: true }), @@ -628,7 +640,8 @@ describe('agent commands in tmux', () => { it('keeps a record of a tagged run exactly as long as the run goes on', async () => { tmuxFake.on = true tmuxFake.statusPaths.clear() - const ledgerDir = join(mkdtempSync(join(tmpdir(), 'sim-ledger-')), 'terminal-runs') + const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) + const ledgerDir = join(scratch, 'terminal-runs') const ledger = createRunLedger(ledgerDir) try { const terminal = new TerminalService({ loadCwd: () => '/tmp', runLedger: ledger }) @@ -652,6 +665,54 @@ describe('agent commands in tmux', () => { ).toEqual([[...tmuxFake.statusPaths.keys()][1]]) } finally { tmuxFake.on = false + rmSync(scratch, { recursive: true, force: true }) + } + }) + + it('forgets a run that finished before its terminal closed', async () => { + tmuxFake.on = true + tmuxFake.statusPaths.clear() + const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) + const ledgerDir = join(scratch, 'terminal-runs') + try { + const terminal = new TerminalService({ + loadCwd: () => '/tmp', + runLedger: createRunLedger(ledgerDir), + }) + const { activeTerminalId } = terminal.start({ cols: 80, rows: 24 }) + await terminal.executeTool('call-long', 'run', { command: 'make build', waitSeconds: 1 }) + const [[, statusPath = ''] = []] = [...tmuxFake.statusPaths] + writeFileSync(statusPath, '0') + + terminal.closeTerminal(activeTerminalId as string) + + expect(createRunLedger(ledgerDir).list()).toEqual([]) + } finally { + tmuxFake.on = false + rmSync(scratch, { recursive: true, force: true }) + } + }) + + it('never starts a tagged run it could not record', async () => { + tmuxFake.on = true + tmuxFake.statusPaths.clear() + const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) + // A file where the ledger's directory should be: no record can be saved. + writeFileSync(join(scratch, 'terminal-runs'), '') + try { + const terminal = new TerminalService({ + loadCwd: () => '/tmp', + runLedger: createRunLedger(join(scratch, 'terminal-runs')), + }) + terminal.start({ cols: 80, rows: 24 }) + + await expect( + terminal.executeTool('call-unrecorded', 'run', { command: 'make build', waitSeconds: 1 }) + ).resolves.toMatchObject({ ok: false, code: 'SPAWN_FAILED' }) + expect(tmuxFake.statusPaths.size).toBe(0) + } finally { + tmuxFake.on = false + rmSync(scratch, { recursive: true, force: true }) } }) diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 0b4785f9eb0..e9dff619bd2 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -130,6 +130,8 @@ interface FakeTmuxState { log: string[] /** Commands the fake fails, with the error tmux would print. */ fail?: Record + /** Once a key is sent, tmux stops answering: every later command fails like a dying server. */ + dieAfterKeys?: boolean /** Attached clients, as `list-clients` reports them. */ clients?: Array<{ pid: string; tty: string; session: string }> /** Commands the fake holds until the file named here exists, like a busy tmux server. */ @@ -222,6 +224,9 @@ switch (args[0]) { case 'kill-pane': { if (!state.panes[target()]) fail("can't find pane") state.log.push(args[0] + ' ' + target() + (args[0] === 'send-keys' ? ' ' + args[args.length - 1] : '')) + if (args[0] === 'send-keys' && state.dieAfterKeys) { + state.fail = { 'display-message': 'server exited unexpectedly', 'kill-pane': 'server exited unexpectedly' } + } if (args[0] === 'kill-pane') delete state.panes[target()] save() break @@ -341,6 +346,50 @@ describe('stopping a run another process started, from its record', () => { expect(Object.keys(tmux.read().panes)).toEqual([record.pane]) }) + it('keeps the record when tmux stops answering part-way through the stop', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + tmux.write({ ...tmux.read(), dieAfterKeys: true }) + + expect(await stopRecordedRun(record, tmux.env, 0)).toBe('unknown') + // Its pane could not be confirmed as the run's, so it was not closed. + expect(tmux.read().log).toEqual([`send-keys ${record.pane} C-c`]) + }) + + it('saves the record before the command may start, and never starts one it could not save', async () => { + const tmux = fakeTmux({ exec: true }) + dirs.push(tmux.dir) + const marker = join(tmux.dir, 'ran') + let ranBeforeRecord = true + const run = await startRun('agent', `touch ${JSON.stringify(marker)}`, null, tmux.env, { + beforeStart: () => { + // Time enough for a command already released to have run. + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 1_000) + ranBeforeRecord = existsSync(marker) + return true + }, + }) + if ('error' in run) throw new Error(run.error) + expect(ranBeforeRecord).toBe(false) + await expect.poll(() => existsSync(marker), { timeout: 10_000 }).toBe(true) + + const unsaved = fakeTmux({ exec: true }) + dirs.push(unsaved.dir) + const unsavedMarker = join(unsaved.dir, 'ran') + const refused = await startRun( + 'agent', + `touch ${JSON.stringify(unsavedMarker)}`, + null, + unsaved.env, + { + beforeStart: () => false, + } + ) + expect(refused).toMatchObject({ error: expect.stringContaining('was not run') }) + await sleep(1_500) + expect(existsSync(unsavedMarker)).toBe(false) + }, 20_000) + it('leaves the record for later while tmux cannot be asked', async () => { const tmux = fakeTmux() const { record } = await recorded(tmux) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index a91d24597e2..375740fc354 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -357,7 +357,14 @@ export async function startRun( session: string, command: string, cwd: string | null, - env: NodeJS.ProcessEnv + env: NodeJS.ProcessEnv, + options: { + /** + * Records a tagged run before its command may start; false keeps the command from starting, + * since a run no later process could find must not outlive this one. + */ + beforeStart?: (run: RecordedRun) => boolean + } = {} ): Promise { const dir = mkdtempSync(join(tmpdir(), 'sim-tmux-run-')) const outPath = join(dir, 'out') @@ -446,6 +453,11 @@ export async function startRun( ? await runTmux(['display-message', '-p', '-t', pane, '#{socket_path}'], env) : null const socket = shown?.ok && shown.stdout.trim().startsWith('/') ? shown.stdout.trim() : null + if (runId && options.beforeStart && !(socket && options.beforeStart({ runId, pane, socket }))) { + // Without the go file the wrapper exits by itself. + dispose() + return { error: 'The command could not be recorded for a later stop, so it was not run.' } + } try { writeFileSync(goPath, '') } catch (error) { @@ -496,10 +508,12 @@ export async function stopRun( if (!isRunComplete(handle)) await closeRunPane(handle, env) } -/** A recorded run as a later process finds it: its pane, its tag and its server. */ +/** A recorded run as a later process finds it: what it takes to find its pane again. */ export interface RecordedRun { + /** The tag on the run's pane; only a pane carrying it is ever acted on. */ runId: string pane: string + /** The tmux server's socket, so a different server is never asked about this pane. */ socket: string } @@ -543,8 +557,10 @@ export async function stopRecordedRun( state = await recordedRunState(run, env) if (state !== 'ours') break } + // The pane is closed only while it is confirmed the run's; a pane tmux could not answer for is + // left alone, and its record kept for the next sweep. + if (state === 'ours') state = await recordedRunState(run, env) if (state === 'ours') { - if ((await recordedRunState(run, env)) !== 'ours') return 'gone' await runTmux(['-S', run.socket, 'kill-pane', '-t', run.pane], env) state = await recordedRunState(run, env) } From 6c154f3bd03def2e6a5af5a043a837367704e7ad Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 22:01:45 -0700 Subject: [PATCH 3/6] fix(desktop): close a finished run's leftover pane before forgetting its record --- apps/desktop/src/main/terminal/index.ts | 8 ++++++-- apps/desktop/src/main/terminal/service.test.ts | 10 ++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index d3c38183556..051cc067475 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -537,8 +537,12 @@ export class TerminalService { const pending = this.pendingRuns.get(terminalId) if (!pending) return for (const handle of pending) { - // A finished run needs no record; an untracked one is never stopped, so is not kept either. - if (isRunComplete(handle)) this.forgetRun(handle) + // A finished run's pane may still be open (`remain-on-exit`): it is closed, while still the + // run's, before the record goes. Without the shell's environment the record stays, and the + // next sweep closes it. An untracked run is never stopped, so it is not kept either. + if (isRunComplete(handle) && env) { + void closeRunPane(handle, env).finally(() => this.forgetRun(handle)) + } if (env && handle.runId !== null && !isRunComplete(handle)) this.orphanedRuns.set(handle, env) this.releaseRun(handle) } diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 4bb9a6893be..334017da1c3 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -669,9 +669,10 @@ describe('agent commands in tmux', () => { } }) - it('forgets a run that finished before its terminal closed', async () => { + it('closes and then forgets a run that finished before its terminal closed', async () => { tmuxFake.on = true tmuxFake.statusPaths.clear() + tmuxFake.open.clear() const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) const ledgerDir = join(scratch, 'terminal-runs') try { @@ -684,9 +685,14 @@ describe('agent commands in tmux', () => { const [[, statusPath = ''] = []] = [...tmuxFake.statusPaths] writeFileSync(statusPath, '0') + const [pane = ''] = [...tmuxFake.statusPaths.keys()] + // Its dead pane is still open, as with `remain-on-exit`. + expect(tmuxFake.open.has(pane)).toBe(true) + terminal.closeTerminal(activeTerminalId as string) - expect(createRunLedger(ledgerDir).list()).toEqual([]) + await vi.waitFor(() => expect(createRunLedger(ledgerDir).list()).toEqual([])) + expect(tmuxFake.open.has(pane)).toBe(false) } finally { tmuxFake.on = false rmSync(scratch, { recursive: true, force: true }) From 10674d03ad94f6ebb6cae9a6f66a3325ab7e7b37 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 22:23:33 -0700 Subject: [PATCH 4/6] fix(desktop): leave a restarted user's collectable tmux runs going, and stop only those nothing can collect A tmux `run` that outlives its wait hands its pane back to the model as `running`, and the model may come back to it with read, input, kill or close. The launch sweep stopped every previous process's run, so a relaunch, a crash or an update restart killed a dev server or a build the model was still watching. - A record now names its call and notes when the run's result was handed back as still going. - At launch, for the same user, a run is stopped only when its result was never handed back, or when the executor's journal, read before recovery rewrites it, shows the call as claimed, started, not started or outcome unknown. Every other run is left going and its record dropped only once its pane is gone. Sign-out and the launch-time account recovery still stop every run, and quitting still stops none. - A record is kept when tmux could not confirm a finished run's pane closed, and forgotten when its run could not start after all. --- .../src/main/desktop-executor/service.test.ts | 36 ++++++++ .../src/main/desktop-executor/service.ts | 24 ++++++ apps/desktop/src/main/index.ts | 12 ++- apps/desktop/src/main/terminal/index.ts | 19 ++++- .../main/terminal/registry-run-ledger.test.ts | 45 ++++++++-- apps/desktop/src/main/terminal/registry.ts | 37 ++++++-- .../src/main/terminal/run-ledger.test.ts | 23 ++++- apps/desktop/src/main/terminal/run-ledger.ts | 51 +++++++++-- .../desktop/src/main/terminal/service.test.ts | 85 ++++++++++++++++++- apps/desktop/src/main/terminal/tmux.test.ts | 60 +++++++++++++ apps/desktop/src/main/terminal/tmux.ts | 5 +- 11 files changed, 366 insertions(+), 31 deletions(-) diff --git a/apps/desktop/src/main/desktop-executor/service.test.ts b/apps/desktop/src/main/desktop-executor/service.test.ts index 20ff6168e68..996a1bca9cf 100644 --- a/apps/desktop/src/main/desktop-executor/service.test.ts +++ b/apps/desktop/src/main/desktop-executor/service.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) import { net } from 'electron' +import { createExecutorJournal } from '@/main/desktop-executor/journal' import { createDesktopExecutorService, deviceName } from '@/main/desktop-executor/service' /** Sim's device routes, with registration answers held until the test releases them. */ @@ -95,6 +96,41 @@ async function service(protocolVersion = 1, userDataPath?: string) { return { sim, desktopExecutor, busy } } +describe('calls never handed back with a result', () => { + it('names claimed and started calls, and results that are not started or unknown', async () => { + const userData = await mkdtemp(join(tmpdir(), 'sim-executor-service-')) + const journal = createExecutorJournal(join(userData, 'desktop-executor-journal.json')) + await journal.put({ toolCallId: 'claimed', state: 'claimed', executionToken: 't1' }) + await journal.put({ toolCallId: 'started', state: 'started', executionToken: 't2' }) + await journal.put({ + toolCallId: 'unknown', + state: 'result', + executionToken: 't3', + completion: { status: 'error', message: 'x', data: { outcomeUnknown: true } }, + }) + await journal.put({ + toolCallId: 'not-started', + state: 'result', + executionToken: 't4', + completion: { status: 'error', message: 'x', data: { notStarted: true } }, + }) + await journal.put({ + toolCallId: 'handed-back', + state: 'result', + executionToken: 't5', + completion: { status: 'success', message: 'running', data: { status: 'running' } }, + }) + const { desktopExecutor } = await service(1, userData) + + expect([...((await desktopExecutor.unresolvedCalls()) ?? [])].sort()).toEqual([ + 'claimed', + 'not-started', + 'started', + 'unknown', + ]) + }) +}) + describe('desktop executor registration', () => { it('offers the device for binding once Sim enables it', async () => { const { sim, desktopExecutor } = await service() diff --git a/apps/desktop/src/main/desktop-executor/service.ts b/apps/desktop/src/main/desktop-executor/service.ts index 5812e06ae9f..fdb36597a31 100644 --- a/apps/desktop/src/main/desktop-executor/service.ts +++ b/apps/desktop/src/main/desktop-executor/service.ts @@ -68,6 +68,12 @@ export interface DesktopExecutorService { /** Re-registers after a sign-in, a session change, or a change to what this device can run. */ refreshRegistration(): void getDevice(): DesktopExecutorDevice | null + /** + * The calls the journal shows as never handed back with a real result: claimed or started with + * none, or settled as not started or outcome unknown. Read before recovery reports them; null + * when the journal cannot be read. + */ + unresolvedCalls(): Promise | null> /** Stores one entry of a claimed import, as this device's registered session. */ importEntry( request: DesktopImportEntryRequest, @@ -443,6 +449,24 @@ export function createDesktopExecutorService( getDevice() { return device }, + async unresolvedCalls() { + try { + const unresolved = new Set() + for (const entry of await journal.load()) { + const data = entry.state === 'result' ? entry.completion.data : undefined + if ( + entry.state !== 'result' || + data?.outcomeUnknown === true || + data?.notStarted === true + ) { + unresolved.add(entry.toolCallId) + } + } + return unresolved + } catch { + return null + } + }, importEntry(request, signal) { if (!client) throw new Error('The Sim desktop app is not signed in to Sim.') return client.importEntry(request, signal) diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 57b18bec8ad..793e366e657 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -810,10 +810,13 @@ function main(): void { } } - // A tmux run the previous process left going belongs to a call it can no longer report (its - // journal settles it as outcome unknown) or to a chat view that is gone: nothing will collect - // what it does, so it is stopped, while its pane still carries its tag. - void terminal.stopRecordedRuns({ excludeLive: true }) + // The same user's tmux runs from a previous process: a run whose call never handed back its + // result (or whose result the journal will report as unknown) has nothing left to collect what + // it does, so it is stopped, while its pane still carries its tag. A run already handed back as + // still going, with its pane, is left to the model, which may come back to it. Read before the + // executor starts, since its recovery rewrites the journal. + const unresolvedCalls = desktopExecutor.unresolvedCalls() + void unresolvedCalls.then((unresolved) => terminal.stopUncollectableRuns(unresolved)) if (!accountDataAvailable()) { logger.warn( @@ -964,6 +967,7 @@ function main(): void { ensureAppSession().cookies.on('changed', (_event, cookie, _cause, removed) => { if (!removed && isSessionCookieName(cookie.name)) desktopExecutor.refreshRegistration() }) + await unresolvedCalls desktopExecutor.start() } await ensureMainWindow() diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index 051cc067475..c8cd73a0672 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -541,7 +541,10 @@ export class TerminalService { // run's, before the record goes. Without the shell's environment the record stays, and the // next sweep closes it. An untracked run is never stopped, so it is not kept either. if (isRunComplete(handle) && env) { - void closeRunPane(handle, env).finally(() => this.forgetRun(handle)) + void closeRunPane(handle, env).then(async () => { + // A pane tmux could not answer for keeps its record, for the next sweep to close. + if ((await runPaneState(handle, env)) === 'gone') this.forgetRun(handle) + }) } if (env && handle.runId !== null && !isRunComplete(handle)) this.orphanedRuns.set(handle, env) this.releaseRun(handle) @@ -1053,7 +1056,7 @@ export class TerminalService { } case 'run': return tmux - ? this.runInTmux(session, tmux.session, args, latch) + ? this.runInTmux(toolCallId, session, tmux.session, args, latch) : this.run(toolCallId, session, args, latch) case 'read': { const requested = Number(args.lines) @@ -1287,6 +1290,7 @@ export class TerminalService { * see through tmux. */ private async runInTmux( + toolCallId: string, terminal: TerminalSession, session: string, args: TerminalToolArgs, @@ -1300,7 +1304,13 @@ export class TerminalService { await this.reapFinishedRuns(terminal.terminalId, terminal.env) const ledger = this.options.runLedger const handle = await startRun(session, command, terminal.currentCwd, terminal.env, { - ...(ledger ? { beforeStart: (run: RecordedRun) => ledger.record(run) } : {}), + ...(ledger + ? { + beforeStart: (run: RecordedRun) => + ledger.record({ ...run, callId: toolCallId, delivered: false }), + abandon: (runId: string) => ledger.forget(runId), + } + : {}), }) if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error) // Tracked from the moment its window exists, so sign-out can stop it even mid-wait. @@ -1341,7 +1351,8 @@ export class TerminalService { handle.dispose() } // Still going, it stays tracked, and nothing polls the status file again: `read` captures - // the pane instead. + // the pane instead. Its result now points the model at that pane, so a restart must not end it. + if (!outcome.done && handle.runId) ledger?.markDelivered(handle.runId) const { text, truncated } = elideOutput(outcome.output) return { diff --git a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts index 55ce31dcea6..ebe402ad5df 100644 --- a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts +++ b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts @@ -5,17 +5,29 @@ import { afterEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) -/** How each recorded run's pane answers when stopped: still tracked by tmux or not. */ +/** + * Each recorded run's pane as tmux has it: running, stopped by Sim, already gone, or one tmux + * cannot answer for. A pane not listed is running. + */ const { panes } = vi.hoisted(() => ({ - panes: new Map(), + panes: new Map(), })) vi.mock('@/main/terminal/tmux', async () => { const actual = await vi.importActual('@/main/terminal/tmux') + const state = (runId: string) => panes.get(runId) ?? 'running' return { ...actual, - stopRecordedRun: async (run: { runId: string }) => panes.get(run.runId) ?? 'gone', + recordedRunState: async (run: { runId: string }) => { + const pane = state(run.runId) + return pane === 'running' ? 'ours' : pane === 'unknown' ? 'unknown' : 'gone' + }, + stopRecordedRun: async (run: { runId: string }) => { + if (state(run.runId) === 'unknown') return 'unknown' + if (state(run.runId) === 'running') panes.set(run.runId, 'stopped') + return 'gone' + }, } }) @@ -30,8 +42,8 @@ function ledgerDir(): string { return join(dir, 'terminal-runs') } -function run(runId: string, pane: string) { - return { runId, pane, socket: '/tmp/tmux-501/default' } +function run(runId: string, pane: string, delivered = false) { + return { runId, pane, socket: '/tmp/tmux-501/default', callId: `call-${runId}`, delivered } } afterEach(() => { @@ -46,6 +58,7 @@ describe('stopping recorded tmux runs', () => { previous.record(run('stopped', '%1')) previous.record(run('unanswered', '%2')) panes.set('unanswered', 'unknown') + // Signing out ends a run whose result the model has, too. const ledger = createRunLedger(dir) await new TerminalRegistry(undefined, undefined, ledger).stopAgentCommands() @@ -53,6 +66,28 @@ describe('stopping recorded tmux runs', () => { expect(ledger.list().map((record) => record.runId)).toEqual(['unanswered']) }) + it("at launch, leaves the same user's runs the model can come back to, and stops the rest", async () => { + const dir = ledgerDir() + const previous = createRunLedger(dir) + previous.record(run('handed-back', '%1', true)) + previous.record(run('never-handed-back', '%2')) + // Handed back, but the call's journal still shows no result: the model never got it. + previous.record(run('lost-on-the-way', '%3', true)) + previous.record(run('handed-back-and-gone', '%4', true)) + panes.set('handed-back-and-gone', 'gone') + const ledger = createRunLedger(dir) + const unresolved = new Set(['call-lost-on-the-way']) + + await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns(unresolved) + + expect(Object.fromEntries(panes)).toEqual({ + 'never-handed-back': 'stopped', + 'lost-on-the-way': 'stopped', + 'handed-back-and-gone': 'gone', + }) + expect(ledger.list().map((record) => record.runId)).toEqual(['handed-back']) + }) + it("at launch, stops the previous process's runs and none this one has started", async () => { const dir = ledgerDir() createRunLedger(dir).record(run('previous', '%1')) diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 2122eb639d4..27e6ee0be94 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -19,8 +19,8 @@ import { type TerminalServiceOptions, type TerminalSink, } from '@/main/terminal' -import type { RunLedger } from '@/main/terminal/run-ledger' -import { stopRecordedRun } from '@/main/terminal/tmux' +import type { RunLedger, RunRecord } from '@/main/terminal/run-ledger' +import { recordedRunState, stopRecordedRun } from '@/main/terminal/tmux' /** How long a recorded run gets to end on Ctrl-C before its pane is closed. */ const RECORDED_RUN_GRACE_MS = 2_000 @@ -387,18 +387,39 @@ export class TerminalRegistry { await this.stopRecordedRuns() } + /** + * At launch, for the same user: stops a previous process's tmux run whose call never handed back + * its result, or whose result the executor's journal (`unresolvedCalls`, null when unreadable) + * shows as never reaching the model. A run handed back as still going, with its pane, is left + * to the model, which may come back to it. + */ + stopUncollectableRuns(unresolvedCalls: ReadonlySet | null): Promise { + return this.stopRecordedRuns({ + excludeLive: true, + keep: (run) => run.delivered && !unresolvedCalls?.has(run.callId), + }) + } + /** * Stops the recorded tmux runs, each only while its pane still carries its tag, and drops the - * records with nothing left to stop. At launch it skips the runs this process has started since: - * every other run belongs to a call the previous process can no longer report, which its - * journal settles as outcome unknown, so nothing is left to collect what it does. + * records with nothing left to stop. `excludeLive` skips the runs this process has started; + * `keep` names runs to leave going, such as a previous process's runs whose results the model + * already has and may come back to. */ - async stopRecordedRuns(options: { excludeLive?: boolean } = {}): Promise { + async stopRecordedRuns( + options: { + excludeLive?: boolean + /** Runs to leave going; their records are only dropped once their panes are gone. */ + keep?: (run: RunRecord) => boolean + } = {} + ): Promise { const ledger = this.runLedger if (!ledger) return await Promise.allSettled( - ledger.list(options).map(async (run) => { - const state = await stopRecordedRun(run, process.env, RECORDED_RUN_GRACE_MS) + ledger.list({ excludeLive: options.excludeLive }).map(async (run) => { + const state = options.keep?.(run) + ? await recordedRunState(run, process.env) + : await stopRecordedRun(run, process.env, RECORDED_RUN_GRACE_MS) if (state === 'gone') ledger.forget(run.runId) }) ) diff --git a/apps/desktop/src/main/terminal/run-ledger.test.ts b/apps/desktop/src/main/terminal/run-ledger.test.ts index f7120c66a46..9baf932b211 100644 --- a/apps/desktop/src/main/terminal/run-ledger.test.ts +++ b/apps/desktop/src/main/terminal/run-ledger.test.ts @@ -12,7 +12,13 @@ function scratch(): string { return join(dir, 'terminal-runs') } -const RUN = { runId: 'run-1', pane: '%3', socket: '/tmp/tmux-501/default' } +const RUN = { + runId: 'run-1', + pane: '%3', + socket: '/tmp/tmux-501/default', + callId: 'call-1', + delivered: false, +} afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) @@ -48,6 +54,11 @@ describe('the tmux run ledger', () => { writeFileSync(join(dir, 'run-9.json'), JSON.stringify({ ...RUN, socket: 'relative.sock' })) // A record under another run's name could stop the wrong run. writeFileSync(join(dir, 'run-8.json'), JSON.stringify({ ...RUN, runId: 'run-7' })) + // Only a pane id names one pane; a target like this one names whatever pane is active there. + writeFileSync( + join(dir, 'run-6.json'), + JSON.stringify({ ...RUN, runId: 'run-6', pane: 'work:0.0' }) + ) expect(ledger.list()).toEqual([RUN]) expect(readdirSync(dir)).toEqual(['run-1.json']) @@ -84,6 +95,16 @@ describe('the tmux run ledger', () => { expect(ledger.record(RUN)).toBe(false) }) + it('notes a run handed back as still going, for the next process too', () => { + const dir = scratch() + const ledger = createRunLedger(dir) + ledger.record(RUN) + + ledger.markDelivered(RUN.runId) + + expect(createRunLedger(dir).list()).toEqual([{ ...RUN, delivered: true }]) + }) + it('records nothing for a run tag that is not a plain id', () => { const dir = scratch() const ledger = createRunLedger(dir) diff --git a/apps/desktop/src/main/terminal/run-ledger.ts b/apps/desktop/src/main/terminal/run-ledger.ts index 7e739a9892e..c9e2948942c 100644 --- a/apps/desktop/src/main/terminal/run-ledger.ts +++ b/apps/desktop/src/main/terminal/run-ledger.ts @@ -18,29 +18,50 @@ import type { RecordedRun } from '@/main/terminal/tmux' const logger = createLogger('DesktopTerminalRunLedger') +/** A recorded run, with the call it belongs to and whether that call's result went back. */ +export interface RunRecord extends RecordedRun { + /** The tool call that started the run, to match it against the executor's journal. */ + callId: string + /** + * True once the run's call handed back its result while the run went on (`running`, with its + * pane): from then on the model can come back to the pane, so a restart must leave it be. + */ + delivered: boolean +} + export interface RunLedger { /** Saves a run's record; false when it could not be saved, so the run must not start. */ - record(run: RecordedRun): boolean + record(run: RunRecord): boolean + /** Notes that the run's call has handed back its result, with the run still going. */ + markDelivered(runId: string): void forget(runId: string): void /** Every recorded run; `excludeLive` leaves out runs this process recorded. */ - list(options?: { excludeLive?: boolean }): RecordedRun[] + list(options?: { excludeLive?: boolean }): RunRecord[] } /** Run tags are generated ids; anything else in the directory is not a record. */ const RUN_ID = /^[A-Za-z0-9_-]{1,128}$/ -function parseRecord(text: string): RecordedRun | null { +function parseRecord(text: string): RunRecord | null { try { - const parsed = JSON.parse(text) as Partial + const parsed = JSON.parse(text) as Partial if ( typeof parsed.runId === 'string' && RUN_ID.test(parsed.runId) && typeof parsed.pane === 'string' && /^%\d+$/.test(parsed.pane) && + typeof parsed.callId === 'string' && + typeof parsed.delivered === 'boolean' && typeof parsed.socket === 'string' && parsed.socket.startsWith('/') ) { - return { runId: parsed.runId, pane: parsed.pane, socket: parsed.socket } + return { + runId: parsed.runId, + pane: parsed.pane, + socket: parsed.socket, + callId: parsed.callId, + delivered: parsed.delivered, + } } } catch { // Unreadable: treated as no record below. @@ -74,6 +95,22 @@ export function createRunLedger(dir: string): RunLedger { return false } }, + markDelivered(runId) { + if (!RUN_ID.test(runId)) return + let record: RunRecord | null = null + try { + record = parseRecord(readFileSync(pathFor(runId), 'utf8')) + } catch { + record = null + } + if (!record || record.delivered) return + try { + writeJsonFileAtomicallySync(pathFor(runId), { ...record, delivered: true }) + } catch (error) { + // Left undelivered, a restart stops the run: the conservative side. + logger.warn('Could not note a tmux run as handed back', { error: getErrorMessage(error) }) + } + }, forget(runId) { live.delete(runId) if (RUN_ID.test(runId)) remove(pathFor(runId)) @@ -85,7 +122,7 @@ export function createRunLedger(dir: string): RunLedger { } catch { return [] } - const runs: RecordedRun[] = [] + const runs: RunRecord[] = [] for (const name of names) { // A write that never finished leaves only its temporary file behind. if (name.endsWith('.tmp')) { @@ -93,7 +130,7 @@ export function createRunLedger(dir: string): RunLedger { continue } if (!name.endsWith('.json')) continue - let record: RecordedRun | null = null + let record: RunRecord | null = null try { record = parseRecord(readFileSync(join(dir, name), 'utf8')) } catch { diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 334017da1c3..7bcf812de56 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -1,6 +1,7 @@ import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { sleep } from '@sim/utils/helpers' import { describe, expect, it, vi } from 'vitest' import { TerminalService } from '@/main/terminal' import { createRunLedger } from '@/main/terminal/run-ledger' @@ -21,6 +22,8 @@ const tmuxFake = vi.hoisted(() => ({ untracked: false, /** Run panes tmux still shows, kept open after their command ends (`remain-on-exit`). */ open: new Set(), + /** tmux stops answering: no pane can be confirmed, so none is closed. */ + unanswered: false, statusPaths: new Map(), })) @@ -67,6 +70,7 @@ vi.mock('@/main/terminal/tmux', async () => { }, runPaneState: async (...args: Parameters) => { if (!tmuxFake.on) return actual.runPaneState(...args) + if (tmuxFake.unanswered) return 'unknown' if (tmuxFake.gone.has(args[0].pane)) return 'gone' return args[0].runId === null ? 'unknown' : 'ours' }, @@ -79,7 +83,9 @@ vi.mock('@/main/terminal/tmux', async () => { }, closeRunPane: async (...args: Parameters) => { if (!tmuxFake.on) return actual.closeRunPane(...args) + if (tmuxFake.unanswered) return tmuxFake.open.delete(args[0].pane) + tmuxFake.gone.add(args[0].pane) }, } }) @@ -651,7 +657,14 @@ describe('agent commands in tmux', () => { // Still going after its call returned: a later process must be able to find it. expect(createRunLedger(ledgerDir).list()).toEqual([ - { runId: `run-${pane.slice(1)}`, pane, socket: '/tmp/tmux-fake/default' }, + { + runId: `run-${pane.slice(1)}`, + pane, + socket: '/tmp/tmux-fake/default', + callId: 'call-long', + // Handed back as still running, with its pane: a restart must leave it be. + delivered: true, + }, ]) writeFileSync(statusPath, '0') @@ -699,6 +712,76 @@ describe('agent commands in tmux', () => { } }) + it("forgets a run that finishes within its call, and a closed tab's run once its pane is gone", async () => { + tmuxFake.on = true + tmuxFake.statusPaths.clear() + const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) + const ledgerDir = join(scratch, 'terminal-runs') + try { + const terminal = new TerminalService({ + loadCwd: () => '/tmp', + runLedger: createRunLedger(ledgerDir), + }) + const { activeTerminalId } = terminal.start({ cols: 80, rows: 24 }) + // Finishes while its call still waits on it. + const quick = terminal.executeTool('call-quick', 'run', { command: 'ls', waitSeconds: 30 }) + await vi.waitFor(() => expect(tmuxFake.statusPaths.size).toBe(1)) + writeFileSync([...tmuxFake.statusPaths.values()][0] ?? '', '0') + await quick + expect(createRunLedger(ledgerDir).list()).toEqual([]) + + // Still going when its tab closes; its pane goes later, and the next run's bookkeeping sees. + await terminal.executeTool('call-long', 'run', { command: 'make build', waitSeconds: 1 }) + const [, longPane = ''] = [...tmuxFake.statusPaths.keys()] + terminal.closeTerminal(activeTerminalId as string) + expect( + createRunLedger(ledgerDir) + .list() + .map((run) => run.pane) + ).toEqual([longPane]) + tmuxFake.gone.add(longPane) + await terminal.executeTool('call-new', 'new', {}) + await terminal.executeTool('call-next', 'run', { command: 'pwd', waitSeconds: 1 }) + + expect( + createRunLedger(ledgerDir) + .list() + .map((run) => run.pane) + ).not.toContain(longPane) + } finally { + tmuxFake.on = false + tmuxFake.gone.clear() + rmSync(scratch, { recursive: true, force: true }) + } + }) + + it('keeps the record of a finished run whose pane tmux could not confirm closing', async () => { + tmuxFake.on = true + tmuxFake.statusPaths.clear() + const scratch = mkdtempSync(join(tmpdir(), 'sim-ledger-')) + const ledgerDir = join(scratch, 'terminal-runs') + try { + const terminal = new TerminalService({ + loadCwd: () => '/tmp', + runLedger: createRunLedger(ledgerDir), + }) + const { activeTerminalId } = terminal.start({ cols: 80, rows: 24 }) + await terminal.executeTool('call-long', 'run', { command: 'make build', waitSeconds: 1 }) + writeFileSync([...tmuxFake.statusPaths.values()][0] ?? '', '0') + tmuxFake.unanswered = true + + terminal.closeTerminal(activeTerminalId as string) + await sleep(200) + + // The next sweep will close its pane. + expect(createRunLedger(ledgerDir).list()).toHaveLength(1) + } finally { + tmuxFake.on = false + tmuxFake.unanswered = false + rmSync(scratch, { recursive: true, force: true }) + } + }) + it('never starts a tagged run it could not record', async () => { tmuxFake.on = true tmuxFake.statusPaths.clear() diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index e9dff619bd2..7bda47f7b4b 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -132,6 +132,10 @@ interface FakeTmuxState { fail?: Record /** Once a key is sent, tmux stops answering: every later command fails like a dying server. */ dieAfterKeys?: boolean + /** On this many-th display-message, the pane is retagged as another run's (a restart race). */ + retagAtCheck?: number + /** display-message calls so far. */ + checks?: number /** Attached clients, as `list-clients` reports them. */ clients?: Array<{ pid: string; tty: string; session: string }> /** Commands the fake holds until the file named here exists, like a busy tmux server. */ @@ -194,6 +198,11 @@ switch (args[0]) { break } case 'display-message': { + state.checks = (state.checks ?? 0) + 1 + if (state.retagAtCheck === state.checks && state.panes[target()]) { + state.panes[target()].options['@sim-run-id'] = 'someone-else' + } + save() // Like tmux 3.x, a pane that is gone answers with an empty line rather than an error. const pane = state.panes[target()] const name = args[args.length - 1].slice(2, -1) @@ -346,6 +355,57 @@ describe('stopping a run another process started, from its record', () => { expect(Object.keys(tmux.read().panes)).toEqual([record.pane]) }) + it('checks the pane is still the run once more right before closing it', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + // The first check (before Ctrl-C) sees the run; the next, right before closing, does not. + tmux.write({ ...tmux.read(), checks: 0, retagAtCheck: 2 }) + + expect(await stopRecordedRun(record, tmux.env, 0)).toBe('gone') + expect(tmux.read().log).toEqual([`send-keys ${record.pane} C-c`]) + expect(Object.keys(tmux.read().panes)).toEqual([record.pane]) + }) + + it('never starts a tagged run whose server it cannot name for a record', async () => { + const tmux = fakeTmux({ exec: true }) + dirs.push(tmux.dir) + tmux.write({ ...tmux.read(), socket: '' }) + const marker = join(tmux.dir, 'ran') + + const result = await startRun('agent', `touch ${JSON.stringify(marker)}`, null, tmux.env, { + beforeStart: () => true, + }) + + expect(result).toMatchObject({ error: expect.stringContaining('was not run') }) + await sleep(1_500) + expect(existsSync(marker)).toBe(false) + }) + + it('forgets the saved record of a run that then could not start', async () => { + const tmux = fakeTmux() + dirs.push(tmux.dir) + const saved = new Set() + let runDir = '' + + const result = await startRun('agent', 'sleep 600', null, tmux.env, { + beforeStart: (run) => { + // The run's directory turns read-only, so its go file cannot be written. + const command = tmux.read().panes[run.pane]?.command ?? '' + const script = /"([^"]+)\/run\.sh"/.exec(command)?.[1] ?? '' + runDir = script + chmodSync(runDir, 0o500) + saved.add(run.runId) + return true + }, + abandon: (runId) => saved.delete(runId), + }) + + if (runDir) chmodSync(runDir, 0o700) + if (runDir) dirs.push(runDir) + expect(result).toMatchObject({ error: expect.stringContaining('could not be started') }) + expect([...saved]).toEqual([]) + }) + it('keeps the record when tmux stops answering part-way through the stop', async () => { const tmux = fakeTmux() const { record } = await recorded(tmux) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 375740fc354..3be3e40bb07 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -364,6 +364,8 @@ export async function startRun( * since a run no later process could find must not outlive this one. */ beforeStart?: (run: RecordedRun) => boolean + /** Undoes `beforeStart` for a run that then could not start after all. */ + abandon?: (runId: string) => void } = {} ): Promise { const dir = mkdtempSync(join(tmpdir(), 'sim-tmux-run-')) @@ -461,6 +463,7 @@ export async function startRun( try { writeFileSync(goPath, '') } catch (error) { + if (runId) options.abandon?.(runId) dispose() return { error: `The command could not be started: ${getErrorMessage(error)}` } } @@ -522,7 +525,7 @@ export interface RecordedRun { * when that server or pane no longer exists or the pane is not tagged as this run's, `unknown` * when tmux could not be asked. */ -async function recordedRunState( +export async function recordedRunState( run: RecordedRun, env: NodeJS.ProcessEnv ): Promise<'ours' | 'gone' | 'unknown'> { From 6e5104c7e47b8e2fdf63f4a54633e93068af36ec Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 22:56:31 -0700 Subject: [PATCH 5/6] fix(desktop): count a tmux run as handed back only once its result is journaled, and check a pane's tag in the same tmux command that acts on it - A run is marked handed back only once the executor has journaled the result that hands it back; a chat-view run cannot prove its result reached the model, so after a restart it is treated as orphaned and stopped. - A run that a stop for everything (sign-out, Terminal off) could not confirm stays marked to stop, so a later launch for the same user never keeps it. - Every tag-guarded action (Ctrl-C, closing the pane, for live and recorded runs) is one `if-shell -F` command that checks the tag and acts, so a tmux restart between a check and an action cannot hand it to another pane. --- .../main/desktop-executor/executor.test.ts | 21 +++++++++- .../src/main/desktop-executor/executor.ts | 9 +++- .../src/main/desktop-executor/service.ts | 4 ++ apps/desktop/src/main/index.ts | 7 ++++ apps/desktop/src/main/terminal/index.ts | 4 +- .../main/terminal/registry-run-ledger.test.ts | 29 +++++++++++++ apps/desktop/src/main/terminal/registry.ts | 20 ++++++++- apps/desktop/src/main/terminal/run-ledger.ts | 42 ++++++++++++------- .../desktop/src/main/terminal/service.test.ts | 4 +- apps/desktop/src/main/terminal/tmux.test.ts | 36 ++++++++++++++++ apps/desktop/src/main/terminal/tmux.ts | 32 +++++++++++--- 11 files changed, 181 insertions(+), 27 deletions(-) diff --git a/apps/desktop/src/main/desktop-executor/executor.test.ts b/apps/desktop/src/main/desktop-executor/executor.test.ts index f1ed7801d55..36164205a21 100644 --- a/apps/desktop/src/main/desktop-executor/executor.test.ts +++ b/apps/desktop/src/main/desktop-executor/executor.test.ts @@ -168,6 +168,8 @@ function setup( const runner = new FakeRunner() const onUnregistered = vi.fn() const busy: boolean[] = [] + /** Calls whose result the executor reported as journaled, in order. */ + const journaled: string[] = [] const executor = new DesktopExecutor({ client: sim.client, journal, @@ -177,12 +179,13 @@ function setup( onUnregistered, onBusyChange: (value) => busy.push(value), onApprovals: (items) => approvals.push(items), + onResultRecorded: (toolCallId) => journaled.push(toolCallId), ...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}), ...(options.deliveryAwakeLimitMs !== undefined ? { deliveryAwakeLimitMs: options.deliveryAwakeLimitMs } : {}), }) - return { sim, journal, runner, executor, onUnregistered, busy, approvals } + return { sim, journal, runner, executor, onUnregistered, busy, approvals, journaled } } describe('claiming', () => { @@ -203,6 +206,22 @@ describe('claiming', () => { expect(executor.heldCallCount()).toBe(0) }) + it('reports a result as durable only once the journal holds it', async () => { + const { sim, journal, runner, executor, journaled } = setup() + runner.immediate = DONE + journal.failOn = 'result' + sim.inbox = [callItem('call-unjournaled', 'chat-a')] + await executor.reconcile() + await vi.waitFor(() => expect(sim.completions).toHaveLength(1)) + expect(journaled).toEqual([]) + + journal.failOn = null + sim.inbox = [callItem('call-journaled', 'chat-a')] + await executor.reconcile() + await vi.waitFor(() => expect(sim.completions).toHaveLength(2)) + expect(journaled).toEqual(['call-journaled']) + }) + it('claims a whole backlog at once, before any of it runs', async () => { const { sim, runner, executor } = setup() sim.inbox = [callItem('a-1', 'chat-a'), callItem('a-2', 'chat-a'), callItem('a-3', 'chat-a')] diff --git a/apps/desktop/src/main/desktop-executor/executor.ts b/apps/desktop/src/main/desktop-executor/executor.ts index 0d56584c027..3132ef93c56 100644 --- a/apps/desktop/src/main/desktop-executor/executor.ts +++ b/apps/desktop/src/main/desktop-executor/executor.ts @@ -67,6 +67,11 @@ export interface DesktopExecutorOptions { onApprovals?: (items: DesktopApprovalItem[]) => void /** Called whenever the number of held calls changes between zero and more. */ onBusyChange?: (busy: boolean) => void + /** + * Called once a call's result is in the journal: from then on it reaches Sim, now or after a + * restart, so anything it hands back (a pane still running) can be counted on. + */ + onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void maxHeldCalls?: number /** First delivery retry delay; tests shorten it. */ retryBaseMs?: number @@ -375,7 +380,9 @@ export class DesktopExecutor { ): Promise { if (this.disposed) return // Best effort: unrecorded, a crash reports the call from its `started` entry as outcome unknown. - await this.record({ toolCallId, state: 'result', executionToken, completion }) + if (await this.record({ toolCallId, state: 'result', executionToken, completion })) { + this.options.onResultRecorded?.(toolCallId, completion) + } const sendingSince = Date.now() try { await this.sendResult(toolCallId, executionToken, completion, sendingSince) diff --git a/apps/desktop/src/main/desktop-executor/service.ts b/apps/desktop/src/main/desktop-executor/service.ts index fdb36597a31..9d4bf9ec69d 100644 --- a/apps/desktop/src/main/desktop-executor/service.ts +++ b/apps/desktop/src/main/desktop-executor/service.ts @@ -7,6 +7,7 @@ import { hostname } from 'node:os' import { join } from 'node:path' import type { DesktopExecutorDevice } from '@sim/desktop-bridge' +import type { DesktopToolCompletion } from '@sim/desktop-bridge/tool-results' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' @@ -61,6 +62,8 @@ export interface DesktopExecutorServiceDeps { onApprovals?: (items: DesktopApprovalItem[]) => void /** Whether any chat has desktop work claimed on this machine changed. */ onBusyChange?: (busy: boolean) => void + /** A call's result is in the journal, so it reaches Sim even across a restart. */ + onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void } export interface DesktopExecutorService { @@ -245,6 +248,7 @@ export function createDesktopExecutorService( onUnregistered: handleUnrecognized, ...(deps.onApprovals ? { onApprovals: deps.onApprovals } : {}), ...(deps.onBusyChange ? { onBusyChange: deps.onBusyChange } : {}), + ...(deps.onResultRecorded ? { onResultRecorded: deps.onResultRecorded } : {}), }) await executor.recover() // Signed out while recovering: sign-out already disposed this executor. diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 793e366e657..a88e4394e31 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -607,6 +607,13 @@ function main(): void { accountDataAvailable, onApprovals: (items) => approvalNotifier.update(items), onBusyChange: (busy) => sleepBlocker.setBusy(busy), + // A result that will reach the model (not one reported as not started or outcome unknown) + // makes a tmux run it handed back as still going collectable across a restart. + onResultRecorded: (toolCallId, completion) => { + if (completion.data?.outcomeUnknown !== true && completion.data?.notStarted !== true) { + terminal.markRunDelivered(toolCallId) + } + }, runner: createDesktopToolRunner({ preferences: () => desktopSettings.getPreferences(), accountDataAvailable, diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index c8cd73a0672..d8f1720b32b 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -1351,8 +1351,8 @@ export class TerminalService { handle.dispose() } // Still going, it stays tracked, and nothing polls the status file again: `read` captures - // the pane instead. Its result now points the model at that pane, so a restart must not end it. - if (!outcome.done && handle.runId) ledger?.markDelivered(handle.runId) + // the pane instead. Its record is marked handed back only once that result is durable + // (the executor's journal); see `TerminalRegistry.markRunDelivered`. const { text, truncated } = elideOutput(outcome.output) return { diff --git a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts index ebe402ad5df..62c0a70013f 100644 --- a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts +++ b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts @@ -88,6 +88,35 @@ describe('stopping recorded tmux runs', () => { expect(ledger.list().map((record) => record.runId)).toEqual(['handed-back']) }) + it('keeps meaning to stop a run a stop for everything could not confirm', async () => { + const dir = ledgerDir() + createRunLedger(dir).record(run('unconfirmed', '%1', true)) + panes.set('unconfirmed', 'unknown') + const ledger = createRunLedger(dir) + // Sign-out could not confirm the run ended. + await new TerminalRegistry(undefined, undefined, ledger).stopAgentCommands() + panes.set('unconfirmed', 'running') + + // A later launch for the same user, with the journal cleared, still stops it. + await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns(new Set()) + + expect(panes.get('unconfirmed')).toBe('stopped') + expect(ledger.list()).toEqual([]) + }) + + it('notes a run as handed back once its call result is durable', () => { + const dir = ledgerDir() + const ledger = createRunLedger(dir) + ledger.record(run('watched', '%1')) + ledger.record(run('other', '%2')) + + new TerminalRegistry(undefined, undefined, ledger).markRunDelivered('call-watched') + + expect( + Object.fromEntries(ledger.list().map((record) => [record.runId, record.delivered])) + ).toEqual({ watched: true, other: false }) + }) + it("at launch, stops the previous process's runs and none this one has started", async () => { const dir = ledgerDir() createRunLedger(dir).record(run('previous', '%1')) diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 27e6ee0be94..354854ab320 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -396,10 +396,22 @@ export class TerminalRegistry { stopUncollectableRuns(unresolvedCalls: ReadonlySet | null): Promise { return this.stopRecordedRuns({ excludeLive: true, - keep: (run) => run.delivered && !unresolvedCalls?.has(run.callId), + keep: (run) => run.delivered && !run.mustStop && !unresolvedCalls?.has(run.callId), }) } + /** + * Notes that a call's result is durable (in the executor's journal), so a tmux run it handed + * back as still going may be left to the model across a restart. + */ + markRunDelivered(callId: string): void { + const ledger = this.runLedger + if (!ledger) return + for (const run of ledger.list()) { + if (run.callId === callId) ledger.markDelivered(run.runId) + } + } + /** * Stops the recorded tmux runs, each only while its pane still carries its tag, and drops the * records with nothing left to stop. `excludeLive` skips the runs this process has started; @@ -417,10 +429,14 @@ export class TerminalRegistry { if (!ledger) return await Promise.allSettled( ledger.list({ excludeLive: options.excludeLive }).map(async (run) => { - const state = options.keep?.(run) + const keeping = options.keep?.(run) ?? false + const state = keeping ? await recordedRunState(run, process.env) : await stopRecordedRun(run, process.env, RECORDED_RUN_GRACE_MS) if (state === 'gone') ledger.forget(run.runId) + // A run this sweep meant to stop but could not confirm stays meant to stop, so no later + // sweep keeps it. + else if (!keeping) ledger.markMustStop(run.runId) }) ) } diff --git a/apps/desktop/src/main/terminal/run-ledger.ts b/apps/desktop/src/main/terminal/run-ledger.ts index c9e2948942c..badf2806feb 100644 --- a/apps/desktop/src/main/terminal/run-ledger.ts +++ b/apps/desktop/src/main/terminal/run-ledger.ts @@ -27,6 +27,8 @@ export interface RunRecord extends RecordedRun { * pane): from then on the model can come back to the pane, so a restart must leave it be. */ delivered: boolean + /** A stop for everything (sign-out, Terminal off) could not confirm this run ended. */ + mustStop?: boolean } export interface RunLedger { @@ -34,6 +36,8 @@ export interface RunLedger { record(run: RunRecord): boolean /** Notes that the run's call has handed back its result, with the run still going. */ markDelivered(runId: string): void + /** Notes that the run must be stopped, whatever a later launch would otherwise decide. */ + markMustStop(runId: string): void forget(runId: string): void /** Every recorded run; `excludeLive` leaves out runs this process recorded. */ list(options?: { excludeLive?: boolean }): RunRecord[] @@ -61,6 +65,7 @@ function parseRecord(text: string): RunRecord | null { socket: parsed.socket, callId: parsed.callId, delivered: parsed.delivered, + ...(parsed.mustStop === true ? { mustStop: true } : {}), } } } catch { @@ -83,6 +88,24 @@ export function createRunLedger(dir: string): RunLedger { const live = new Set() const pathFor = (runId: string) => join(dir, `${runId}.json`) + /** Rewrites a saved record; `change` returns null to leave it as it is. */ + const update = (runId: string, change: (record: RunRecord) => RunRecord | null): void => { + if (!RUN_ID.test(runId)) return + let record: RunRecord | null = null + try { + record = parseRecord(readFileSync(pathFor(runId), 'utf8')) + } catch { + record = null + } + const changed = record ? change(record) : null + if (!changed) return + try { + writeJsonFileAtomicallySync(pathFor(runId), changed) + } catch (error) { + logger.warn('Could not update a tmux run record', { error: getErrorMessage(error) }) + } + } + return { record(run) { if (!RUN_ID.test(run.runId)) return false @@ -96,20 +119,11 @@ export function createRunLedger(dir: string): RunLedger { } }, markDelivered(runId) { - if (!RUN_ID.test(runId)) return - let record: RunRecord | null = null - try { - record = parseRecord(readFileSync(pathFor(runId), 'utf8')) - } catch { - record = null - } - if (!record || record.delivered) return - try { - writeJsonFileAtomicallySync(pathFor(runId), { ...record, delivered: true }) - } catch (error) { - // Left undelivered, a restart stops the run: the conservative side. - logger.warn('Could not note a tmux run as handed back', { error: getErrorMessage(error) }) - } + // Left undelivered, a restart stops the run: the conservative side. + update(runId, (record) => (record.delivered ? null : { ...record, delivered: true })) + }, + markMustStop(runId) { + update(runId, (record) => (record.mustStop ? null : { ...record, mustStop: true })) }, forget(runId) { live.delete(runId) diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 7bcf812de56..1f05adb3d00 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -662,8 +662,8 @@ describe('agent commands in tmux', () => { pane, socket: '/tmp/tmux-fake/default', callId: 'call-long', - // Handed back as still running, with its pane: a restart must leave it be. - delivered: true, + // Handed back, but only the executor's journal can make that durable. + delivered: false, }, ]) diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 7bda47f7b4b..8254c98b3f8 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -136,6 +136,8 @@ interface FakeTmuxState { retagAtCheck?: number /** display-message calls so far. */ checks?: number + /** tmux restarts and the user's pane takes the id just before the next guarded action. */ + retagBeforeAction?: boolean /** Attached clients, as `list-clients` reports them. */ clients?: Array<{ pid: string; tty: string; session: string }> /** Commands the fake holds until the file named here exists, like a busy tmux server. */ @@ -174,6 +176,18 @@ if (state.hold && state.hold[args[0]]) { } state.held = state.held.filter((command) => command !== args[0]) } +// \`if-shell -F -t pane '#{==:#{option},value}' command\`: the check and the action in one command. +if (args[0] === 'if-shell') { + const pane = state.panes[target()] + if (pane && state.retagBeforeAction) { + pane.options['@sim-run-id'] = 'someone-else' + state.retagBeforeAction = false + save() + } + const check = /^#\\{==:#\\{([^}]+)\\},(.*)\\}$/.exec(args[args.length - 2]) + if (!pane || !check || (pane.options[check[1]] ?? '') !== check[2]) process.exit(0) + args = args[args.length - 1].split(' ') +} if (state.fail && state.fail[args[0]]) fail(state.fail[args[0]]) switch (args[0]) { case 'new-window': { @@ -406,6 +420,17 @@ describe('stopping a run another process started, from its record', () => { expect([...saved]).toEqual([]) }) + it('acts on no pane that took the recorded id between the last check and the action', async () => { + const tmux = fakeTmux() + const { record } = await recorded(tmux) + tmux.write({ ...tmux.read(), retagBeforeAction: true }) + + await stopRecordedRun(record, tmux.env, 0) + + expect(tmux.read().log).toEqual([]) + expect(Object.keys(tmux.read().panes)).toEqual([record.pane]) + }) + it('keeps the record when tmux stops answering part-way through the stop', async () => { const tmux = fakeTmux() const { record } = await recorded(tmux) @@ -485,6 +510,17 @@ describe('stopping a tmux run touches only its own pane', () => { expect(Object.keys(tmux.read().panes)).toEqual([users]) }) + it('sends a live run nothing once its pane is retagged between the check and the action', async () => { + const tmux = fakeTmux() + const run = await started(tmux) + tmux.write({ ...tmux.read(), retagBeforeAction: true }) + + await stopRun(run, tmux.env, 0) + + expect(tmux.read().log).toEqual([]) + expect(Object.keys(tmux.read().panes)).toEqual([run.pane]) + }) + it('sends nothing to a pane that reused the run pane id after tmux restarted', async () => { const tmux = fakeTmux() const run = await started(tmux) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 3be3e40bb07..b5330736933 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -499,13 +499,30 @@ export async function runPaneState( * step first checks the pane is still the run's, and only that pane is ever closed, so a pane the * user split off beside it, or a window that reused its ids, is never touched. */ +/** + * Arguments for one tmux command that runs `command` on a pane only while the pane carries the + * run's tag. tmux checks the tag and acts within the one command, so a restart between a check + * and an action can never hand the action to a pane that took the id. + */ +function ifTagged(pane: string, runId: string, command: string, socket?: string): string[] { + return [ + ...(socket ? ['-S', socket] : []), + 'if-shell', + '-F', + '-t', + pane, + `#{==:#{${RUN_ID_OPTION}},${runId}}`, + command, + ] +} + export async function stopRun( handle: TmuxRunHandle, env: NodeJS.ProcessEnv, graceMs: number ): Promise { - if ((await runPaneState(handle, env)) !== 'ours') return - await sendKey(handle.pane, 'C-c', env) + if ((await runPaneState(handle, env)) !== 'ours' || !handle.runId) return + await runTmux(ifTagged(handle.pane, handle.runId, `send-keys -t ${handle.pane} C-c`), env) const deadline = Date.now() + graceMs while (!isRunComplete(handle) && Date.now() < deadline) await sleep(100) if (!isRunComplete(handle)) await closeRunPane(handle, env) @@ -552,7 +569,7 @@ export async function stopRecordedRun( ): Promise<'gone' | 'unknown'> { const before = await recordedRunState(run, env) if (before !== 'ours') return before - await runTmux(['-S', run.socket, 'send-keys', '-t', run.pane, 'C-c'], env) + await runTmux(ifTagged(run.pane, run.runId, `send-keys -t ${run.pane} C-c`, run.socket), env) const deadline = Date.now() + graceMs let state: 'ours' | 'gone' | 'unknown' = 'ours' while (Date.now() < deadline) { @@ -564,7 +581,7 @@ export async function stopRecordedRun( // left alone, and its record kept for the next sweep. if (state === 'ours') state = await recordedRunState(run, env) if (state === 'ours') { - await runTmux(['-S', run.socket, 'kill-pane', '-t', run.pane], env) + await runTmux(ifTagged(run.pane, run.runId, `kill-pane -t ${run.pane}`, run.socket), env) state = await recordedRunState(run, env) } return state === 'gone' ? 'gone' : 'unknown' @@ -657,7 +674,12 @@ export async function closeRunPane(handle: TmuxRunHandle, env: NodeJS.ProcessEnv isRunComplete(handle) && (await startedByRun(handle, env)) if (state !== 'ours' && !finishedUntracked) return - const killed = await runTmux(['kill-pane', '-t', handle.pane], env) + const killed = await runTmux( + handle.runId + ? ifTagged(handle.pane, handle.runId, `kill-pane -t ${handle.pane}`) + : ['kill-pane', '-t', handle.pane], + env + ) if (!killed.ok) { logger.warn('Could not close the tmux run pane', { error: killed.stderr.trim() }) } From 7e3fe3ad5f3af93ccfdaf1951aa8a12b67dae24c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 23:15:14 -0700 Subject: [PATCH 6/6] 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. --- .../main/desktop-executor/executor.test.ts | 28 +++++++------- .../src/main/desktop-executor/executor.ts | 12 +++--- .../src/main/desktop-executor/service.test.ts | 11 ++---- .../src/main/desktop-executor/service.ts | 38 +++++++++---------- apps/desktop/src/main/index.ts | 12 +++--- apps/desktop/src/main/terminal/index.ts | 19 ++++++---- .../main/terminal/registry-run-ledger.test.ts | 18 ++++++--- apps/desktop/src/main/terminal/registry.ts | 16 ++++---- .../desktop/src/main/terminal/service.test.ts | 25 ++++++++++++ apps/desktop/src/main/terminal/tmux.test.ts | 2 + apps/desktop/src/main/terminal/tmux.ts | 4 +- 11 files changed, 109 insertions(+), 76 deletions(-) diff --git a/apps/desktop/src/main/desktop-executor/executor.test.ts b/apps/desktop/src/main/desktop-executor/executor.test.ts index 36164205a21..795d4df54de 100644 --- a/apps/desktop/src/main/desktop-executor/executor.test.ts +++ b/apps/desktop/src/main/desktop-executor/executor.test.ts @@ -168,8 +168,8 @@ function setup( const runner = new FakeRunner() const onUnregistered = vi.fn() const busy: boolean[] = [] - /** Calls whose result the executor reported as journaled, in order. */ - const journaled: string[] = [] + /** Calls whose result the executor reported as reaching the model, in order. */ + const delivered: string[] = [] const executor = new DesktopExecutor({ client: sim.client, journal, @@ -179,13 +179,13 @@ function setup( onUnregistered, onBusyChange: (value) => busy.push(value), onApprovals: (items) => approvals.push(items), - onResultRecorded: (toolCallId) => journaled.push(toolCallId), + onResultDelivered: (toolCallId) => delivered.push(toolCallId), ...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}), ...(options.deliveryAwakeLimitMs !== undefined ? { deliveryAwakeLimitMs: options.deliveryAwakeLimitMs } : {}), }) - return { sim, journal, runner, executor, onUnregistered, busy, approvals, journaled } + return { sim, journal, runner, executor, onUnregistered, busy, approvals, delivered } } describe('claiming', () => { @@ -206,20 +206,22 @@ describe('claiming', () => { expect(executor.heldCallCount()).toBe(0) }) - it('reports a result as durable only once the journal holds it', async () => { - const { sim, journal, runner, executor, journaled } = setup() + it('reports a result as reaching the model only once Sim takes it as the call own', async () => { + const { sim, journal, runner, executor, delivered } = setup() runner.immediate = DONE - journal.failOn = 'result' - sim.inbox = [callItem('call-unjournaled', 'chat-a')] + // Sim settled this call first: the result never reached the model. + sim.completionOutcome = 'superseded' + sim.inbox = [callItem('call-superseded', 'chat-a')] await executor.reconcile() await vi.waitFor(() => expect(sim.completions).toHaveLength(1)) - expect(journaled).toEqual([]) + expect(delivered).toEqual([]) - journal.failOn = null - sim.inbox = [callItem('call-journaled', 'chat-a')] + // Taken by Sim, even with nothing written locally (no OS encryption, say). + sim.completionOutcome = 'recorded' + journal.failOn = 'result' + sim.inbox = [callItem('call-recorded', 'chat-a')] await executor.reconcile() - await vi.waitFor(() => expect(sim.completions).toHaveLength(2)) - expect(journaled).toEqual(['call-journaled']) + await vi.waitFor(() => expect(delivered).toEqual(['call-recorded'])) }) it('claims a whole backlog at once, before any of it runs', async () => { diff --git a/apps/desktop/src/main/desktop-executor/executor.ts b/apps/desktop/src/main/desktop-executor/executor.ts index 3132ef93c56..6ca794182cd 100644 --- a/apps/desktop/src/main/desktop-executor/executor.ts +++ b/apps/desktop/src/main/desktop-executor/executor.ts @@ -68,10 +68,10 @@ export interface DesktopExecutorOptions { /** Called whenever the number of held calls changes between zero and more. */ onBusyChange?: (busy: boolean) => void /** - * Called once a call's result is in the journal: from then on it reaches Sim, now or after a - * restart, so anything it hands back (a pane still running) can be counted on. + * Called once Sim has taken a call's result as the call's own (recorded, or a duplicate of one + * it recorded): the model has it, so anything it hands back (a pane still running) is in use. */ - onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void + onResultDelivered?: (toolCallId: string, completion: DesktopToolCompletion) => void maxHeldCalls?: number /** First delivery retry delay; tests shorten it. */ retryBaseMs?: number @@ -380,9 +380,7 @@ export class DesktopExecutor { ): Promise { if (this.disposed) return // Best effort: unrecorded, a crash reports the call from its `started` entry as outcome unknown. - if (await this.record({ toolCallId, state: 'result', executionToken, completion })) { - this.options.onResultRecorded?.(toolCallId, completion) - } + await this.record({ toolCallId, state: 'result', executionToken, completion }) const sendingSince = Date.now() try { await this.sendResult(toolCallId, executionToken, completion, sendingSince) @@ -408,6 +406,8 @@ export class DesktopExecutor { completion: pending, }) logger.info('Desktop call result acknowledged', { toolCallId, outcome }) + // Superseded: Sim settled the call first, so this result never reached the model. + if (outcome !== 'superseded') this.options.onResultDelivered?.(toolCallId, pending) break } catch (error) { // Encoding failed on this machine, so nothing was sent; the same data would fail again. diff --git a/apps/desktop/src/main/desktop-executor/service.test.ts b/apps/desktop/src/main/desktop-executor/service.test.ts index 996a1bca9cf..924ad2b551b 100644 --- a/apps/desktop/src/main/desktop-executor/service.test.ts +++ b/apps/desktop/src/main/desktop-executor/service.test.ts @@ -96,8 +96,8 @@ async function service(protocolVersion = 1, userDataPath?: string) { return { sim, desktopExecutor, busy } } -describe('calls never handed back with a result', () => { - it('names claimed and started calls, and results that are not started or unknown', async () => { +describe('results recovery will hand to the model', () => { + it('names the calls whose real result the journal holds for recovery to send', async () => { const userData = await mkdtemp(join(tmpdir(), 'sim-executor-service-')) const journal = createExecutorJournal(join(userData, 'desktop-executor-journal.json')) await journal.put({ toolCallId: 'claimed', state: 'claimed', executionToken: 't1' }) @@ -122,12 +122,7 @@ describe('calls never handed back with a result', () => { }) const { desktopExecutor } = await service(1, userData) - expect([...((await desktopExecutor.unresolvedCalls()) ?? [])].sort()).toEqual([ - 'claimed', - 'not-started', - 'started', - 'unknown', - ]) + expect([...(await desktopExecutor.pendingResults())]).toEqual(['handed-back']) }) }) diff --git a/apps/desktop/src/main/desktop-executor/service.ts b/apps/desktop/src/main/desktop-executor/service.ts index 9d4bf9ec69d..7611dfd5c2f 100644 --- a/apps/desktop/src/main/desktop-executor/service.ts +++ b/apps/desktop/src/main/desktop-executor/service.ts @@ -62,8 +62,8 @@ export interface DesktopExecutorServiceDeps { onApprovals?: (items: DesktopApprovalItem[]) => void /** Whether any chat has desktop work claimed on this machine changed. */ onBusyChange?: (busy: boolean) => void - /** A call's result is in the journal, so it reaches Sim even across a restart. */ - onResultRecorded?: (toolCallId: string, completion: DesktopToolCompletion) => void + /** Sim has taken a call's result as the call's own, so the model has it. */ + onResultDelivered?: (toolCallId: string, completion: DesktopToolCompletion) => void } export interface DesktopExecutorService { @@ -72,11 +72,11 @@ export interface DesktopExecutorService { refreshRegistration(): void getDevice(): DesktopExecutorDevice | null /** - * The calls the journal shows as never handed back with a real result: claimed or started with - * none, or settled as not started or outcome unknown. Read before recovery reports them; null - * when the journal cannot be read. + * The calls whose real result (not one reported as not started or outcome unknown) is in the + * journal and not yet acknowledged, so recovery will hand it to the model. Read before recovery + * changes the journal; empty when it cannot be read. */ - unresolvedCalls(): Promise | null> + pendingResults(): Promise> /** Stores one entry of a claimed import, as this device's registered session. */ importEntry( request: DesktopImportEntryRequest, @@ -248,7 +248,7 @@ export function createDesktopExecutorService( onUnregistered: handleUnrecognized, ...(deps.onApprovals ? { onApprovals: deps.onApprovals } : {}), ...(deps.onBusyChange ? { onBusyChange: deps.onBusyChange } : {}), - ...(deps.onResultRecorded ? { onResultRecorded: deps.onResultRecorded } : {}), + ...(deps.onResultDelivered ? { onResultDelivered: deps.onResultDelivered } : {}), }) await executor.recover() // Signed out while recovering: sign-out already disposed this executor. @@ -453,23 +453,21 @@ export function createDesktopExecutorService( getDevice() { return device }, - async unresolvedCalls() { + async pendingResults() { + const pending = new Set() try { - const unresolved = new Set() for (const entry of await journal.load()) { - const data = entry.state === 'result' ? entry.completion.data : undefined - if ( - entry.state !== 'result' || - data?.outcomeUnknown === true || - data?.notStarted === true - ) { - unresolved.add(entry.toolCallId) - } + if (entry.state !== 'result') continue + const data = entry.completion.data + if (data?.outcomeUnknown === true || data?.notStarted === true) continue + pending.add(entry.toolCallId) } - return unresolved - } catch { - return null + } catch (error) { + logger.warn('Could not read the executor journal for pending results', { + error: getErrorMessage(error), + }) } + return pending }, importEntry(request, signal) { if (!client) throw new Error('The Sim desktop app is not signed in to Sim.') diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index a88e4394e31..d777615929f 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -607,9 +607,9 @@ function main(): void { accountDataAvailable, onApprovals: (items) => approvalNotifier.update(items), onBusyChange: (busy) => sleepBlocker.setBusy(busy), - // A result that will reach the model (not one reported as not started or outcome unknown) - // makes a tmux run it handed back as still going collectable across a restart. - onResultRecorded: (toolCallId, completion) => { + // A result the model has (not one reported as not started or outcome unknown) makes a tmux + // run it handed back as still going collectable across a restart. + onResultDelivered: (toolCallId, completion) => { if (completion.data?.outcomeUnknown !== true && completion.data?.notStarted !== true) { terminal.markRunDelivered(toolCallId) } @@ -822,8 +822,8 @@ function main(): void { // it does, so it is stopped, while its pane still carries its tag. A run already handed back as // still going, with its pane, is left to the model, which may come back to it. Read before the // executor starts, since its recovery rewrites the journal. - const unresolvedCalls = desktopExecutor.unresolvedCalls() - void unresolvedCalls.then((unresolved) => terminal.stopUncollectableRuns(unresolved)) + const pendingResults = desktopExecutor.pendingResults() + void pendingResults.then((pending) => terminal.stopUncollectableRuns(pending)) if (!accountDataAvailable()) { logger.warn( @@ -974,7 +974,7 @@ function main(): void { ensureAppSession().cookies.on('changed', (_event, cookie, _cause, removed) => { if (!removed && isSessionCookieName(cookie.name)) desktopExecutor.refreshRegistration() }) - await unresolvedCalls + await pendingResults desktopExecutor.start() } await ensureMainWindow() diff --git a/apps/desktop/src/main/terminal/index.ts b/apps/desktop/src/main/terminal/index.ts index d8f1720b32b..0150434ef0a 100644 --- a/apps/desktop/src/main/terminal/index.ts +++ b/apps/desktop/src/main/terminal/index.ts @@ -538,13 +538,16 @@ export class TerminalService { if (!pending) return for (const handle of pending) { // A finished run's pane may still be open (`remain-on-exit`): it is closed, while still the - // run's, before the record goes. Without the shell's environment the record stays, and the - // next sweep closes it. An untracked run is never stopped, so it is not kept either. + // run's, before the record goes, and its files go only after that check, which an untracked + // run needs them for. Without the shell's environment the record stays, and the next sweep + // closes it. An untracked run is never stopped, so it is not kept either. if (isRunComplete(handle) && env) { - void closeRunPane(handle, env).then(async () => { - // A pane tmux could not answer for keeps its record, for the next sweep to close. - if ((await runPaneState(handle, env)) === 'gone') this.forgetRun(handle) - }) + void closeRunPane(handle, env) + .then(async () => { + if ((await runPaneState(handle, env)) === 'gone') this.forgetRun(handle) + }) + .finally(() => this.releaseRun(handle)) + continue } if (env && handle.runId !== null && !isRunComplete(handle)) this.orphanedRuns.set(handle, env) this.releaseRun(handle) @@ -1351,8 +1354,8 @@ export class TerminalService { handle.dispose() } // Still going, it stays tracked, and nothing polls the status file again: `read` captures - // the pane instead. Its record is marked handed back only once that result is durable - // (the executor's journal); see `TerminalRegistry.markRunDelivered`. + // the pane instead. Its record is marked handed back only once that result reaches the model; + // see `TerminalRegistry.markRunDelivered`. const { text, truncated } = elideOutput(outcome.output) return { diff --git a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts index 62c0a70013f..4e3cc036c5f 100644 --- a/apps/desktop/src/main/terminal/registry-run-ledger.test.ts +++ b/apps/desktop/src/main/terminal/registry-run-ledger.test.ts @@ -69,23 +69,29 @@ describe('stopping recorded tmux runs', () => { it("at launch, leaves the same user's runs the model can come back to, and stops the rest", async () => { const dir = ledgerDir() const previous = createRunLedger(dir) + // Sim took its result: the model has the pane. previous.record(run('handed-back', '%1', true)) previous.record(run('never-handed-back', '%2')) - // Handed back, but the call's journal still shows no result: the model never got it. - previous.record(run('lost-on-the-way', '%3', true)) + // Its result is in the journal, unacknowledged: recovery will hand the pane to the model. + previous.record(run('on-its-way', '%3')) previous.record(run('handed-back-and-gone', '%4', true)) panes.set('handed-back-and-gone', 'gone') const ledger = createRunLedger(dir) - const unresolved = new Set(['call-lost-on-the-way']) - await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns(unresolved) + await new TerminalRegistry(undefined, undefined, ledger).stopUncollectableRuns( + new Set(['call-on-its-way']) + ) expect(Object.fromEntries(panes)).toEqual({ 'never-handed-back': 'stopped', - 'lost-on-the-way': 'stopped', 'handed-back-and-gone': 'gone', }) - expect(ledger.list().map((record) => record.runId)).toEqual(['handed-back']) + expect( + ledger + .list() + .map((record) => record.runId) + .sort() + ).toEqual(['handed-back', 'on-its-way']) }) it('keeps meaning to stop a run a stop for everything could not confirm', async () => { diff --git a/apps/desktop/src/main/terminal/registry.ts b/apps/desktop/src/main/terminal/registry.ts index 354854ab320..6ecc7a8a38a 100644 --- a/apps/desktop/src/main/terminal/registry.ts +++ b/apps/desktop/src/main/terminal/registry.ts @@ -388,21 +388,21 @@ export class TerminalRegistry { } /** - * At launch, for the same user: stops a previous process's tmux run whose call never handed back - * its result, or whose result the executor's journal (`unresolvedCalls`, null when unreadable) - * shows as never reaching the model. A run handed back as still going, with its pane, is left - * to the model, which may come back to it. + * At launch, for the same user: leaves a previous process's tmux run going only when the model + * has, or will get, the result that handed it back as still going: Sim acknowledged it + * (`delivered`), or the executor's journal holds it for recovery to send (`pendingResults`). + * Every other run is stopped, as is one a stop for everything could not confirm. */ - stopUncollectableRuns(unresolvedCalls: ReadonlySet | null): Promise { + stopUncollectableRuns(pendingResults: ReadonlySet): Promise { return this.stopRecordedRuns({ excludeLive: true, - keep: (run) => run.delivered && !run.mustStop && !unresolvedCalls?.has(run.callId), + keep: (run) => !run.mustStop && (run.delivered || pendingResults.has(run.callId)), }) } /** - * Notes that a call's result is durable (in the executor's journal), so a tmux run it handed - * back as still going may be left to the model across a restart. + * Notes that a call's result reached the model, so a tmux run it handed back as still going may + * be left to the model across a restart. */ markRunDelivered(callId: string): void { const ledger = this.runLedger diff --git a/apps/desktop/src/main/terminal/service.test.ts b/apps/desktop/src/main/terminal/service.test.ts index 1f05adb3d00..b2f24c9cec8 100644 --- a/apps/desktop/src/main/terminal/service.test.ts +++ b/apps/desktop/src/main/terminal/service.test.ts @@ -84,6 +84,10 @@ vi.mock('@/main/terminal/tmux', async () => { closeRunPane: async (...args: Parameters) => { if (!tmuxFake.on) return actual.closeRunPane(...args) if (tmuxFake.unanswered) return + // An untracked run's pane is proven its own only from the run's files, read after tmux + // has answered, as the real check does. + await sleep(10) + if (args[0].runId === null && !existsSync(args[0].statusPath)) return tmuxFake.open.delete(args[0].pane) tmuxFake.gone.add(args[0].pane) }, @@ -755,6 +759,27 @@ describe('agent commands in tmux', () => { } }) + it("closes a finished untracked run's pane when its terminal closes", async () => { + tmuxFake.on = true + tmuxFake.untracked = true + tmuxFake.statusPaths.clear() + tmuxFake.open.clear() + try { + const terminal = new TerminalService({ loadCwd: () => '/tmp' }) + const { activeTerminalId } = terminal.start({ cols: 80, rows: 24 }) + await terminal.executeTool('call-old-tmux', 'run', { command: 'make build', waitSeconds: 1 }) + const [[pane = '', statusPath = ''] = []] = [...tmuxFake.statusPaths] + writeFileSync(statusPath, '0') + + terminal.closeTerminal(activeTerminalId as string) + + await vi.waitFor(() => expect(tmuxFake.open.has(pane)).toBe(false)) + } finally { + tmuxFake.on = false + tmuxFake.untracked = false + } + }) + it('keeps the record of a finished run whose pane tmux could not confirm closing', async () => { tmuxFake.on = true tmuxFake.statusPaths.clear() diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 8254c98b3f8..adb0cfdf139 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -391,6 +391,8 @@ describe('stopping a run another process started, from its record', () => { }) expect(result).toMatchObject({ error: expect.stringContaining('was not run') }) + // The pane it opened and tagged is closed with it. + expect(tmux.read().panes).toEqual({}) await sleep(1_500) expect(existsSync(marker)).toBe(false) }) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index b5330736933..37c205c9dad 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -456,7 +456,9 @@ export async function startRun( : null const socket = shown?.ok && shown.stdout.trim().startsWith('/') ? shown.stdout.trim() : null if (runId && options.beforeStart && !(socket && options.beforeStart({ runId, pane, socket }))) { - // Without the go file the wrapper exits by itself. + // The pane was tagged a moment ago, so it is closed as the run's; without the go file its + // command never starts in any case. + await runTmux(ifTagged(pane, runId, `kill-pane -t ${pane}`), env) dispose() return { error: 'The command could not be recorded for a later stop, so it was not run.' } }