From ec5dd58a529827d8b70a448f7c3f32fffeb28247 Mon Sep 17 00:00:00 2001 From: Quintin Botes Date: Mon, 3 Aug 2026 08:26:37 +0200 Subject: [PATCH] Hold input until the child can actually receive it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shell's line editor calls `tcsetattr` with `TCSAFLUSH` when it starts reading, and that discards whatever is already queued on the pty. Input written a moment too early is therefore not delivered late — it is destroyed. The visible symptom is a command that runs one character short: `cho tervin-block-one` for `echo tervin-block-one`, which is not a useful error but a command that does not exist. This is a product bug, not a test artifact. Tervin writes to panes programmatically as well as on keystrokes, so a restored session or a command Tervin runs on the user's behalf lands in exactly that window. It was found by Linux CI, where a loaded runner widens the window enough to hit reliably, and it had been papered over in the tests with sleeps for as long as the suite has existed. `PtySession` now holds input until the child has taken the terminal out of canonical mode, and releases it in order under the same lock. Reading `ICANON` from the master reports the pty's own line discipline, so this is the child's answer rather than an inference from elapsed time or from output — both of which say only that a prompt was *printed*, which is the earlier and wrong moment. The wait is bounded. Plenty of programs read in canonical mode for their whole life, discard nothing, and must not have their input held: `cat`, a pager. Those hit the bound and the gate opens. Three fixed sleeps come out of the tests as a result, and the PTY suite goes from a bit under 5 seconds to 0.89. Every sleep removed here was approximating this, badly. The remaining prompt waits in the Blocks suite stay: they space successive commands so each forms its own Block, which is a different question. Also fixes two tests that had never tested what they claimed. Every marker in the PTY suite was satisfied by the terminal's echo of the command that was typed, so `a_real_shell_receives_input_and_returns_output` passed on the echo alone and had never once proven output came back from a real shell. Markers are now typed split by an empty quote, so the echo reads `t''ervin-...` and only real output can match. rust 682 to 683. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 1 + crates/terminal-core/Cargo.toml | 5 + crates/terminal-core/src/pty.rs | 103 ++++++++++++++++++- crates/terminal-core/tests/pty_roundtrip.rs | 92 +++++++++++++---- crates/tervin-app/tests/blocks_end_to_end.rs | 102 ++++++++++++++++-- 5 files changed, 270 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d05b996..69b5759 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4492,6 +4492,7 @@ dependencies = [ "base64 0.23.0", "chrono", "criterion", + "libc", "parking_lot", "portable-pty", "serde", diff --git a/crates/terminal-core/Cargo.toml b/crates/terminal-core/Cargo.toml index 29aab84..95d8475 100644 --- a/crates/terminal-core/Cargo.toml +++ b/crates/terminal-core/Cargo.toml @@ -16,6 +16,11 @@ base64.workspace = true chrono.workspace = true portable-pty = "0.9.0" +# For `tcgetattr`, which is the only way to ask whether the child has taken the +# terminal out of canonical mode. That is the moment input stops being discarded. +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [dev-dependencies] criterion = "0.8.2" tempfile = "3.27.0" diff --git a/crates/terminal-core/src/pty.rs b/crates/terminal-core/src/pty.rs index 0577183..e031e19 100644 --- a/crates/terminal-core/src/pty.rs +++ b/crates/terminal-core/src/pty.rs @@ -31,6 +31,38 @@ const FLUSH_BYTES: usize = 32 * 1024; /// Size of each blocking PTY read. const READ_BUF: usize = 64 * 1024; +/// How often to ask whether the child has taken over the terminal. +const INPUT_GATE_POLL: Duration = Duration::from_millis(5); + +/// Longest to hold input waiting for a child that may never take over. +/// +/// Bounded because plenty of programs read in canonical mode for their whole life +/// and are perfectly able to receive input. They discard nothing, so gating them +/// would delay real keystrokes for no benefit. +const INPUT_GATE_MAX: Duration = Duration::from_millis(1500); + +/// Whether the child has put the terminal into a mode it reads input from itself. +/// +/// A line editor — zsh's ZLE, bash's readline — clears `ICANON` as it takes over, +/// and it does so with a `tcsetattr` that discards input already queued. Once the +/// flag is clear that call has happened, so anything written from here on survives. +/// +/// The master's termios reports the pty's line discipline, which makes this the +/// child's own answer rather than a guess from elapsed time or from output, both of +/// which say only that a prompt was *printed* — a different and earlier moment. +#[cfg(unix)] +fn accepts_input(fd: std::os::unix::io::RawFd) -> bool { + // SAFETY: `fd` is the pty master, owned by this session and open for as long as + // the watcher runs. `tcgetattr` only reads, and only into the local struct. + unsafe { + let mut settings: libc::termios = std::mem::zeroed(); + if libc::tcgetattr(fd, &mut settings) != 0 { + return false; + } + settings.c_lflag & libc::ICANON == 0 + } +} + /// Longest a synchronized-output frame may hold back a flush. /// /// An application that sets DEC 2026 and then blocks — or crashes before @@ -207,10 +239,16 @@ pub struct PtySession { /// `MasterPty` is `Send` but not `Sync`, and the session is shared across /// threads by the pane registry and every IPC command that touches it. master: Mutex>, - writer: Mutex>, + /// Shared with the readiness watcher, which flushes any gated input. + writer: Arc>>, child: Arc>>, alive: Arc, size: Mutex<(u16, u16)>, + /// Input written before the child could receive it. + /// + /// `Some` while the child is still taking over the terminal, `None` once + /// anything written goes straight through. See [`PtySession::write`]. + pending_input: Arc>>>, } impl PtySession { @@ -462,13 +500,53 @@ impl PtySession { .map_err(PtyError::Io)?; } + let pending_input = Arc::new(Mutex::new(Some(Vec::new()))); + let master = Mutex::new(pair.master); + let writer = Arc::new(Mutex::new(writer)); + + // Release input as soon as the child can actually receive it. + #[cfg(unix)] + { + let fd = master.lock().as_raw_fd(); + let pending = pending_input.clone(); + let writer = writer.clone(); + let alive = alive.clone(); + std::thread::Builder::new() + .name(format!("tervin-pty-ready-{}", config.pane_id)) + .spawn(move || { + let deadline = Instant::now() + INPUT_GATE_MAX; + while Instant::now() < deadline + && alive.load(Ordering::SeqCst) + && !fd.is_some_and(accepts_input) + { + std::thread::sleep(INPUT_GATE_POLL); + } + + // Open the gate either way, and under the same lock `write` + // takes, so nothing written afterwards can overtake what was + // held. A child that never leaves canonical mode — `cat`, a + // pager reading lines — discards nothing and must not be gated. + let mut pending = pending.lock(); + if let Some(queued) = pending.take().filter(|q| !q.is_empty()) { + let mut w = writer.lock(); + let _ = w.write_all(&queued).and_then(|()| w.flush()); + } + }) + .map_err(PtyError::Io)?; + } + #[cfg(not(unix))] + { + *pending_input.lock() = None; + } + Ok(Self { pane_id: config.pane_id, - master: Mutex::new(pair.master), - writer: Mutex::new(writer), + master, + writer, child, alive, size: Mutex::new((config.cols, config.rows)), + pending_input, }) } @@ -485,10 +563,29 @@ impl PtySession { } /// Write user input to the PTY. + /// + /// Input written before the child has taken over the terminal is held rather + /// than sent. A shell's line editor calls `tcsetattr` with `TCSAFLUSH` when it + /// starts reading, and that **discards whatever is already queued** — so input + /// sent a moment too early is not delayed, it is destroyed. Tervin writes to a + /// pane programmatically as well as on keystrokes, and a restored session or an + /// agent-issued command lands squarely in that window: the symptom is a command + /// that runs with its first character missing. + /// + /// The gate is held under the same lock the watcher releases it with, so a write + /// arriving after the gate opens can never overtake one that was held. It costs + /// one uncontended lock once the pane is running. pub fn write(&self, data: &[u8]) -> Result<(), PtyError> { if !self.is_alive() { return Err(PtyError::NotRunning(self.pane_id.clone())); } + { + let mut pending = self.pending_input.lock(); + if let Some(queue) = pending.as_mut() { + queue.extend_from_slice(data); + return Ok(()); + } + } let mut w = self.writer.lock(); w.write_all(data)?; w.flush()?; diff --git a/crates/terminal-core/tests/pty_roundtrip.rs b/crates/terminal-core/tests/pty_roundtrip.rs index 68529a1..1523c49 100644 --- a/crates/terminal-core/tests/pty_roundtrip.rs +++ b/crates/terminal-core/tests/pty_roundtrip.rs @@ -45,9 +45,8 @@ fn run(program: &str, args: &[&str], input: &[&str], done: impl Fn(&str) -> bool let session = PtySession::spawn(config, sink).expect("could not open a pty"); - // Give the shell a moment to reach its first prompt before typing, so input - // is not swallowed by a shell still setting up its line editor. - std::thread::sleep(Duration::from_millis(600)); + // No sleep before typing. `PtySession` holds input until the child has taken + // the terminal over, which is the only thing a sleep here ever approximated. for line in input { session .write(line.as_bytes()) @@ -87,6 +86,22 @@ fn run(program: &str, args: &[&str], input: &[&str], done: impl Fn(&str) -> bool collected } +/// Write a word so the terminal's echo of it cannot pass for the shell's output. +/// +/// A PTY echoes what is typed. A test that types `echo FOO` and then waits for +/// `FOO` is therefore satisfied by its own echo, before the shell has run at all, +/// and every assertion after it is reading the input back. Splitting the word with +/// an empty quote leaves the echo reading `F''OO` while the shell still prints +/// `FOO`, so only real output can match. +/// +/// This is not hypothetical. Every marker in this file was written the plain way, +/// which is why the burst test stopped collecting after 81 bytes and then reported +/// the missing lines as though the pump had dropped them. +fn only_in_output(word: &str) -> String { + let (head, tail) = word.split_at(1); + format!("{head}''{tail}") +} + /// Strip escape sequences so assertions match what a user would read. fn plain(text: &str) -> String { let mut out = String::with_capacity(text.len()); @@ -135,7 +150,8 @@ fn a_real_shell_receives_input_and_returns_output() { // The input path: keystrokes written to the PTY must reach the shell and its // output must come back through the pump. A screenshot of a prompt proves // only the output half. - let collected = run("/bin/sh", &[], &["echo tervin-roundtrip-ok\n"], |text| { + let line = format!("echo {}\n", only_in_output("tervin-roundtrip-ok")); + let collected = run("/bin/sh", &[], &[&line], |text| { text.contains("tervin-roundtrip-ok") }); let text = plain(&collected.text); @@ -148,12 +164,12 @@ fn a_real_shell_receives_input_and_returns_output() { #[test] fn output_arrives_in_order_across_several_commands() { // The coalescer batches reads; batching must never reorder them. - let collected = run( - "/bin/sh", - &[], - &["echo one\n", "echo two\n", "echo three\n"], - |text| text.contains("three"), - ); + let lines: Vec = ["one", "two", "three"] + .iter() + .map(|w| format!("echo {}\n", only_in_output(w))) + .collect(); + let refs: Vec<&str> = lines.iter().map(String::as_str).collect(); + let collected = run("/bin/sh", &[], &refs, |text| text.contains("three")); let text = plain(&collected.text); let one = text.find("one"); let two = text.find("two"); @@ -168,16 +184,45 @@ fn output_arrives_in_order_across_several_commands() { ); } +#[test] +fn input_written_the_instant_a_pane_opens_is_not_eaten() { + // The bug the input gate exists for, and the one a sleep only hid. A shell's + // line editor calls `tcsetattr` with `TCSAFLUSH` as it starts, which discards + // input already queued — so writing a moment too early does not arrive late, it + // does not arrive at all, and the command runs a character short. + // + // Tervin writes to panes programmatically as well as on keystrokes, so this is + // a restored session or an agent-issued command losing its leading byte, not a + // hypothetical. Written with no delay whatsoever, which is the whole point. + let marker = only_in_output("tervin-first-byte-intact"); + let collected = run("zsh", &[], &[&format!("echo {marker}\n")], |text| { + text.contains("tervin-first-byte-intact") + }); + let text = plain(&collected.text); + + assert!( + text.contains("tervin-first-byte-intact"), + "the command never ran:\n{text}" + ); + // `cho` is the exact signature of the loss, and it is not a failure the shell + // reports usefully: it is simply a command that does not exist. + assert!( + !text.contains("cho tervin-first-byte-intact"), + "the leading character was eaten before the shell could read it:\n{text}" + ); +} + #[test] fn a_large_burst_of_output_arrives_intact() { // The pump flushes early at a size threshold; nothing may be dropped at the // boundary. 5000 lines crosses it many times over. - let collected = run( - "/bin/sh", - &[], - &["i=0; while [ $i -lt 5000 ]; do echo line-$i; i=$((i+1)); done; echo BURST-DONE\n"], - |text| text.contains("BURST-DONE"), + let script = format!( + "i=0; while [ $i -lt 5000 ]; do echo line-$i; i=$((i+1)); done; echo {}\n", + only_in_output("BURST-DONE"), ); + let collected = run("/bin/sh", &[], &[&script], |text| { + text.contains("BURST-DONE") + }); let text = plain(&collected.text); assert!(text.contains("BURST-DONE"), "burst never completed"); for probe in ["line-0", "line-2500", "line-4999"] { @@ -192,14 +237,17 @@ fn a_large_burst_of_output_arrives_intact() { fn shell_integration_markers_survive_the_round_trip() { // Emitted by the shell, extracted by the tap, delivered on the chunk. If this // breaks, Blocks silently stop forming. - let script = concat!( - r#"printf '\033]7373;cmd=ZWNobyBoaQ==\007';"#, - r#"printf '\033]133;C\007';"#, - "echo hi;", - r#"printf '\033]133;D;0\007';"#, - "echo MARKERS-DONE\n", + let script = format!( + concat!( + r#"printf '\033]7373;cmd=ZWNobyBoaQ==\007';"#, + r#"printf '\033]133;C\007';"#, + "echo hi;", + r#"printf '\033]133;D;0\007';"#, + "echo {}\n", + ), + only_in_output("MARKERS-DONE"), ); - let collected = run("/bin/sh", &[], &[script], |text| { + let collected = run("/bin/sh", &[], &[&script], |text| { text.contains("MARKERS-DONE") }); diff --git a/crates/tervin-app/tests/blocks_end_to_end.rs b/crates/tervin-app/tests/blocks_end_to_end.rs index d2ef605..67bf1cb 100644 --- a/crates/tervin-app/tests/blocks_end_to_end.rs +++ b/crates/tervin-app/tests/blocks_end_to_end.rs @@ -15,13 +15,27 @@ use shell_integration::{InjectionMode, Shell}; use std::sync::mpsc; use std::sync::Arc; use std::time::{Duration, Instant}; -use terminal_core::{PtyConfig, PtyEvent}; +use terminal_core::{PtyConfig, PtyEvent, ShellSignal}; use tervin_core::{PaneId, SessionId}; /// Generous, because a login shell sources the user's rc files — which on a real /// machine can mean a version manager and a completion framework. const TIMEOUT: Duration = Duration::from_secs(30); +/// How long after `133;A` to treat a shell that sends no `133;B` as ready. +const PROMPT_SETTLE: Duration = Duration::from_millis(300); + +/// Longest to wait for a prompt before deciding this shell does not report them. +/// +/// Bounded separately from [`TIMEOUT`] because it is a different question. Waiting +/// the full timeout for a signal that is never coming is what turned this file from +/// a 32-second run into a 90-second one. +const PROMPT_WAIT: Duration = Duration::from_secs(5); + +/// Fallback spacing for a shell that reports no prompts, as this file used +/// throughout before it learned to wait for one. +const BLIND_SETTLE: Duration = Duration::from_millis(400); + /// A scratch directory that cleans itself up. struct Scratch(std::path::PathBuf); @@ -89,17 +103,25 @@ fn blocks_from(shell_program: &str, shell: Shell, commands: &[&str]) -> Vec = Vec::new(); + + // Wait for the shell to say it is ready, rather than betting on how long that + // takes. One command at a time, each typed into a drawn prompt, so each forms + // its own Block. + // + // A shell that does not report prompts at all — bash 3.2, which macOS still + // ships — is asked once and then left alone, because asking again only buys + // another wait for an answer that is not coming. + let mut reports_prompts = wait_for_prompt(&rx, &mut builder, &mut finished); for command in commands { session.write(command.as_bytes()).expect("write failed"); - // One at a time, so each produces its own Block rather than being typed - // into a shell that has not yet drawn a new prompt. - std::thread::sleep(Duration::from_millis(400)); + if reports_prompts { + reports_prompts = wait_for_prompt(&rx, &mut builder, &mut finished); + } else { + std::thread::sleep(BLIND_SETTLE); + } } - let mut finished: Vec = Vec::new(); let deadline = Instant::now() + TIMEOUT; while Instant::now() < deadline && finished.len() < commands.len() { @@ -128,6 +150,70 @@ fn blocks_from(shell_program: &str, shell: Shell, commands: &[&str]) -> Vec, + builder: &mut BlockBuilder, + finished: &mut Vec, +) -> bool { + let deadline = Instant::now() + PROMPT_WAIT; + let mut prompt_started: Option = None; + + while Instant::now() < deadline { + match rx.recv_timeout(Duration::from_millis(50)) { + Ok(PtyEvent::Chunk(chunk)) => { + let mut ready = false; + for positioned in &chunk.signals { + match positioned.signal { + // Definitive: the prompt is drawn and the shell is reading. + ShellSignal::PromptEnd => ready = true, + ShellSignal::PromptStart => { + prompt_started.get_or_insert_with(Instant::now); + } + _ => {} + } + } + for event in builder.consume(&chunk) { + if let BlockEvent::Finished(block) = event { + finished.push(block); + } + } + if ready { + return true; + } + } + Ok(PtyEvent::Exited { .. }) => return false, + Err(mpsc::RecvTimeoutError::Timeout) => {} + Err(mpsc::RecvTimeoutError::Disconnected) => return false, + } + + // A shell that announced a prompt but sends no `133;B` still told us + // something. Take it, once it has had a moment to finish drawing. + if prompt_started.is_some_and(|t| t.elapsed() >= PROMPT_SETTLE) { + return true; + } + } + false +} + /// Small helper so the sink closure stays readable at the call site. fn spawn_session(config: PtyConfig, tx: mpsc::Sender) -> terminal_core::PtySession { terminal_core::PtySession::spawn(