core/teardown: ask pixelpass to stop before killing it

The peerspeak half of phase 0c (design v3.4 §7.4 item 1). Stop Share and
session teardown both went straight to `Child::kill()`, i.e. SIGKILL, which
skips pixelpass's own cleanup and leaks one null-sink module every time.

`ReapOnDrop::shutdown` now asks first: SIGINT, a bounded 2 s wait, then SIGKILL
only if the child ignored the request. SIGINT specifically, not SIGTERM —
pixelpass installs only a `ctrl_c()` handler, so SIGTERM would take the default
disposition and be indistinguishable from SIGKILL.

Signalling by pid is safe against pid reuse here: we have not reaped the child,
so it is a zombie whose pid the kernel reserves until we wait it, and the pid
cannot name a stranger. (Same reasoning that dismissed pixelpass bug #6.)

The grace is 2 s because it is awaited inline in the core command loop, so it
is also how long a wedged child can delay other commands. A healthy pixelpass
never spends it.

The drop/unwind path deliberately stays a hard kill: `Drop` cannot await, and
there the ordering invariant (§7.1) outranks tidiness. Once the pixelpass half
of 0c lands, the capture sink is connection-owned and that path stops leaking
by construction.

`libc` becomes a direct unix-only dependency, pinned to 0.2.186 — the version
already in the tree via alsa/cpal/tokio — so Cargo.lock gains one line and no
new code enters the build.

Mutation-verified, five mutations, each killing its own gate: no wait (6 fail),
reversed field order (2, reap test green), no reap loop (2, ordering test
green), no SIGINT (6), no SIGKILL fallback (exactly 1 — the wedged-child test).
633 lib tests, clippy clean, fmt clean.

Not yet field-tested: the live Stop Share gate (SIGINT sent, child exits within
the bound, no fallback kill on the normal path) still owes a real run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Cargo.lock

@@ -4883,6 +4883,7 @@ dependencies = [
  "image",
  "iroh",
  "iroh-gossip",
+ "libc",
  "opus",
  "pipewire",
  "rand 0.10.1",

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"

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<C: ChildProcess> ReapOnDrop<C> {
         }
     }
 
-    /// 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<Mutex<Vec<String>>>;
 
@@ -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<Output = ()> + 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");