Skip to content

Commit aa07d1a

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 f55a277 commit aa07d1a

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,
@@ -150,10 +151,11 @@ switch (args[0]) {
150151
break
151152
}
152153
case 'display-message': {
154+
// Like tmux 3.x, a pane that is gone answers with an empty line rather than an error.
153155
const pane = state.panes[target()]
154-
if (!pane) fail("can't find pane")
155156
const name = args[args.length - 1].slice(2, -1)
156-
process.stdout.write((pane.options[name] ?? '') + '\\n')
157+
const value = !pane ? '' : name === 'pane_id' ? target() : (pane.options[name] ?? '')
158+
process.stdout.write(value + '\\n')
157159
break
158160
}
159161
case 'send-keys':
@@ -256,28 +258,62 @@ describe('stopping a tmux run touches only its own pane', () => {
256258
expect(tmux.read().log).toEqual([])
257259
})
258260

259-
it('starts a run it could not tag, untracked, and never stops it by a pane id', async () => {
261+
it.each(['invalid option: @sim-run-id', 'unknown flag -p'])(
262+
'starts a run untracked on a tmux without pane options (%s), and never stops it by a pane id',
263+
async (refusal) => {
264+
const tmux = fakeTmux()
265+
dirs.push(tmux.dir)
266+
// tmux before 3.0 has no pane options.
267+
tmux.write({ ...tmux.read(), fail: { 'set-option': refusal } })
268+
269+
const run = await startRun('agent', 'sleep 600', null, tmux.env)
270+
if ('error' in run) throw new Error(run.error)
271+
272+
expect(run.runId).toBeNull()
273+
expect(existsSync(join(run.statusPath, '..', 'go'))).toBe(true)
274+
expect(await runPaneState(run, tmux.env)).toBe('unknown')
275+
await stopRun(run, tmux.env, 0)
276+
// No pane is touched by an id that a restarted server might have handed to the user.
277+
expect(tmux.read().log).toEqual([])
278+
expect(Object.keys(tmux.read().panes)).toEqual([run.pane])
279+
280+
// Once its pane is gone, it can be let go.
281+
const state = tmux.read()
282+
delete state.panes[run.pane]
283+
tmux.write(state)
284+
expect(await runPaneState(run, tmux.env)).toBe('gone')
285+
}
286+
)
287+
288+
it.each(['tmux did not respond', 'server exited unexpectedly'])(
289+
'refuses a run that a tmux able to tag panes did not tag (%s)',
290+
async (failure) => {
291+
const tmux = fakeTmux()
292+
dirs.push(tmux.dir)
293+
tmux.write({ ...tmux.read(), fail: { 'set-option': failure } })
294+
295+
const result = await startRun('agent', 'sleep 600', null, tmux.env)
296+
297+
// Untagged on a tmux that tags, nothing could stop it later, so it never starts.
298+
expect(result).toMatchObject({ error: expect.stringContaining('was not run') })
299+
expect(tmux.read().log).toEqual([])
300+
}
301+
)
302+
303+
it("closes a finished untracked run's pane, and only once it has finished", async () => {
260304
const tmux = fakeTmux()
261305
dirs.push(tmux.dir)
262-
// tmux before 3.0 has no pane options.
263306
tmux.write({ ...tmux.read(), fail: { 'set-option': 'invalid option: @sim-run-id' } })
264-
265-
const run = await startRun('agent', 'sleep 600', null, tmux.env)
307+
const run = await startRun('agent', 'make build', null, tmux.env)
266308
if ('error' in run) throw new Error(run.error)
267309

268-
expect(run.runId).toBeNull()
269-
expect(existsSync(join(run.statusPath, '..', 'go'))).toBe(true)
270-
expect(await runPaneState(run, tmux.env)).toBe('unknown')
271-
await stopRun(run, tmux.env, 0)
272-
// No pane is touched by an id that a restarted server might have handed to the user.
273-
expect(tmux.read().log).toEqual([])
310+
await closeRunPane(run, tmux.env)
274311
expect(Object.keys(tmux.read().panes)).toEqual([run.pane])
275312

276-
// Once its pane is gone, it can be let go.
277-
const state = tmux.read()
278-
delete state.panes[run.pane]
279-
tmux.write(state)
280-
expect(await runPaneState(run, tmux.env)).toBe('gone')
313+
// Its command ended; with `remain-on-exit` its dead pane would otherwise stay open.
314+
writeFileSync(run.statusPath, '0')
315+
await closeRunPane(run, tmux.env)
316+
expect(Object.keys(tmux.read().panes)).toEqual([])
281317
})
282318

283319
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
@@ -325,6 +325,9 @@ const RUN_GATE_POLLS = Math.ceil((2 * TMUX_TIMEOUT_MS + 5_000) / 50)
325325
/** The tmux user option that marks a pane as one run's own. */
326326
const RUN_ID_OPTION = '@sim-run-id'
327327

328+
/** How tmux before 3.0, which has no pane options, refuses `set-option -p`. */
329+
const NO_PANE_OPTIONS = /unknown flag|invalid option/i
330+
328331
/**
329332
* Starts a command in a dedicated tmux window.
330333
*
@@ -409,10 +412,18 @@ export async function startRun(
409412
const [window = '', pane = ''] = created.stdout.trim().split(' ')
410413
const tag = generateId()
411414
// An untagged pane is never treated as the run's: without the tag a stop could not tell it from
412-
// a pane the user opened later under the same id, so the run is left untracked.
415+
// a pane the user opened later under the same id.
413416
const tagged = await runTmux(['set-option', '-p', '-t', pane, RUN_ID_OPTION, tag], env)
417+
if (!tagged.ok && !NO_PANE_OPTIONS.test(tagged.stderr)) {
418+
// A tmux that can tag panes but did not (it timed out, or failed otherwise) gets no command
419+
// that nothing could stop: without the go file the wrapper exits by itself.
420+
dispose()
421+
return {
422+
error: `tmux could not mark the command's pane (${tagged.stderr.trim() || 'no detail'}), so the command was not run.`,
423+
}
424+
}
414425
if (!tagged.ok) {
415-
logger.warn('tmux could not tag a run pane; the run goes ahead untracked', {
426+
logger.warn('This tmux cannot tag a run pane; the run goes ahead untracked', {
416427
error: tagged.stderr.trim(),
417428
})
418429
}
@@ -442,7 +453,10 @@ export async function runPaneState(
442453
// An untracked run's pane can still be found missing, with a format every tmux knows.
443454
const format = handle.runId === null ? '#{pane_id}' : `#{${RUN_ID_OPTION}}`
444455
const shown = await runTmux(['display-message', '-p', '-t', handle.pane, format], env)
445-
if (shown.ok && handle.runId === null) return 'unknown'
456+
// tmux 3.x answers for a missing pane with an empty line rather than an error.
457+
if (shown.ok && handle.runId === null) {
458+
return shown.stdout.trim() === handle.pane ? 'unknown' : 'gone'
459+
}
446460
if (shown.ok) return shown.stdout.trim() === handle.runId ? 'ours' : 'gone'
447461
return /can't find|no server running/i.test(shown.stderr) ? 'gone' : 'unknown'
448462
}
@@ -526,10 +540,14 @@ export async function killPane(target: string, env: NodeJS.ProcessEnv): Promise<
526540

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

0 commit comments

Comments
 (0)