diff --git a/CHANGELOG.md b/CHANGELOG.md index 4abdeff..9876b45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added - activity indicator: tabs with output while not focused now show the activity dot in the tab bar; the marker clears as soon as the tab is focused +### Changed +- log the cause when the PTY reader thread stops (EOF, I/O error, or closed output channel) under `RUST_LOG=debug`, instead of silently collapsing EOF and errors into a bare break + ### Fixed - scroll the tab bar to keep the active tab visible when many tabs are open; previously tabs overflowed the window width and the header rendered garbled - avoid a ghost pane and a crash when a PTY spawn fails: a failed split/tab/restore no longer wires the layout or focus to a non-existent pane, and closing a pane in that state no longer panics diff --git a/src/pty/session.rs b/src/pty/session.rs index 3dade98..bb0e484 100644 --- a/src/pty/session.rs +++ b/src/pty/session.rs @@ -10,6 +10,50 @@ pub struct PtySession { child_pid: Option, } +/// Classification of a single `Read::read` result from the PTY master. +/// +/// The reader loop must distinguish a clean EOF (the shell exited normally and +/// closed its end of the PTY) from a genuine I/O error (e.g. EIO when the shell +/// crashed, EBADF on a closed fd). Collapsing both into a bare `break` discards +/// the cause and makes disconnections invisible under `RUST_LOG`. +#[derive(Debug)] +enum ReadOutcome { + /// `read` returned 0 bytes — the shell closed the PTY (normal exit). + Eof, + /// `read` returned an I/O error — abnormal termination. + Error(std::io::Error), + /// `read` returned `n > 0` bytes to forward to the parser. + Data(usize), +} + +/// Maps a raw `read` result to a [`ReadOutcome`]. Pure and side-effect free so +/// the EOF-vs-error distinction can be unit-tested without a live PTY. +fn classify_read(r: std::io::Result) -> ReadOutcome { + match r { + Ok(0) => ReadOutcome::Eof, + Ok(n) => ReadOutcome::Data(n), + Err(e) => ReadOutcome::Error(e), + } +} + +/// Body of the PTY reader thread: forward bytes to the parser until EOF, an I/O +/// error, or a closed output channel — logging the cause of every termination. +fn pty_reader_loop( + mut reader: Box, + output_tx: Sender>, + wakeup: Box, +) { + let mut buf = [0u8; 4096]; + loop { + match classify_read(reader.read(&mut buf)) { + ReadOutcome::Eof => return log::debug!("PTY EOF (shell exited)"), + ReadOutcome::Error(e) => return log::debug!("PTY read failed: {e}"), + ReadOutcome::Data(n) if output_tx.send(buf[..n].to_vec()).is_ok() => wakeup(), + ReadOutcome::Data(_) => return log::debug!("PTY output channel closed"), + } + } +} + impl PtySession { #[allow(dead_code)] pub fn spawn(cols: u16, rows: u16, output_tx: Sender>) -> anyhow::Result { @@ -70,21 +114,8 @@ impl PtySession { let _ = child.wait(); }); - let mut reader = master.try_clone_reader()?; - thread::spawn(move || { - let mut buf = [0u8; 4096]; - loop { - match reader.read(&mut buf) { - Ok(0) | Err(_) => break, - Ok(n) => { - if output_tx.send(buf[..n].to_vec()).is_err() { - break; - } - wakeup(); - } - } - } - }); + let reader = master.try_clone_reader()?; + thread::spawn(move || pty_reader_loop(reader, output_tx, wakeup)); let writer = master.take_writer()?; diff --git a/src/pty/session_test.rs b/src/pty/session_test.rs index 7dfdf1a..ed15f68 100644 --- a/src/pty/session_test.rs +++ b/src/pty/session_test.rs @@ -1,7 +1,34 @@ use crossbeam_channel::unbounded; use std::time::{Duration, Instant}; -use super::PtySession; +use super::{PtySession, ReadOutcome, classify_read}; + +/// Regression: the reader loop must distinguish a clean EOF (`Ok(0)`) from a +/// genuine I/O error (`Err(_)`). The previous `Ok(0) | Err(_) => break` arm +/// collapsed both, so the disconnection cause was lost. `classify_read` keeps +/// them as separate variants. +#[test] +fn classify_read_distinguishes_eof_error_and_data() { + assert!( + matches!(classify_read(Ok(0)), ReadOutcome::Eof), + "zero-length read must be EOF, not an error" + ); + assert!( + matches!(classify_read(Ok(5)), ReadOutcome::Data(5)), + "non-zero read must carry the byte count" + ); + let err = std::io::Error::from_raw_os_error(libc_eio()); + match classify_read(Err(err)) { + ReadOutcome::Error(_) => {} + other => panic!("I/O error must classify as Error, got {other:?}"), + } +} + +/// EIO (5) — the error a crashed shell produces on the master fd. Hardcoded to +/// avoid a `libc` dependency just for the constant. +fn libc_eio() -> i32 { + 5 +} #[test] fn spawn_with_shell_succeeds_with_bin_true() { @@ -56,6 +83,34 @@ fn cwd_returns_path_or_none_after_spawn() { let _ = session.cwd(); } +/// The reader thread must terminate (not hang) when the shell exits and the +/// PTY reaches EOF. We spawn `/bin/true` (exits immediately) keeping the +/// receiver alive, then drain it: the channel's senders are all owned by the +/// reader thread, so once that thread breaks out of its loop and drops the +/// sender, `recv` returns `Err`. Reaching that within the deadline proves the +/// loop exited on EOF rather than spinning or blocking. +#[test] +fn reader_thread_exits_on_pty_eof() { + let (tx, rx) = unbounded::>(); + let session = PtySession::spawn_with_shell(80, 24, tx, "/bin/true", None, Box::new(|| {})) + .expect("spawn failed"); + // Keep the session (and thus the master) alive until the child has exited. + let deadline = Instant::now() + Duration::from_secs(5); + loop { + match rx.recv_timeout(Duration::from_millis(100)) { + Ok(_) => {} // drain any startup bytes + Err(crossbeam_channel::RecvTimeoutError::Disconnected) => break, // reader thread ended + Err(crossbeam_channel::RecvTimeoutError::Timeout) => { + assert!( + Instant::now() < deadline, + "reader thread did not terminate after PTY EOF within 5s" + ); + } + } + } + drop(session); +} + /// Verify that a child process does not become a zombie after it exits. /// /// Without the reaper thread in `spawn_with_shell`, every shell spawned for a