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(