From 9c2d3688782aaebf0e2cd4f009ac5020bb475366 Mon Sep 17 00:00:00 2001 From: Oz Date: Sun, 2 Aug 2026 15:11:12 +0000 Subject: [PATCH] Drain buffered PTY output on child exit to prevent truncated command output On macOS, a command that prints a large table in a fast burst is sometimes cut off mid-table when run in a Warp dev build launched via `script/run`. Root cause: the PTY reader event loop breaks out of its loop the instant it observes the child process has exited (both the SIGCHLD `child_event_token` path and the Windows `Message::ChildExited` channel path), without first draining output still buffered in the PTY. When the child writes a large final burst and exits quickly, the exit notification can win the race against reading that buffer, so the tail of the output is never read/parsed and is dropped. This is intermittent and especially visible on macOS due to SIGCHLD delivery ordering. Fix: add `EventLoop::drain_pty_after_exit`, which loops `pty_read` until the PTY reports it can no longer be read (WouldBlock/EOF), and call it before `terminal.exit()` at both child-exit sites. The PTY leader is non-blocking so this cannot block; `DRAIN_MAX_READS` bounds it as a safety net. Adds a regression test (event_loop_tests.rs) with a mock EventedPty holding a burst larger than MAX_LOCKED_READ, asserting the drain reads all buffered bytes, plus a control test showing a single pty_read stops early. CHANGELOG-BUG-FIX: Fixed command output (e.g. large tables) sometimes being cut off mid-print on macOS. Co-Authored-By: Oz Co-Authored-By: Warp --- app/src/terminal/local_tty/event_loop.rs | 51 +++++ .../terminal/local_tty/event_loop_tests.rs | 196 ++++++++++++++++++ 2 files changed, 247 insertions(+) create mode 100644 app/src/terminal/local_tty/event_loop_tests.rs diff --git a/app/src/terminal/local_tty/event_loop.rs b/app/src/terminal/local_tty/event_loop.rs index 5addfe5a4a0..ddbd70da0a9 100644 --- a/app/src/terminal/local_tty/event_loop.rs +++ b/app/src/terminal/local_tty/event_loop.rs @@ -29,6 +29,14 @@ const READ_BUFFER_SIZE: usize = 0x4_0000; /// someone else an opportunity to lock it. const MAX_LOCKED_READ: usize = 0x1_0000; +/// Maximum number of `pty_read` passes performed when draining the PTY after +/// the child process has exited (see [`EventLoop::drain_pty_after_exit`]). Each +/// pass processes up to [`MAX_LOCKED_READ`] bytes, so this bounds the post-exit +/// drain at `DRAIN_MAX_READS * MAX_LOCKED_READ` bytes — far more than any +/// realistic final output burst, while still guaranteeing the drain cannot be +/// wedged indefinitely. +const DRAIN_MAX_READS: usize = 512; + pub const CHANNEL_TOKEN: mio::Token = mio::Token(0); pub const PTY_TOKEN: mio::Token = mio::Token(1); pub const SIGNALS_TOKEN: mio::Token = mio::Token(2); @@ -320,6 +328,35 @@ where Ok(()) } + /// Drain and process any output still buffered in the PTY after the child + /// process has exited. + /// + /// The child exiting and the final burst of its output becoming readable + /// are two independent events. If the event loop tears down the instant it + /// observes the exit, any output the child wrote just before exiting that is + /// still buffered in the PTY is never read or parsed, so the tail of the + /// output (e.g. the bottom rows of a large table) is silently dropped. This + /// is especially visible on macOS, where `SIGCHLD` delivery can win the race + /// against draining the PTY on a fast, large output burst. + /// + /// Reading until the PTY reports it can no longer be read (`WouldBlock` / + /// EOF) ensures the complete output is rendered before the terminal is + /// marked as exited. The PTY leader is non-blocking, so this cannot block; + /// `DRAIN_MAX_READS` additionally bounds the work as a safety net. + fn drain_pty_after_exit(&mut self, state: &mut State, buf: &mut [u8]) { + let mut can_read = true; + for _ in 0..DRAIN_MAX_READS { + if !can_read { + break; + } + // A read error here (e.g. `EIO` once the child's PTY slave is fully + // closed) simply means there is nothing left to drain, so stop. + if self.pty_read(state, buf, &mut can_read).is_err() { + break; + } + } + } + pub fn spawn(mut self) -> JoinHandle<()> { #[cfg(test)] let feature_flag_overrides = warp_core::features::get_overrides(); @@ -397,6 +434,10 @@ where child_exited: exited, } => { if exited { + // Drain any output still buffered in + // the PTY before winding down, so a + // final burst isn't dropped. + self.drain_pty_after_exit(&mut state, &mut buf); self.terminal .lock() .exit(ExitReason::ShellProcessExited); @@ -412,6 +453,12 @@ where if let Some(local_tty::ChildEvent::Exited) = self.pty.next_child_event() { + // Drain any output the child wrote just + // before exiting that is still buffered in + // the PTY, so the tail of a large/fast output + // burst (e.g. a table) isn't dropped. See + // `drain_pty_after_exit`. + self.drain_pty_after_exit(&mut state, &mut buf); self.terminal.lock().exit(ExitReason::ShellProcessExited); child_exited = true; self.event_listener.send_wakeup_event(); @@ -492,3 +539,7 @@ where .expect("thread spawn works") } } + +#[cfg(test)] +#[path = "event_loop_tests.rs"] +mod tests; diff --git a/app/src/terminal/local_tty/event_loop_tests.rs b/app/src/terminal/local_tty/event_loop_tests.rs new file mode 100644 index 00000000000..ad94e10dd60 --- /dev/null +++ b/app/src/terminal/local_tty/event_loop_tests.rs @@ -0,0 +1,196 @@ +//! Tests for the PTY [`EventLoop`], focused on the post-child-exit drain that +//! prevents a final burst of output (e.g. the tail of a large table) from being +//! dropped when the child process exits — the defect behind APP-5099. + +use std::io::{self, Read}; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use mio::{Interest, Poll, Token}; +use parking_lot::FairMutex; + +use super::*; +use crate::terminal::SizeInfo; +use crate::terminal::event_listener::ChannelEventListener; +use crate::terminal::local_tty::{ChildEvent, EventedPty, EventedReadWrite, mio_channel}; +use crate::terminal::model::TerminalModel; +use crate::terminal::writeable_pty::Message; + +/// A [`Read`]er that hands out an in-memory buffer in fixed-size chunks and +/// reports EOF once drained, mimicking a PTY leader that still has a burst of +/// the child's final output buffered. It records the total number of bytes the +/// event loop actually read so a test can assert nothing was left behind. +struct ChunkedReader { + data: Vec, + pos: usize, + chunk: usize, + total_read: Arc, +} + +impl Read for ChunkedReader { + fn read(&mut self, out: &mut [u8]) -> io::Result { + if self.pos >= self.data.len() { + // Everything buffered has been consumed: the PTY is fully drained. + return Ok(0); + } + let remaining = self.data.len() - self.pos; + let n = remaining.min(self.chunk).min(out.len()); + out[..n].copy_from_slice(&self.data[self.pos..self.pos + n]); + self.pos += n; + self.total_read.fetch_add(n, Ordering::SeqCst); + Ok(n) + } +} + +/// A minimal [`EventedPty`] whose reader replays a fixed buffer. Only the pieces +/// exercised by `pty_read` / `drain_pty_after_exit` do anything meaningful; the +/// mio registration hooks and writer are inert. +struct MockPty { + reader: ChunkedReader, + writer: io::Sink, + exited_reported: bool, +} + +impl MockPty { + fn new(data: Vec, chunk: usize, total_read: Arc) -> Self { + MockPty { + reader: ChunkedReader { + data, + pos: 0, + chunk, + total_read, + }, + writer: io::sink(), + exited_reported: false, + } + } +} + +impl EventedReadWrite for MockPty { + type Reader = ChunkedReader; + type Writer = io::Sink; + + fn register(&mut self, _: &Poll, _: Interest) -> io::Result<()> { + Ok(()) + } + + fn reregister(&mut self, _: &Poll, _: Interest) -> io::Result<()> { + Ok(()) + } + + fn deregister(&mut self, _: &Poll) -> io::Result<()> { + Ok(()) + } + + fn reader(&mut self) -> &mut Self::Reader { + &mut self.reader + } + + fn read_token(&self) -> Token { + PTY_TOKEN + } + + fn writer(&mut self) -> &mut Self::Writer { + &mut self.writer + } + + fn write_token(&self) -> Token { + PTY_TOKEN + } +} + +impl EventedPty for MockPty { + fn child_event_token(&self) -> Token { + SIGNALS_TOKEN + } + + fn next_child_event(&mut self) -> Option { + if self.exited_reported { + None + } else { + self.exited_reported = true; + Some(ChildEvent::Exited) + } + } + + fn on_resize(&mut self, _: &SizeInfo) {} + + fn kill(self) -> anyhow::Result<()> { + Ok(()) + } +} + +/// Builds a byte buffer that stands in for a large, fast burst of table output. +/// It is deliberately larger than [`MAX_LOCKED_READ`] so that draining it +/// requires more than one `pty_read` pass. +fn large_output_burst() -> Vec { + let mut out = String::new(); + let mut row = 0; + while out.len() <= MAX_LOCKED_READ * 4 { + out.push_str(&format!( + "| col-a-{row:05} | col-b-{row:05} | col-c-{row:05} |\n" + )); + row += 1; + } + out.into_bytes() +} + +fn make_event_loop(pty: MockPty) -> EventLoop { + let terminal = Arc::new(FairMutex::new(TerminalModel::mock(None, None))); + let listener = ChannelEventListener::new_for_test(); + let (_tx, rx) = mio_channel::channel::(); + EventLoop::new(terminal, listener, pty, rx) +} + +/// Regression test for APP-5099: after the child exits, the event loop must +/// drain and process **all** output still buffered in the PTY, so the tail of a +/// large/fast burst (e.g. the bottom of a table) is not dropped. +#[test] +fn drain_pty_after_exit_reads_all_buffered_output() { + let data = large_output_burst(); + let total = data.len(); + let total_read = Arc::new(AtomicUsize::new(0)); + + let pty = MockPty::new(data, 4096, total_read.clone()); + let mut event_loop = make_event_loop(pty); + + let mut state = State::default(); + let mut buf = vec![0u8; READ_BUFFER_SIZE]; + + event_loop.drain_pty_after_exit(&mut state, &mut buf); + + assert_eq!( + total_read.load(Ordering::SeqCst), + total, + "the entire buffered PTY output must be read on child exit; leaving any \ + bytes unread is exactly the mid-table truncation this fix prevents", + ); +} + +/// Guards the assumption the fix relies on: a single `pty_read` intentionally +/// stops after [`MAX_LOCKED_READ`] bytes to yield the terminal lock, so it does +/// **not** drain a burst larger than that on its own. This is why the child-exit +/// path must loop via `drain_pty_after_exit` — without it, tearing down the loop +/// right after observing the exit would drop the remainder. +#[test] +fn single_pty_read_stops_before_draining_large_burst() { + let data = large_output_burst(); + let total = data.len(); + let total_read = Arc::new(AtomicUsize::new(0)); + + let pty = MockPty::new(data, 4096, total_read.clone()); + let mut event_loop = make_event_loop(pty); + + let mut state = State::default(); + let mut buf = vec![0u8; READ_BUFFER_SIZE]; + let mut can_read = true; + event_loop + .pty_read(&mut state, &mut buf, &mut can_read) + .expect("read from the mock PTY succeeds"); + + assert!( + total_read.load(Ordering::SeqCst) < total, + "a single pty_read should stop early (after MAX_LOCKED_READ) and leave \ + output buffered, demonstrating why the drain loop is required", + ); +}