From 6eeeb3b8deca411607ba1d617cecfef6c9de3c4e Mon Sep 17 00:00:00 2001 From: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc Date: Sun, 2 Aug 2026 12:54:01 -0400 Subject: [PATCH] fix(desktop): drop PTY slave in drain test and gate unix-only test modules Two CI failures in buzz-terminal, both platform-shaped. lifecycle_tests::reader_drains_through_termination_and_reap took 10.008s on Linux and passed on macOS. The test never dropped `pair.slave`, so the master could not reach EOF: a PTY master stays readable while any process holds the slave open, and the test itself was one. Darwin ends the read when the session leader exits, which hid the retained slave on the platform the test was written on. The runtime already drops the slave (terminal_runtime.rs:441); the test now mirrors it. 10.009s -> 5.24ms. The timing assertion is also narrowed to the instant `stop` is called, so it measures child termination rather than reader teardown -- the stall was misreporting itself as a wedged child. Narrowing alone would make the test blind to a wedged reader, so `join`'s silent deadline abandon becomes an assert: with a wedged-reader mutant and the narrowed clock, the old `return` passed green in 10.07s. `--all-targets` compiles `#[cfg(test)]` modules, so a Windows check built env_fence_tests and lifecycle_tests, which drive real PTYs, `libc::kill`, and unix permission bits (E0432/E0433/E0425). Both modules are now `#[cfg(all(test, unix))]`. context_tests is pure string logic and stays portable. Co-authored-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc Signed-off-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc --- .../src-tauri/crates/buzz-terminal/src/lib.rs | 10 +++- .../buzz-terminal/src/lifecycle_tests.rs | 47 +++++++++++++++++-- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs index 58fe89982..5cf448c3a 100644 --- a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs +++ b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs @@ -19,9 +19,15 @@ pub mod units; #[cfg(test)] mod context_tests; -#[cfg(test)] +// `--all-targets` compiles `#[cfg(test)]` modules, so a Windows `cargo check` +// builds these two -- and they drive real PTYs, `libc::kill`, and unix +// permission bits, which do not exist there. Gating the *modules* rather than +// their contents keeps the unix-only shape honest: the code under test is +// itself `#[cfg(unix)]`, so a Windows build has nothing to assert against. +// `context_tests` is pure string logic and stays portable. +#[cfg(all(test, unix))] mod env_fence_tests; -#[cfg(test)] +#[cfg(all(test, unix))] mod lifecycle_tests; use alacritty_terminal::grid::Dimensions; diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs index 7f29cb5cc..3ddeadbf9 100644 --- a/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs +++ b/desktop/src-tauri/crates/buzz-terminal/src/lifecycle_tests.rs @@ -430,7 +430,17 @@ fn reader_drains_through_termination_and_reap() { let pid = child.process_id().expect("pid") as i32; let order = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); - let reader = RecordingReader::spawn(&pair, pid, order.clone()); + let stop_at: StopClock = std::sync::Arc::new(std::sync::Mutex::new(None)); + let reader = RecordingReader::spawn(&pair, pid, order.clone(), stop_at.clone()); + // Release the slave, exactly as the runtime does after spawning + // (`terminal_runtime.rs:441`). Not hygiene: a PTY master does not reach + // EOF while *any* process holds the slave open, and this test is one -- + // so with the slave retained the reader parks in `read()` forever after + // the child is reaped, and every wait on it burns its whole bound. Linux + // honours that rule strictly; Darwin ends the read when the session + // leader exits, so the retained slave was invisible on the platform this + // was written on and failed only in CI. + drop(pair.slave); // Establish that this is a live draining reader, not a quiet fixture. assert!( @@ -441,7 +451,15 @@ fn reader_drains_through_termination_and_reap() { let started = Instant::now(); let outcome = shutdown_draining(&mut child, Box::new(reader)).expect("shutdown"); - let elapsed = started.elapsed(); + // `stop` runs the instant `shutdown` returns, so this is the child's half + // of the window and nothing else. Timing the whole call would fold reader + // teardown into an assertion whose message is about child termination -- + // which is exactly how a stalled reader once read as a wedged child. + let elapsed = stop_at + .lock() + .unwrap() + .expect("stop was never called") + .duration_since(started); assert_eq!( outcome, @@ -479,13 +497,19 @@ struct RecordingReader { total: std::sync::Arc, handle: std::thread::JoinHandle<()>, order: std::sync::Arc>>, + /// When `stop` was called -- i.e. the instant `shutdown` returned. + stop_at: StopClock, } +/// Shared slot for the instant the reader was asked to stop. +type StopClock = std::sync::Arc>>; + impl RecordingReader { fn spawn( pair: &PtyPair, pid: i32, order: std::sync::Arc>>, + stop_at: StopClock, ) -> Self { let mut reader = pair.master.try_clone_reader().expect("reader"); let total = std::sync::Arc::new(std::sync::atomic::AtomicU64::new(0)); @@ -505,6 +529,7 @@ impl RecordingReader { total, handle, order, + stop_at, } } @@ -519,6 +544,7 @@ impl DrainingReader for RecordingReader { } fn stop(&self) { + *self.stop_at.lock().unwrap() = Some(Instant::now()); assert!( !pid_alive(self.pid), "reader stop must not be requested before the child is reaped" @@ -536,9 +562,20 @@ impl DrainingReader for RecordingReader { // indistinguishable from a broken test. Waiting to a deadline and // abandoning the thread converts the hang into an assertion failure // the harness can report. - if !poll_until(|| self.handle.is_finished()) { - return; - } + // + // The deadline must *assert*, not return. A silent abandon is + // indistinguishable from a clean join, and that is not hypothetical: + // it is how a 10 s stall in this fixture masqueraded as a + // child-termination failure in the caller's timing assertion. The + // caller's clock covers `shutdown()` only, so this is the sole gate + // on reader teardown -- with a wedged reader, `shutdown()` still + // returns in ~58 ms and every other assertion here passes. + assert!( + poll_until(|| self.handle.is_finished()), + "reader thread never finished within {BOUND:?} after the child was \ + reaped: the master never reached EOF, so output was not being \ + drained through termination" + ); let _ = self.handle.join(); } }