diff --git a/CHANGELOG.md b/CHANGELOG.md index 944b7bb..b90c371 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,32 @@ reads that marker. which moved to the next row because it did not fit in an empty last column is not recorded as a wrap, so `logical_text()` splits the line there (#585). +- Dropping a `Terminal` no longer holds the process-wide PTY lock while + the child stops (#589). It sent SIGHUP and polled in portable-pty's fixed + 50 ms steps under the lock, so a drop took 50 ms (200 ms when the child + ignores SIGHUP), and every spawn in the test binary waited it out. The + child is now stopped first, polled from 1 ms, and the lock is taken only + to close the descriptors: measured, 1 ms per drop and no wait for a + concurrent spawn. The responder threads' copies of the master now close + under the lock too. +- `Error::Write` at the deadline says what happens to the bytes (#590): the + write is not withdrawn, the rest of it reaches the application if it + reads again, and later input queues behind it. The message and the docs + said the write "gave up", so a retry typed its input twice. +- A query's reply is queued before the screen the query is drawn on can be + seen (#591). A test that waited for that screen and typed could have its + keystroke reach the application ahead of the reply: 11 runs in 9,600 at + 32 threads, and none since. +- On Unix, a program named with a directory part, such as + `target/debug/app`, is found from the working directory, as + `std::process::Command` finds it (#592). Only `PATH` was searched, so the + spawn failed with "No viable candidates found in PATH", or under + `env_clear()` could not resolve `PATH` at all. `PATH` is still searched + after. +- On Windows, a reaped child is the terminal closing 500 ms after the reap, + however short the waits that follow (#593). Each wait restarted that + grace, so waits shorter than it never saw EOF, and a send to the dead + child was not refused. - `termlens` no longer panics with exit code 101 (#594). A closed or full stderr (`2>&1 | true`, `2>/dev/full`) made every diagnostic and the `inspect` trailer panic; the write is now checked, and a trailer that cannot diff --git a/crates/termlens/src/error.rs b/crates/termlens/src/error.rs index 46da6a5..df62739 100644 --- a/crates/termlens/src/error.rs +++ b/crates/termlens/src/error.rs @@ -129,6 +129,13 @@ pub enum Error { /// means the bytes could not be handed over at all, which is a fact /// about the child rather than about the test, so the screen at the /// moment of the failure is embedded the way a timeout's is. + /// + /// A write that gave up at the deadline is **not withdrawn**: the bytes + /// not yet written stay queued and reach the application, in order, if + /// it ever reads again, and input sent afterwards waits behind them — + /// failing the same way while the queue does not move. So a retry types + /// its input twice, and "the application never got these bytes" is not + /// what this error says (#590). #[error("failed to send {what}\n--- screen at the failed write ---\n{screen}")] Write { /// What was being sent, which command it was going to, and why the diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs index e349dea..1a2e6a8 100644 --- a/crates/termlens/src/spawn.rs +++ b/crates/termlens/src/spawn.rs @@ -202,9 +202,14 @@ mod unix { }) } - /// The file the PTY layer would execute for `program` run in `dir`, - /// chosen by its rules, or `None` for the PTY layer to say why there is - /// none. + /// The file to execute for `program` run in `dir`, or `None` for the PTY + /// layer to say why there is none. + /// + /// The PTY layer's rules, with one change: a name with a directory part + /// is a path from `dir` first, as `execvp` and `std::process::Command` + /// take it. The PTY layer searched PATH for `target/debug/app`, never + /// the directory the test runs in (#592); PATH is still searched after, + /// so nothing it found stops being found. fn resolve(program: &OsStr, dir: &Path, path: Option<&OsStr>) -> Option { let program = Path::new(program); let runnable = |file: &Path| !file.is_dir() && executable(file); @@ -219,8 +224,14 @@ mod unix { let file = dir.join(program); return runnable(&file).then_some(file); } - // Anything else, `sub/x` included, along PATH, each entry taken - // relative to `dir`. + // `sub/x` in `dir`, then along PATH; a bare name along PATH only. + if program.components().count() > 1 { + let file = dir.join(program); + if runnable(&file) { + return Some(file); + } + } + // Each PATH entry taken relative to `dir`. std::env::split_paths(path?) .map(|entry| dir.join(entry).join(program)) .find(|file| runnable(file)) diff --git a/crates/termlens/src/terminal.rs b/crates/termlens/src/terminal.rs index c5730ef..256b49e 100644 --- a/crates/termlens/src/terminal.rs +++ b/crates/termlens/src/terminal.rs @@ -78,6 +78,9 @@ impl ExitWatch { _ => return, } } + // From the first time anything saw the child reaped, kept on the + // terminal: a watch made per wait restarted the grace at every wait, + // so waits shorter than it never saw the terminal close (#593). let reaped_at = *self.reaped_at.get_or_insert_with(Instant::now); if reaped_at.elapsed() >= DRAIN_GRACE { state.eof = true; @@ -86,6 +89,11 @@ impl ExitWatch { state.generation += 1; } } + + /// The child was reaped just now, by something other than a tick. + fn reaped(&mut self) { + self.reaped_at.get_or_insert_with(Instant::now); + } } /// How many distinct unanswered query shapes a terminal remembers for its @@ -231,8 +239,10 @@ struct WriteRequest { /// test binary that never exits is a worse one. const DROP_REAP_GRACE: Duration = Duration::from_secs(2); -/// Serializes every PTY *lifecycle edge* (open+spawn on one side, kill+reap+ -/// master-close on the other) across all `Terminal`s in this process. +/// Serializes every PTY *lifecycle edge* (open+spawn on one side, the close +/// of each master descriptor on the other) across all `Terminal`s in this +/// process. The child is stopped before the lock is taken, while its +/// master is still open (#589). /// /// Why: macOS tears PTYs down with `revoke()`, and PTY device numbers are /// recycled immediately. With concurrent terminals, one thread's teardown @@ -267,6 +277,49 @@ pub(crate) fn pty_lifecycle_guard() -> LifecycleGuard { None } +/// A duplicate of the PTY master owned by a responder thread, closed under +/// the lifecycle lock wherever the thread drops it. +/// +/// The reader and writer threads each hold one, and end on their own after +/// `Drop` returns — never joined, so a grandchild holding the terminal +/// cannot hang teardown. Their closes would then fall outside the lock, +/// and on macOS the last close of a master is the one that revokes the +/// device; through this, every close of a master is serialized against +/// every open, whichever comes last (#589). +struct MasterDup(Option); + +impl Drop for MasterDup { + fn drop(&mut self) { + let _lifecycle = pty_lifecycle_guard(); + drop(self.0.take()); + } +} + +impl Read for MasterDup { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + match &mut self.0 { + Some(inner) => inner.read(buf), + None => Ok(0), + } + } +} + +impl Write for MasterDup { + fn write(&mut self, buf: &[u8]) -> io::Result { + match &mut self.0 { + Some(inner) => inner.write(buf), + None => Err(io::ErrorKind::BrokenPipe.into()), + } + } + + fn flush(&mut self) -> io::Result<()> { + match &mut self.0 { + Some(inner) => inner.flush(), + None => Ok(()), + } + } +} + /// Attempts to open a PTY before giving up, and the step between them. The /// ceiling is the sum, about 1.6s — long enough to outlast a burst of /// teardowns, short enough that a genuinely broken environment still reports @@ -1606,8 +1659,11 @@ impl TerminalBuilder { /// fallback, `$HOME`, while this text claimed otherwise: #215.) /// /// A relative `dir` is relative to the test process's working directory, - /// as with `std::process::Command`, and a relative program path such as - /// `./myapp` is then found inside `dir`, where the child runs. + /// as with `std::process::Command`, and a program named with a directory + /// part — `./myapp`, `target/debug/app` — is then found inside `dir`, + /// where the child runs; a bare name is searched for along `PATH`. That + /// is the Unix rule (#592); on Windows the pseudoconsole layer finds the + /// program by its own. /// /// [`spawn`](Self::spawn) fails with [`Error::Spawn`] if `dir` is not /// an existing directory — running somewhere else instead would make @@ -1944,10 +2000,10 @@ impl TerminalBuilder { // buffered at teardown — see docs/DESIGN.md §2 — so the window // between child start and first read must be as close to zero as // userspace can make it.) - let reader = pair - .master - .try_clone_reader() - .map_err(|e| Error::Pty(format!("cloning PTY reader failed: {e}")))?; + let reader: Box = + Box::new(MasterDup(Some(pair.master.try_clone_reader().map_err( + |e| Error::Pty(format!("cloning PTY reader failed: {e}")), + )?))); let writer = pair .master .take_writer() @@ -1985,7 +2041,8 @@ impl TerminalBuilder { // a full output buffer), and the test thread would hang with no // deadline and no diagnosis. let write_tx = match dup_writer(pair.master.as_ref()) { - Some(mut pty_writer) => { + Some(pty_writer) => { + let mut pty_writer = MasterDup(Some(pty_writer)); let (tx, rx) = mpsc::channel::(); let written = Arc::clone(&replies_pending); let drained = Arc::clone(&queued_bytes); @@ -2080,6 +2137,7 @@ impl TerminalBuilder { shared, default_timeout: self.timeout, exit_status: None, + exit_watch: ExitWatch::default(), command_desc, frame_cursor: 0, last_frame: None, @@ -2400,9 +2458,10 @@ fn reader_loop( // The emulator stops at each DEC 2026 frame end and at // each query, so state read there (screen, cursor) is // exact — even when the same chunk already carries the - // following bytes. Replies are BUILT under the state - // lock but WRITTEN after it is released; the state lock - // and the writer lock are never held together. + // following bytes. Replies are built and queued for the + // writer thread under the state lock, and written after it + // is released; the state lock and the writer lock are never + // held together. let replies = shared.mutate(|state| { // A panicked emulator is never fed again: its grid is in // whatever state the panic left it. The loop still runs, so @@ -2482,6 +2541,14 @@ fn reader_loop( } state.utf8_pending = utf8_pending; state.touch(); + // Queued before the lock is released: a waiter that sees + // this read's screen — woken by the release, or by its own + // tick — and types in response must find the replies ahead + // of its input in the writer's queue, as a terminal answers + // a query before it shows anything after it (#591). + if let Some(tx) = replies_to { + queue_replies(tx, &mut replies, state, pending, queued_bytes); + } replies }); // One queue entry per *read*, not per reply. A single read @@ -2494,49 +2561,15 @@ fn reader_loop( // millisecond between the queries. Batching is what removes // that mismatch; ordering is unchanged, since the bytes are // concatenated in the order they were built. + // No responder thread (no descriptor to duplicate): the + // replies are still here, and are written inline, as before — + // after the state lock is released, since writing needs the + // writer lock. if !replies.is_empty() { let batch: Vec = replies.concat(); - let count = replies.len(); - match replies_to { - // Still without waiting. Now that a full queue means - // 64 reads' worth of answers are already pending, it - // really does mean the application has stopped - // reading — and the drain must keep running - // regardless, or the child blocks writing into a full - // output buffer and the harness deadlocks itself. - Some(tx) => { - // Refuse only on the memory bound. The queue is - // unbounded, so this hand-off cannot block and the - // drain cannot stall — which is the constraint - // that rules out backpressuring the reader. - let size = batch.len(); - if queued_bytes.load(Ordering::Relaxed) + size > REPLY_QUEUE_BYTES { - shared.mutate(|state| state.replies_dropped += count); - } else { - // Counted as pending *before* the hand-off, so - // a wait that fails while the write is blocked - // can say how many answers are stuck. - pending.fetch_add(count, Ordering::Relaxed); - queued_bytes.fetch_add(size, Ordering::Relaxed); - let request = WriteRequest { - bytes: batch, - ack: None, // fire and forget: never wait - replies: count, - }; - if tx.send(request).is_err() { - pending.fetch_sub(count, Ordering::Relaxed); - queued_bytes.fetch_sub(size, Ordering::Relaxed); - } - } - } - // No responder thread (no descriptor to duplicate): - // write inline, as before. - None => { - let mut writer = writer.lock().unwrap_or_else(PoisonError::into_inner); - if let Some(writer) = writer.as_mut() { - let _ = writer.write_all(&batch).and_then(|()| writer.flush()); - } - } + let mut writer = writer.lock().unwrap_or_else(PoisonError::into_inner); + if let Some(writer) = writer.as_mut() { + let _ = writer.write_all(&batch).and_then(|()| writer.flush()); } } } @@ -2550,6 +2583,56 @@ fn reader_loop( }); } +/// Hand this read's replies to the writer thread, leaving `replies` empty. +/// +/// One queue entry per *read*, not per reply. A single read can carry +/// dozens of queries — a startup batch is a legitimate pattern — and +/// enqueueing each answer separately made the reader outrun the writer, +/// which issues one `write(2)` per entry. The queue then filled and answers +/// were discarded although nothing was blocked: measured at 173 of 200 +/// delivered back-to-back, and 200 of 200 with a millisecond between the +/// queries. Batching is what removes that mismatch; ordering is unchanged, +/// since the bytes are concatenated in the order they were built. +/// +/// Never waits, so it is safe under the state lock. A full queue — 64 reads' +/// worth of answers already pending — really does mean the application has +/// stopped reading, and the drain must keep running regardless, or the +/// child blocks writing into a full output buffer and the harness +/// deadlocks itself. So the only refusal is the memory bound, recorded as +/// dropped replies. +fn queue_replies( + tx: &mpsc::Sender, + replies: &mut Vec>, + state: &mut EmuState, + pending: &AtomicUsize, + queued_bytes: &AtomicUsize, +) { + if replies.is_empty() { + return; + } + let batch: Vec = replies.concat(); + let count = replies.len(); + replies.clear(); + let size = batch.len(); + if queued_bytes.load(Ordering::Relaxed) + size > REPLY_QUEUE_BYTES { + state.replies_dropped += count; + return; + } + // Counted as pending *before* the hand-off, so a wait that fails while + // the write is blocked can say how many answers are stuck. + pending.fetch_add(count, Ordering::Relaxed); + queued_bytes.fetch_add(size, Ordering::Relaxed); + let request = WriteRequest { + bytes: batch, + ack: None, // fire and forget: never wait + replies: count, + }; + if tx.send(request).is_err() { + pending.fetch_sub(count, Ordering::Relaxed); + queued_bytes.fetch_sub(size, Ordering::Relaxed); + } +} + /// A program running inside a real PTY, observed through an emulated screen. /// /// See the crate-level docs for a full example. Dropping a `Terminal` kills @@ -2567,6 +2650,9 @@ pub struct Terminal { shared: Arc>, default_timeout: Duration, exit_status: Option, + /// Where nothing else says the child is gone, when it was first seen + /// reaped — across waits, not per wait (#593). + exit_watch: ExitWatch, command_desc: String, /// Index of the newest frame [`wait_frame`](Terminal::wait_frame) has /// already returned from this terminal. Each call looks only past it, @@ -2713,7 +2799,8 @@ impl Terminal { /// released the terminal, or it stopped reading its input and the write /// gave up at the terminal's deadline rather than blocking forever. The /// error embeds the screen, so a CI log shows what the application was - /// displaying when the keystroke had nowhere to go. + /// displaying when the keystroke had nowhere to go. A write that gave + /// up is not withdrawn: [`Error::Write`] says what happens to its bytes. /// /// This is the same answer on every platform. A raw write to a closed /// terminal fails with `EIO` on macOS and *succeeds* on Linux, where @@ -3177,8 +3264,15 @@ impl Terminal { /// never reports EOF while it is open, so there the reaped child is /// the signal (see [`EXIT_CLOSES_THE_TERMINAL`]). fn ensure_deliverable(&mut self, what: &str) -> Result<()> { - if !self.shared.lock().eof { - return Ok(()); + { + let mut state = self.shared.lock(); + // Where a reaped child is the terminal closing, a send asks too: + // input long after the exit must not need a wait to be refused. + self.exit_watch + .tick(&mut self.child, &mut self.exit_status, &mut state); + if !state.eof { + return Ok(()); + } } let reason = match &self.exit_status { Some(status) => format!("the child is gone ({status}) and the terminal is closed"), @@ -3215,10 +3309,14 @@ impl Terminal { }; }; + // The write is not withdrawn: it is mid-`write(2)` on the writer + // thread, which no portable call can interrupt, and the rest of it + // follows once the application reads again (#590). let not_reading = || { format!( "the application is not reading its input, and the PTY buffer is \ - full — no progress in {:?}", + full — no progress in {:?}; what was not written stays queued and \ + reaches the application if it reads again", self.default_timeout ) }; @@ -3314,11 +3412,11 @@ impl Terminal { const WHAT: &str = "the screen predicate to hold"; let deadline = Instant::now() + timeout; let mut seen_generation = None; - let mut exit_watch = ExitWatch::default(); let outcome = self.shared.wait_until(deadline, |state| { // Ahead of the generation check: a child that exited changes // nothing on the grid, and on Windows is the only EOF there is. - exit_watch.tick(&mut self.child, &mut self.exit_status, state); + self.exit_watch + .tick(&mut self.child, &mut self.exit_status, state); // Spurious wake (poll-cap tick, unrelated notify): the state is // unchanged, so the predicate's verdict is too. if seen_generation == Some(state.generation) { @@ -3495,9 +3593,9 @@ impl Terminal { let deadline = Instant::now() + timeout; let cursor = self.frame_cursor; let mut seen_frame = None; - let mut exit_watch = ExitWatch::default(); let outcome = self.shared.wait_until(deadline, |state| { - exit_watch.tick(&mut self.child, &mut self.exit_status, state); + self.exit_watch + .tick(&mut self.child, &mut self.exit_status, state); if let Some(failure) = state.emulator_failure() { return Some(Err(failure)); } @@ -3651,10 +3749,10 @@ impl Terminal { fn wait_idle_deadline(&mut self, quiet: Duration, timeout: Duration) -> Result<()> { let deadline = Instant::now() + timeout; - let mut exit_watch = ExitWatch::default(); let mut guard = self.shared.lock(); loop { - exit_watch.tick(&mut self.child, &mut self.exit_status, &mut guard); + self.exit_watch + .tick(&mut self.child, &mut self.exit_status, &mut guard); // Ahead of the EOF shortcut: a stream that stopped because the // emulator died is not a stream that went quiet. if let Some(failure) = guard.emulator_failure() { @@ -3843,9 +3941,9 @@ impl Terminal { let mut last = guard.peek_snapshot(); let mut since = guard.last_activity; let mut seen_generation = guard.generation; - let mut exit_watch = ExitWatch::default(); loop { - exit_watch.tick(&mut self.child, &mut self.exit_status, &mut guard); + self.exit_watch + .tick(&mut self.child, &mut self.exit_status, &mut guard); // Ahead of the EOF shortcut: a grid that stopped changing // because the emulator died has not settled. if let Some(failure) = guard.emulator_failure() { @@ -3957,16 +4055,8 @@ impl Terminal { }; let pid = libc::pid_t::try_from(pid) .map_err(|_| Error::Input(format!("pid {pid} exceeds the platform's pid range")))?; - // SAFETY: kill(2) touches no memory. The pid is our own un-reaped - // child (guarded above): worst case it is a zombie, for which kill - // is defined and harmless — never an unrelated, recycled pid. - #[allow(unsafe_code)] - let rc = unsafe { libc::kill(pid, signal.raw()) }; - if rc == 0 { - Ok(()) - } else { - Err(Error::Io(io::Error::last_os_error())) - } + // Our own un-reaped child, guarded above. + send_signal(pid, signal.raw()).map_err(Error::Io) } /// Block until the child exits, then return its status. Idempotent: @@ -4005,6 +4095,7 @@ impl Terminal { if let Some(status) = self.child.try_wait().map_err(Error::Io)? { let status = ExitStatus::from_pty(&status); self.exit_status = Some(status.clone()); + self.exit_watch.reaped(); let drained = self .shared .wait_until(Instant::now() + DRAIN_GRACE, |state| { @@ -4237,39 +4328,92 @@ impl Terminal { } } +/// How long a hung-up child has to exit on its own before it is killed: +/// portable-pty's own grace, kept so a program's SIGHUP handler still runs. +#[cfg(unix)] +const HANGUP_GRACE: Duration = Duration::from_millis(200); + +/// Hang the child up, give it [`HANGUP_GRACE`] to go, then kill it — the +/// sequence portable-pty's `kill` runs, with the reap polled from 1 ms where +/// portable-pty slept in fixed 50 ms steps, so a child that dies on SIGHUP +/// at once is reaped in about a millisecond rather than fifty (#589). +/// Gives up at `deadline`, as `Drop` must terminate. +fn stop_child(child: &mut PtyChild, deadline: Instant) { + #[cfg(unix)] + if let Some(pid) = child + .process_id() + .and_then(|pid| libc::pid_t::try_from(pid).ok()) + { + // The child is not reaped: `Drop` asked just before, and only this + // thread reaps it. + let _ = send_signal(pid, libc::SIGHUP); + if reaped_by(child, (Instant::now() + HANGUP_GRACE).min(deadline)) { + return; + } + let _ = send_signal(pid, libc::SIGKILL); + reaped_by(child, deadline); + return; + } + let _ = child.kill(); + reaped_by(child, deadline); +} + +/// Poll for the child's exit until `deadline`; whether it was reaped. +fn reaped_by(child: &mut PtyChild, deadline: Instant) -> bool { + let mut backoff = INITIAL_BACKOFF; + loop { + if matches!(child.try_wait(), Ok(Some(_))) { + return true; + } + let now = Instant::now(); + if now >= deadline { + return false; + } + thread::sleep(backoff.min(deadline - now)); + backoff = next_backoff(backoff); + } +} + +/// `kill(2)`, the one call site. Every caller passes its own child, not yet +/// reaped. +#[cfg(unix)] +fn send_signal(pid: libc::pid_t, signal: libc::c_int) -> io::Result<()> { + // SAFETY: kill(2) touches no memory. The pid is the caller's own + // un-reaped child: worst case a zombie, for which kill is defined and + // harmless — never an unrelated, recycled pid. + #[allow(unsafe_code)] + let rc = unsafe { libc::kill(pid, signal) }; + if rc == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + impl Drop for Terminal { /// Kill and reap the child. No zombies, even when a test panics before /// `wait_exit`. The reader thread ends on its own at EOF and is never /// joined here — a grandchild holding the PTY open must not hang Drop. /// - /// The whole teardown (including closing the master/writer fds) runs - /// under the process-wide PTY lifecycle lock: on macOS, letting a - /// master close overlap a concurrent `openpty()` can revoke the *other* - /// terminal's freshly recycled PTY device (see `PTY_LIFECYCLE` in this module). + /// Closing the master and writer descriptors runs under the + /// process-wide PTY lifecycle lock: on macOS, letting a master close + /// overlap a concurrent `openpty()` can revoke the *other* terminal's + /// freshly recycled PTY device (see `PTY_LIFECYCLE` in this module). + /// Stopping the child comes first and outside it: this terminal's + /// master is still open then, so its device cannot be recycled under + /// anyone, and every spawn in the binary used to wait out the child's + /// shutdown (#589). /// /// The reap is bounded (`DROP_REAP_GRACE`). A child that cannot be /// reaped is a bad outcome; a test binary that never exits — which an /// unbounded `wait` here produced whenever the child was wedged — is a /// worse one, and unlike the first it cannot even be diagnosed. fn drop(&mut self) { - let _lifecycle = pty_lifecycle_guard(); - if self.exit_status.is_none() { - let already_exited = matches!(self.child.try_wait(), Ok(Some(_))); - if !already_exited { - let _ = self.child.kill(); - let deadline = Instant::now() + DROP_REAP_GRACE; - let mut backoff = INITIAL_BACKOFF; - while !matches!(self.child.try_wait(), Ok(Some(_))) { - let now = Instant::now(); - if now >= deadline { - break; - } - thread::sleep(backoff.min(deadline - now)); - backoff = next_backoff(backoff); - } - } + if self.exit_status.is_none() && !matches!(self.child.try_wait(), Ok(Some(_))) { + stop_child(&mut self.child, Instant::now() + DROP_REAP_GRACE); } - // Close the PTY fds while still holding the lock. Taking the writer + let _lifecycle = pty_lifecycle_guard(); + // Close the PTY fds while holding the lock. Taking the writer // out of the shared cell closes its fd even though the reader // thread keeps the (now empty) cell alive — and it must happen // before the master is dropped, since the reader polls the @@ -4365,6 +4509,62 @@ mod tests { )) } + /// A waiter that first sees the screen a query was drawn on finds the + /// query's reply already queued for the writer, so whatever it types in + /// response is queued behind the reply — as a terminal answers a query + /// before it shows anything that followed it (#591). The predicate here + /// runs under the state lock at every observation, which is when a + /// waiter would decide to type. The reply used to be queued after the + /// lock was released, so the window is a few instructions wide: this + /// pins the order rather than hunting the race. + #[test] + fn a_query_s_reply_is_queued_before_its_screen_can_be_seen() { + use std::sync::mpsc; + use std::time::Duration; + + let shared = Arc::new(Monitor::new(EmuState::new( + Box::new(super::Vt100Emulator::new( + 4, + 20, + 0, + crate::graphics::DEFAULT_CAPTURE, + false, + )), + Responder { + respond: true, + background: (0, 0, 0), + foreground: (255, 255, 255), + cell_size: None, + graphics: Graphics::default(), + term_name: "xterm-256color".into(), + }, + Arc::new(AtomicUsize::new(0)), + ))); + let (tx, rx) = mpsc::channel::(); + let reader_shared = Arc::clone(&shared); + let reader = std::thread::spawn(move || { + let writer: super::SharedWriter = Arc::new(std::sync::Mutex::new(None)); + reader_loop( + Box::new(std::io::Cursor::new(b"ready\x1b[6n".to_vec())), + &reader_shared, + &writer, + Some(&tx), + &AtomicUsize::new(0), + &AtomicUsize::new(0), + ); + }); + let mut state = shared.lock(); + while !state.build_snapshot().contains("ready") { + state = shared.wait_timeout(state, Duration::from_millis(50)); + } + let reply = rx + .try_recv() + .expect("the screen showed the query before its reply was queued"); + assert_eq!(reply.bytes, b"\x1b[1;6R"); + drop(state); + reader.join().expect("the reader ends at EOF"); + } + /// The reader turns an emulator panic into state every wait can report, /// instead of dying and leaving the grid frozen (#211). #[test] diff --git a/crates/termlens/tests/drain.rs b/crates/termlens/tests/drain.rs index c48c206..acae4a0 100644 --- a/crates/termlens/tests/drain.rs +++ b/crates/termlens/tests/drain.rs @@ -145,3 +145,57 @@ fn replies_still_reach_an_application_that_reads_them() -> termlens::Result<()> assert!(t.wait_exit()?.success()); Ok(()) } + +/// A send that gives up at the deadline is not withdrawn (#590). The +/// application here sleeps before it reads, so 200,000 bytes fill the +/// buffer and the send fails — and so does the next one, queued behind it. +/// Once the application reads, it receives both, in order. The error says +/// so, because "the application never got these bytes" would be false. +#[test] +#[cfg_attr( + windows, + ignore = "ConPTY buffers typed input on its own side, so a full queue is not reached the same way (#149)" +)] +fn a_send_that_gives_up_at_the_deadline_is_still_delivered() -> termlens::Result<()> { + const N: usize = 200_000; + let count = N.to_string(); + let mut t = common::spawn_emit( + // The builder's deadline is the send's; every wait below names + // its own, generous one. + Terminal::builder().timeout(Duration::from_millis(500)), + &[ + "--raw-mode", + "ready", + "NL", + "--sleep", + "2s", + "--read-count", + &count, + "x", + "NL", + "--read-count", + "1", + "y", + "NL", + "--wait", + ], + )?; + t.wait_until_for(|s| s.contains("ready"), Duration::from_secs(10))?; + for input in ["x".repeat(N), "y".to_owned()] { + match t.send_str(&input) { + Err(Error::Write { what, .. }) => assert!( + what.contains("stays queued and reaches the application if it reads again"), + "{what}" + ), + other => panic!("the queue is not moving, so the send gives up: {other:?}"), + } + } + // Both arrive, the x's first. + t.wait_until_for( + |s| s.contains(&count) && s.text().lines().nth(2) == Some("1"), + Duration::from_secs(20), + )?; + t.send(termlens::Key::Enter)?; + assert!(t.wait_exit_for(Duration::from_secs(10))?.success()); + Ok(()) +} diff --git a/crates/termlens/tests/process.rs b/crates/termlens/tests/process.rs index 17a9ad8..f2d5c0f 100644 --- a/crates/termlens/tests/process.rs +++ b/crates/termlens/tests/process.rs @@ -183,6 +183,63 @@ fn an_argument_the_kernel_refuses_is_a_spawn_error_with_its_reason() { } } +/// A program named with a directory part is a path from the working +/// directory, as `std::process::Command` and `execvp` take it. The PTY +/// layer's rules searched PATH for `sub/x` and never the directory the test +/// runs in, so `spawn("target/debug/app")` failed with "No viable +/// candidates found in PATH" — or, under `env_clear()`, could not even +/// resolve PATH (#592). +#[cfg(unix)] +#[test] +fn a_program_with_a_directory_part_is_found_from_the_working_directory() -> termlens::Result<()> { + let scratch = std::path::Path::new(env!("CARGO_TARGET_TMPDIR")) + .join(format!("dir-part-{}", std::process::id())); + std::fs::create_dir_all(scratch.join("sub"))?; + common::write_script( + &scratch.join("sub/x"), + "#!/bin/sh\necho ran from sub\nread _\n", + ); + for clear in [false, true] { + let mut builder = Terminal::builder() + .timeout(Duration::from_secs(10)) + .current_dir(&scratch); + if clear { + builder = builder.env_clear(); + } + let mut t = builder.spawn("sub/x")?; + t.wait_until(|s| s.contains("ran from sub"))?; + t.send(Key::Enter)?; + assert!(t.wait_exit()?.success(), "env_clear: {clear}"); + } + Ok(()) +} + +/// A child that ignores the hang-up is gone all the same once `drop` +/// returns: it gets the hang-up's grace, then SIGKILL. `drop` now stops the +/// child itself, outside the lifecycle lock (#589), and must still not +/// leave one behind. +#[cfg(unix)] +#[test] +fn drop_stops_a_child_that_ignores_the_hangup() -> termlens::Result<()> { + let mut t = Terminal::builder() + .timeout(Duration::from_secs(10)) + .args([ + "-c", + "trap '' HUP; echo ready; while :; do sleep 0.05; done", + ]) + .spawn("sh")?; + t.wait_until(|s| s.contains("ready"))?; + let pid = t.pid().expect("a pid").to_string(); + drop(t); + let alive = std::process::Command::new("kill") + .args(["-0", &pid]) + .stderr(std::process::Stdio::null()) + .status()? + .success(); + assert!(!alive, "pid {pid} outlived the drop"); + Ok(()) +} + /// A failure before the exec reads as std reports it, and does not blame a /// program that is fine. A NUL in a variable is refused before anything is /// forked. A working directory that cannot be entered fails in the child, @@ -492,3 +549,32 @@ fn a_panicking_child_puts_its_message_on_the_screen() -> termlens::Result<()> { assert_eq!(t.wait_exit()?.code(), Some(101)); Ok(()) } + +/// A pseudoconsole never reports EOF while it is open, so on Windows a +/// reaped child plus the drain grace is the terminal closing — measured from +/// the reap. Every wait used to restart the grace, so waits shorter than it +/// never saw the exit and a send to the dead child was not refused (#593). +#[cfg(windows)] +#[test] +fn short_waits_after_the_exit_see_the_terminal_close() -> termlens::Result<()> { + let mut t = Terminal::builder() + .timeout(Duration::from_secs(10)) + .args(["/c", "echo done"]) + .spawn("cmd")?; + t.wait_until(|s| s.contains("done"))?; + // Waits of a fifth of the grace each, for four graces in all. + for _ in 0..20 { + match t.wait_until_for(|_| false, Duration::from_millis(100)) { + Err(Error::Eof { .. }) => { + assert!( + matches!(t.send_str("x"), Err(Error::Write { .. })), + "a closed terminal refuses input" + ); + return Ok(()); + } + Err(Error::Timeout { .. }) => {} + other => panic!("only the exit can end this wait: {other:?}"), + } + } + panic!("two seconds of short waits after the exit never saw the terminal close"); +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d42e737..65ac179 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -527,9 +527,14 @@ Related, and fixed inside the library: macOS tears PTYs down with terminals one thread's teardown could revoke another thread's *freshly opened* PTY and kill its child at birth (~1/800 spawns under CI load; the same suite ran 100/100 on Linux). termlens therefore serializes every PTY -lifecycle edge — open+spawn on one side, kill+reap+close on the other — +lifecycle edge — open+spawn on one side, the close of each master +descriptor on the other, the responder threads' duplicates included — behind a process-wide lock (`PTY_LIFECYCLE` in `terminal.rs`). The lock is -held for microseconds per edge; steady-state I/O never touches it. +held for an open and a spawn, or for a close; steady-state I/O never +touches it. Stopping a child is not an edge: it happens before the lock is +taken, while the terminal's master is still open and its device cannot be +recycled. It used to be one, and portable-pty's kill sleeps in 50 ms steps, +so every spawn in a test binary waited out every drop (#589). ### Scrollback diff --git a/skills/termlens/SKILL.md b/skills/termlens/SKILL.md index e0c069d..4c7761b 100644 --- a/skills/termlens/SKILL.md +++ b/skills/termlens/SKILL.md @@ -418,7 +418,7 @@ included in `X`, and `terminal closed (EOF) while waiting for X` is | `… note: the application queried the terminal (^[[?u …) and received no answer` | the app is blocked on a probe termlens deliberately does not answer | the app needs a fallback; see [the questions termlens leaves unanswered](https://github.com/vyncint/termlens/blob/main/docs/LIMITATIONS.md#waits-frames-and-queries) | | `terminal closed (EOF) while waiting for …` | the app exited before the predicate held | check `wait_exit()` first, or the app crashed — the final screen shows why | | ``failed to send Char('x') to `…` (the child is gone (exit code 7) and the terminal is closed)`` | input was sent after the child exited | check `wait_exit()` first and inspect the screen embedded under `--- screen at the failed write ---` | -| ``failed to send … to `…` (the application is not reading its input, and the PTY buffer is full — no progress in 5s)`` | the child is alive but has stopped draining terminal input | wait for the application to draw the state that is ready for input before typing | +| ``failed to send … to `…` (the application is not reading its input, and the PTY buffer is full — no progress in 5s; …)`` | the child is alive but has stopped draining terminal input; the unwritten bytes stay queued, so do not retry the send | wait for the application to draw the state that is ready for input before typing | | `the application never emitted a DEC 2026 synchronized update` | `wait_frame` or `record().stop()` against an app without synchronized output | use `snapshot_after` / `wait_until` (rule 8) | | `--- last returned frame → live screen ---` under a `wait_frame` timeout | the app repainted, but never into the predicate | the diff shows what did change; the predicate is looking at the wrong thing | | `input not receivable: the application has not enabled mouse tracking` | `click`/`drag`/`scroll` before the app enabled the mouse | `wait_until(|s| s.mouse_mode() != MouseMode::None)` first |