Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
61 changes: 46 additions & 15 deletions src/pty/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,50 @@ pub struct PtySession {
child_pid: Option<u32>,
}

/// 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<usize>) -> 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<dyn Read + Send>,
output_tx: Sender<Vec<u8>>,
wakeup: Box<dyn Fn() + Send + 'static>,
) {
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<Vec<u8>>) -> anyhow::Result<Self> {
Expand Down Expand Up @@ -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()?;

Expand Down
57 changes: 56 additions & 1 deletion src/pty/session_test.rs
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down Expand Up @@ -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::<Vec<u8>>();
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
Expand Down