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.

Author: waleedlatif1

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)

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<TerminalServiceOptions['runLedger']> = []
+    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 = {

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)

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)

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<RunRecord>
+    const parsed = JSON.parse(text) as Partial<RecordedRun>
     if (
       typeof parsed.runId === 'string' &&
       RUN_ID.test(parsed.runId) &&
@@ -54,26 +48,35 @@ 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<string>()
   const pathFor = (runId: string) => join(dir, `${runId}.json`)
 
   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,18 +85,23 @@ 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 {
           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 })
+          remove(join(dir, name))
           continue
         }
         if (options.excludeLive && live.has(record.runId)) continue

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

@@ -37,17 +37,29 @@ vi.mock('@/main/terminal/tmux', async () => {
         : actual.resolveAttachment(pid, env),
     startRun: async (...args: Parameters<typeof actual.startRun>) => {
       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)
       tmuxFake.open.add(pane)
       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 })
     }
   })