diff --git a/crates/openshell-supervisor-process/src/managed_children.rs b/crates/openshell-supervisor-process/src/managed_children.rs index df8b58c748..95c4deaa66 100644 --- a/crates/openshell-supervisor-process/src/managed_children.rs +++ b/crates/openshell-supervisor-process/src/managed_children.rs @@ -8,48 +8,176 @@ //! the orchestrator's `SIGCHLD` reaper can distinguish supervised processes //! from incidental zombies. -#![cfg(target_os = "linux")] - +#[cfg(target_os = "linux")] use std::collections::HashSet; +#[cfg(target_os = "linux")] use std::sync::{LazyLock, Mutex, MutexGuard, PoisonError}; +#[cfg(target_os = "linux")] static MANAGED_CHILDREN: LazyLock>> = LazyLock::new(|| Mutex::new(HashSet::new())); +#[cfg(target_os = "linux")] fn lock_children() -> MutexGuard<'static, HashSet> { MANAGED_CHILDREN .lock() .unwrap_or_else(PoisonError::into_inner) } -fn insert(children: &mut HashSet, pid: Option) { - let Some(pid) = pid.and_then(|pid| i32::try_from(pid).ok()) else { - return; - }; - if pid > 0 { - children.insert(pid); - } +fn valid_pid(pid: Option) -> Option { + let pid = pid.and_then(|pid| i32::try_from(pid).ok())?; + (pid > 0).then(|| u32::try_from(pid).expect("positive i32 fits in u32")) +} + +#[cfg(target_os = "linux")] +fn insert(children: &mut HashSet, pid: Option) -> Option { + let pid = valid_pid(pid)?; + children.insert(i32::try_from(pid).expect("validated PID fits in i32")); + Some(pid) } -/// Spawn a supervised child and register its PID before the orphan reaper can -/// inspect it. +/// A child whose exit status is owned by the supervisor. /// -/// The registry lock intentionally spans `spawn`. Otherwise a fast child can -/// exit and be reaped after `spawn` returns but before its PID is registered, -/// causing the child's explicit waiter to fail with `ECHILD`. -pub fn spawn_registered( - spawn: impl FnOnce() -> Result, - pid: impl FnOnce(&T) -> Option, -) -> Result { - let mut children = lock_children(); - let child = spawn()?; - insert(&mut children, pid(&child)); - Ok(child) +/// The wrapper keeps lifecycle ownership inside the supervisor. On Linux, +/// construction holds the registry lock across the real spawn and PID +/// insertion, so the orphan reaper cannot consume a fast child's status +/// between those operations. Other platforms retain the same API without +/// reaper bookkeeping. +pub struct ManagedChild { + child: C, + pid: Option, +} + +impl ManagedChild { + /// Spawn a supervised child and register its PID before returning it. + pub fn spawn( + spawn: impl FnOnce() -> Result, + pid: impl FnOnce(&C) -> Option, + ) -> Result { + #[cfg(target_os = "linux")] + { + let mut children = lock_children(); + let child = spawn()?; + let pid = insert(&mut children, pid(&child)); + Ok(Self { child, pid }) + } + + #[cfg(not(target_os = "linux"))] + { + let child = spawn()?; + let pid = valid_pid(pid(&child)); + Ok(Self { child, pid }) + } + } + + /// Return the child's PID when it was available at spawn time. + #[must_use] + pub const fn id(&self) -> Option { + self.pid + } + + fn unregister(&mut self) { + #[cfg(target_os = "linux")] + if let Some(pid) = self.pid.take() { + unregister(pid); + } + + #[cfg(not(target_os = "linux"))] + let _ = self.pid.take(); + } +} + +impl Drop for ManagedChild { + fn drop(&mut self) { + self.unregister(); + } +} + +fn log_wait_error(pid: Option, error: &std::io::Error) { + if error.raw_os_error() == Some(libc::ECHILD) { + tracing::error!( + pid = ?pid, + error = %error, + "managed child status was reaped before its explicit waiter" + ); + } +} + +impl ManagedChild { + /// Wait for a Tokio child and release its managed PID. + pub async fn wait(&mut self) -> std::io::Result { + let status = self.child.wait().await; + if let Err(error) = &status { + log_wait_error(self.pid, error); + } + self.unregister(); + status + } + + /// Observe a Tokio child without blocking. + pub fn try_wait(&mut self) -> std::io::Result> { + match self.child.try_wait() { + Ok(status) => { + if status.is_some() { + self.unregister(); + } + Ok(status) + } + Err(error) => { + log_wait_error(self.pid, &error); + self.unregister(); + Err(error) + } + } + } + + /// Take the child's stdin handle without exposing the child itself. + pub fn take_stdin(&mut self) -> Option { + self.child.stdin.take() + } + + /// Take the child's stdout handle without exposing the child itself. + pub fn take_stdout(&mut self) -> Option { + self.child.stdout.take() + } + + /// Take the child's stderr handle without exposing the child itself. + pub fn take_stderr(&mut self) -> Option { + self.child.stderr.take() + } +} + +impl ManagedChild { + /// Wait for a standard-library child and release its managed PID. + pub fn wait(&mut self) -> std::io::Result { + let status = self.child.wait(); + if let Err(error) = &status { + log_wait_error(self.pid, error); + } + self.unregister(); + status + } + + /// Take the child's stdin handle without exposing the child itself. + pub fn take_stdin(&mut self) -> Option { + self.child.stdin.take() + } + + /// Take the child's stdout handle without exposing the child itself. + pub fn take_stdout(&mut self) -> Option { + self.child.stdout.take() + } + + /// Take the child's stderr handle without exposing the child itself. + pub fn take_stderr(&mut self) -> Option { + self.child.stderr.take() + } } /// Remove `pid` from the supervised-child set. Non-positive or out-of-range /// values are silently ignored. -pub fn unregister(pid: u32) { +#[cfg(target_os = "linux")] +fn unregister(pid: u32) { let Ok(pid) = i32::try_from(pid) else { return; }; @@ -63,6 +191,7 @@ pub fn unregister(pid: u32) { /// /// The registry lock remains held while `reap` runs so child creation cannot /// open a spawn-to-registration window between the membership check and reap. +#[cfg(target_os = "linux")] pub fn reap_if_unmanaged(pid: i32, reap: impl FnOnce() -> T) -> Option { let children = lock_children(); if children.contains(&pid) { @@ -72,28 +201,33 @@ pub fn reap_if_unmanaged(pid: i32, reap: impl FnOnce() -> T) -> Option { } } -#[cfg(test)] +#[cfg(all(test, target_os = "linux"))] mod tests { - use super::{reap_if_unmanaged, spawn_registered, unregister}; + use super::{ManagedChild, reap_if_unmanaged}; use nix::sys::wait::{Id, WaitPidFlag, WaitStatus, waitid, waitpid}; use nix::unistd::Pid; - use std::process::Command; + use std::process::{Command, Stdio}; use std::sync::mpsc; use std::time::Duration; + use tokio::process::Command as TokioCommand; #[test] fn spawned_child_is_registered_before_returning() { let pid = 1_000_000_u32; - let child = spawn_registered(|| Ok::<_, ()>(pid), |pid| Some(*pid)).unwrap(); + let child = ManagedChild::spawn(|| Ok::<_, ()>(pid), |pid| Some(*pid)).unwrap(); let mut reaped = false; - let result = reap_if_unmanaged(i32::try_from(child).unwrap(), || { + let result = reap_if_unmanaged(i32::try_from(child.id().unwrap()).unwrap(), || { reaped = true; }); assert!(result.is_none()); assert!(!reaped); - unregister(pid); + drop(child); + assert_eq!( + reap_if_unmanaged(i32::try_from(pid).unwrap(), || "reaped"), + Some("reaped") + ); } #[test] @@ -108,7 +242,7 @@ mod tests { let (reap_result_tx, reap_result_rx) = mpsc::channel(); let spawn = std::thread::spawn(move || { - spawn_registered( + ManagedChild::spawn( || { spawn_entered_tx.send(()).unwrap(); complete_spawn_rx.recv().unwrap(); @@ -116,7 +250,7 @@ mod tests { }, |child| Some(*child), ) - .unwrap(); + .unwrap() }); spawn_entered_rx.recv().unwrap(); @@ -135,10 +269,10 @@ mod tests { ); complete_spawn_tx.send(()).unwrap(); - spawn.join().unwrap(); + let child = spawn.join().unwrap(); assert_eq!(reap_result_rx.recv().unwrap(), None); reaper.join().unwrap(); - unregister(pid); + drop(child); } #[test] @@ -150,13 +284,12 @@ mod tests { #[test] fn fast_child_remains_waitable_after_orphan_reap_attempt() { - let mut child = spawn_registered( + let mut child = ManagedChild::spawn( || Command::new("sh").args(["-c", "exit 11"]).spawn(), |child| Some(child.id()), ) .expect("spawn and register fast child"); - let child_pid = child.id(); - let pid = Pid::from_raw(i32::try_from(child_pid).unwrap()); + let pid = Pid::from_raw(i32::try_from(child.id().unwrap()).unwrap()); // Observe the completed child without consuming its status, exactly // as the orphan reaper does before its managed-PID check. @@ -178,10 +311,95 @@ mod tests { ); let wait = child.wait(); - unregister(child_pid); assert!( wait.is_ok(), "the explicit child waiter must retain the exit status" ); + assert_eq!( + reap_if_unmanaged(pid.as_raw(), || "reaped"), + Some("reaped"), + "a completed child must no longer block orphan reaping" + ); + } + + #[tokio::test] + async fn tokio_try_wait_retains_then_releases_management() { + let mut child = ManagedChild::spawn( + || { + let mut command = TokioCommand::new("sh"); + command.args(["-c", "read -r _"]).stdin(Stdio::piped()); + command.spawn() + }, + |child| child.id(), + ) + .expect("spawn managed Tokio child"); + let pid = i32::try_from(child.id().unwrap()).unwrap(); + let stdin = child.take_stdin().expect("stdin must be piped"); + + assert!(child.try_wait().expect("observe Tokio child").is_none()); + assert_eq!( + reap_if_unmanaged(pid, || "reaped"), + None, + "a running child must remain managed" + ); + + drop(stdin); + child.wait().await.expect("wait for Tokio child"); + assert_eq!( + reap_if_unmanaged(pid, || "reaped"), + Some("reaped"), + "a waited child must release its managed PID" + ); + } +} + +#[cfg(test)] +mod cross_platform_tests { + use super::ManagedChild; + use std::process::{Command, Stdio}; + + #[test] + fn managed_child_retains_valid_pid_on_every_platform() { + let child = ManagedChild::spawn(|| Ok::<_, ()>(1_000_003_u32), |pid| Some(*pid)) + .expect("construct managed child"); + + assert_eq!(child.id(), Some(1_000_003)); + } + + #[test] + fn standard_child_waits_through_managed_wrapper() { + let executable = std::env::current_exe().expect("current test executable"); + let mut child = ManagedChild::spawn( + || { + Command::new(executable) + .arg("--help") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + }, + |child| Some(child.id()), + ) + .expect("spawn test child"); + + assert!(child.wait().expect("wait for test child").success()); + } + + #[tokio::test] + async fn tokio_child_waits_through_managed_wrapper() { + let executable = std::env::current_exe().expect("current test executable"); + let mut child = ManagedChild::spawn( + || { + let mut command = tokio::process::Command::new(executable); + command + .arg("--help") + .stdout(Stdio::null()) + .stderr(Stdio::null()); + command.spawn() + }, + tokio::process::Child::id, + ) + .expect("spawn test child"); + + assert!(child.wait().await.expect("wait for test child").success()); } } diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 4bb81581dd..41cc1d160d 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -4,7 +4,6 @@ //! Process management and signal handling. use crate::child_env; -#[cfg(target_os = "linux")] use crate::managed_children; #[cfg(target_os = "linux")] use crate::netns::NetworkNamespace; @@ -450,9 +449,7 @@ pub fn supervisor_identity_mount_from_env() -> Result std::io::Result { +fn spawn_command_with_supervisor_identity_namespace(mut cmd: Command) -> std::io::Result { let namespace = supervisor_identity_mount_from_env() .map_err(|err| std::io::Error::other(err.to_string()))?; let Some(namespace) = namespace else { @@ -462,7 +459,7 @@ pub fn spawn_command_with_supervisor_identity_namespace( } #[cfg(target_os = "linux")] -pub fn spawn_std_command_with_supervisor_identity_namespace( +fn spawn_std_command_with_supervisor_identity_namespace( mut cmd: std::process::Command, ) -> std::io::Result { let namespace = supervisor_identity_mount_from_env() @@ -473,6 +470,25 @@ pub fn spawn_std_command_with_supervisor_identity_namespace( namespace.spawn_std_command(cmd) } +/// Spawn a standard child through the supervisor identity namespace when one +/// is active, while retaining managed lifecycle ownership on every platform. +pub(crate) fn spawn_managed_std_command( + mut cmd: std::process::Command, +) -> std::io::Result> { + #[cfg(target_os = "linux")] + { + managed_children::ManagedChild::spawn( + || spawn_std_command_with_supervisor_identity_namespace(cmd), + |child| Some(child.id()), + ) + } + + #[cfg(not(target_os = "linux"))] + { + managed_children::ManagedChild::spawn(|| cmd.spawn(), |child| Some(child.id())) + } +} + #[cfg(target_os = "linux")] impl SupervisorIdentityMountNamespace { fn spawn_tokio_command(&self, mut cmd: Command) -> std::io::Result { @@ -653,7 +669,7 @@ fn mount_empty_tmpfs(target: &CString) -> std::io::Result<()> { /// Handle to a running process. pub struct ProcessHandle { - child: Child, + child: managed_children::ManagedChild, pid: u32, io: Option, } @@ -900,29 +916,21 @@ impl ProcessHandle { } } - #[cfg(target_os = "linux")] - let mut child = managed_children::spawn_registered( + let mut child = managed_children::ManagedChild::spawn( || spawn_command_with_supervisor_identity_namespace(cmd), Child::id, ) .into_diagnostic() .wrap_err("failed to spawn sandbox entrypoint process")?; - #[cfg(not(target_os = "linux"))] - let mut child = cmd - .spawn() - .into_diagnostic() - .wrap_err("failed to spawn sandbox entrypoint process")?; let pid = child.id().unwrap_or(0); - let io = if let Some(master) = pty_master { - ProcessIo::Pty(master) - } else { - ProcessIo::Pipes { - stdin: child.stdin.take().expect("canonical stdin must be piped"), - stdout: child.stdout.take().expect("canonical stdout must be piped"), - stderr: child.stderr.take().expect("canonical stderr must be piped"), - } - }; - + let io = pty_master.map_or_else( + || ProcessIo::Pipes { + stdin: child.take_stdin().expect("canonical stdin must be piped"), + stdout: child.take_stdout().expect("canonical stdout must be piped"), + stderr: child.take_stderr().expect("canonical stderr must be piped"), + }, + ProcessIo::Pty, + ); debug!(pid, program, "Process spawned"); Ok(Self { @@ -1054,20 +1062,20 @@ impl ProcessHandle { } } - let mut child = cmd.spawn().into_diagnostic()?; + let mut child = + managed_children::ManagedChild::spawn(|| cmd.spawn(), Child::id).into_diagnostic()?; let pid = child.id().unwrap_or(0); debug!(pid, program, "Process spawned"); - let io = if let Some(master) = pty_master { - ProcessIo::Pty(master) - } else { - ProcessIo::Pipes { - stdin: child.stdin.take().expect("canonical stdin must be piped"), - stdout: child.stdout.take().expect("canonical stdout must be piped"), - stderr: child.stderr.take().expect("canonical stderr must be piped"), - } - }; + let io = pty_master.map_or_else( + || ProcessIo::Pipes { + stdin: child.take_stdin().expect("canonical stdin must be piped"), + stdout: child.take_stdout().expect("canonical stdout must be piped"), + stderr: child.take_stderr().expect("canonical stderr must be piped"), + }, + ProcessIo::Pty, + ); Ok(Self { child, @@ -1094,8 +1102,6 @@ impl ProcessHandle { /// Returns an error if waiting fails. pub async fn wait(&mut self) -> std::io::Result { let status = self.child.wait().await; - #[cfg(target_os = "linux")] - managed_children::unregister(self.pid); let status = status?; Ok(ProcessStatus::from(status)) } @@ -1103,10 +1109,6 @@ impl ProcessHandle { /// Observe an already-terminated child without blocking. pub fn try_wait(&mut self) -> std::io::Result> { let status = self.child.try_wait()?; - if status.is_some() { - #[cfg(target_os = "linux")] - managed_children::unregister(self.pid); - } Ok(status.map(ProcessStatus::from)) } @@ -1152,13 +1154,6 @@ impl ProcessHandle { } } -impl Drop for ProcessHandle { - fn drop(&mut self) { - #[cfg(target_os = "linux")] - managed_children::unregister(self.pid); - } -} - /// Validate the configured process user. /// /// Numeric identities do not require a passwd entry. The legacy explicit diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 4e32f34777..b0b67b0c53 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -5,8 +5,6 @@ use crate::child_env; use crate::main_session::{MainOutput, MainSession}; -#[cfg(target_os = "linux")] -use crate::managed_children; use crate::process::{ ProcessEnforcementMode, ResolvedProcessIdentity, ResolvedWorkspace, drop_privileges_with_identity, is_supervisor_only_env_var, session_user_and_home, @@ -1341,15 +1339,7 @@ fn spawn_pty_shell( ); } - #[cfg(target_os = "linux")] - let mut child = managed_children::spawn_registered( - || crate::process::spawn_std_command_with_supervisor_identity_namespace(cmd), - |child| Some(child.id()), - )?; - #[cfg(not(target_os = "linux"))] - let mut child = cmd.spawn()?; - #[cfg(target_os = "linux")] - let child_pid = child.id(); + let mut child = crate::process::spawn_managed_std_command(cmd)?; let master_file = master; let (sender, receiver) = mpsc::channel::>(); @@ -1394,8 +1384,6 @@ fn spawn_pty_shell( let runtime_exit = runtime; std::thread::spawn(move || { let status = child.wait().ok(); - #[cfg(target_os = "linux")] - managed_children::unregister(child_pid); let code = status.and_then(|s| s.code()).unwrap_or(1).unsigned_abs(); // Wait for the reader thread to finish forwarding all output before // sending exit-status and closing the channel. This prevents the @@ -1501,18 +1489,10 @@ fn spawn_pipe_exec( ); } - #[cfg(target_os = "linux")] - let mut child = managed_children::spawn_registered( - || crate::process::spawn_std_command_with_supervisor_identity_namespace(cmd), - |child| Some(child.id()), - )?; - #[cfg(not(target_os = "linux"))] - let mut child = cmd.spawn()?; - #[cfg(target_os = "linux")] - let child_pid = child.id(); - let child_stdin = child.stdin.take(); - let child_stdout = child.stdout.take().expect("stdout must be piped"); - let child_stderr = child.stderr.take().expect("stderr must be piped"); + let mut child = crate::process::spawn_managed_std_command(cmd)?; + let child_stdin = child.take_stdin(); + let child_stdout = child.take_stdout().expect("stdout must be piped"); + let child_stderr = child.take_stderr().expect("stderr must be piped"); // stdin writer thread let (sender, receiver) = mpsc::channel::>(); @@ -1579,8 +1559,6 @@ fn spawn_pipe_exec( let runtime_exit = runtime; std::thread::spawn(move || { let status = child.wait().ok(); - #[cfg(target_os = "linux")] - managed_children::unregister(child_pid); let code = status.and_then(|s| s.code()).unwrap_or(1).unsigned_abs(); // Wait for both reader threads. let _ = reader_done_rx.recv_timeout(Duration::from_secs(2));