Fix macOS command output truncated mid-table by draining PTY on child exit

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;

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<u8>,
+    pos: usize,
+    chunk: usize,
+    total_read: Arc<AtomicUsize>,
+}
+
+impl Read for ChunkedReader {
+    fn read(&mut self, out: &mut [u8]) -> io::Result<usize> {
+        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<u8>, chunk: usize, total_read: Arc<AtomicUsize>) -> 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<ChildEvent> {
+        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<u8> {
+    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<MockPty> {
+    let terminal = Arc::new(FairMutex::new(TerminalModel::mock(None, None)));
+    let listener = ChannelEventListener::new_for_test();
+    let (_tx, rx) = mio_channel::channel::<Message>();
+    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",
+    );
+}