From fe504703db73a01a93df85e425a0e6c3f3992467 Mon Sep 17 00:00:00 2001 From: David Danialy Date: Thu, 1 Oct 2026 10:12:33 -0700 Subject: [PATCH 1/2] fix(tui): exit when the terminal hangs up instead of spinning forever Closing a terminal window with `kit tui` running left the process alive, orphaned, and pinning a CPU core until killed by hand. On hangup the TUI tears down and `Events::drop` joins the input reader thread. That thread sits in crossterm's `event::poll`, whose read loop retries a dead terminal descriptor forever (it only breaks on WouldBlock), so the poll never returns, the stop flag is never rechecked, and the join never completes. Bound the wait: the reader signals its exit by dropping a channel sender, and teardown detaches a reader that has not exited within two seconds. A live reader still returns within one 25ms poll interval and is joined as before, so terminal ownership handoff to authentication is unchanged. The PTY lifecycle test gains a hangup case that closes the master and requires the child to exit. The master is now close-on-exec so the child no longer holds its own terminal open. Co-Authored-By: Claude Fable 5.1 --- src/tui/input.rs | 18 ++++++++++++++++++ src/tui/input_tests.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/tui/input.rs b/src/tui/input.rs index 5e01daa..ea0044e 100644 --- a/src/tui/input.rs +++ b/src/tui/input.rs @@ -9,6 +9,7 @@ use std::{ sync::{ Arc, atomic::{AtomicBool, Ordering}, + mpsc::{self as exit, RecvTimeoutError}, }, task::{Context, Poll}, thread::{self, JoinHandle}, @@ -19,10 +20,16 @@ use crossterm::event::{self, Event}; use futures_util::Stream; use tokio::sync::mpsc; +/// A stopped reader returns within one poll interval. On a terminal that hung +/// up, crossterm's poll retries the dead descriptor forever and never returns, +/// so teardown abandons the reader after this long instead of joining it. +const JOIN_TIMEOUT: Duration = Duration::from_secs(2); + pub(super) struct Events { receiver: mpsc::Receiver>, stopped: Arc, reader: Option>, + exited: exit::Receiver<()>, } impl Events { @@ -30,9 +37,12 @@ impl Events { let (sender, receiver) = mpsc::channel(256); let stopped = Arc::new(AtomicBool::new(false)); let reader_stopped = Arc::clone(&stopped); + let (exiting, exited) = exit::channel(); let reader = thread::Builder::new() .name("kit-terminal-input".into()) .spawn(move || { + // Dropped when this thread returns or unwinds; never sent on. + let _exiting = exiting; // No other EventStream or synchronous reader may coexist with this owner. // poll and read always execute on this same OS thread. while !reader_stopped.load(Ordering::Acquire) { @@ -59,6 +69,7 @@ impl Events { receiver, stopped, reader: Some(reader), + exited, }) } @@ -83,6 +94,13 @@ impl Drop for Events { self.receiver.close(); self.stopped.store(true, Ordering::Release); if let Some(reader) = self.reader.take() { + // A reader that outlives the timeout is stuck on a dead terminal: + // there is no input left to steal, and waiting would keep the + // process alive and spinning after its terminal closed. Detach it + // so the process can exit. + if self.exited.recv_timeout(JOIN_TIMEOUT) == Err(RecvTimeoutError::Timeout) { + return; + } // A worker panic is already reported by the panic hook and closes // the stream. Never turn cleanup (possibly unwinding) into a panic. let _ = reader.join(); diff --git a/src/tui/input_tests.rs b/src/tui/input_tests.rs index ea98b17..663c225 100644 --- a/src/tui/input_tests.rs +++ b/src/tui/input_tests.rs @@ -108,6 +108,26 @@ fn child_case(case: &str) { } drop(terminal); } + "hangup" => { + // The parent closes the PTY master, as a closed terminal window + // does. Take the hangup through the real stop boundary, then tear + // input down: it must return although the terminal is gone. + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + runtime.block_on(async { + let mut stop = crate::tui::Stop::new().unwrap(); + let (_terminal, _images) = enter().unwrap(); + let events = Events::new().unwrap(); + marker("HANGUP"); + assert!(stop.until(std::future::pending::<()>()).await.is_none()); + drop(events); + // Nothing is left to restore or report to; only the exit + // status still reaches the parent. + std::process::exit(0); + }); + } "cancel" => { // Poll through actual terminal setup and input acquisition, suspend // at an await, then cancel by dropping the owning future. @@ -211,6 +231,7 @@ fn terminal_input_lifecycle() { "unwind", "worker_unwind", "setup_failure", + "hangup", ] { run_pty(case); } @@ -231,6 +252,13 @@ fn run_pty(case: &str) { }, 0 ); + // The child must not inherit the master, or the parent closing it would + // never hang the terminal up. + // SAFETY: master_fd is the open descriptor openpty just returned. + assert_eq!( + unsafe { libc::fcntl(master_fd, libc::F_SETFD, libc::FD_CLOEXEC) }, + 0 + ); let (mut master, slave) = unsafe { (File::from_raw_fd(master_fd), File::from_raw_fd(slave_fd)) }; let mut command = Command::new(std::env::current_exe().unwrap()); @@ -313,6 +341,18 @@ fn run_pty(case: &str) { sent.push(name); } } + if case == "hangup" && output.contains("INPUT_TEST:HANGUP") { + drop(master); + let status = loop { + assert!(Instant::now() < deadline, "{case} never exited: {output:?}"); + if let Some(status) = child.0.try_wait().unwrap() { + break status; + } + std::thread::sleep(Duration::from_millis(5)); + }; + assert!(status.success(), "{case} failed ({status}): {output:?}"); + return; + } if let Some(status) = child.0.try_wait().unwrap() { assert!(status.success(), "{case} failed ({status}): {output:?}"); assert!(output.contains("INPUT_TEST:DONE"), "{case}: {output:?}"); From 197fb73ccf8c031876933116f94956c94b466dee Mon Sep 17 00:00:00 2001 From: David Danialy Date: Thu, 1 Oct 2026 10:26:42 -0700 Subject: [PATCH 2/2] fix(tui): restore the terminal without panicking after a hangup With the input reader no longer blocking teardown, a hangup now reaches `leave`. `ratatui::restore` reports a failed restore with `eprintln!`, which panics when stderr is the same dead terminal. The panic skipped the session close that follows teardown and exited with status 101. Report the failure with a write whose error is ignored instead. The PTY hangup case now runs `leave` as well, so it fails if teardown panics. Co-Authored-By: Claude Fable 5.1 --- src/tui/input.rs | 9 +++++---- src/tui/input_tests.rs | 10 ++++++---- src/tui/mod.rs | 13 ++++++++++--- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/tui/input.rs b/src/tui/input.rs index ea0044e..d11caa8 100644 --- a/src/tui/input.rs +++ b/src/tui/input.rs @@ -1,5 +1,6 @@ //! Single-owner terminal input. Create only after capability queries finish; //! drop (and join) before restoring the terminal or handing stdin to a child. +//! The join is bounded: a reader stuck on a hung-up terminal is detached. //! Image/keyboard capability queries may read synchronously during setup, before //! this owner exists. During its lifetime the UI only consumes the bounded queue: //! it never polls the OS input reader or acquires crossterm's input lock. @@ -94,10 +95,10 @@ impl Drop for Events { self.receiver.close(); self.stopped.store(true, Ordering::Release); if let Some(reader) = self.reader.take() { - // A reader that outlives the timeout is stuck on a dead terminal: - // there is no input left to steal, and waiting would keep the - // process alive and spinning after its terminal closed. Detach it - // so the process can exit. + // A reader that outlives the timeout is stuck inside crossterm, + // in practice on a dead terminal with no input left to steal. + // Waiting would keep the process alive and spinning after its + // terminal closed. Detach it so the process can exit. if self.exited.recv_timeout(JOIN_TIMEOUT) == Err(RecvTimeoutError::Timeout) { return; } diff --git a/src/tui/input_tests.rs b/src/tui/input_tests.rs index 663c225..b550e40 100644 --- a/src/tui/input_tests.rs +++ b/src/tui/input_tests.rs @@ -111,20 +111,22 @@ fn child_case(case: &str) { "hangup" => { // The parent closes the PTY master, as a closed terminal window // does. Take the hangup through the real stop boundary, then tear - // input down: it must return although the terminal is gone. + // down: both steps must return although the terminal is gone. let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); runtime.block_on(async { let mut stop = crate::tui::Stop::new().unwrap(); - let (_terminal, _images) = enter().unwrap(); + let (mut terminal, _images) = enter().unwrap(); let events = Events::new().unwrap(); marker("HANGUP"); assert!(stop.until(std::future::pending::<()>()).await.is_none()); drop(events); - // Nothing is left to restore or report to; only the exit - // status still reaches the parent. + // Restoring a dead terminal fails; it must not panic either. + leave(&mut terminal); + // Nothing is left to report to; only the exit status still + // reaches the parent. std::process::exit(0); }); } diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 93ef110..07fbde6 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -3804,7 +3804,7 @@ fn draw_frame( } /// Owns terminal modes across fallible setup, dropped futures, and unwind. -/// Declare input after this guard so its reader joins before mode restoration. +/// Declare input after this guard so its reader stops before mode restoration. struct TerminalSession { terminal: DefaultTerminal, active: bool, @@ -3832,7 +3832,7 @@ impl Drop for TerminalSession { } fn enter() -> std::io::Result<(TerminalSession, image::ImageRuntime)> { - // Unwinding restores through TerminalSession, after Events has joined. + // Unwinding restores through TerminalSession, after Events has dropped. // A process-global unwind hook would restore too early, including for an // unrelated worker panic whose terminal owner remains alive. #[cfg(panic = "abort")] @@ -4036,7 +4036,14 @@ fn leave(terminal: &mut TerminalSession) { terminal.active = false; restore_modes(); let _ = terminal.show_cursor(); - ratatui::restore(); + if let Err(error) = ratatui::try_restore() { + // After a hangup stderr is the same dead terminal: `eprintln!` would + // panic there and skip the session close that follows teardown. + let _ = std::io::Write::write_all( + &mut std::io::stderr(), + format!("Failed to restore terminal: {error}\n").as_bytes(), + ); + } TERMINAL_ACTIVE.store(false, Ordering::Relaxed); }