diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f70f8..594500f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,36 @@ All notable changes to SSHub are documented in this file. ## [Unreleased] +### Fixed + +- **The embedded terminal answers cursor-position and status queries** (issue + #113, reported by [@ikerib](https://github.com/ikerib)) - an application + asking the terminal where the cursor is (`ESC [ 6 n`) got no answer from our + vt100 emulator and blocked until its own timeout, so atuin's history search + died on the first Up arrow with "The cursor position could not be read within + a normal duration" — over plain `ssh` the real terminal replies and it works. + The emulator now answers the cursor position report from its own grid, the + device status report, and the primary device attributes query — the last of + which also ends the two-second stall in any TUI that probes for the kitty + keyboard protocol (crossterm's `supports_keyboard_enhancement`), and the + middle one the same hang in probes that end on `ESC [ 5 n` to be sure + *something* answers. Queries we do not actually speak + (the kitty keyboard protocol itself, secondary device attributes) stay + unanswered on purpose: silence is what tells a caller they are unsupported. + The remote decides how often it asks, so the answers are rate-limited — a + burst costs nothing, but a host stuck in a query loop cannot crowd the user's + keystrokes out of the PTY write queue. + +- **A remote that stops reading its input can no longer freeze the app** - + writing to the PTY blocks once the child stops reading, and it was happening + on the frame loop, so a host in a terminal-query loop that never read its own + stdin parked every tab, input and rendering included, with no timeout and no + recovery. Measured: `drain()` stopped returning after ~80 s of such a flood + and never came back. PTY writes now go through their own thread behind a + bounded queue — keystrokes, the auto-typed secret, pastes and query answers + stay in order, and a queue that fills means nothing is reaching the remote + anyway. The same flood now runs indefinitely with a worst frame of 98 ms. + ## [0.15.0] - 2026-08-18 ### Added diff --git a/src/session/mod.rs b/src/session/mod.rs index 24628bb..627e7ce 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -202,6 +202,10 @@ pub struct Session { pub host_name: String, /// Optional PTY transcript writer; closed on session end. log: Option, + /// Remaining byte allowance for terminal-query answers, and when it was + /// last topped up. See [`REPLY_BURST_BYTES`]. + reply_tokens: usize, + reply_refilled_at: Instant, } impl Session { @@ -283,6 +287,8 @@ impl Session { host_name: config.host_name.clone(), config, log, + reply_tokens: REPLY_BURST_BYTES, + reply_refilled_at: Instant::now(), }) } @@ -522,7 +528,12 @@ impl Session { } } if had_bytes { + // Secret first: both write to the same PTY, and a reply queued + // ahead of the password would be read as part of it by whatever + // asks for a line. Answering after leaves the reply as harmless + // leftover input instead. self.maybe_send_pending_secret(); + self.answer_terminal_queries(); self.maybe_reveal(); } if had_stderr { @@ -538,6 +549,39 @@ impl Session { self.maybe_detect_connected(); } + /// Write the emulator's answers to terminal status queries back into the + /// PTY. Driven from [`Self::drain`] rather than from the frame, because + /// unlike a clipboard write there is nothing to decide: an application + /// waiting on a reply blocks whether or not its tab is the visible one. + fn answer_terminal_queries(&mut self) { + let replies = self.parser.take_replies(); + if replies.is_empty() { + return; + } + let now = Instant::now(); + let refill = (now.duration_since(self.reply_refilled_at).as_millis() as usize) + .saturating_mul(REPLY_BYTES_PER_SEC) + / 1000; + if refill > 0 { + self.reply_tokens = self + .reply_tokens + .saturating_add(refill) + .min(REPLY_BURST_BYTES); + self.reply_refilled_at = now; + } + // Whole batch or nothing: half an escape sequence in the remote's input + // is worse than a query left unanswered. + if replies.len() > self.reply_tokens { + return; + } + self.reply_tokens -= replies.len(); + // Neither an over-budget batch nor a failed write says anything out + // loud. The application that asked reports its own timeout, which tells + // the user more than a line in the SSH log would — and a diagnostic here + // would be pushed on every frame for as long as the flood lasts. + let _ = self.runtime.write(&replies); + } + /// Throw away whatever the PTY asked us to copy, including the drop /// counters. Used for every session that is not the visible one this /// frame, so a background tab can neither relay now nor replay later. @@ -921,6 +965,24 @@ const CONNECTED_NEEDLES: &[&str] = &["authenticated to ", "authenticated ("]; /// dropped from the front so a long session can't grow it without bound. const DEBUG_LOG_CAP: usize = 64 * 1024; +/// Token bucket over the bytes of terminal-query answers we write back into the +/// PTY, as a burst allowance and a sustained refill rate. Real applications ask +/// a handful of times per keystroke, so neither is ever felt; the sustained rate +/// is a fraction of what a person typing already writes into the same PTY. +/// +/// It exists because the remote decides how often it asks, and the write queue +/// it answers into is shared with the user's keystrokes. Unthrottled, a host in +/// a query loop fills the PTY write queue on its own and +/// the typing gets dropped instead. Keeping the answers to a trickle leaves the +/// queue for the person at the keyboard. +/// +/// What it does *not* do is prevent the freeze: measured against the blocking +/// write this bucket only moved the wedge from under a second to ~80 s +/// (4096 + 256/s reaching the master's ~20 KiB limit at t≈64 s). That is what +/// the writer thread is for. +const REPLY_BURST_BYTES: usize = 4096; +const REPLY_BYTES_PER_SEC: usize = 256; + /// Ordered (lowercase needle → plain-language reason) map for failed connects. /// First match wins, so keep more specific patterns before generic ones. const FAILURE_EXPLANATIONS: &[(&str, &str)] = &[ @@ -1327,6 +1389,94 @@ mod prompt_tests { ); } + /// End-to-end for #113: a child asks the terminal where the cursor is and + /// *reads the answer back*. Nothing in the parser unit tests proves the + /// reply actually leaves the process, and this is exactly the loop that + /// hangs atuin — it asks, waits two seconds, and errors out. + /// + /// `stty raw -echo` so the reply arrives byte-for-byte rather than + /// line-buffered and echoed back at us, and `-icanon min 0 time 5` so the + /// read returns after half a second with whatever arrived — reading a fixed + /// byte count instead would have to guess the answer's length, and would + /// silently truncate it (leaving the tail in the input queue) the moment the + /// cursor is anywhere but the home position. `tr` strips the escape so the + /// marker is printable on the grid. + fn asks_for_the_cursor_position(prelude: &str) -> Session { + let script = format!( + r"stty raw -echo -icanon min 0 time 5; {prelude} printf '\033[6n'; \ + r=$(dd bs=1 count=32 2>/dev/null | tr -d '\033['); \ + stty sane; printf 'CPR<%s>\r\n' $r" + ); + let config = SessionConfig { + argv: vec!["sh".into(), "-c".into(), script], + display_name: "t".into(), + meta: SessionMeta::default(), + pending_secret: None, + key_push_identity: None, + host_name: "t".into(), + }; + let mut s = Session::spawn(config, 24, 80, None).unwrap(); + // Bounded poll: a child that never gets its answer must fail the assert, + // not park the test binary. `Drop for PtyRuntime` kills the process + // group on the way out, so a `dd` still blocked on the read is reaped. + for _ in 0..300 { + s.drain(); + if s.screen_tail_snippet().contains("CPR<") { + break; + } + std::thread::sleep(Duration::from_millis(10)); + } + s + } + + #[test] + fn the_pty_child_receives_a_cursor_position_report() { + let s = asks_for_the_cursor_position(""); + assert!( + s.screen_tail_snippet().contains("CPR<1;1R>"), + "child never got its cursor position back, tail: {:?}", + s.screen_tail_snippet() + ); + } + + #[test] + fn the_report_the_child_receives_is_clamped_at_the_right_margin() { + // The grid is 80 wide, so 80 characters leave the cursor in vt100's + // pending-wrap state at column 80 (0-based) — one past the last cell. + // Unclamped the child would be told `1;81R`, a column its terminal does + // not have, and code measuring the room left from it underflows. This is + // the state a prompt that fills the line leaves behind, i.e. the moment + // #113's reporter presses Up. + let s = asks_for_the_cursor_position(r"printf '%080d' 0;"); + assert!( + s.screen_tail_snippet().contains("CPR<1;80R>"), + "child should be told column 80 of 80, tail: {:?}", + s.screen_tail_snippet() + ); + } + + #[test] + fn discarding_clipboard_writes_keeps_the_query_answers() { + // The clipboard relay is gated on visibility: a background tab discards + // instead of relaying. Query answers must never be gated the same way — + // the application waits for its reply whether or not the user is looking + // at its tab — so the discard path has to leave them alone. Both come + // out of the same `PtyCallbacks`, which is what makes this worth an + // assertion rather than a comment. + let mut s = scratch_session(); + s.parser.process(b"\x1b]52;c;R0VIRUlN\x07\x1b[5n"); + s.discard_clipboard_writes(); + assert!( + s.parser.take_clipboard_writes().is_empty(), + "the clipboard write is the one that gets discarded" + ); + assert_eq!( + s.parser.take_replies(), + b"\x1b[0n".to_vec(), + "the query answer must survive the discard" + ); + } + /// Build a throwaway session for tests that only need a live `Session`. fn scratch_session() -> Session { let config = SessionConfig { diff --git a/src/session/parser.rs b/src/session/parser.rs index 88d8993..82d4308 100644 --- a/src/session/parser.rs +++ b/src/session/parser.rs @@ -1,6 +1,8 @@ //! VT100 parser wrapper. Maintains an in-memory `vt100::Screen` that the -//! renderer reads via `tui-term`, and relays OSC 52 clipboard writes that -//! applications inside the PTY emit. +//! renderer reads via `tui-term`, relays OSC 52 clipboard writes that +//! applications inside the PTY emit, and answers the terminal status queries +//! they send — `vt100` implements none of the latter, and an application that +//! asks and hears nothing back blocks until its own timeout. /// Largest decoded payload we'll relay from the PTY to the host clipboard /// (64 KiB). Keeps a remote from flooding the clipboard with a huge write. @@ -10,6 +12,12 @@ const CLIPBOARD_RELAY_MAX_BYTES: usize = 64 * 1024; /// copy loop can't grow the queue without bound; the excess is dropped. const CLIPBOARD_RELAY_MAX_QUEUED: usize = 8; +/// Largest queue of answers to terminal status queries we hold between drains +/// (1 KiB). Every answer is a dozen bytes at most, so this is far more than any +/// real application asks for in one frame — it exists so a remote stuck in a +/// query loop can't make us buffer unbounded input for it. +const REPLY_QUEUE_MAX_BYTES: usize = 1024; + /// Exact decoded byte length of a base64 payload, without decoding it. The /// payload is relayed verbatim, so a real decoder would be pure waste — this /// only exists to enforce the size cap and to size the "n bytes" notice. @@ -37,22 +45,26 @@ pub(crate) struct ClipboardDrops { pub(crate) queue_full: usize, } -/// Collects OSC 52 clipboard writes coming out of the PTY so the session can -/// re-emit them toward the real terminal. +/// Everything the emulator has to answer for, collected per drain: OSC 52 +/// clipboard writes headed for the real terminal, and answers to terminal +/// status queries headed back into the PTY. /// -/// Without this, `vt100` parses `ESC ] 52 ; c ; BEL` and hands it to -/// the default `Callbacks for ()` impl, which silently drops it — so anything -/// copying inside the PTY (herdr, tmux, neovim, lazygit…) appears to work but -/// never reaches the system clipboard. +/// Without this, `vt100` hands both to the default `Callbacks for ()` impl, +/// which silently drops them. A dropped clipboard write means anything copying +/// inside the PTY (herdr, tmux, neovim, lazygit…) appears to work but never +/// reaches the system clipboard; a dropped *query* means the application waits +/// for an answer that never comes. #[derive(Default)] -struct ClipboardRelay { +struct PtyCallbacks { /// Pending base64 payloads, in arrival order. pending: Vec, /// Writes rejected since the last drain, by reason. drops: ClipboardDrops, + /// Answers to terminal status queries, waiting to go back into the PTY. + replies: Vec, } -impl vt100::Callbacks for ClipboardRelay { +impl vt100::Callbacks for PtyCallbacks { fn copy_to_clipboard(&mut self, _: &mut vt100::Screen, _ty: &[u8], data: &[u8]) { // An empty payload is a clipboard *clear* on terminals that honour it. // We neither forward it nor count it as a drop: a remote must not be @@ -81,19 +93,81 @@ impl vt100::Callbacks for ClipboardRelay { // `paste_from_clipboard` is deliberately left as the no-op default: // answering `ESC]52;c;?BEL` would let any host we're SSH'd into *read* the // local clipboard, which is far worse than a write and buys us nothing. + + /// vt100 implements no terminal *query* at all, so every one of them lands + /// here — and an application that asks and hears nothing back blocks until + /// its own timeout expires. atuin's history search dies outright ("The + /// cursor position could not be read within a normal duration", #113), and + /// every crossterm-based TUI stalls two seconds at startup probing for the + /// kitty keyboard protocol. Answering is simply what the terminal on the + /// other side does when `ssh` runs without sshub in front of it. + fn unhandled_csi( + &mut self, + screen: &mut vt100::Screen, + i1: Option, + _i2: Option, + params: &[&[u16]], + c: char, + ) { + // Private sequences (`CSI ? … u`, the kitty keyboard protocol query; + // `CSI > c`, secondary device attributes) stay unanswered on purpose. + // We don't speak them, and claiming otherwise is worse than silence: + // crossterm reads a missing `?u` reply *plus* the DA1 answer below as + // a definitive "not supported", which is the truth. + if i1.is_some() { + return; + } + let param = params.first().and_then(|p| p.first().copied()).unwrap_or(0); + let reply = match (c, param) { + // DSR 6 — cursor position report. Our grid mirrors the remote + // screen, so its cursor *is* the answer. Reported 1-based, and + // clamped: vt100 parks the cursor one column *past* the right + // margin after a character lands in the last column (the pending + // wrap is only resolved when the next one arrives), so a prompt + // that fills the line would otherwise be reported at column + // `cols + 1` — a real terminal answers `cols`, and code measuring + // the room left underflows on anything else. Origin mode is the + // one case we still get wrong: vt100 handles DECOM itself and + // exposes no accessor, so the row here is absolute where a + // conformant terminal would report it relative to the region. + ('n', 6) => { + let (row, col) = screen.cursor_position(); + let (rows, cols) = screen.size(); + let (row, col) = ((row + 1).min(rows), (col + 1).min(cols)); + format!("\x1b[{row};{col}R").into_bytes() + } + // DSR 5 — device status. Nothing can go wrong in an in-memory grid. + ('n', 5) => b"\x1b[0n".to_vec(), + // DA1 — device attributes. VT100 with the advanced video option is + // the honest floor for what vt100 emulates; callers only care that + // an answer arrives at all, not what it claims. + ('c', 0) => b"\x1b[?1;2c".to_vec(), + _ => return, + }; + if self.replies.len() + reply.len() <= REPLY_QUEUE_MAX_BYTES { + self.replies.extend_from_slice(&reply); + } + } } pub struct ParserState { - inner: vt100::Parser, + inner: vt100::Parser, } impl ParserState { pub fn new(rows: u16, cols: u16) -> Self { Self { - inner: vt100::Parser::new_with_callbacks(rows, cols, 10_000, ClipboardRelay::default()), + inner: vt100::Parser::new_with_callbacks(rows, cols, 10_000, PtyCallbacks::default()), } } + /// Take the answers to terminal queries seen since the last call. Unlike a + /// clipboard write these go straight back into the PTY, not to the host + /// terminal, and every session owes them whether or not it is on screen. + pub(crate) fn take_replies(&mut self) -> Vec { + std::mem::take(&mut self.inner.callbacks_mut().replies) + } + /// Take the drops recorded since the last call, resetting the counters. pub(crate) fn take_clipboard_drops(&mut self) -> ClipboardDrops { std::mem::take(&mut self.inner.callbacks_mut().drops) @@ -327,6 +401,108 @@ mod tests { assert_eq!(p.take_clipboard_writes().len(), 1); } + // ── Terminal status queries ─────────────────────────────────── + // + // vt100 implements none of these, so they arrive at `unhandled_csi`. An + // application that asks and hears nothing back hangs on its own timeout. + + #[test] + fn cursor_position_report_answers_the_grid_cursor() { + // `CSI 6 n` is what crossterm's `cursor::position()` writes, and what + // atuin's history search needs before it can draw (#113). The answer is + // 1-based, so a cursor parked after "hi" on the first row is (1, 3). + let mut p = parser_with(10, 80, b"hi\x1b[6n"); + assert_eq!(p.take_replies(), b"\x1b[1;3R".to_vec()); + } + + #[test] + fn cursor_position_report_follows_the_cursor() { + // Same query from elsewhere on the grid must not hand back a constant. + let mut p = parser_with(10, 80, b"\x1b[5;7H\x1b[6n"); + assert_eq!(p.take_replies(), b"\x1b[5;7R".to_vec()); + } + + #[test] + fn cursor_position_report_is_clamped_at_the_right_margin() { + // vt100 parks the cursor at column `cols` (0-based, i.e. one past the + // last cell) once a character lands in the last column, resolving the + // wrap only when the next one arrives. Unclamped that reports column + // `cols + 1` — a column the terminal does not have. A prompt that + // fills the line and then asks where it is gets this every time, and + // whoever measures the room left from it underflows. + let mut p = parser_with(3, 10, b"0123456789\x1b[6n"); + assert_eq!(p.screen().cursor_position(), (0, 10), "vt100 behaviour"); + assert_eq!(p.take_replies(), b"\x1b[1;10R".to_vec()); + } + + #[test] + fn device_status_report_answers_ok() { + let mut p = parser_with(10, 80, b"\x1b[5n"); + assert_eq!(p.take_replies(), b"\x1b[0n".to_vec()); + } + + #[test] + fn device_attributes_are_answered_with_and_without_a_param() { + // crossterm probes kitty-keyboard support with `ESC[?u ESC[c` and waits + // two seconds for *either* reply. The DA1 answer is what ends that wait. + for query in [&b"\x1b[c"[..], &b"\x1b[0c"[..]] { + let mut p = parser_with(10, 80, query); + assert_eq!(p.take_replies(), b"\x1b[?1;2c".to_vec(), "query {query:?}"); + } + } + + #[test] + fn private_queries_stay_unanswered() { + // We don't speak the kitty keyboard protocol (`CSI ? u`) or secondary + // device attributes (`CSI > c`). Silence is the honest answer, and it's + // what makes crossterm conclude "unsupported" once DA1 arrives. + let mut p = parser_with(10, 80, b"\x1b[?u\x1b[>c\x1b[?6n"); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn unknown_dsr_parameters_are_not_answered() { + // Making something up for a query we don't recognise is worse than not + // replying: the application would parse our answer as the wrong event. + let mut p = parser_with(10, 80, b"\x1b[n\x1b[99n"); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn take_replies_drains() { + let mut p = parser_with(10, 80, b"\x1b[5n"); + assert_eq!(p.take_replies().len(), 4); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn reply_queue_is_capped() { + // A remote spinning on `CSI 5 n` must not grow our buffer without + // bound between drains. + let mut stream = Vec::new(); + for _ in 0..1000 { + stream.extend_from_slice(b"\x1b[5n"); + } + let mut p = parser_with(10, 80, &stream); + // The invariant is the bound, not an exact number: only whole replies + // are queued, so where the cap lands depends on how long they are. + let queued = p.take_replies().len(); + assert!(queued <= REPLY_QUEUE_MAX_BYTES, "over the cap: {queued}"); + assert!( + queued > REPLY_QUEUE_MAX_BYTES - 4, + "cap not actually reached: {queued}" + ); + } + + #[test] + fn queries_do_not_reach_the_grid() { + // Regression: the query must stay invisible. If it ever landed on a + // cell the user would see escape gibberish mid-session. + let mut p = parser_with(10, 80, b"before\x1b[6nafter"); + assert_eq!(p.screen().contents().trim(), "beforeafter"); + assert_eq!(p.take_replies(), b"\x1b[1;7R".to_vec()); + } + #[test] fn decoded_len_matches_real_decode() { // Exact decoded size without pulling in a base64 decoder — the payload diff --git a/src/session/pty.rs b/src/session/pty.rs index e618854..e01f6cf 100644 --- a/src/session/pty.rs +++ b/src/session/pty.rs @@ -8,7 +8,7 @@ use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::OpenOptionsExt; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::sync::mpsc::{self, Receiver}; +use std::sync::mpsc::{self, Receiver, SyncSender, TrySendError}; use std::sync::Arc; use std::thread::{self, JoinHandle}; use std::time::Duration; @@ -18,6 +18,19 @@ use portable_pty::{native_pty_system, Child, CommandBuilder, MasterPty, PtySize} const READ_BUF: usize = 4096; +/// How many pending writes toward the PTY we hold before dropping them. +/// +/// The writer sits on its own thread for one reason: `write_all` on the master +/// blocks once the child stops reading its stdin. A remote stuck in a terminal +/// query loop that never reads its own input makes `ssh` stop reading ours, the +/// master fills after ~20 KiB, and doing that write on the frame loop parked +/// every tab, input and rendering included — measured, not theorised. Off the +/// frame loop the only cost of a wedged remote is that writes aimed at it pile +/// up, and this is where the pile stops: a session that cannot be written to is +/// one where dropping is the honest outcome. Human typing never queues this +/// deep, so a full queue always means the far end is gone. +const WRITE_QUEUE_LEN: usize = 64; + /// Env var carrying the stderr FIFO path into the `sh` wrapper. const STDERR_FIFO_ENV: &str = "SSHUB_STDERR_FIFO"; @@ -88,7 +101,9 @@ impl Drop for StderrFifo { pub struct PtyRuntime { master: Box, - writer: Box, + /// Sender into the writer thread. `None` only while dropping, which is what + /// tells the thread to exit. + write_tx: Option>>, rx: Receiver, /// Set when the reader has signalled EOF / child exit. Used so we don't /// keep spinning on a dead PTY. @@ -168,7 +183,24 @@ impl PtyRuntime { drop(pair.slave); let mut reader = pair.master.try_clone_reader().context("clone pty reader")?; - let writer = pair.master.take_writer().context("take pty writer")?; + let mut writer = pair.master.take_writer().context("take pty writer")?; + + // Everything written toward the child goes through here, in order: + // keystrokes, the auto-typed secret, pastes, and the emulator's answers + // to terminal queries. One thread keeps that ordering while taking the + // blocking write off the frame loop. It ends when the sender drops. + let (write_tx, write_rx) = mpsc::sync_channel::>(WRITE_QUEUE_LEN); + thread::Builder::new() + .name("sshub-pty-writer".into()) + .spawn(move || { + for chunk in write_rx { + if writer.write_all(&chunk).is_err() { + break; + } + writer.flush().ok(); + } + }) + .context("spawn pty writer thread")?; let (tx, rx) = mpsc::channel(); let stderr_tx = tx.clone(); @@ -238,7 +270,7 @@ impl PtyRuntime { Ok(Self { master: pair.master, - writer, + write_tx: Some(write_tx), rx, closed, reader_thread: Some(reader_thread), @@ -254,11 +286,23 @@ impl PtyRuntime { self.rx.try_recv().ok() } - /// Write bytes to the master side. Called for each forwarded keystroke. + /// Hand bytes to the writer thread. Never blocks: see [`WRITE_QUEUE_LEN`] + /// for why the write cannot happen on the caller's thread. The cost is that + /// a write failure is no longer reported synchronously — a full queue is, + /// and it is the only failure a caller can do anything about (nothing is + /// reaching the remote). pub fn write(&mut self, bytes: &[u8]) -> Result<()> { - self.writer.write_all(bytes)?; - self.writer.flush().ok(); - Ok(()) + let tx = self + .write_tx + .as_ref() + .ok_or_else(|| anyhow!("pty writer is gone"))?; + match tx.try_send(bytes.to_vec()) { + Ok(()) => Ok(()), + Err(TrySendError::Full(_)) => { + Err(anyhow!("pty write queue full — the remote is not reading")) + } + Err(TrySendError::Disconnected(_)) => Err(anyhow!("pty writer has exited")), + } } pub fn resize(&self, rows: u16, cols: u16) -> Result<()> { @@ -317,6 +361,12 @@ fn terminate_child_process(child: &mut dyn portable_pty::Child) { impl Drop for PtyRuntime { fn drop(&mut self) { self.terminate_child(); + // Closing the channel is what ends the writer thread; it is deliberately + // not joined. A write blocked on a child that stopped reading only + // returns once the slave closes, and while `terminate_child` above makes + // that happen, waiting on it here would move the freeze we just fixed + // into session teardown. The thread owns nothing but its own fd. + self.write_tx = None; self.stderr_stop.store(true, Ordering::Relaxed); if let Some(handle) = self.stderr_reader.take() { let _ = handle.join();