diff --git a/Cargo.lock b/Cargo.lock index 95dffd3..7a8568d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4883,6 +4883,7 @@ dependencies = [ "image", "iroh", "iroh-gossip", + "libc", "opus", "pipewire", "rand 0.10.1", diff --git a/Cargo.toml b/Cargo.toml index dfd50bc..5cb4483 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -109,3 +109,10 @@ windows-sys = { version = "0.61", features = [ "Win32_System_Diagnostics_ToolHelp", "Win32_System_Threading", ] } + +# Unix-only. Used for exactly one thing: sending SIGINT to our own +# pixelpass child so it can run its cleanup before we resort to SIGKILL +# (src/core/teardown.rs). Already in the tree via alsa/cpal/tokio, so +# declaring it directly adds no new code to the build. +[target.'cfg(unix)'.dependencies] +libc = "0.2.186" diff --git a/src/core/teardown.rs b/src/core/teardown.rs index df1de40..eb659d5 100644 --- a/src/core/teardown.rs +++ b/src/core/teardown.rs @@ -48,6 +48,14 @@ const REAP_BUDGET: Duration = Duration::from_millis(250); /// Poll interval while waiting out [`REAP_BUDGET`]. const REAP_POLL: Duration = Duration::from_millis(5); +/// How long a child gets to honour the graceful stop before it is killed. +/// +/// A healthy pixelpass exits in well under this, so the normal path never +/// spends it; only a wedged child does. It is awaited inline in the core +/// command loop, so it is also how long a wedged child can delay other +/// commands — hence seconds, not tens of seconds. +const STOP_GRACE: Duration = Duration::from_secs(2); + /// The child-process operations the teardown ordering actually depends on. /// /// Deliberately narrow, and deliberately not `ExitStatus`-shaped: the ordering @@ -55,6 +63,10 @@ const REAP_POLL: Duration = Duration::from_millis(5); /// has been reaped, so the test double is a few lines instead of a fabricated /// exit status. pub(super) trait ChildProcess { + /// Ask the child to exit **gracefully**, so it can run its own cleanup. + /// Does **not** wait, and is not guaranteed to be honoured. + fn request_stop(&mut self) -> std::io::Result<()>; + /// Signal the child to die. Does **not** wait. fn start_kill(&mut self) -> std::io::Result<()>; @@ -66,6 +78,37 @@ pub(super) trait ChildProcess { } impl ChildProcess for tokio::process::Child { + /// **SIGINT, not SIGTERM.** pixelpass installs only a `tokio::signal::ctrl_c()` + /// handler (`pixelpass/src/common/signal.rs`), so SIGTERM would be the default + /// disposition — instant death, no cleanup — which is indistinguishable from + /// SIGKILL for our purposes. + /// + /// Signalling by pid is safe against pid reuse here because we have not + /// reaped this child: an exited-but-unreaped child is a zombie whose pid the + /// kernel reserves until we `wait` it, so the pid cannot name a stranger. + #[cfg(unix)] + fn request_stop(&mut self) -> std::io::Result<()> { + let Some(pid) = self.id() else { + // Already reaped — nothing to signal. + return Ok(()); + }; + // SAFETY: `kill` is async-signal-safe and takes no pointers; the pid is + // this process's own unreaped child (see above). + if unsafe { libc::kill(pid as libc::pid_t, libc::SIGINT) } == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } + } + + /// Windows has no SIGINT to send to another process without attaching to its + /// console, so the graceful request degrades to the hard kill and the + /// bounded wait below simply returns early. + #[cfg(not(unix))] + fn request_stop(&mut self) -> std::io::Result<()> { + tokio::process::Child::start_kill(self) + } + fn start_kill(&mut self) -> std::io::Result<()> { tokio::process::Child::start_kill(self) } @@ -116,16 +159,31 @@ impl ReapOnDrop { } } - /// Kill the child and wait for it to be reaped. Idempotent. + /// Stop the child gracefully if it will go, and by force if it will not. + /// Waits for it to be reaped either way. Idempotent. /// - /// The wait is the point: returning after `start_kill` would let the caller + /// Ask, then insist (design v3.4 §7.4): a pixelpass host that gets SIGINT + /// unloads its capture sink on the way out, whereas SIGKILL skips that and + /// leaks a null-sink module on every Stop Share. + /// + /// The wait is the point: returning after signalling would let the caller /// proceed to unload the AEC while the child is still running. pub(super) async fn shutdown(&mut self) { let Some(mut child) = self.child.take() else { return; }; - let _ = child.start_kill(); - child.wait_reaped().await; + let _ = child.request_stop(); + if tokio::time::timeout(STOP_GRACE, child.wait_reaped()) + .await + .is_err() + { + crate::log_msg(&format!( + "teardown: {} ignored the graceful stop; killing it", + self.label + )); + let _ = child.start_kill(); + child.wait_reaped().await; + } } } @@ -248,6 +306,7 @@ mod tests { use super::{ChildProcess, ReapOnDrop, ScreenshareTeardown}; use std::future::Future; use std::sync::{Arc, Mutex}; + use std::time::Duration; type Log = Arc>>; @@ -263,29 +322,44 @@ mod tests { entries(log).iter().position(|e| e == entry) } - /// Records the two events the ordering rules turn on. `killed` gates - /// reaping so the double cannot report a reap that never followed a kill. + /// Records the events the ordering rules turn on. Death is gated on an + /// actual signal, so the double cannot report a reap that nothing caused. struct FakeChild { log: Log, label: &'static str, + interrupted: bool, killed: bool, reaped: bool, - /// When true the child is already dead before anyone kills it — the + /// A well-behaved child exits on SIGINT. A wedged one ignores it and + /// dies only to SIGKILL. + honours_interrupt: bool, + /// When true the child is already dead before anyone signals it — the /// closed-player-window case that `sweep_exited_viewers` looks for. exited_on_its_own: bool, } impl FakeChild { + /// A well-behaved child: exits when asked. fn new(log: &Log, label: &'static str) -> Self { Self { log: log.clone(), label, + interrupted: false, killed: false, reaped: false, + honours_interrupt: true, exited_on_its_own: false, } } + /// A child that ignores the graceful stop entirely. + fn wedged(log: &Log, label: &'static str) -> Self { + Self { + honours_interrupt: false, + ..Self::new(log, label) + } + } + fn already_exited(log: &Log, label: &'static str) -> Self { Self { exited_on_its_own: true, @@ -293,6 +367,11 @@ mod tests { } } + /// Has anything actually made this child exit yet? + fn is_dead(&self) -> bool { + self.killed || self.exited_on_its_own || (self.interrupted && self.honours_interrupt) + } + fn record(&self, event: &str) { self.log .lock() @@ -309,6 +388,14 @@ mod tests { } impl ChildProcess for FakeChild { + fn request_stop(&mut self) -> std::io::Result<()> { + if !self.interrupted { + self.interrupted = true; + self.record("sigint"); + } + Ok(()) + } + fn start_kill(&mut self) -> std::io::Result<()> { if !self.killed { self.killed = true; @@ -318,16 +405,26 @@ mod tests { } fn try_reap(&mut self) -> bool { - if self.killed || self.exited_on_its_own { + if self.is_dead() { self.mark_reaped(); return true; } false } + /// Pending until something actually kills the child, so a wedged child + /// really does make the caller wait out `STOP_GRACE`. No waker is + /// registered: under `start_paused` the runtime auto-advances its clock + /// when every task is idle, which is exactly what fires the timeout. fn wait_reaped(&mut self) -> impl Future + Send { - self.mark_reaped(); - std::future::ready(()) + std::future::poll_fn(move |_cx| { + if self.is_dead() { + self.mark_reaped(); + std::task::Poll::Ready(()) + } else { + std::task::Poll::Pending + } + }) } } @@ -395,7 +492,43 @@ mod tests { assert_eq!(entries(&log), vec!["host:kill", "host:reap", "aec:unload"]); } - // --- The explicit path --- + // --- The explicit path: ask, then insist --- + + /// A healthy child must be *asked*, never killed. If Stop Share went + /// straight to SIGKILL, pixelpass would skip its own cleanup and leak a + /// null-sink module every time (design v3.4 §7.4). + #[tokio::test] + async fn a_healthy_child_is_asked_to_stop_and_never_killed() { + let log = log(); + let mut t = teardown(&log); + t.set_host(FakeChild::new(&log, "host")); + + assert!(t.stop_host().await); + + assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]); + assert!( + !entries(&log).contains(&"host:kill".to_string()), + "a child that honoured the graceful stop must not be killed: {:?}", + entries(&log) + ); + } + + /// ...but a child that ignores the request must not be able to hold the + /// session open forever: the grace is bounded and SIGKILL follows. + #[tokio::test(start_paused = true)] + async fn a_wedged_child_is_killed_once_the_grace_expires() { + let log = log(); + let mut t = teardown(&log); + t.set_host(FakeChild::wedged(&log, "host")); + + // The outer bound turns "the fallback was removed" into a failure + // rather than a hung test. Under `start_paused` no real time passes. + tokio::time::timeout(Duration::from_secs(60), t.stop_host()) + .await + .expect("a wedged child must not block teardown indefinitely"); + + assert_eq!(entries(&log), vec!["host:sigint", "host:kill", "host:reap"]); + } /// Mutation gate #3 (remove the wait after the host kill). #[tokio::test] @@ -410,8 +543,8 @@ mod tests { // Reaped by the explicit path — before the guard is anywhere near dropped. assert_eq!( entries(&log), - vec!["host:kill", "host:reap", "viewer:kill", "viewer:reap",], - "children must be killed and reaped by the explicit path" + vec!["host:sigint", "host:reap", "viewer:sigint", "viewer:reap"], + "children must be stopped and reaped by the explicit path" ); drop(t); @@ -427,9 +560,12 @@ mod tests { t.set_host(FakeChild::new(&log, "host")); t.shutdown_children().await; drop(t); - // Exactly one kill and one reap: the drop path must not re-signal a + // Exactly one stop and one reap: the drop path must not re-signal a // child the explicit path already took. - assert_eq!(entries(&log), vec!["host:kill", "host:reap", "aec:unload"]); + assert_eq!( + entries(&log), + vec!["host:sigint", "host:reap", "aec:unload"] + ); } // --- Host/viewer bookkeeping --- @@ -445,7 +581,7 @@ mod tests { assert!(t.is_sharing()); assert!(t.stop_host().await); assert!(!t.is_sharing()); - assert_eq!(entries(&log), vec!["host:kill", "host:reap"]); + assert_eq!(entries(&log), vec!["host:sigint", "host:reap"]); } #[test] @@ -475,7 +611,7 @@ mod tests { t.push_viewer("ticket-B".to_string(), FakeChild::new(&log, "b")); assert!(t.replace_viewer("ticket-A").await); - assert_eq!(entries(&log), vec!["a:kill", "a:reap"]); + assert_eq!(entries(&log), vec!["a:sigint", "a:reap"]); assert_eq!(t.viewers.len(), 1); assert_eq!(t.viewers[0].0, "ticket-B");