Repository navigation
fix(minimald): unwind the terminal when the session process exits (#1210) - #1261
Conversation
`TeardownDueToProcessExit` was the one teardown that carried no unwind codes: its three siblings (supercede, shutdown, detach) each write them before anything else, but a session process that died — SIGKILL included — left the binding to render the exit notices and the shell-exit prompt into whatever input modes the dead app had set, and the user's own shell inherited them afterwards. A full-screen app killed mid-run therefore left mouse reporting on and every mouse move printed escapes into the shell (#1210). The variant now carries the codes, computed at the single send site where the host's parser is in scope, and the mainloop writes them before the notices and before the prompt. Belt-and-braces on the client: `min` arms an RAII `TerminalUnwind` around an interactive attach so the tty is restored on every exit path, however the attach ended. That means waiting on ssh rather than `exec()`ing it — no Drop impl survives `exec()` — with ssh's status propagated as the client's exit code so the observable behaviour is unchanged. It covers the cases the daemon cannot: a daemon predating this fix, and a transport that drops without a teardown at all. Refs #1210
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe change adds shared terminal reset sequences, client-side RAII cleanup, SSH exit-status propagation, and daemon-side teardown codes. The daemon sends state-specific reset bytes before exit notices. Tests cover reset composition, status handling, propagation, and output ordering. ChangesTerminal teardown
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change resets terminal state when a session process exits and adds a fallba 8000 ck client reset; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| /// "only if currently set" narrowing — a client has no screen model to diff | ||
| /// against, and every sequence here is a no-op against a terminal that was | ||
| /// never in that mode. | ||
| const TERMINAL_UNWIND: &[u8] = concat!( |
There was a problem hiding this comment.
All of this already exists in the session host, grep for unwind codes
There was a problem hiding this comment.
Right that it shouldn't live in two places — single-sourced in sessions now (1d3bb332), both sides reference the same constants.
But they're not equivalent, so I'd keep the session-host version:
unwind_codes()is state-aware — diffs live vs clean viainput_mode_diff, emits?1049l/?25honly when actually set.- The client constant is blind; it has no screen model to diff against.
- Deleting the daemon side would make the common path stop narrowing. Strictly worse.
Your comment did surface a real bug in mine, though: ?1049l isn't the no-op I claimed. On xterm-family, rmcup-from-the-normal-buffer does a DECRC-style cursor restore — so after a clean detach the client's extra write can jump the cursor and redraw the prompt over existing content. A fix about cosmetic damage causing cosmetic damage.
So the fallback now fires only when ssh dies abnormally (255 or a signal):
- Clean teardown → daemon spoke, client stays quiet, no duplicate write.
- Broken transport → daemon never spoke, blind write earns its place.
Cross-PR note: I'd justified the fallback partly with "a daemon predating #1210 sends none." #1260 gates cmd_attach, and session_via_ssh sits downstream — once it lands, a CLI can't attach across builds at all. That half is void; dropped-transport is the only surviving case. Comment updated.
|
Yeah if the plan is to replace the session host unwind codes we should move/delete that code in this PR also, duplicating it likely isn’t ideal |
…back The DEC private-mode sequences were spelled out twice — once in the daemon's screen-aware `unwind_codes`, once in the `min` client's blind fallback — so an edit to one could silently diverge from the other. Move the byte literals into `sessions::terminal` as named constants and have both sides reference them; the daemon keeps its narrowing (the `input_mode_diff`, the `alternate_screen()` and `hide_cursor()` guards) and its emitted bytes are unchanged. The client's fallback also stops firing on the common path. `\x1b[?1049l` is not inert against a terminal that never left the normal buffer: on xterm-family terminals rmcup still performs a DECRC-style cursor restore, so writing the blind set after a clean detach — where the daemon already sent the correct narrow sequence — could jump the cursor to a stale saved position and have the shell prompt redraw over existing content. The guard is now disarmed unless ssh's exit says the transport broke (255, or a death by signal); a normal exit, including the session process's own non-zero status, proves the daemon was alive and speaking. A session process that itself exits 255 is indistinguishable from an ssh transport failure and gets a redundant unwind — documented at the predicate. Also drops the stale second justification for the fallback: #1260 version-gates `cmd_attach`, so a CLI can no longer attach to a daemon predating #1210 at all. The surviving reason is a dropped transport. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the remaining half of #1210.
#1253 fixed the notification half — the user is now told when a session is
SIGKILLed — but it made the other half more visible: on teardown the terminal was
left in a broken state. Three of the four teardown arms wrote
self.unwind_codes();the
TeardownDueToProcessExitarm did not, so a session whose process simply exitedleft mouse reporting, the alt screen, and cursor visibility as the session had set them.
The ProcessExit arm now emits the same unwind sequence as its siblings, and the client
gained a blind unwind so the reset still happens against an old daemon or a dropped
transport.
Reviewer notes
\x1b[?1049lunconditionally, while the daemon'sunwind_codes()gates it behindif live.alternate_screen(). On xterm-familyterminals rmcup-from-the-normal-buffer does a DECRC-style cursor restore, so after a
clean detach the cursor can jump to a previously-saved position. Cosmetic — but it
is cosmetic damage introduced by a fix about cosmetic damage. The doc comment's claim
that every sequence "is a no-op against a terminal that was never in that mode" holds
for every code in the set except
?1049l; at minimum that claim wants qualifying.session_via_ssh's interactive arm changed fromexec()to fork+wait+exit, sominnow sits between the pty and ssh. A signalled ssh used to make
mindie by thatsignal; it now exits 128+n.
$?is identical, but a supervisor callingwaitpidseesWIFEXITEDwhere it sawWIFSIGNALED. Intentional and unit-tested.scripts/e2e-attach-pty.py's hang guard SIGKILLs the pty child by pid — nowmin,not ssh — so a wedged attach can orphan an ssh plus its
min proxygrandchild on therunner. Only reachable on the already-failing timeout path.
min dash'sattach_and_resumedoes not armTerminalUnwind, so the dash attachpath lacks the belt-and-braces this PR adds to
min session attach. Not a regression,but it is the other place a user attaches interactively.
notify_remote_teardowncomputesunwind_codes()unconditionally, allocating a freshparser even when
self.remoteisNone. Forced by the borrow checker; anif self.remote.is_some()pre-check would avoid it.🤖 Generated with Claude Code
Summary by CodeRabbit