From 4440d78e99ab4dbde17bf2175c38fc9210afcdeb Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 23 Aug 2026 13:32:31 +0800 Subject: [PATCH] fix(daemon): finish the poison work the condvar wait was left out of `wait_below_high_water` takes its mutex through `Locked::locked` and then unwrapped the condvar wait on the next line. `Condvar::wait_timeout` hands back the same poison the lock does, so the guard and the hole in it sat four lines apart: a thread that panicked holding that mutex would stop the PTY reader parking on it, which is the thread that pumps a pane's output. The drift guard that was supposed to prevent exactly this only looked for `.lock().unwrap()`, so it never saw the wait. It now covers both ways poison reaches a caller, and names this site when the fix is reverted. `host/server.rs` already did it the tolerant way and `control.rs` reads a poisoned wait as "not done", so this was the one site out of step. --- crates/tty7-core/src/core/threads.rs | 18 +++++++++++++++++- crates/tty7-core/src/daemon/pane.rs | 10 +++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/crates/tty7-core/src/core/threads.rs b/crates/tty7-core/src/core/threads.rs index ed546967..6ee3eecc 100644 --- a/crates/tty7-core/src/core/threads.rs +++ b/crates/tty7-core/src/core/threads.rs @@ -77,6 +77,11 @@ mod tests { /// Scoped to `daemon/` deliberately. That is the process holding every /// shell on the machine, where the cascade costs the most; a tool that /// panics takes only itself with it. + /// + /// Condvar waits count too. `Condvar::wait`/`wait_timeout` hand back the + /// same poison the lock does, and `pane.rs` had one site that took its + /// mutex through `locked()` and then unwrapped the wait on the very next + /// line — poison-tolerant and poison-fatal, four lines apart. #[test] fn the_daemon_takes_no_lock_that_dies_of_poison() { fn walk(dir: &std::path::Path, found: &mut Vec) { @@ -100,7 +105,18 @@ mod tests { let mut in_tests = false; for (n, line) in text.lines().enumerate() { in_tests |= line.contains("#[cfg(test)]"); - if !in_tests && line.contains(".lock().unwrap()") { + if in_tests { + continue; + } + // Both ways a poisoned mutex reaches a caller. Taking the + // lock was covered from the start; waiting on its condvar + // was not, and one site took the lock poison-tolerantly and + // then panicked on the same poison a line later — the guard + // and the hole in it, in four lines of code. + let dies = line.contains(".lock().unwrap()") + || line.contains("wait_timeout(") && line.contains(".unwrap()") + || line.contains(".wait(") && line.contains(".unwrap()"); + if dies { found.push(format!("{}:{}", path.display(), n + 1)); } } diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 2075203e..abaac8b9 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -675,7 +675,15 @@ impl OutputGate { if left.is_zero() { return; } - let (guard, _) = self.drained.wait_timeout(park, left).unwrap(); + // Poison-tolerant for the same reason the lock above is: this is + // the PTY reader parking on a pane's own backlog, and a thread that + // panicked holding the mutex must not stop this one reading. The + // `.locked()` a line up and an `.unwrap()` here would have been the + // guard and the hole in it. + let (guard, _) = self + .drained + .wait_timeout(park, left) + .unwrap_or_else(|e| e.into_inner()); park = guard; } }