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.
This commit is contained in:
l0ng-ai
2026-08-23 13:32:31 +08:00
parent 32ccd6609e
commit 4440d78e99
2 changed files with 26 additions and 2 deletions
+17 -1
View File
@@ -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<String>) {
@@ -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));
}
}
+9 -1
View File
@@ -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;
}
}