From cfc480044f9e84f9d505040710f04f761753beb0 Mon Sep 17 00:00:00 2001
From: Mollusk <silvernode@gmail.com>
Date: Sun, 31 May 2026 15:27:07 -0400
Subject: [PATCH] fix: three robustness bugs outside the friends list
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Found in a wider bug audit of the streaming/process-management code.

- Viewer ctrl-c/SIGINT was ignored mid-stream: viewer::run raced the
  cancel token only against listener.accept(), not the bridge itself, so
  once the local player connected nothing checked it. CLI needed a second
  ctrl-c to quit and a GUI "Disconnect" only took effect via the child's 2s
  SIGKILL backstop (and the host saw the viewer ~2s longer). Now races the
  bridge against cancel, mirroring the host's handle_peer. (viewer/mod.rs)

- Wayland portal pipewire fd leaked on a capture-setup error: wayland::start
  into_raw_fd'd the fd and relied on pipeline::spawn's after_spawn hook to
  close it, but setup_audio/gst-spawn can ?-return before the hook runs,
  leaking the fd per failed attempt. Now the OwnedFd is moved into the hook,
  so it's closed whether the hook runs or (on early error) the unused closure
  is dropped. (host/wayland.rs)

- Detached players (mpv/vlc) zombied under the long-lived GUI: spawn_detached
  dropped the std Child, which has no orphan reaping, so each closed player
  left a <defunct> entry until the GUI exited. Now a detached thread wait()s
  it; the setsid'd player still survives a parent exit (init reaps it then).
  A double-fork was avoided deliberately — fork(2) + non-trivial work in this
  multithreaded process is unsound. (common/process.rs)

47 gui / 8 headless tests pass, clippy + fmt clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 src/common/process.rs | 23 ++++++++++++++++++-----
 src/host/wayland.rs   | 16 +++++++++-------
 src/viewer/mod.rs     | 13 ++++++++++++-
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/src/common/process.rs b/src/common/process.rs
index d9ed536..2bb6bb8 100644
--- a/src/common/process.rs
+++ b/src/common/process.rs
@@ -6,10 +6,19 @@ use std::process::{Command, Stdio};
 ///
 /// The child gets its own session via `setsid(2)` and null stdio, so it
 /// survives the parent exiting and doesn't take a SIGKILL cascade when
-/// pixelpass dies. The `Child` is dropped immediately — `std::process::Child::drop`
-/// does not kill the process on Unix.
+/// pixelpass dies.
+///
+/// A detached reaper thread `wait()`s the child so it doesn't linger as a
+/// `<defunct>` zombie under a long-lived parent — the `--gui` front-end launches
+/// players itself and lives for the whole session, and `std::process::Child`
+/// (unlike tokio's) has no orphan reaping, so simply dropping the handle would
+/// leak a zombie per closed player. If the parent exits while the player is
+/// still up, the reaper thread dies with it but the `setsid`'d player survives
+/// and is reaped by init. (A double-fork would also avoid the zombie, but
+/// `fork(2)` followed by non-trivial work in this multithreaded process is
+/// unsound — the reaper thread is the safe equivalent.)
 pub fn spawn_detached(prog: &str, args: &[&str]) -> io::Result<()> {
-    unsafe {
+    let child = unsafe {
         Command::new(prog)
             .args(args)
             .stdin(Stdio::null())
@@ -19,7 +28,11 @@ pub fn spawn_detached(prog: &str, args: &[&str]) -> io::Result<()> {
                 nix::unistd::setsid().ok();
                 Ok(())
             })
-            .spawn()?;
-    }
+            .spawn()?
+    };
+    std::thread::spawn(move || {
+        let mut child = child;
+        let _ = child.wait();
+    });
     Ok(())
 }
diff --git a/src/host/wayland.rs b/src/host/wayland.rs
index d41092b..f487a11 100644
--- a/src/host/wayland.rs
+++ b/src/host/wayland.rs
@@ -12,8 +12,7 @@ use ashpd::{
     },
 };
 use nix::fcntl::{FcntlArg, FdFlag, fcntl};
-use nix::unistd::close;
-use std::os::fd::{AsFd, IntoRawFd, OwnedFd, RawFd};
+use std::os::fd::{AsFd, AsRawFd, OwnedFd, RawFd};
 
 use super::pipeline::{self, CaptureHandle};
 use super::quality::EffectiveQuality;
@@ -61,11 +60,14 @@ pub async fn start(opts: &HostOpts, quality: &EffectiveQuality) -> Result<Captur
     let pw_fd: OwnedFd = proxy.open_pipe_wire_remote(&session).await?;
     tracing::info!(node_id, width = w, height = h, "portal handshake complete");
     // The fd is CLOEXEC by default; the gst child needs to inherit it across
-    // exec. We then leak it via into_raw_fd so its lifetime spans the spawn,
-    // and close the parent's copy once gst is running (the pipeline's
-    // after_spawn hook below).
+    // exec, so clear CLOEXEC. We keep the OwnedFd alive across the spawn (gst
+    // inherits its own copy at exec) by moving it into the after_spawn hook,
+    // which drops — and so closes — the parent's copy once gst is running. If
+    // pipeline::spawn errors *before* calling the hook (e.g. audio setup or the
+    // gst spawn fails), the unused closure is dropped, dropping the fd just the
+    // same — so the portal fd never leaks on the error path.
     clear_cloexec(&pw_fd)?;
-    let raw_fd: RawFd = pw_fd.into_raw_fd();
+    let raw_fd: RawFd = pw_fd.as_raw_fd();
 
     let source_args = vec![
         "pipewiresrc".to_string(),
@@ -81,7 +83,7 @@ pub async fn start(opts: &HostOpts, quality: &EffectiveQuality) -> Result<Captur
         source_args,
         move || {
             // Parent no longer needs the pipewire fd — gst inherited its own copy.
-            let _ = close(raw_fd);
+            drop(pw_fd);
         },
     )
     .await
diff --git a/src/viewer/mod.rs b/src/viewer/mod.rs
index f6fe7d8..4e9ad00 100644
--- a/src/viewer/mod.rs
+++ b/src/viewer/mod.rs
@@ -71,7 +71,18 @@ pub async fn run(ticket: EndpointTicket, opts: ViewerOpts) -> Result<()> {
             accepted = listener.accept() => {
                 let (tcp, peer) = accepted?;
                 tracing::info!(%peer, "local viewer connected");
-                crate::common::tunnel::bridge(quic_send, quic_recv, tcp).await
+                // Race the bridge against ctrl-c so a disconnect lands promptly
+                // mid-stream (mirrors the host's handle_peer). Without this, the
+                // cancel token is set but nothing checks it once the player has
+                // connected — ctrl-c is ignored until a second press, and a GUI
+                // "Disconnect" only takes effect via the child's SIGKILL backstop.
+                tokio::select! {
+                    res = crate::common::tunnel::bridge(quic_send, quic_recv, tcp) => res,
+                    _ = cancel.cancelled() => {
+                        tracing::info!("ctrl-c received during stream — disconnecting");
+                        Ok(())
+                    }
+                }
             }
             _ = cancel.cancelled() => {
                 tracing::info!("ctrl-c received before local viewer connected");