Repository navigation
fix(tmux): reap a closed terminal's shell once its undo window has passed - #1058
Merged
Merged
Conversation
…ssed #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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1030 item 4.
The leak
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 — losing the scrollback is the bug that shape exists to prevent).
Nothing killed it afterwards.
TmuxRegistry.killSessionhad exactly two callers: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 this accumulates. The unified stage made it more visible rather than causing it: boot now hibernates every lane but the focused one, so main does not even observe most of those closes as session stops.
The fix
DetachedTerminalSweep(src/main/tmux/detachedSweep.ts) runs every five minutes and reaps a managed tmux session that:terminalInventoryFromWorkspaceText, extracted so there is only ever one decoder — two was the shape that produced bug(terminals): read v2 workspace references before tmux reconciliation #898); andSessionManager.getLiveTmuxNames), which covers a terminal autosave has not flushed yet; andUNDO_CLOSE_RETENTION_MS + DETACHED_REAP_GRACE_MS.The retention window moves to
src/shared/undoRetention.ts, because two processes have to agree on it and previously agreed by accident: 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: only a COMPLETE inventory proves orphanhood, so an unreadable or partially-decoded workspace file withholds every kill — without resetting the timers, so one autosave caught mid-write does not grant everything another full hour.
Why not tell main on close
The obvious alternative is a renderer message on close, cancelled on undo. It cannot cover a hibernated terminal: the pane has no live session either side of the close, so there is nothing to send. And a reaping decision behind a message a crashed or reloaded renderer never sends is not a decision.
Why the clock starts at first sighting
The sweep holds no close events, so it knows when it first observed a name unreferenced, not when it became so. With a five-minute interval the error is bounded by one interval and always in the safe direction. An Undo Close restore puts the name back in both authorities, which clears the timer.
Tests
src/main/tmux/detachedSweep.test.tsdrives the sweep against the real recordedworkspace.json(testing/fixtures/workspace-v2/2026-09-19-live-workspace.sanitized.json, which carries one real terminal reference). Only the tmux process boundary is faked — same rule astmuxRecovery.test.ts.Four invariants pinned by mutation (each mutation was applied and the run recorded):
Plus schedule tests: no immediate first run (startup reconciliation just did that pass), the in-flight flag collapsing ticks behind a hung
tmux list-sessions, a throwing sweep reported and the timer continuing, andstop()ending it.npx tsc -bclean;src/main/tmux/,applicationShutdown,src/renderer/src/lib/andsrc/renderer/src/workspace/hook/actions/all green (80 + 139 tests).🤖 Generated with Claude Code