From f774cbfaafe281d8c806625e27e691e174cdbf85 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 23:13:52 -0700 Subject: [PATCH 1/2] fix(tmux): reap a closed terminal's shell once its undo window has passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1030 item 4. Closing a tmux-backed terminal stops its attach-PTY and deliberately leaves the tmux session running, so Undo Close can re-attach the same shell with its scrollback intact (#671). Nothing killed it afterwards: `killSession` had two callers, failed-spawn rollback and startup reconciliation. So every terminal a user closed kept an idle shell alive until the next launch. The unified stage made it more visible rather than causing it — boot hibernates every lane but the focused one, so main does not even observe most of those closes. `DetachedTerminalSweep` runs every five minutes and reaps a managed tmux session that no longer appears in either authority — the persisted workspace file, decoded by the SAME function startup uses, and main's own live registry (which covers a terminal autosave has not written yet) — and has been absent from both for longer than the undo window plus a grace. The retention window is now one constant in `shared/`, because two processes have to agree on it: the renderer expires the entry that can ask for the shell back, main reaps the shell. Reaping earlier restores a pane onto a dead terminal; reaping never is the leak. Cleanup authority follows startup's rule (#898): only a COMPLETE inventory proves orphanhood, so an unreadable or partially-decoded workspace file withholds every kill without resetting the timers. WHY no renderer message on close: a hibernated terminal has no live session on either side of the close, so no such message exists to send — and a reaping decision behind a message a crashed renderer never sends is not a decision. Tests drive the sweep against the real recorded workspace.json (one live terminal reference) and pin four invariants by mutation: no kill inside the window, no kill of a name main owns, no kill on an incomplete inventory, and a failed kill retried on the next sweep rather than granted a fresh hour. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/applicationShutdown.test.ts | 1 + src/main/applicationShutdown.ts | 5 + src/main/index.ts | 38 +++ src/main/sessionManager.ts | 18 ++ src/main/tmux/detachedSweep.test.ts | 283 ++++++++++++++++++ src/main/tmux/detachedSweep.ts | 215 +++++++++++++ src/main/tmux/tmuxRecovery.ts | 15 +- ...eEditorBeforeUnloadGuard.renderer.test.tsx | 1 + src/renderer/src/lib/undoClose.ts | 8 +- src/shared/undoRetention.ts | 22 ++ 10 files changed, 603 insertions(+), 3 deletions(-) create mode 100644 src/main/tmux/detachedSweep.test.ts create mode 100644 src/main/tmux/detachedSweep.ts create mode 100644 src/shared/undoRetention.ts diff --git a/src/main/applicationShutdown.test.ts b/src/main/applicationShutdown.test.ts index 7514f3a66..24c81392a 100644 --- a/src/main/applicationShutdown.test.ts +++ b/src/main/applicationShutdown.test.ts @@ -35,6 +35,7 @@ function harness() { disposeWorkflowBridge: vi.fn(async (): Promise => undefined), disposeCaffeinate: vi.fn(async (): Promise => undefined), stopHeapWatchdog: vi.fn(async (): Promise => undefined), + stopDetachedTmuxSweep: vi.fn(async (): Promise => undefined), drainWorkspace: vi.fn(async (): Promise => undefined), drainDictationHistory: vi.fn(async (): Promise => undefined), flushGhosts: vi.fn(async (): Promise => undefined), diff --git a/src/main/applicationShutdown.ts b/src/main/applicationShutdown.ts index 550cb5b7a..012bb933d 100644 --- a/src/main/applicationShutdown.ts +++ b/src/main/applicationShutdown.ts @@ -21,6 +21,10 @@ export interface ApplicationShutdownServices { disposeWorkflowBridge: Stop disposeCaffeinate: Stop stopHeapWatchdog: Stop + /** The periodic detached-tmux reaper (#1030 item 4). Stopping it is pure + * timer teardown — it must NEVER kill tmux sessions at quit, because those + * shells are exactly what the next launch recovers terminals from. */ + stopDetachedTmuxSweep: Stop drainWorkspace: Stop drainDictationHistory: Stop flushGhosts: Stop @@ -131,6 +135,7 @@ export function installApplicationShutdown(options: { run('workflow-bridge', services.disposeWorkflowBridge), run('caffeinate', services.disposeCaffeinate), run('heap-watchdog', services.stopHeapWatchdog), + run('detached-tmux-sweep', services.stopDetachedTmuxSweep), ]) await join([ run('workspace', services.drainWorkspace), diff --git a/src/main/index.ts b/src/main/index.ts index 9467aef4b..21bedb459 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -55,6 +55,8 @@ import { } from '@main/pasteDebugJournal.js' import { TmuxRegistry } from '@main/tmux/TmuxRegistry.js' import { reconcileWorkspace } from '@main/tmux/tmuxRecovery.js' +import { startDetachedTerminalSweep } from '@main/tmux/detachedSweep.js' +import type { DetachedSweepSchedule } from '@main/tmux/detachedSweep.js' import { handleExtensionScheme, @@ -277,6 +279,8 @@ let manager: SessionManager | null = null let remoteController: RemoteController | null = null let remoteWorkspaceProjection: RemoteWorkspaceProjection | null = null let tmuxRegistry: TmuxRegistry | null = null +// The running-app tmux reaper's timer handle, so quit can stop it (#1030). +let detachedTmuxSweep: DetachedSweepSchedule | null = null let stateProcessLock: Extract | null = null let appRunJournal: AppRunJournal | null = null let workflowService: WorkflowService | null = null @@ -945,6 +949,39 @@ async function startApp(): Promise { systemSuspension.on('suspension', (suspension: import('@shared/types/systemSuspension.js').SystemSuspension) => { manager?.noteSystemSuspension(suspension) }) + + // The running-app half of tmux cleanup (#1030 item 4). Startup reconciliation + // above reaps what a previous run left behind; this reaps what THIS run + // leaves behind, once the undo window that kept each shell alive has closed. + // Armed only when tmux is available and only after the manager exists, + // because the manager is one of the two authorities the sweep consults — the + // other is the persisted workspace file, read through the same decoder + // startup uses. + if (tmuxAvailable && manager) { + const sweepingManager = manager + detachedTmuxSweep = startDetachedTerminalSweep({ + registry: tmuxRegistry, + readWorkspace: () => readFile(STATE_FILE, 'utf8'), + liveTmuxNames: () => sweepingManager.getLiveTmuxNames(), + onSweep: report => { + // Only worth a line when something actually happened; a five-minute + // heartbeat saying "nothing" would bury the log. + if (report.reaped.length === 0) return + console.log(`[tmux] reaped ${report.reaped.length} detached terminal session(s)`) + appRunJournal?.record({ + area: 'app.tmux', + name: 'tmux.detached.reaped', + data: { reaped: report.reaped.length, pending: report.pending.length }, + }) + }, + onError: error => { + // Never fatal: a failed sweep is a leak that gets another chance in + // five minutes, not a reason to stop cleaning for the rest of the run. + performanceService.error('app.tmux.detachedSweep.error', error) + appRunJournal?.recordError('tmux.detached_sweep.error', error) + }, + }) + } // Project ownership lives in renderer state, while backend/transcript facts // live in SessionManager. Construct this bridge only after both the MCP host // and manager exist so tool calls cannot observe a half-wired authority. @@ -1421,6 +1458,7 @@ const sessionShutdownGate = installApplicationShutdown({ disposeWorkflowBridge: () => workflowBridge?.dispose(), disposeCaffeinate: () => caffeinateController.dispose(), stopHeapWatchdog: stopMainHeapWatchdog, + stopDetachedTmuxSweep: () => { detachedTmuxSweep?.stop(); detachedTmuxSweep = null }, drainWorkspace: () => shutdownWorkspaceStore?.drainAdmittedWrites(), drainDictationHistory: flushHistoryWrites, flushGhosts: () => ghostJournals.flushAll(), diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index ba35ee699..fb4efe56a 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -4925,6 +4925,24 @@ export class SessionManager extends EventEmitter { return this.terminalForeground.snapshot() } + /** + * Every tmux session name main currently owns a live terminal for. + * + * The detached sweep (`tmux/detachedSweep.ts`) needs this as the second + * authority beside the persisted workspace file: autosave is debounced, so a + * terminal created seconds ago is live here and absent from the file, and the + * file alone would read it as an orphan and kill the user's brand-new shell. + * Read live from the registry rather than cached — a snapshot taken before an + * await is exactly the stale evidence that would authorize that kill. + */ + getLiveTmuxNames(): string[] { + const names: string[] = [] + for (const entry of this.sessions.values()) { + if (entry.kind === 'terminal' && entry.tmuxName) names.push(entry.tmuxName) + } + return names + } + /** Kill every live session. Called on app quit. */ async killAll(): Promise { this.shuttingDown = true diff --git a/src/main/tmux/detachedSweep.test.ts b/src/main/tmux/detachedSweep.test.ts new file mode 100644 index 000000000..742a0de5c --- /dev/null +++ b/src/main/tmux/detachedSweep.test.ts @@ -0,0 +1,283 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it, vi } from 'vitest' + +import { DETACHED_REAP_GRACE_MS, DetachedTerminalSweep, startDetachedTerminalSweep } from '@main/tmux/detachedSweep.js' +import { UNDO_CLOSE_RETENTION_MS } from '@shared/undoRetention.js' + +// The running-app half of tmux cleanup (#1030 item 4). Startup reconciliation +// is covered by tmuxRecovery.test.ts and shares this file's decoder; what is +// pinned here is the part that had NO cleanup at all: a terminal closed while +// the app keeps running left its shell alive until the next launch. +// +// Only the tmux process boundary is fake — same rule as tmuxRecovery.test.ts, +// no test may address the user's real server. The workspace input is the real +// persisted file. + +function registryWith(...names: string[]) { + return { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => names.map(name => ({ name, createdAt: 1 }))), + killSession: vi.fn(async (_name: string) => {}), + } +} + +// The live workspace.json the app persisted on 2026-09-19, sanitized +// (testing/fixtures/workspace-v2/README.md). It holds 27 sessions, exactly one +// of which is a terminal with a tmuxName — the reference a sweep must respect. +const RECORDED = readFileSync( + new URL('../../../testing/fixtures/workspace-v2/2026-09-19-live-workspace.sanitized.json', import.meta.url), + 'utf8', +) + +function recordedTerminalNames(): string[] { + const parsed = JSON.parse(RECORDED) as { + windows: { workspace: { sessions: Record } }[] + } + return parsed.windows + .flatMap(window => Object.values(window.workspace.sessions)) + .filter(session => session.kind === 'terminal' && session.tmuxName) + .map(session => session.tmuxName!) +} + +const REAP_AFTER_MS = UNDO_CLOSE_RETENTION_MS + DETACHED_REAP_GRACE_MS + +describe('detached tmux sweep', () => { + it('reaps a closed terminal once the undo window has passed, and never the live one', async () => { + const live = recordedTerminalNames() + expect(live.length).toBeGreaterThan(0) + + // `agentcode-closed` is the shape of the leak: the pane is gone, so the + // renderer's autosave no longer lists it, but the shell is still running. + const registry = registryWith(...live, 'agentcode-closed') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => live, + now: () => clock, + }) + + // First sighting only starts the clock — Undo Close can still bring it back. + expect(await sweep.run()).toEqual({ reaped: [], pending: ['agentcode-closed'] }) + expect(registry.killSession).not.toHaveBeenCalled() + + // Still inside the window one minute before the deadline. + clock += REAP_AFTER_MS - 60_000 + expect((await sweep.run()).reaped).toEqual([]) + expect(registry.killSession).not.toHaveBeenCalled() + + clock += 60_001 + expect((await sweep.run()).reaped).toEqual(['agentcode-closed']) + expect(registry.killSession).toHaveBeenCalledExactlyOnceWith('agentcode-closed') + for (const name of live) expect(registry.killSession).not.toHaveBeenCalledWith(name) + }) + + it('never reaps a terminal main still owns, even before autosave has written it', async () => { + // A terminal created seconds ago: live in main's registry, absent from the + // persisted file the renderer has not flushed yet. The workspace file alone + // would call it an orphan. + const registry = registryWith('agentcode-just-spawned') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => ['agentcode-just-spawned'], + now: () => clock, + }) + + for (let i = 0; i < 4; i += 1) { + expect((await sweep.run()).reaped).toEqual([]) + clock += REAP_AFTER_MS + } + expect(registry.killSession).not.toHaveBeenCalled() + }) + + it('restarts the clock when Undo Close brings the terminal back', async () => { + const registry = registryWith('agentcode-undone') + let clock = 1_000_000 + let restored = false + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + // The restore re-attaches the same tmux session, so it reappears as a + // live name (and, once autosave runs, in the file again). + liveTmuxNames: () => (restored ? ['agentcode-undone'] : []), + now: () => clock, + }) + + await sweep.run() + clock += REAP_AFTER_MS - 1_000 + restored = true + await sweep.run() + + // Closed again, well past the ORIGINAL deadline: the window must start over. + restored = false + clock += 2_000 + expect((await sweep.run()).reaped).toEqual([]) + clock += REAP_AFTER_MS - 1_000 + expect((await sweep.run()).reaped).toEqual([]) + clock += 2_000 + expect((await sweep.run()).reaped).toEqual(['agentcode-undone']) + }) + + it('withholds cleanup when the inventory is not complete', async () => { + // A partially-decoded file cannot prove orphanhood (#898). The discarded + // region is exactly where a terminal's only reference may have been. + const registry = registryWith('agentcode-unknown') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => JSON.stringify({ version: 2, windows: 'not-an-array' }), + liveTmuxNames: () => [], + now: () => clock, + }) + + expect(await sweep.run()).toEqual({ reaped: [], pending: [], withheld: 'inventory-incomplete' }) + clock += REAP_AFTER_MS * 2 + expect((await sweep.run()).withheld).toBe('inventory-incomplete') + expect(registry.killSession).not.toHaveBeenCalled() + }) + + it('withholds cleanup when the workspace file cannot be read', async () => { + const registry = registryWith('agentcode-unknown') + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => { throw Object.assign(new Error('gone'), { code: 'ENOENT' }) }, + liveTmuxNames: () => [], + now: () => 1_000_000, + }) + expect((await sweep.run()).withheld).toBe('inventory-incomplete') + expect(registry.killSession).not.toHaveBeenCalled() + }) + + it('does nothing at all when tmux is unavailable', async () => { + const registry = { ...registryWith('agentcode-closed'), isAvailable: () => false } + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => 1_000_000, + }) + expect((await sweep.run()).withheld).toBe('tmux-unavailable') + expect(registry.listManagedSessions).not.toHaveBeenCalled() + }) + + it('retries a kill that failed instead of restarting the window', async () => { + const registry = registryWith('agentcode-stubborn') + registry.killSession = vi.fn(async () => { throw new Error('tmux busy') }) + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => clock, + }) + + await sweep.run() + clock += REAP_AFTER_MS + 1 + await expect(sweep.run()).rejects.toThrow('tmux busy') + // The timestamp survived the throw, so the very next sweep tries again + // rather than granting another full hour. + registry.killSession = vi.fn(async (_name: string) => {}) + expect((await sweep.run()).reaped).toEqual(['agentcode-stubborn']) + }) + + it('forgets a name that died on its own, so a recycled name is not born overdue', async () => { + let alive = ['agentcode-recycled'] + const registry = { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => alive.map(name => ({ name, createdAt: 1 }))), + killSession: vi.fn(async (_name: string) => {}), + } + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => clock, + }) + + await sweep.run() // clock starts + alive = [] // the user typed `exit` + clock += REAP_AFTER_MS + 1 + await sweep.run() + + // Same name again — tmux names are UUID-derived so this is defensive, but + // a stale timestamp would kill a brand-new shell on its first sighting. + alive = ['agentcode-recycled'] + expect((await sweep.run()).reaped).toEqual([]) + expect(registry.killSession).not.toHaveBeenCalled() + }) +}) + +describe('detached tmux sweep schedule', () => { + it('runs on the interval, never overlaps itself, and survives a throwing run', async () => { + vi.useFakeTimers() + try { + let release!: () => void + const gate = new Promise(resolve => { release = resolve }) + let started = 0 + const registry = { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => { + started += 1 + // The FIRST run hangs on a slow `tmux list-sessions`; later runs do not. + if (started === 1) await gate + if (started === 2) throw new Error('tmux went away') + return [] + }), + killSession: vi.fn(async (_name: string) => {}), + } + const errors: unknown[] = [] + const schedule = startDetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + intervalMs: 1_000, + onError: error => { errors.push(error) }, + }) + + // Three ticks while the first run is still in flight: the in-flight flag + // must collapse them, or a hung tmux call queues a backlog of sweeps. + await vi.advanceTimersByTimeAsync(3_500) + expect(started).toBe(1) + + release() + await vi.advanceTimersByTimeAsync(1_000) + expect(started).toBe(2) + + // A throwing sweep is reported and the timer keeps going. + await vi.advanceTimersByTimeAsync(1_000) + expect(errors).toHaveLength(1) + expect(started).toBe(3) + + schedule.stop() + await vi.advanceTimersByTimeAsync(10_000) + expect(started).toBe(3) + } finally { + vi.useRealTimers() + } + }) + + it('does not run immediately — startup reconciliation just did that pass', async () => { + vi.useFakeTimers() + try { + const registry = { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => []), + killSession: vi.fn(async (_name: string) => {}), + } + const schedule = startDetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + intervalMs: 1_000, + }) + await vi.advanceTimersByTimeAsync(999) + expect(registry.listManagedSessions).not.toHaveBeenCalled() + schedule.stop() + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/main/tmux/detachedSweep.ts b/src/main/tmux/detachedSweep.ts new file mode 100644 index 000000000..cee73e0f6 --- /dev/null +++ b/src/main/tmux/detachedSweep.ts @@ -0,0 +1,215 @@ +// Reap tmux sessions whose terminal pane is gone for good, WHILE THE APP RUNS. +// +// ── THE LEAK (#1030 item 4) ── +// Closing a tmux-backed terminal stops its attach-PTY and deliberately leaves +// the tmux session alive, so Undo Close can re-attach the same shell with its +// scrollback intact. Nothing ever killed it afterwards: `TmuxRegistry.killSession` +// had exactly two callers — failed-spawn rollback, and `reconcileWorkspace` at +// STARTUP. So every terminal a user closed kept an idle shell (and whatever it +// was running) alive until the next launch. On a machine that stays awake for +// days, with a workflow that opens and closes terminals freely, that is a real +// pile of processes. The unified stage made it worse rather than causing it: +// boot now hibernates every lane but the focused one, so there are more +// terminals whose close main never even observes as a session stop. +// +// ── WHY A SWEEP AND NOT A KILL AT CLOSE TIME ── +// Close is not the moment the shell becomes unreachable — the undo entry is. +// Killing at close would break Undo Close for terminals, which is the entire +// reason the session is left running (#671: losing the scrollback is the bug). +// +// ── WHY THE RENDERER IS NOT ASKED ── +// The obvious alternative is for the renderer to tell main "this tmuxName is +// detached" on close, and to cancel on undo. That cannot cover a HIBERNATED +// terminal — the pane has no live session either side of the close, and main +// hears nothing. It also puts a durable reaping decision behind a message that +// a crashed or reloaded renderer simply never sends. So this asks the two +// authorities that are always right instead: the persisted workspace file +// (which terminals still exist) and main's own registry (which are live right +// now, including ones autosave has not written yet). +// +// ── THE AUTHORITY RULE, INHERITED FROM STARTUP RECOVERY ── +// The `agentcode-` prefix proves a session is OURS, never that it is abandoned. +// Only a COMPLETE workspace inventory can supply the second fact, so an +// unreadable or partially-decoded file withholds cleanup entirely, exactly as +// `reconcileWorkspace` does (#898). A sweep that guessed here would delete a +// terminal whose only reference sat in a region the decoder discarded. +// +// ── WHY THE CLOCK STARTS AT FIRST SIGHTING, NOT AT CLOSE ── +// This holds no close events, so it cannot know when a name became +// unreferenced; it knows when it first OBSERVED that. With a sweep interval +// far shorter than the retention window the error is bounded by one interval +// and always in the safe direction (reaping later than necessary). Restoring a +// terminal through Undo Close puts its name back in the workspace file and in +// main's registry, which clears the timer — a later close starts a fresh one. +// +// The map is in-memory on purpose. Across a restart the undo stack is gone too +// (it is deliberately not persisted), so startup reconcile is free to kill an +// unreferenced session immediately; nothing needs to survive the process. + +import { UNDO_CLOSE_RETENTION_MS } from '@shared/undoRetention.js' + +import { terminalInventoryFromWorkspaceText } from '@main/tmux/tmuxRecovery.js' + +/** The slice of TmuxRegistry a sweep needs; see tmuxRecovery's RecoveryRegistry. */ +export type SweepRegistry = { + isAvailable: () => boolean + listManagedSessions: () => Promise> + killSession: (name: string) => Promise +} + +export type DetachedSweepReport = { + /** Names killed by THIS run. */ + reaped: string[] + /** Alive, unreferenced, still inside the retention window. */ + pending: string[] + /** Why cleanup was withheld, when it was. Absent on a normal run. */ + withheld?: 'tmux-unavailable' | 'inventory-incomplete' +} + +/** + * Grace added on top of the undo window before a shell is reaped. + * + * The undo entry expires at exactly UNDO_CLOSE_RETENTION_MS, and the renderer + * prunes LAZILY — on push/pop/peek/length — so an entry can still be sitting + * in the stack, restorable the instant the user presses ⌘⇧T, a few moments + * after its nominal expiry. Reaping on the same edge would turn that into a + * restore onto a dead shell. Five minutes is far longer than any such lag and + * irrelevant against a one-hour window. + */ +export const DETACHED_REAP_GRACE_MS = 5 * 60 * 1000 + +export type DetachedSweepOptions = { + registry: SweepRegistry + /** The persisted workspace file's text — the same reader startup uses. */ + readWorkspace: () => Promise + /** tmux names main currently owns. Covers terminals autosave has not written yet. */ + liveTmuxNames: () => Iterable + now?: () => number + retentionMs?: number +} + +export class DetachedTerminalSweep { + /** name → when this sweep FIRST saw it alive with no reference anywhere. */ + private firstUnreferencedAt = new Map() + + private readonly now: () => number + private readonly retentionMs: number + + constructor(private readonly options: DetachedSweepOptions) { + this.now = options.now ?? Date.now + this.retentionMs = options.retentionMs ?? UNDO_CLOSE_RETENTION_MS + DETACHED_REAP_GRACE_MS + } + + async run(): Promise { + const { registry, readWorkspace, liveTmuxNames } = this.options + if (!registry.isAvailable()) { + // Not evidence about any session: forget nothing, kill nothing. + return { reaped: [], pending: [], withheld: 'tmux-unavailable' } + } + + let text: string + try { + text = await readWorkspace() + } catch { + return { reaped: [], pending: [], withheld: 'inventory-incomplete' } + } + const inventory = terminalInventoryFromWorkspaceText(text) + if (inventory.kind !== 'complete') { + // Same rule as startup: an inventory we cannot vouch for authorizes + // nothing. Deliberately NOT clearing the timers — the next readable + // sweep should not have to start the clock over because one autosave + // was caught mid-write. + return { reaped: [], pending: [], withheld: 'inventory-incomplete' } + } + + const referenced = new Set(inventory.references.map(reference => reference.tmuxName)) + for (const name of liveTmuxNames()) referenced.add(name) + + const alive = await registry.listManagedSessions() + const aliveNames = new Set(alive.map(session => session.name)) + const now = this.now() + const reaped: string[] = [] + const pending: string[] = [] + + for (const name of aliveNames) { + if (referenced.has(name)) { + // Back in use — an Undo Close restore, or autosave catching up with a + // terminal that was live all along. Its clock restarts if it is ever + // unreferenced again. + this.firstUnreferencedAt.delete(name) + continue + } + const since = this.firstUnreferencedAt.get(name) + if (since === undefined) { + this.firstUnreferencedAt.set(name, now) + pending.push(name) + continue + } + if (now - since < this.retentionMs) { + pending.push(name) + continue + } + // kill first, forget second: a kill that throws keeps its timer, so the + // next sweep retries instead of restarting the whole window. + await registry.killSession(name) + this.firstUnreferencedAt.delete(name) + reaped.push(name) + } + + // A name that is no longer alive was killed by something else (a user + // typing `exit`, startup recovery, tmux itself). Holding its timestamp + // would make a RECYCLED name look overdue the moment it appeared. + for (const name of [...this.firstUnreferencedAt.keys()]) { + if (!aliveNames.has(name)) this.firstUnreferencedAt.delete(name) + } + + return { reaped, pending } + } +} + +/** + * How often the sweep looks. Bounds how late a reap can be (one interval on + * top of the retention window) and is the whole cost of the feature: one + * `tmux list-sessions` and one workspace-file read every five minutes. + */ +export const DETACHED_SWEEP_INTERVAL_MS = 5 * 60 * 1000 + +export type DetachedSweepSchedule = { stop: () => void } + +/** + * Run the sweep on a timer until stopped. + * + * WHY there is no immediate first run: startup reconciliation has just + * finished, and it already killed everything unreferenced (with no retention, + * because the undo stack does not survive a restart). A sweep firing seconds + * later could only re-examine what that pass already judged, and the names it + * would newly see are terminals THIS run has not even created yet. + * + * A throwing sweep is logged by the caller's `onError` and the timer keeps + * going: the tmux binary being momentarily unavailable, or one kill failing, + * must not silently end cleanup for the rest of the app's life. Overlap is + * prevented by an in-flight flag rather than by chaining timeouts, so a hung + * `tmux list-sessions` cannot queue a backlog of sweeps behind it. + */ +export function startDetachedTerminalSweep( + options: DetachedSweepOptions & { + intervalMs?: number + onSweep?: (report: DetachedSweepReport) => void + onError?: (error: unknown) => void + }, +): DetachedSweepSchedule { + const sweep = new DetachedTerminalSweep(options) + let running = false + const timer = setInterval(() => { + if (running) return + running = true + void sweep + .run() + .then(report => options.onSweep?.(report)) + .catch(error => options.onError?.(error)) + .finally(() => { running = false }) + }, options.intervalMs ?? DETACHED_SWEEP_INTERVAL_MS) + // Reaping idle shells is never a reason to hold the process open. + timer.unref?.() + return { stop: () => clearInterval(timer) } +} diff --git a/src/main/tmux/tmuxRecovery.ts b/src/main/tmux/tmuxRecovery.ts index 90b317ce2..9326608d2 100644 --- a/src/main/tmux/tmuxRecovery.ts +++ b/src/main/tmux/tmuxRecovery.ts @@ -31,7 +31,7 @@ type InventoryIssue = // workspace content, paths, or per-row parse errors into the startup journal. type InventoryIssues = Partial> -type TerminalInventory = { +export type TerminalInventory = { kind: 'complete' | 'incomplete' | 'unknown' references: PersistedTerminalRef[] issues: InventoryIssues @@ -135,7 +135,18 @@ export async function reconcileWorkspace( // different inventories apart. A digest carries that evidence without // copying workspace paths, prompts, or terminal content into diagnostics. const digest = createHash('sha256').update(text, 'utf8').digest('hex') - return reconcile(registry, terminalInventory(parseWorkspaceFile(text, randomUUID)), digest) + return reconcile(registry, terminalInventoryFromWorkspaceText(text), digest) +} + +/** + * Decode workspace.json text into the terminal inventory, through the canonical + * envelope decoder. Exported for the running-app sweep (`detachedSweep.ts`), + * which needs the SAME answer startup gets — including `kind`, the authority + * bit that says whether the absence of a reference may be read as orphanhood. + * Two decoders here is exactly the shape that produced #898. + */ +export function terminalInventoryFromWorkspaceText(text: string): TerminalInventory { + return terminalInventory(parseWorkspaceFile(text, randomUUID)) } async function reconcile( diff --git a/src/renderer/src/features/global-editor/hooks/useEditorBeforeUnloadGuard.renderer.test.tsx b/src/renderer/src/features/global-editor/hooks/useEditorBeforeUnloadGuard.renderer.test.tsx index 0e51816b6..e10a7db3c 100644 --- a/src/renderer/src/features/global-editor/hooks/useEditorBeforeUnloadGuard.renderer.test.tsx +++ b/src/renderer/src/features/global-editor/hooks/useEditorBeforeUnloadGuard.renderer.test.tsx @@ -36,6 +36,7 @@ it('keeps the real editor veto ahead of all application service disposal', async stopBuiltInMcp: supportStop, stopRemote: supportStop, stopLsp: supportStop, stopExternalControl: supportStop, disposeControl: supportStop, disposeWorkflowBridge: supportStop, disposeCaffeinate: supportStop, stopHeapWatchdog: supportStop, + stopDetachedTmuxSweep: supportStop, drainWorkspace: supportStop, drainDictationHistory: supportStop, flushGhosts: supportStop, flushRecordings: supportStop, flushDictationDebug: supportStop, flushPasteDebug: supportStop, stopPerformance: supportStop, stopExtensions: supportStop, diff --git a/src/renderer/src/lib/undoClose.ts b/src/renderer/src/lib/undoClose.ts index 2b052688b..b5efae26f 100644 --- a/src/renderer/src/lib/undoClose.ts +++ b/src/renderer/src/lib/undoClose.ts @@ -1,3 +1,5 @@ +import { UNDO_CLOSE_RETENTION_MS } from '@shared/undoRetention' + import type { SessionId, SessionMeta, @@ -68,7 +70,11 @@ import type { // background timer whose only job would be making command-palette // visibility slightly fresher. -export const UNDO_CLOSE_RETENTION_MS = 60 * 60 * 1000 // 1 hour +// The retention window is defined in shared/ and re-exported here: main derives +// its detached-tmux reap deadline from the same constant, because a closed +// terminal's shell is kept alive for exactly as long as an undo entry can still +// ask for it back. See src/shared/undoRetention.ts. +export { UNDO_CLOSE_RETENTION_MS } export const UNDO_CLOSE_MAX_ENTRIES = 10 // ---- Entry types ---- diff --git a/src/shared/undoRetention.ts b/src/shared/undoRetention.ts new file mode 100644 index 000000000..4f28ee6a9 --- /dev/null +++ b/src/shared/undoRetention.ts @@ -0,0 +1,22 @@ +/** + * How long a closed session stays restorable by Undo Close. + * + * WHY THIS LIVES IN `shared/` RATHER THAN BESIDE THE UNDO STACK: + * two processes have to agree on it, and they used to agree by accident. + * + * Closing a tmux-backed terminal detaches its attach-PTY but deliberately + * leaves the tmux session running, so that Undo Close can re-attach the SAME + * shell with its scrollback intact (`sessionManager.killOwnedInternal`, + * `undoClose.ts`'s `recoverTmuxName`). That is the only reason the shell is + * kept alive — once the entry that names it has expired off the renderer's + * stack, nothing can ever reach it again, and main is the only side that can + * reap it (`detachedSweep.ts`). + * + * So main's reap deadline is DERIVED from this window: reap earlier and Undo + * Close restores a pane whose shell is already dead; reap never and every + * closed terminal's shell survives until the next launch's startup reconcile, + * which on a machine that stays awake for days is a real pile of idle + * processes. Two copies of the number would drift, and the failure would be + * silent in both directions. + */ +export const UNDO_CLOSE_RETENTION_MS = 60 * 60 * 1000 // 1 hour From 9715a317e0f410069f7f002571c74c35762beed2 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 19 Sep 2026 23:24:19 -0700 Subject: [PATCH 2/2] fix(tmux): close the three kill races the review reproduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #1058, all P1 — this code kills user processes, and each of these was reproduced as a real kill of a terminal the user still had. 1. Close → undo → close between two sweeps kept the ORIGINAL deadline, so the shell died at minute 65 while its new undo entry stayed valid until minute 116; the undo then produced a fresh shell with none of the original processes or scrollback. The retention clock is now cleared the moment ownership becomes true, not the next time a sweep happens to observe it: SessionManager publishes `onTmuxAttached` on the same tick it registers a tmux-backed terminal entry. 2. Every authority was a snapshot taken before an await — `tmux list-sessions` spawns a process, the workspace read hits the disk — so an Undo Close landing in either gap was killed by evidence that was already false. Both authorities are now re-read immediately before the kill phase, and each name is re-checked against them (and against anything attached since the scan began) with no await between the check and the kill. 3. `stop()` only cleared the interval, so a run already awaiting tmux could still kill afterwards — during quit, when those shells are exactly what the next launch recovers terminals from. `stop()` now disarms the sweep itself, and the disposer moved into shutdown's FIRST wave: it is the one stage that is purely about NOT doing something, so it must not sit behind a stage that can stall. 4. The tests did not protect a persisted-only terminal: the recorded fixture's name was also handed in as a live name, so removing the workspace authority entirely still passed. Added the hibernated case — in the file, absent from main's registry — which is the shape the unified stage made common. Five new tests, each mutation-checked: dropping the persisted authority, making noteAttached not clear the timer, removing the pre-kill re-check, and making stop() interval-only each fail one. Co-Authored-By: Claude Opus 5 (1M context) --- src/main/applicationShutdown.ts | 8 +- src/main/index.ts | 6 + src/main/sessionManager.ts | 29 +++++ src/main/tmux/detachedSweep.test.ts | 164 ++++++++++++++++++++++++++++ src/main/tmux/detachedSweep.ts | 144 +++++++++++++++++++----- 5 files changed, 323 insertions(+), 28 deletions(-) diff --git a/src/main/applicationShutdown.ts b/src/main/applicationShutdown.ts index 012bb933d..29d96ac5c 100644 --- a/src/main/applicationShutdown.ts +++ b/src/main/applicationShutdown.ts @@ -109,6 +109,11 @@ export function installApplicationShutdown(options: { async function drain(): Promise { for (const [name, stage] of stages) if (stage.state === 'failed') stages.delete(name) const earlyStops = stopExecution() + // Disarmed FIRST, before anything that can stall: it is the one shutdown + // stage that is purely about NOT doing something. Leaving it armed behind a + // hung provider stop would let a five-minute tick fire during quit and kill + // the tmux sessions the next launch recovers terminals from (#1030 item 4). + const detachedTmuxSweepStop = run('detached-tmux-sweep', services.stopDetachedTmuxSweep) const dictationStop = run('dictation', services.stopDictation) // Startup checks the committed gate after asynchronous acquisition. Its // settlement closes the resource inventory; a missing SessionManager alone @@ -117,7 +122,7 @@ export function installApplicationShutdown(options: { // Extensions stop only after startup settles: startup creates the // runtime, and `run` keeps the first receipt, so an early stop against a // not-yet-published runtime would complete and let the real one escape. - await join([...earlyStops, ...stopExecution(), dictationStop, run('extensions', services.stopExtensions)]) + await join([...earlyStops, ...stopExecution(), detachedTmuxSweepStop, dictationStop, run('extensions', services.stopExtensions)]) await run('observations', services.flushObservations) await run('proxy-sweep', services.sweepOwnedProxies) @@ -135,7 +140,6 @@ export function installApplicationShutdown(options: { run('workflow-bridge', services.disposeWorkflowBridge), run('caffeinate', services.disposeCaffeinate), run('heap-watchdog', services.stopHeapWatchdog), - run('detached-tmux-sweep', services.stopDetachedTmuxSweep), ]) await join([ run('workspace', services.drainWorkspace), diff --git a/src/main/index.ts b/src/main/index.ts index 21bedb459..258c3b3af 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -981,6 +981,12 @@ async function startApp(): Promise { appRunJournal?.recordError('tmux.detached_sweep.error', error) }, }) + // Ownership published on the tick it becomes true. Every authority the + // sweep reads is a snapshot taken before an await; this is the one signal + // that cannot be stale, and it is what keeps an Undo Close restore from + // being killed by a scan that started before it. + const sweepAttachments = detachedTmuxSweep + manager.onTmuxAttached(name => sweepAttachments.noteAttached(name)) } // Project ownership lives in renderer state, while backend/transcript facts // live in SessionManager. Construct this bridge only after both the MCP host diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index fb4efe56a..7e3f745c4 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -475,6 +475,8 @@ export class SessionManager extends EventEmitter { timer?: ReturnType }>() private readonly lastActivityAt = new Map() + /** Subscribers to tmux ownership becoming true; see onTmuxAttached. */ + private readonly tmuxAttachListeners = new Set<(tmuxName: string) => void>() // Latest per-session UI-state snapshots, cached at the emit sites below. // WHY: consumers that attach mid-flight (the remote mobile companion's // SessionFeedSource/RemoteServer — enabled long after sessions started — @@ -3182,6 +3184,11 @@ export class SessionManager extends EventEmitter { }) this.sessions.set(sessionId, terminalEntry) + // Publish ownership at the moment it becomes true, on the same tick. The + // detached sweep uses this to disqualify a name from an in-flight scan's + // kills, and to reset a retention clock the next sweep would otherwise + // still be counting from a PREVIOUS close (#1030 item 4 review). + this.noteTmuxAttached(tmuxSessionName) this.rememberSessionId(sessionId) this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) try { @@ -4925,6 +4932,28 @@ export class SessionManager extends EventEmitter { return this.terminalForeground.snapshot() } + /** + * Called synchronously whenever a tmux session is bound to a live terminal + * entry. The detached sweep subscribes; see its `noteAttached`. + * + * WHY a listener rather than the sweep polling `getLiveTmuxNames()`: every + * answer the sweep computes is a snapshot taken before an await, and an Undo + * Close restore can land inside one. This is the one signal that cannot be + * stale, because it fires on the same tick as the registration. + */ + onTmuxAttached(listener: (tmuxName: string) => void): () => void { + this.tmuxAttachListeners.add(listener) + return () => { this.tmuxAttachListeners.delete(listener) } + } + + private noteTmuxAttached(tmuxName: string | null): void { + if (!tmuxName) return + for (const listener of this.tmuxAttachListeners) { + // A listener must never be able to fail a terminal spawn. + try { listener(tmuxName) } catch { /* diagnostics only */ } + } + } + /** * Every tmux session name main currently owns a live terminal for. * diff --git a/src/main/tmux/detachedSweep.test.ts b/src/main/tmux/detachedSweep.test.ts index 742a0de5c..869a6ecd1 100644 --- a/src/main/tmux/detachedSweep.test.ts +++ b/src/main/tmux/detachedSweep.test.ts @@ -120,6 +120,170 @@ describe('detached tmux sweep', () => { expect((await sweep.run()).reaped).toEqual(['agentcode-undone']) }) + it('protects a persisted-only terminal, and reaps an overdue one that is in neither authority', async () => { + // The hibernated case, which is the one the unified stage made common: the + // terminal exists in the workspace file and has NO live registry entry. + // Without the persisted authority this shell would be killed; the earlier + // tests could not see that, because they also handed the recorded name in + // as a live name (review finding 4). + const persisted = recordedTerminalNames() + const registry = registryWith(...persisted, 'agentcode-really-gone') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], // nothing hibernated is live in main + now: () => clock, + }) + + await sweep.run() + clock += REAP_AFTER_MS + 1 + expect((await sweep.run()).reaped).toEqual(['agentcode-really-gone']) + for (const name of persisted) expect(registry.killSession).not.toHaveBeenCalledWith(name) + }) + + it('an undo between sweeps restarts the window even though no sweep saw it', async () => { + // Close at t0, undo at t0+55min, close again a minute later, all between + // two ticks. With the clock reset only by an observing sweep, the ORIGINAL + // deadline stood and the shell died while its new undo entry was still + // valid — the undo then produced a fresh shell with none of the original + // processes or scrollback. + const registry = registryWith('agentcode-reopened') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => clock, + }) + + await sweep.run() // clock starts + clock += REAP_AFTER_MS - 60_000 + sweep.noteAttached('agentcode-reopened') // ⌘⇧T re-attached the same session + clock += 60_000 // closed again, one minute later + clock += 1_000 + + // Past the original deadline, nowhere near a new one. + expect((await sweep.run()).reaped).toEqual([]) + clock += REAP_AFTER_MS - 1_000 + expect((await sweep.run()).reaped).toEqual([]) + clock += 2_000 + expect((await sweep.run()).reaped).toEqual(['agentcode-reopened']) + }) + + it('does not kill a terminal restored while the scan was awaiting tmux', async () => { + // Every authority the sweep reads is a snapshot taken before an await, and + // `tmux list-sessions` spawns a process. A restore landing in that gap used + // to be killed by evidence that was already false. + let release!: () => void + const listing = new Promise(resolve => { release = resolve }) + const registry = { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => { + await listing + return [{ name: 'agentcode-raced', createdAt: 1 }] + }), + killSession: vi.fn(async (_name: string) => {}), + } + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => clock, + }) + + registry.listManagedSessions = vi.fn(async () => [{ name: 'agentcode-raced', createdAt: 1 }]) + await sweep.run() // clock starts + clock += REAP_AFTER_MS + 1 + + registry.listManagedSessions = vi.fn(async () => { await listing; return [{ name: 'agentcode-raced', createdAt: 1 }] }) + const inFlight = sweep.run() + sweep.noteAttached('agentcode-raced') // the user pressed ⌘⇧T + release() + + expect((await inFlight).reaped).toEqual([]) + expect(registry.killSession).not.toHaveBeenCalled() + }) + + it('does not kill a terminal restored after the scan already judged it overdue', async () => { + // The narrower half of the same race: the restore lands AFTER the + // classification loop has put the name on the kill list, while the sweep is + // re-reading its evidence. Only the check taken immediately before the kill + // — with no await after it — can catch this one. + let release!: () => void + const confirmRead = new Promise(resolve => { release = resolve }) + let announceConfirm!: () => void + const reachedConfirm = new Promise(resolve => { announceConfirm = resolve }) + let reads = 0 + const registry = registryWith('agentcode-late-undo') + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => { + reads += 1 + // Read 1 is the first sweep. Read 2 is the second sweep's + // classification; read 3 is its PRE-KILL confirmation, i.e. the sweep + // has already decided this name is overdue. Hold that one open. + if (reads === 3) { + announceConfirm() + await confirmRead + } + return RECORDED + }, + liveTmuxNames: () => [], + now: () => clock, + }) + + await sweep.run() // read 1: clock starts (nothing due, so no confirm read) + clock += REAP_AFTER_MS + 1 + + const inFlight = sweep.run() + // Only once the confirm read is in flight is the name genuinely on the + // kill list — that is the state this test is about. + await reachedConfirm + expect(reads).toBe(3) + sweep.noteAttached('agentcode-late-undo') + release() + + expect((await inFlight).reaped).toEqual([]) + expect(registry.killSession).not.toHaveBeenCalled() + }) + + it('issues no kill after stop(), even from a run already in flight', async () => { + // Quit. These shells are exactly what the next launch recovers terminals + // from, so a tick that fires during shutdown must not reach killSession. + let release!: () => void + const listing = new Promise(resolve => { release = resolve }) + const registry = { + isAvailable: () => true, + listManagedSessions: vi.fn(async () => [{ name: 'agentcode-quitting', createdAt: 1 }]), + killSession: vi.fn(async (_name: string) => {}), + } + let clock = 1_000_000 + const sweep = new DetachedTerminalSweep({ + registry, + readWorkspace: async () => RECORDED, + liveTmuxNames: () => [], + now: () => clock, + }) + + await sweep.run() + clock += REAP_AFTER_MS + 1 + + registry.listManagedSessions = vi.fn(async () => { await listing; return [{ name: 'agentcode-quitting', createdAt: 1 }] }) + const inFlight = sweep.run() + sweep.stop() + release() + expect((await inFlight).reaped).toEqual([]) + expect(registry.killSession).not.toHaveBeenCalled() + + // And it stays disarmed. + registry.listManagedSessions = vi.fn(async () => [{ name: 'agentcode-quitting', createdAt: 1 }]) + expect((await sweep.run()).withheld).toBe('stopped') + expect(registry.killSession).not.toHaveBeenCalled() + }) + it('withholds cleanup when the inventory is not complete', async () => { // A partially-decoded file cannot prove orphanhood (#898). The discarded // region is exactly where a terminal's only reference may have been. diff --git a/src/main/tmux/detachedSweep.ts b/src/main/tmux/detachedSweep.ts index cee73e0f6..02486a41a 100644 --- a/src/main/tmux/detachedSweep.ts +++ b/src/main/tmux/detachedSweep.ts @@ -63,7 +63,7 @@ export type DetachedSweepReport = { /** Alive, unreferenced, still inside the retention window. */ pending: string[] /** Why cleanup was withheld, when it was. Absent on a normal run. */ - withheld?: 'tmux-unavailable' | 'inventory-incomplete' + withheld?: 'tmux-unavailable' | 'inventory-incomplete' | 'stopped' } /** @@ -92,6 +92,29 @@ export class DetachedTerminalSweep { /** name → when this sweep FIRST saw it alive with no reference anywhere. */ private firstUnreferencedAt = new Map() + /** + * Names main bound to a live terminal at ANY point since the current run + * began reading its evidence. + * + * WHY a second structure instead of trusting the authorities (review + * finding 2): every authority this reads is a snapshot taken before an + * `await`. `listManagedSessions()` spawns a tmux process, the workspace read + * hits the disk, and an Undo Close restore can land in either gap — after + * which the snapshot says "nobody owns this" about a terminal the user is + * looking at. `noteAttached` is called SYNCHRONOUSLY by SessionManager the + * instant a tmux session is bound to a registry entry, so a name that + * appears here after a scan started is disqualified from that scan's kills + * with no race left: the check and the kill have no await between them. + */ + private attachedDuringScan = new Set() + + /** + * Set by `stop()`. A cleared interval does not stop a run already awaiting + * tmux (review finding 3), and quit is exactly when a kill must not happen: + * those shells are what the NEXT launch recovers terminals from. + */ + private stopped = false + private readonly now: () => number private readonly retentionMs: number @@ -100,40 +123,74 @@ export class DetachedTerminalSweep { this.retentionMs = options.retentionMs ?? UNDO_CLOSE_RETENTION_MS + DETACHED_REAP_GRACE_MS } - async run(): Promise { - const { registry, readWorkspace, liveTmuxNames } = this.options - if (!registry.isAvailable()) { - // Not evidence about any session: forget nothing, kill nothing. - return { reaped: [], pending: [], withheld: 'tmux-unavailable' } - } + /** + * Main bound `tmuxName` to a live terminal. + * + * Clearing the timer here rather than at the next sweep is what makes + * close → undo → close survive (review finding 1): with the timer only ever + * reset by an observing sweep, an undo at minute 55 and a second close at + * minute 56 left the ORIGINAL deadline standing, so the shell died at minute + * 65 while its new undo entry stayed valid until minute 116 — and the undo + * silently produced a fresh shell with none of the original processes or + * scrollback. + */ + noteAttached(tmuxName: string): void { + this.firstUnreferencedAt.delete(tmuxName) + this.attachedDuringScan.add(tmuxName) + } + /** Permanently disarm. Any run already in flight stops before its next kill. */ + stop(): void { + this.stopped = true + } + + /** + * Both authorities as one set, or null when the workspace file cannot be + * vouched for. Re-read immediately before the kill phase, because + * everything computed from the first read happened across awaits. + */ + private async readReferences(): Promise | null> { let text: string try { - text = await readWorkspace() + text = await this.options.readWorkspace() } catch { - return { reaped: [], pending: [], withheld: 'inventory-incomplete' } + return null } const inventory = terminalInventoryFromWorkspaceText(text) - if (inventory.kind !== 'complete') { - // Same rule as startup: an inventory we cannot vouch for authorizes - // nothing. Deliberately NOT clearing the timers — the next readable - // sweep should not have to start the clock over because one autosave - // was caught mid-write. - return { reaped: [], pending: [], withheld: 'inventory-incomplete' } + // Same rule as startup: an inventory we cannot vouch for authorizes + // nothing (#898). Deliberately NOT clearing the timers — the next + // readable sweep should not have to start the clock over because one + // autosave was caught mid-write. + if (inventory.kind !== 'complete') return null + const referenced = new Set(inventory.references.map(reference => reference.tmuxName)) + for (const name of this.options.liveTmuxNames()) referenced.add(name) + return referenced + } + + async run(): Promise { + const { registry } = this.options + if (this.stopped) return { reaped: [], pending: [], withheld: 'stopped' } + // The window opens here: anything main attaches from now until this run's + // last kill is off limits, however stale the snapshots below turn out. + this.attachedDuringScan.clear() + + if (!registry.isAvailable()) { + // Not evidence about any session: forget nothing, kill nothing. + return { reaped: [], pending: [], withheld: 'tmux-unavailable' } } - const referenced = new Set(inventory.references.map(reference => reference.tmuxName)) - for (const name of liveTmuxNames()) referenced.add(name) + const referenced = await this.readReferences() + if (!referenced) return { reaped: [], pending: [], withheld: 'inventory-incomplete' } const alive = await registry.listManagedSessions() const aliveNames = new Set(alive.map(session => session.name)) const now = this.now() - const reaped: string[] = [] const pending: string[] = [] + const due: string[] = [] for (const name of aliveNames) { if (referenced.has(name)) { - // Back in use — an Undo Close restore, or autosave catching up with a + // In use — an Undo Close restore, or autosave catching up with a // terminal that was live all along. Its clock restarts if it is ever // unreferenced again. this.firstUnreferencedAt.delete(name) @@ -149,11 +206,7 @@ export class DetachedTerminalSweep { pending.push(name) continue } - // kill first, forget second: a kill that throws keeps its timer, so the - // next sweep retries instead of restarting the whole window. - await registry.killSession(name) - this.firstUnreferencedAt.delete(name) - reaped.push(name) + due.push(name) } // A name that is no longer alive was killed by something else (a user @@ -163,6 +216,34 @@ export class DetachedTerminalSweep { if (!aliveNames.has(name)) this.firstUnreferencedAt.delete(name) } + if (due.length === 0) return { reaped: [], pending } + + // A kill is imminent, so pay for fresh evidence. `referenced` above was + // read before the tmux listing, which spawns a process; on a loaded + // machine that gap is long enough for a user to press ⌘⇧T. + const confirmed = await this.readReferences() + if (!confirmed) return { reaped: [], pending: [...pending, ...due], withheld: 'inventory-incomplete' } + + const reaped: string[] = [] + for (const name of due) { + // Re-checked per name: each kill awaits, and the next name's evidence + // must be as fresh as the first one's was. + if (this.stopped) { + pending.push(name) + continue + } + if (confirmed.has(name) || this.attachedDuringScan.has(name)) { + this.firstUnreferencedAt.delete(name) + pending.push(name) + continue + } + // kill first, forget second: a kill that throws keeps its timer, so the + // next sweep retries instead of restarting the whole window. + await registry.killSession(name) + this.firstUnreferencedAt.delete(name) + reaped.push(name) + } + return { reaped, pending } } } @@ -174,7 +255,12 @@ export class DetachedTerminalSweep { */ export const DETACHED_SWEEP_INTERVAL_MS = 5 * 60 * 1000 -export type DetachedSweepSchedule = { stop: () => void } +export type DetachedSweepSchedule = { + /** Disarm permanently, including any run already in flight. */ + stop: () => void + /** Main bound this tmux name to a live terminal; see DetachedTerminalSweep.noteAttached. */ + noteAttached: (tmuxName: string) => void +} /** * Run the sweep on a timer until stopped. @@ -211,5 +297,11 @@ export function startDetachedTerminalSweep( }, options.intervalMs ?? DETACHED_SWEEP_INTERVAL_MS) // Reaping idle shells is never a reason to hold the process open. timer.unref?.() - return { stop: () => clearInterval(timer) } + return { + // `sweep.stop()` is the half that matters: clearing the interval leaves a + // run already awaiting tmux free to kill afterwards, and quit is precisely + // when it must not (review finding 3). + stop: () => { sweep.stop(); clearInterval(timer) }, + noteAttached: name => { sweep.noteAttached(name) }, + } }