fix(client): restore terminal on SIGHUP and SIGTERM - #2041
Conversation
The client only installed a Ctrl+C handler and depended on ctrlc without its "termination" feature, so it caught SIGINT alone. On SIGHUP or SIGTERM the process died without unwinding, so neither TerminalGuard::Drop nor the panic hook ran and clear_host_mouse_reporting() never fired. Mouse tracking stayed enabled on the host terminal, leaking SGR mouse reports as garbage on every pointer move. Enable ctrlc's "termination" feature so the existing handler also catches SIGTERM and SIGHUP. It sets should_quit, the client loop exits, and TerminalGuard::Drop restores the terminal as it does for Ctrl+C. refs herdrdev#1713
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesSignal handling and terminal restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Signal as Termination signal
participant Client as Thin client
participant Guard as TerminalGuard
participant PTY as Client PTY
Signal->>Client: Trigger quit path
Client->>Guard: Drop terminal guard
Guard->>PTY: Emit mouse-reporting teardown
PTY-->>Client: Captured teardown markers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
coderabbit-review
greptile-review
Greptile SummaryEnables graceful client exit on SIGHUP/SIGTERM so TerminalGuard restores the terminal and clears mouse reporting.
Confidence Score: 5/5This PR appears safe to merge; no blocking failures remain from prior or current review findings. No blocking failure remains. The change wires termination signals into the existing should_quit path so TerminalGuard can restore the terminal, and tests cover the SIGHUP and server-EOF restore cases.
|
| Filename | Overview |
|---|---|
| Cargo.toml | Enables ctrlc termination feature so SIGTERM/SIGHUP share the existing quit handler. |
| src/client/mod.rs | Installs the expanded signal handler and warns on install failure without changing SIGINT behavior. |
| tests/client_mode.rs | Adds PTY-based regression coverage for mouse teardown on server EOF and client SIGHUP. |
Sequence Diagram
sequenceDiagram
participant Term as Terminal emulator
participant Client as herdr client
participant Guard as TerminalGuard
Term->>Client: SIGHUP / SIGTERM
Client->>Client: ctrlc handler sets should_quit
Client->>Client: client loop exits
Client->>Guard: drop(TerminalGuard)
Guard->>Term: clear_host_mouse_reporting + restore
Reviews (9): Last reviewed commit: "Merge branch 'master' into fix/client-re..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/mod.rs (1)
1228-1233: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate
ctrlc::set_handlerfailures. Ignoring thisResultleaves SIGTERM/SIGHUP handling disabled if registration fails, so the quit path may never run whileTerminalGuardis active. Return the error instead of discarding it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 272d15de-ba72-4589-bb66-c39330c8d9e7
📒 Files selected for processing (2)
Cargo.tomlsrc/client/mod.rs
Surface a failed ctrlc::set_handler instead of discarding the Result. On failure the terminal restore falls back to TerminalGuard::Drop and the panic hook, and the warning makes the degraded state visible rather than silent. refs herdrdev#1713
|
Good catch, fixed in 66772d1. I log the failure with |
|
thanks for the pr. i think this addresses a different path: when ssh dies, the local thin client gets eof through the bridge socket and exits normally; it does not receive sigterm or sighup. your reproduction signals the client directly, so it does not reproduce an i also couldn’t reproduce an sgr leak by killing the ssh bridge. do you have a reproduction where the bridge dies but the thin client fails to restore the terminal? |
|
Hey, i think we were just talking past each other, youre right. went and wrote two integration tests in the ssh-death path never hits a signal. where it does apply is a direct SIGHUP/SIGTERM to the client, like when a terminal emulator sighups its foreground child as the window closes. that test fails on master with zero teardown bytes and passes once one other thing while i was in there. i think the #1713 leak might be ordering, not signals. in |
kangal-bot
added
the
ai-review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/client_mode.rs (1)
711-749: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared restore-test body.
client_restores_terminal_on_server_eofandclient_restores_terminal_on_sighupduplicate the full setup/watermark/drain/assert/cleanup sequence, differing only in the trigger action. Extracting a shared helper parameterized by the trigger would remove ~35 lines of duplication and keep the two tests focused on the interesting behavior.♻️ Proposed refactor
+fn run_restore_test(trigger: impl FnOnce(&mut SpawnedHerdr, &mut SpawnedHerdr)) { + let _lock = test_lock(); + let base = unique_test_dir(); + let config_home = base.join("config"); + let runtime_dir = base.join("runtime"); + let api_socket = runtime_dir.join("herdr.sock"); + let client_socket = runtime_dir.join("herdr-client.sock"); + + let (mut spawned_server, mut thin_client, pty_output) = + attach_thin_client(&config_home, &runtime_dir, &api_socket, &client_socket); + + let since = output_len(&pty_output); + trigger(&mut spawned_server, &mut thin_client); + + let output = drain_until_client_exits(&mut thin_client, &pty_output, since); + assert!( + output_has_mouse_teardown(&output), + "client must emit mouse teardown after trigger; output after trigger: {output:?}" + ); + + drop(spawned_server); + cleanup_spawned_herdr(thin_client, base); +} + #[test] fn client_restores_terminal_on_server_eof() { - let _lock = test_lock(); - let base = unique_test_dir(); - ... - drop(spawned_server); - cleanup_spawned_herdr(thin_client, base); + run_restore_test(|server, _client| { + if let Some(pid) = server.child.process_id() { + unsafe { libc::kill(pid as libc::pid_t, libc::SIGKILL); } + } + server.close_master(); + }); } #[test] fn client_restores_terminal_on_sighup() { - let _lock = test_lock(); - ... - drop(spawned_server); - cleanup_spawned_herdr(thin_client, base); + run_restore_test(|_server, client| { + let pid = client.child.process_id().expect("thin client pid") as libc::pid_t; + unsafe { libc::kill(pid, libc::SIGHUP); } + }); }Also applies to: 751-787
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c925a8ce-9b12-4d90-aeab-006ddb38c3e6
📒 Files selected for processing (1)
tests/client_mode.rs
…hup-v2' into fix/client-restore-terminal-on-sighup-v2
|
done in be1d2ce - shared setup pulled into |
|
ty! |
4 participants
What
The client only enabled
ctrlcwithout itsterminationfeature, so it caught SIGINT alone. On a direct SIGHUP or SIGTERM the process died without unwinding, so neitherTerminalGuard::Dropnor the panic hook ran andclear_host_mouse_reporting()never fired. Mouse tracking stayed on and leaked SGR reports as garbage on every pointer move.This is a direct-signal path, e.g. a terminal emulator SIGHUPing its foreground child on window close. It is not the
herdr --remotessh-death path. Under--remotethe localherdr clientis a subprocess talking to the bridge socket; when ssh dies the client sees EOF on the socket, the loop returns an error, anddrop(terminal_guard)restores the terminal. No signal is delivered, so that path was already clean.Fix
Enable
ctrlc'sterminationfeature so the existing handler also catches SIGTERM and SIGHUP. It setsshould_quit, the loop exits, andTerminalGuard::Droprestores the terminal the same way it does for Ctrl+C. Handler install failure is logged withwarn!rather than swallowed. No behaviour change on SIGINT.Tests
tests/client_mode.rsdrives a client under a pty and checks for the mouse teardown bytes emitted after the trigger:client_restores_terminal_on_server_eof(kill server → EOF)client_restores_terminal_on_sighup(SIGHUP to client)The EOF test passing on master confirms the
--remote/ssh path already restores. The SIGHUP test is the regression this PR fixes.Note
The #1713 SGR leak looks like an ordering issue rather than signals:
restore_terminal_stateclears the mouse modes beforeratatui::restore()leaves the alt screen, and terminals like Ghostty restore saved private-mode state onLeaveAlternateScreen. Not addressed here.refs #1713