Skip to content

Commit 1ce44c8

Browse files
committed
fix(desktop): run untracked only on tmux without pane options, and reap and close finished untracked panes
- Only tmux before 3.0, which refuses `set-option -p` as an unknown flag or invalid option, runs a command untracked. A tmux that can tag panes but did not (it timed out, or failed otherwise) gets the run refused, as before, so no command starts that Stop and sign-out could never end. - tmux 3.x answers `display-message` for a pane that is gone with an empty line rather than an error, so an untracked run's pane is gone unless tmux echoes its id back. - A finished untracked run's pane is closed, and a run that finishes after its call returned has its pane closed when it is reaped, so dead panes kept by `remain-on-exit` do not pile up.
1 parent 67b544b commit 1ce44c8

4 files changed

Lines changed: 105 additions & 23 deletions

File tree

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,10 @@ export class TerminalService {
494494
}
495495
for (const handle of this.pendingRuns.get(terminalId) ?? []) {
496496
if (this.awaitedRuns.has(handle)) continue
497-
if (isRunComplete(handle) || (await runPaneState(handle, env)) === 'gone') {
497+
const complete = isRunComplete(handle)
498+
if (complete || (await runPaneState(handle, env)) === 'gone') {
499+
// A pane kept open after its command ended (`remain-on-exit`) closes with its run.
500+
if (complete) await closeRunPane(handle, env)
498501
this.untrackRun(terminalId, handle)
499502
handle.dispose()
500503
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ const tmuxFake = vi.hoisted(() => ({
1818
gone: new Set<string>(),
1919
/** Runs start untracked, as on a tmux too old to tag their panes. */
2020
untracked: false,
21+
/** Run panes tmux still shows, kept open after their command ends (`remain-on-exit`). */
22+
open: new Set<string>(),
2123
statusPaths: new Map<string, string>(),
2224
}))
2325

@@ -39,6 +41,7 @@ vi.mock('@/main/terminal/tmux', async () => {
3941
const statusPath = join(dir, 'status')
4042
writeFileSync(join(dir, 'out'), 'partial output')
4143
tmuxFake.statusPaths.set(pane, statusPath)
44+
tmuxFake.open.add(pane)
4245
return {
4346
window: `@${pane.slice(1)}`,
4447
pane,
@@ -62,6 +65,7 @@ vi.mock('@/main/terminal/tmux', async () => {
6265
},
6366
closeRunPane: async (...args: Parameters<typeof actual.closeRunPane>) => {
6467
if (!tmuxFake.on) return actual.closeRunPane(...args)
68+
tmuxFake.open.delete(args[0].pane)
6569
},
6670
}
6771
})
@@ -619,6 +623,27 @@ describe('agent commands in tmux', () => {
619623
}
620624
})
621625

626+
it("closes a run's pane when a later run reaps it after it finished", async () => {
627+
tmuxFake.on = true
628+
tmuxFake.statusPaths.clear()
629+
tmuxFake.open.clear()
630+
try {
631+
const terminal = new TerminalService({ loadCwd: () => '/tmp' })
632+
terminal.start({ cols: 80, rows: 24 })
633+
await terminal.executeTool('call-long', 'run', { command: 'make build', waitSeconds: 1 })
634+
const [[pane = '', statusPath = ''] = []] = [...tmuxFake.statusPaths]
635+
// It finishes after its call returned, and its dead pane stays open.
636+
writeFileSync(statusPath, '0')
637+
expect(tmuxFake.open.has(pane)).toBe(true)
638+
639+
await terminal.executeTool('call-next', 'run', { command: 'ls', waitSeconds: 1 })
640+
641+
expect(tmuxFake.open.has(pane)).toBe(false)
642+
} finally {
643+
tmuxFake.on = false
644+
}
645+
})
646+
622647
it("keeps a run's output readable for its call when the terminal goes away mid-wait", async () => {
623648
tmuxFake.on = true
624649
tmuxFake.statusPaths.clear()

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

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { sleep } from '@sim/utils/helpers'
55
import { afterEach, describe, expect, it } from 'vitest'
66
import {
77
awaitRun,
8+
closeRunPane,
89
isDescendantOf,
910
parseFormatLines,
1011
pollRun,
@@ -167,10 +168,11 @@ switch (args[0]) {
167168
break
168169
}
169170
case 'display-message': {
171+
// Like tmux 3.x, a pane that is gone answers with an empty line rather than an error.
170172
const pane = state.panes[target()]
171-
if (!pane) fail("can't find pane")
172173
const name = args[args.length - 1].slice(2, -1)
173-
process.stdout.write((pane.options[name] ?? '') + '\\n')
174+
const value = !pane ? '' : name === 'pane_id' ? target() : (pane.options[name] ?? '')
175+
process.stdout.write(value + '\\n')
174176
break
175177
}
176178
case 'list-clients': {
@@ -307,28 +309,62 @@ describe('stopping a tmux run touches only its own pane', () => {
307309
expect(tmux.read().log).toEqual([])
308310
})
309311

310-
it('starts a run it could not tag, untracked, and never stops it by a pane id', async () => {
312+
it.each(['invalid option: @sim-run-id', 'unknown flag -p'])(
313+
'starts a run untracked on a tmux without pane options (%s), and never stops it by a pane id',
314+
async (refusal) => {
315+
const tmux = fakeTmux()
316+
dirs.push(tmux.dir)
317+
// tmux before 3.0 has no pane options.
318+
tmux.write({ ...tmux.read(), fail: { 'set-option': refusal } })
319+
320+
const run = await startRun('agent', 'sleep 600', null, tmux.env)
321+
if ('error' in run) throw new Error(run.error)
322+
323+
expect(run.runId).toBeNull()
324+
expect(existsSync(join(run.statusPath, '..', 'go'))).toBe(true)
325+
expect(await runPaneState(run, tmux.env)).toBe('unknown')
326+
await stopRun(run, tmux.env, 0)
327+
// No pane is touched by an id that a restarted server might have handed to the user.
328+
expect(tmux.read().log).toEqual([])
329+
expect(Object.keys(tmux.read().panes)).toEqual([run.pane])
330+
331+
// Once its pane is gone, it can be let go.
332+
const state = tmux.read()
333+
delete state.panes[run.pane]
334+
tmux.write(state)
335+
expect(await runPaneState(run, tmux.env)).toBe('gone')
336+
}
337+
)
338+
339+
it.each(['tmux did not respond', 'server exited unexpectedly'])(
340+
'refuses a run that a tmux able to tag panes did not tag (%s)',
341+
async (failure) => {
342+
const tmux = fakeTmux()
343+
dirs.push(tmux.dir)
344+
tmux.write({ ...tmux.read(), fail: { 'set-option': failure } })
345+
346+
const result = await startRun('agent', 'sleep 600', null, tmux.env)
347+
348+
// Untagged on a tmux that tags, nothing could stop it later, so it never starts.
349+
expect(result).toMatchObject({ error: expect.stringContaining('was not run') })
350+
expect(tmux.read().log).toEqual([])
351+
}
352+
)
353+
354+
it("closes a finished untracked run's pane, and only once it has finished", async () => {
311355
const tmux = fakeTmux()
312356
dirs.push(tmux.dir)
313-
// tmux before 3.0 has no pane options.
314357
tmux.write({ ...tmux.read(), fail: { 'set-option': 'invalid option: @sim-run-id' } })
315-
316-
const run = await startRun('agent', 'sleep 600', null, tmux.env)
358+
const run = await startRun('agent', 'make build', null, tmux.env)
317359
if ('error' in run) throw new Error(run.error)
318360

319-
expect(run.runId).toBeNull()
320-
expect(existsSync(join(run.statusPath, '..', 'go'))).toBe(true)
321-
expect(await runPaneState(run, tmux.env)).toBe('unknown')
322-
await stopRun(run, tmux.env, 0)
323-
// No pane is touched by an id that a restarted server might have handed to the user.
324-
expect(tmux.read().log).toEqual([])
361+
await closeRunPane(run, tmux.env)
325362
expect(Object.keys(tmux.read().panes)).toEqual([run.pane])
326363

327-
// Once its pane is gone, it can be let go.
328-
const state = tmux.read()
329-
delete state.panes[run.pane]
330-
tmux.write(state)
331-
expect(await runPaneState(run, tmux.env)).toBe('gone')
364+
// Its command ended; with `remain-on-exit` its dead pane would otherwise stay open.
365+
writeFileSync(run.statusPath, '0')
366+
await closeRunPane(run, tmux.env)
367+
expect(Object.keys(tmux.read().panes)).toEqual([])
332368
})
333369

334370
it('lets a tagged run start only once its pane is tagged', async () => {

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

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,9 @@ const RUN_GATE_POLLS = Math.ceil((2 * TMUX_TIMEOUT_MS + 5_000) / 50)
330330
/** The tmux user option that marks a pane as one run's own. */
331331
const RUN_ID_OPTION = '@sim-run-id'
332332

333+
/** How tmux before 3.0, which has no pane options, refuses `set-option -p`. */
334+
const NO_PANE_OPTIONS = /unknown flag|invalid option/i
335+
333336
/**
334337
* Starts a command in a dedicated tmux window.
335338
*
@@ -414,10 +417,18 @@ export async function startRun(
414417
const [window = '', pane = ''] = created.stdout.trim().split(' ')
415418
const tag = generateId()
416419
// An untagged pane is never treated as the run's: without the tag a stop could not tell it from
417-
// a pane the user opened later under the same id, so the run is left untracked.
420+
// a pane the user opened later under the same id.
418421
const tagged = await runTmux(['set-option', '-p', '-t', pane, RUN_ID_OPTION, tag], env)
422+
if (!tagged.ok && !NO_PANE_OPTIONS.test(tagged.stderr)) {
423+
// A tmux that can tag panes but did not (it timed out, or failed otherwise) gets no command
424+
// that nothing could stop: without the go file the wrapper exits by itself.
425+
dispose()
426+
return {
427+
error: `tmux could not mark the command's pane (${tagged.stderr.trim() || 'no detail'}), so the command was not run.`,
428+
}
429+
}
419430
if (!tagged.ok) {
420-
logger.warn('tmux could not tag a run pane; the run goes ahead untracked', {
431+
logger.warn('This tmux cannot tag a run pane; the run goes ahead untracked', {
421432
error: tagged.stderr.trim(),
422433
})
423434
}
@@ -447,7 +458,10 @@ export async function runPaneState(
447458
// An untracked run's pane can still be found missing, with a format every tmux knows.
448459
const format = handle.runId === null ? '#{pane_id}' : `#{${RUN_ID_OPTION}}`
449460
const shown = await runTmux(['display-message', '-p', '-t', handle.pane, format], env)
450-
if (shown.ok && handle.runId === null) return 'unknown'
461+
// tmux 3.x answers for a missing pane with an empty line rather than an error.
462+
if (shown.ok && handle.runId === null) {
463+
return shown.stdout.trim() === handle.pane ? 'unknown' : 'gone'
464+
}
451465
if (shown.ok) return shown.stdout.trim() === handle.runId ? 'ours' : 'gone'
452466
return /can't find|no server running/i.test(shown.stderr) ? 'gone' : 'unknown'
453467
}
@@ -531,10 +545,14 @@ export async function killPane(target: string, env: NodeJS.ProcessEnv): Promise<
531545

532546
/**
533547
* Closes the pane opened by {@link startRun}, and with it the window once that pane is the last
534-
* one in it. Only the run's own pane, and only while it is still the run's.
548+
* one in it. Only the run's own pane, and only while it is still the run's. An untracked run's
549+
* pane is closed only once the run has written its exit status: its command has just ended in
550+
* that pane, so the id is still the one the run opened.
535551
*/
536552
export async function closeRunPane(handle: TmuxRunHandle, env: NodeJS.ProcessEnv): Promise<void> {
537-
if ((await runPaneState(handle, env)) !== 'ours') return
553+
const state = await runPaneState(handle, env)
554+
const finishedUntracked = handle.runId === null && state === 'unknown' && isRunComplete(handle)
555+
if (state !== 'ours' && !finishedUntracked) return
538556
const killed = await runTmux(['kill-pane', '-t', handle.pane], env)
539557
if (!killed.ok) {
540558
logger.warn('Could not close the tmux run pane', { error: killed.stderr.trim() })

0 commit comments

Comments
 (0)