From d0b93dd41f53134b7d5573d44f9f47f222dca1d1 Mon Sep 17 00:00:00 2001 From: Tommy McCormick Date: Mon, 27 Apr 2026 07:24:59 -0400 Subject: [PATCH 1/2] fix(cells): handle exit codes and prevent process orphaning on stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop on a tracked executable previously left grandchildren alive on the host. tokio::process::Child::kill signals only the leader PID — even when process_group(0) was set on the Command, the spawned PGID's members are not signaled. The fix sets each executable's process group to its own PGID via process_group(0), then signals the entire group with killpg(SIGKILL) on stop. Adds: - Executable::start: process_group(0) so the spawned child is its own PGID leader. Captures the leader PID at spawn so killpg targets the right group even after Tokio reaps the child internally. - Executable::kill: replaces child.kill() with killpg(SIGKILL) on the captured PGID. Always reaps the child and joins the stdout/stderr reader tasks even if killpg fails, surfacing the killpg error after cleanup. - Executable::pid: now infallible (Option); pid is read from the captured field, not from a possibly-reaped Child. - NestedAuraed::kill: tolerates ESRCH from nix::sys::signal::kill (process already gone) so cell teardown is idempotent. - Executables::stop: distinguishes 'never inserted' (ExecutableNotFound) from 'process already exited' (new ExecutableAlreadyExited variant) via ESRCH/ECHILD classification on the io::Error. Cache is evicted in both cases. Other errors still propagate as FailedToStopExecutable. - Executables::broadcast_stop: logs kill failures instead of dropping them silently. - ExecutablesError::ExecutableAlreadyExited: new variant; mapped to Status::not_found in CellsServiceError -> Status. - CellService::stop: reads pid from the infallible pid() and decouples observe-service channel cleanup from the stop result so channels are unregistered even when kill fails. Translates ExecutableNotFound and ExecutableAlreadyExited to an Ok response for idempotency. Tests (auraed/tests/cell_start_stop_delete.rs): - cells_start_stop_delete: happy-path allocate / start / stop / free. - cells_stop_kills_entire_process_group: regression test for https://github.com/aurae-runtime/aurae/issues/534. Spawns a bash wrapper that forks two background sleeps; after stop, every PID in the leader's PGID must be gone within 3s. - cells_double_stop_is_idempotent: pins that a second stop returns Ok. - cells_stop_after_natural_exit_is_ok: stop on a process that has already exited on its own returns Ok (drives the ESRCH/ECHILD classification path). Unit tests (executables.rs): - start_should_cache_pid_and_reject_duplicates - stop_after_natural_exit_returns_ok_and_evicts - stop_unknown_name_returns_not_found --- auraed/src/cells/cell_service/cell_service.rs | 38 +- .../cells/nested_auraed/nested_auraed.rs | 7 +- auraed/src/cells/cell_service/error.rs | 3 +- .../cells/cell_service/executables/error.rs | 2 + .../cell_service/executables/executable.rs | 39 +- .../cell_service/executables/executables.rs | 145 ++++++-- auraed/tests/cell_start_stop_delete.rs | 332 ++++++++++++++++++ auraed/tests/common/cells.rs | 12 +- 8 files changed, 520 insertions(+), 58 deletions(-) create mode 100644 auraed/tests/cell_start_stop_delete.rs diff --git a/auraed/src/cells/cell_service/cell_service.rs b/auraed/src/cells/cell_service/cell_service.rs index 23105c402..96a16de6a 100644 --- a/auraed/src/cells/cell_service/cell_service.rs +++ b/auraed/src/cells/cell_service/cell_service.rs @@ -39,9 +39,7 @@ use proto::{ }, observe::LogChannelType, }; -use std::os::unix::fs::MetadataExt; -use std::time::Duration; -use std::{process::ExitStatus, sync::Arc}; +use std::{os::unix::fs::MetadataExt, sync::Arc, time::Duration}; use tokio::sync::Mutex; use tonic::{Code, Request, Response, Status}; use tracing::{info, instrument, trace, warn}; @@ -223,8 +221,7 @@ impl CellService { // Retrieve the process ID (PID) of the started executable let pid = executable .pid() - .map_err(CellsServiceError::Io)? - .expect("pid") + .expect("started executable has captured pid") .as_raw(); // Register the stdout log channel for the executable's PID @@ -290,28 +287,24 @@ impl CellService { assert!(cell_name.is_none()); info!("CellService: stop() executable_name={:?}", executable_name,); - let pid = { + let (pid, stop_result) = { let mut executables = self.executables.lock().await; - // Retrieve the process ID (PID) of the executable to be stopped + // pid is captured at spawn (see Executable::start), so it is + // available for cache entries regardless of whether Tokio has + // already reaped the leader. let pid = executables .get(&executable_name) .map_err(CellsServiceError::ExecutablesError)? .pid() - .map_err(CellsServiceError::Io)? - .expect("pid") + .expect("started executable has captured pid") .as_raw(); - // Stop the executable and handle any errors - let _: ExitStatus = executables - .stop(&executable_name) - .await - .map_err(CellsServiceError::ExecutablesError)?; + let result = executables.stop(&executable_name).await; - pid + (pid, result) }; - // Remove the executable's logs from the observe service. if let Err(e) = self .observe_service .unregister_sub_process_channel(pid, LogChannelType::Stdout) @@ -327,7 +320,18 @@ impl CellService { warn!("failed to unregister stderr channel for pid {pid}: {e}"); } - Ok(Response::new(CellServiceStopResponse::default())) + use super::executables::ExecutablesError; + match stop_result { + Ok(_) + | Err(ExecutablesError::ExecutableNotFound { .. }) + | Err(ExecutablesError::ExecutableAlreadyExited { .. }) => { + Ok(Response::new(CellServiceStopResponse::default())) + } + Err(e) => Err(Status::internal(format!( + "executable '{}' failed to stop: {}", + executable_name, e + ))), + } } #[tracing::instrument(skip(self))] diff --git a/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs b/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs index 0d382936d..1c591c8eb 100644 --- a/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs +++ b/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs @@ -189,8 +189,11 @@ impl NestedAuraed { ) -> io::Result<()> { let signal = signal.into(); let pid = Pid::from_raw(self.process.pid); - nix::sys::signal::kill(pid, signal)?; - Ok(()) + + match nix::sys::signal::kill(pid, signal) { + Ok(()) | Err(Errno::ESRCH) => Ok(()), + Err(e) => Err(io::Error::from(e)), + } } fn wait(&mut self) -> io::Result { diff --git a/auraed/src/cells/cell_service/error.rs b/auraed/src/cells/cell_service/error.rs index 0e6676f5a..276a820c1 100644 --- a/auraed/src/cells/cell_service/error.rs +++ b/auraed/src/cells/cell_service/error.rs @@ -63,7 +63,8 @@ impl From for Status { ExecutablesError::ExecutableExists { .. } => { Status::already_exists(msg) } - ExecutablesError::ExecutableNotFound { .. } => { + ExecutablesError::ExecutableNotFound { .. } + | ExecutablesError::ExecutableAlreadyExited { .. } => { Status::not_found(msg) } ExecutablesError::FailedToStartExecutable { .. } diff --git a/auraed/src/cells/cell_service/executables/error.rs b/auraed/src/cells/cell_service/executables/error.rs index 85fafa452..6420a53e4 100644 --- a/auraed/src/cells/cell_service/executables/error.rs +++ b/auraed/src/cells/cell_service/executables/error.rs @@ -25,6 +25,8 @@ pub enum ExecutablesError { ExecutableExists { executable_name: ExecutableName }, #[error("executable '{executable_name}' not found")] ExecutableNotFound { executable_name: ExecutableName }, + #[error("executable '{executable_name}' had already exited before stop")] + ExecutableAlreadyExited { executable_name: ExecutableName }, #[error("executable '{executable_name}' failed to start: {source}")] FailedToStartExecutable { executable_name: ExecutableName, diff --git a/auraed/src/cells/cell_service/executables/executable.rs b/auraed/src/cells/cell_service/executables/executable.rs index 7a6aa4c43..c4fbd4d52 100644 --- a/auraed/src/cells/cell_service/executables/executable.rs +++ b/auraed/src/cells/cell_service/executables/executable.rs @@ -14,6 +14,7 @@ \* -------------------------------------------------------------------------- */ use super::{ExecutableName, ExecutableSpec}; use crate::logging::log_channel::LogChannel; +use nix::sys::signal::{Signal, killpg}; use nix::unistd::Pid; use std::{ ffi::OsString, @@ -34,6 +35,9 @@ pub struct Executable { pub stdout: LogChannel, pub stderr: LogChannel, state: ExecutableState, + // Captured at spawn so killpg targets the right group even after Tokio + // internally reaps the leader (which clears `Child::id()`). + pid: Option, } #[derive(Debug)] @@ -59,7 +63,7 @@ impl Executable { let state = ExecutableState::Init { command }; let stdout = LogChannel::new(format!("{name}::stdout")); let stderr = LogChannel::new(format!("{name}::stderr")); - Self { name, description, stdout, stderr, state } + Self { name, description, stdout, stderr, state, pid: None } } /// Starts the underlying process. @@ -77,7 +81,9 @@ impl Executable { .kill_on_drop(true) .current_dir("/") .stdout(Stdio::piped()) - .stderr(Stdio::piped()); + .stderr(Stdio::piped()) + .process_group(0); + if let Some(uid) = uid { command = command.uid(uid); } @@ -85,6 +91,7 @@ impl Executable { command = command.gid(gid); } let mut child = command.spawn()?; + self.pid = child.id().map(|id| Pid::from_raw(id as i32)); let log_channel = self.stdout.clone(); let stdout = child.stdout.take().expect("stdout"); @@ -140,26 +147,36 @@ impl Executable { /// Stops the executable and returns the [ExitStatus]. /// If the executable has never been started, returns [None]. pub async fn kill(&mut self) -> io::Result> { + // Pid is captured at spawn (Pid is Copy), so we can use it even + // after Tokio internally reaps the leader. + let captured_pid = self.pid; Ok(match &mut self.state { ExecutableState::Init { .. } => None, ExecutableState::Started { child, stdout, stderr, .. } => { - child.kill().await?; + // killpg the whole group (PGID == child PID via + // process_group(0)); child.kill() would only signal the + // leader and orphan grandchildren that joined the group. + let killpg_result = killpg( + captured_pid.expect("started exe has captured pid"), + Signal::SIGKILL, + ) + .map_err(io::Error::from); + // Always reap and join the reader tasks, even if killpg + // failed — otherwise the Child stays un-awaited and the + // stdout/stderr handles leak until their pipes close. let exit_status = child.wait().await?; let _ = tokio::join!(stdout, stderr); self.state = ExecutableState::Stopped(exit_status); + killpg_result?; Some(exit_status) } ExecutableState::Stopped(status) => Some(*status), }) } - /// Returns the [Pid] while [Executable] is running, otherwise returns [None]. - pub fn pid(&self) -> io::Result> { - let ExecutableState::Started { child: process, .. } = &self.state - else { - return Ok(None); - }; - - Ok(process.id().map(|id| Pid::from_raw(id as i32))) + /// Returns the captured [Pid] for executables that have been started. + /// Returns [None] before [Executable::start] has been called. + pub fn pid(&self) -> Option { + self.pid } } diff --git a/auraed/src/cells/cell_service/executables/executables.rs b/auraed/src/cells/cell_service/executables/executables.rs index 22778bacb..2097ee3e6 100644 --- a/auraed/src/cells/cell_service/executables/executables.rs +++ b/auraed/src/cells/cell_service/executables/executables.rs @@ -16,7 +16,9 @@ use super::{ Executable, ExecutableName, ExecutableSpec, ExecutablesError, Result, }; +use nix::libc; use std::{collections::HashMap, process::ExitStatus}; +use tracing::warn; type Cache = HashMap; @@ -82,37 +84,53 @@ impl Executables { }); }; - let exit_status = executable.kill().await.map_err(|e| { - ExecutablesError::FailedToStopExecutable { - executable_name: executable_name.clone(), - source: e, + match executable.kill().await { + Ok(Some(status)) => { + let _ = self.cache.remove(executable_name); + Ok(status) } - })?; - - let Some(exit_status) = exit_status else { - // Exes that never started return None - let executable = - self.cache.remove(executable_name).expect("exe in cache"); - return Err(ExecutablesError::ExecutableNotFound { - executable_name: executable.name, - }); - }; - - let _ = self.cache.remove(executable_name).ok_or_else(|| { - // get_mut would have already thrown this error, so we should never reach here - ExecutablesError::ExecutableNotFound { - executable_name: executable_name.clone(), + Ok(None) => { + // Cache invariant: only started executables are inserted + // into the cache (see `start` above), so kill() on a cached + // entry cannot return Ok(None). + unreachable!( + "executable {executable_name:?} is in cache without \ + having been started" + ); } - })?; - - Ok(exit_status) + Err(e) + if matches!( + e.raw_os_error(), + Some(libc::ESRCH) | Some(libc::ECHILD) + ) => + { + // killpg ESRCH (group already empty) or wait ECHILD (kernel + // already reaped). Process is gone; evict and report + // distinctly so callers can render stop idempotent without + // collapsing this with "name not in cache". + warn!( + "executable {executable_name:?} already exited before \ + stop: {e}" + ); + let _ = self.cache.remove(executable_name); + Err(ExecutablesError::ExecutableAlreadyExited { + executable_name: executable_name.clone(), + }) + } + Err(e) => Err(ExecutablesError::FailedToStopExecutable { + executable_name: executable_name.clone(), + source: e, + }), + } } /// Stops all executables concurrently pub async fn broadcast_stop(&mut self) { let mut names = vec![]; for exe in self.cache.values_mut() { - let _ = exe.kill().await; + if let Err(e) = exe.kill().await { + warn!("broadcast_stop: failed to kill {:?}: {e}", exe.name); + } names.push(exe.name.clone()) } @@ -129,9 +147,16 @@ mod tests { use tokio::process::Command; fn spec_for(name: &ExecutableName) -> ExecutableSpec { + spec_with_command(name, "sleep 60") + } + + fn spec_with_command( + name: &ExecutableName, + sh_arg: &str, + ) -> ExecutableSpec { let mut command = Command::new("sh"); let _ = command.arg("-c"); - let _ = command.arg("sleep 60"); + let _ = command.arg(sh_arg); ExecutableSpec { name: name.clone(), description: format!("test executable {name}"), @@ -150,8 +175,10 @@ mod tests { let executable = executables .start(spec_for(&exe_name), None, None) .expect("start executable"); - let pid = executable.pid().expect("read pid"); - assert!(pid.is_some(), "expected spawned process to expose a pid"); + assert!( + executable.pid().is_some(), + "expected spawned process to expose a pid" + ); let err = executables .start(spec_for(&exe_name), None, None) @@ -168,4 +195,70 @@ mod tests { "expected graceful stop or SIGKILL, got status {status:?}" ); } + + /// Stopping a short-lived executable that has already finished running + /// must still return Ok (the cache holds the Stopped state) and must + /// evict the cache entry. + #[tokio::test] + async fn stop_after_natural_exit_returns_ok_and_evicts() { + let mut executables = Executables::default(); + let exe_name = ExecutableName::new(format!( + "unit-test-self-exit-{}", + uuid::Uuid::new_v4() + )); + + let pid = executables + .start(spec_with_command(&exe_name, "true"), None, None) + .expect("start executable") + .pid() + .expect("captured pid") + .as_raw(); + + // Give the leader time to exit. It will sit as a zombie until + // child.wait() is called inside stop(); we just need to ensure the + // process has actually finished its work before we test stop(). + let deadline = + std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::fs::metadata(format!("/proc/{pid}/cmdline")) + .map(|_| true) + .unwrap_or(false) + && std::time::Instant::now() < deadline + { + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + + let _ = executables + .stop(&exe_name) + .await + .expect("stop after natural exit should be Ok"); + + // Cache must have been evicted, so a second stop reports + // ExecutableNotFound (the cache-miss variant), distinct from + // ExecutableAlreadyExited. + let err = executables + .stop(&exe_name) + .await + .expect_err("second stop should report ExecutableNotFound"); + assert!( + matches!(err, ExecutablesError::ExecutableNotFound { .. }), + "expected ExecutableNotFound after eviction, got {err:?}" + ); + } + + /// Stopping a name that was never inserted must return ExecutableNotFound, + /// not the already-exited variant. + #[tokio::test] + async fn stop_unknown_name_returns_not_found() { + let mut executables = Executables::default(); + let exe_name = ExecutableName::new("never-started".to_string()); + + let err = executables + .stop(&exe_name) + .await + .expect_err("stop on unknown name should fail"); + assert!( + matches!(err, ExecutablesError::ExecutableNotFound { .. }), + "expected ExecutableNotFound, got {err:?}" + ); + } } diff --git a/auraed/tests/cell_start_stop_delete.rs b/auraed/tests/cell_start_stop_delete.rs new file mode 100644 index 000000000..5319a824c --- /dev/null +++ b/auraed/tests/cell_start_stop_delete.rs @@ -0,0 +1,332 @@ +/* -------------------------------------------------------------------------- *\ + * | █████╗ ██╗ ██╗██████╗ █████╗ ███████╗ | * + * | ██╔══██╗██║ ██║██╔══██╗██╔══██╗██╔════╝ | * + * | ███████║██║ ██║██████╔╝███████║█████╗ | * + * | ██╔══██║██║ ██║██╔══██╗██╔══██║██╔══╝ | * + * | ██║ ██║╚██████╔╝██║ ██║██║ ██║███████╗ | * + * | ╚═╝ ╚═╝ ╚═════╝ ╚═╝ ╚═╝╚═╝ ╚═╝╚══════╝ | * + * +--------------------------------------------+ * + * * + * Distributed Systems Runtime * + * -------------------------------------------------------------------------- * + * Copyright 2022 - 2024, the aurae contributors * + * SPDX-License-Identifier: Apache-2.0 * +\* -------------------------------------------------------------------------- */ +use client::cells::cell_service::CellServiceClient; +use test_helpers::*; + +mod common; + +#[test_helpers_macros::shared_runtime_test] +async fn cells_start_stop_delete() { + skip_if_not_root!("cells_start_stop_delete"); + skip_if_seccomp!("cells_start_stop_delete"); + + let client = common::auraed_client().await; + + // Allocate a cell + let cell_name = retry!( + client + .allocate( + common::cells::CellServiceAllocateRequestBuilder::new().build() + ) + .await + ) + .unwrap() + .into_inner() + .cell_name; + + // Start the executable + let req = common::cells::CellServiceStartRequestBuilder::new() + .cell_name(cell_name.clone()) + .executable_name("aurae-exe".to_string()) + .build(); + let _ = retry!(client.start(req.clone()).await).unwrap().into_inner(); + + // Stop the executable + let _ = retry!( + client + .stop(proto::cells::CellServiceStopRequest { + cell_name: Some(cell_name.clone()), + executable_name: "aurae-exe".to_string(), + }) + .await + ) + .unwrap(); + + // Delete the cell + let _ = retry!( + client + .free(proto::cells::CellServiceFreeRequest { + cell_name: cell_name.clone() + }) + .await + ) + .unwrap(); +} + +/// Regression test for https://github.com/aurae-runtime/aurae/issues/534. +/// +/// Runs a command that spawns two background children of a `bash` wrapper +/// (the original bug: the `sh -c`/`bash -c` wrapper is tracked but its +/// children leak when stopped). After stop, the wrapper PID and both child +/// PIDs must all be gone from /proc. +#[test_helpers_macros::shared_runtime_test] +async fn cells_stop_kills_entire_process_group() { + skip_if_not_root!("cells_stop_kills_entire_process_group"); + skip_if_seccomp!("cells_stop_kills_entire_process_group"); + + let client = common::auraed_client().await; + + let cell_name = retry!( + client + .allocate( + common::cells::CellServiceAllocateRequestBuilder::new().build() + ) + .await + ) + .unwrap() + .into_inner() + .cell_name; + + // auraed wraps the supplied command as `sh -c ` (see the + // ValidatedExecutable -> ExecutableSpec conversion in validation.rs), + // so the recorded leader pid is the `sh` process. `process_group(0)` + // makes that leader its own pgid leader; `bash` and the two `sleep` + // children inherit the pgid. We therefore expect at least 2 group + // members under the leader after start. + let req = common::cells::CellServiceStartRequestBuilder::new() + .cell_name(cell_name.clone()) + .executable_name("group-leaker".to_string()) + .command("bash -c 'sleep 9000 & sleep 9000 & wait'".to_string()) + .build(); + let start = retry!(client.start(req.clone()).await).unwrap().into_inner(); + let leader_pid = start.pid; + assert!(leader_pid > 0, "expected a valid leader pid"); + + // Wait for bash to fork its two sleep children rather than guessing a + // fixed sleep — under load the fork can take longer than a few hundred + // millis, which would flake on a fixed delay. + let spawned = wait_until(std::time::Duration::from_secs(2), || { + children_by_pgid(leader_pid).len() >= 2 + }) + .await; + assert!(spawned, "bash did not spawn its sleep children within 2s"); + + let children_before = children_by_pgid(leader_pid); + assert!( + children_before.len() >= 2, + "expected leader to have spawned >= 2 children in pgid {leader_pid}, \ + found {:?}", + children_before + ); + + let _ = retry!( + client + .stop(proto::cells::CellServiceStopRequest { + cell_name: Some(cell_name.clone()), + executable_name: "group-leaker".to_string(), + }) + .await + ) + .unwrap(); + + // After stop returns, every PID in the group must be gone. Poll for a + // short window to tolerate reaping latency. + let all_gone = wait_until(std::time::Duration::from_secs(3), || { + !pid_exists(leader_pid) + && children_before.iter().all(|pid| !pid_exists(*pid)) + }) + .await; + assert!( + all_gone, + "orphans remain after stop: leader {}={}, children={:?}", + leader_pid, + pid_exists(leader_pid), + children_before + .iter() + .map(|p| (*p, pid_exists(*p))) + .collect::>() + ); + + let _ = retry!( + client + .free(proto::cells::CellServiceFreeRequest { + cell_name: cell_name.clone() + }) + .await + ) + .unwrap(); +} + +/// Calling stop twice on the same executable should return Ok both times. +#[test_helpers_macros::shared_runtime_test] +async fn cells_double_stop_is_idempotent() { + skip_if_not_root!("cells_double_stop_is_idempotent"); + skip_if_seccomp!("cells_double_stop_is_idempotent"); + + let client = common::auraed_client().await; + + let cell_name = retry!( + client + .allocate( + common::cells::CellServiceAllocateRequestBuilder::new().build() + ) + .await + ) + .unwrap() + .into_inner() + .cell_name; + + let req = common::cells::CellServiceStartRequestBuilder::new() + .cell_name(cell_name.clone()) + .executable_name("double-stopper".to_string()) + .command("sleep 9000".to_string()) + .build(); + let _ = retry!(client.start(req.clone()).await).unwrap().into_inner(); + + let _ = retry!( + client + .stop(proto::cells::CellServiceStopRequest { + cell_name: Some(cell_name.clone()), + executable_name: "double-stopper".to_string(), + }) + .await + ) + .expect("first stop should succeed"); + + let second = client + .stop(proto::cells::CellServiceStopRequest { + cell_name: Some(cell_name.clone()), + executable_name: "double-stopper".to_string(), + }) + .await; + assert!( + second.is_ok(), + "second stop should be idempotent; got {:?}", + second.err() + ); + + let _ = retry!( + client + .free(proto::cells::CellServiceFreeRequest { + cell_name: cell_name.clone() + }) + .await + ) + .unwrap(); +} + +/// Stopping an executable that has already exited on its own must still +/// return Ok. This drives the ESRCH/ECHILD path in `Executables::stop` — +/// a refactor that moves those errno values into the FailedToStopExecutable +/// branch would silently turn natural-exit stops into Status::internal. +#[test_helpers_macros::shared_runtime_test] +async fn cells_stop_after_natural_exit_is_ok() { + skip_if_not_root!("cells_stop_after_natural_exit_is_ok"); + skip_if_seccomp!("cells_stop_after_natural_exit_is_ok"); + + let client = common::auraed_client().await; + + let cell_name = retry!( + client + .allocate( + common::cells::CellServiceAllocateRequestBuilder::new().build() + ) + .await + ) + .unwrap() + .into_inner() + .cell_name; + + let req = common::cells::CellServiceStartRequestBuilder::new() + .cell_name(cell_name.clone()) + .executable_name("self-exit".to_string()) + .command("true".to_string()) + .build(); + let start = retry!(client.start(req.clone()).await).unwrap().into_inner(); + let leader_pid = start.pid; + + // Wait for the process to disappear from /proc on its own. + let exited = wait_until(std::time::Duration::from_secs(5), || { + !pid_exists(leader_pid) + }) + .await; + assert!(exited, "expected leader pid {leader_pid} to exit on its own"); + + let stop = client + .stop(proto::cells::CellServiceStopRequest { + cell_name: Some(cell_name.clone()), + executable_name: "self-exit".to_string(), + }) + .await; + assert!( + stop.is_ok(), + "stop after natural exit should be idempotent; got {:?}", + stop.err() + ); + + let _ = retry!( + client + .free(proto::cells::CellServiceFreeRequest { + cell_name: cell_name.clone() + }) + .await + ) + .unwrap(); +} + +fn pid_exists(pid: i32) -> bool { + std::fs::metadata(format!("/proc/{pid}")).is_ok() +} + +/// Enumerate /proc and return pids whose PGID equals `pgid`. +/// Returns pids other than `pgid` itself (i.e. the leader's group members). +fn children_by_pgid(pgid: i32) -> Vec { + let mut pids = Vec::new(); + let entries = match std::fs::read_dir("/proc") { + Ok(e) => e, + Err(_) => return pids, + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(s) = name.to_str() else { continue }; + let Ok(pid) = s.parse::() else { continue }; + if pid == pgid { + continue; + } + let Ok(stat) = std::fs::read_to_string(format!("/proc/{pid}/stat")) + else { + continue; + }; + // The /proc//stat comm field is wrapped in `(...)` and may + // contain spaces/parens; rsplit on the last `)` to skip past it + // safely. Fields beyond `)` are space-separated and ordered: + // [0]=state, [1]=ppid, [2]=pgrp, [3]=session, ... + let Some(after_comm) = stat.rsplit_once(')').map(|x| x.1) else { + continue; + }; + let fields: Vec<&str> = after_comm.split_whitespace().collect(); + if let Some(pgrp_str) = fields.get(2) + && let Ok(pgrp) = pgrp_str.parse::() + && pgrp == pgid + { + pids.push(pid); + } + } + pids +} + +async fn wait_until bool>( + timeout: std::time::Duration, + check: F, +) -> bool { + let start = std::time::Instant::now(); + while start.elapsed() < timeout { + if check() { + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + check() +} diff --git a/auraed/tests/common/cells.rs b/auraed/tests/common/cells.rs index 9c4c68448..3fd8aba4c 100644 --- a/auraed/tests/common/cells.rs +++ b/auraed/tests/common/cells.rs @@ -83,7 +83,7 @@ impl CellServiceAllocateRequestBuilder { } } -struct ExecutableBuilder { +pub struct ExecutableBuilder { name: String, command: String, description: String, @@ -103,6 +103,11 @@ impl ExecutableBuilder { self } + pub fn command(&mut self, command: String) -> &mut Self { + self.command = command; + self + } + pub fn build(&self) -> Executable { Executable { name: self.name.clone(), @@ -139,6 +144,11 @@ impl CellServiceStartRequestBuilder { self } + pub fn command(&mut self, command: String) -> &mut Self { + let _ = self.executable_builder.command(command); + self + } + pub fn uid(&mut self, uid: u32) -> &mut Self { self.uid = Some(uid); self From 71146093e774b4ad33a16a44b94bebbc921017ad Mon Sep 17 00:00:00 2001 From: Tommy McCormick Date: Mon, 27 Apr 2026 07:25:08 -0400 Subject: [PATCH 2/2] fix(vms): remove SIGCHLD SIG_IGN that auto-reaped cells children VirtualMachines::new called libc::signal(SIGCHLD, SIG_IGN). The disposition is inherited across execve into every nested auraed, so the kernel auto-reaped spawned children. That made Executable::kill's child.wait().await return ECHILD on every cells stop and caused waitpid in nested_auraed to hang waiting for a SIGCHLD that the kernel never delivered. Cloud Hypervisor's Vm::HANDLED_SIGNALS and Vmm::HANDLED_SIGNALS do not include SIGCHLD, so the block_signal loops below are unaffected. --- .../cell_service/cells/nested_auraed/nested_auraed.rs | 7 ++----- auraed/src/vms/virtual_machines.rs | 5 ----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs b/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs index 1c591c8eb..0d382936d 100644 --- a/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs +++ b/auraed/src/cells/cell_service/cells/nested_auraed/nested_auraed.rs @@ -189,11 +189,8 @@ impl NestedAuraed { ) -> io::Result<()> { let signal = signal.into(); let pid = Pid::from_raw(self.process.pid); - - match nix::sys::signal::kill(pid, signal) { - Ok(()) | Err(Errno::ESRCH) => Ok(()), - Err(e) => Err(io::Error::from(e)), - } + nix::sys::signal::kill(pid, signal)?; + Ok(()) } fn wait(&mut self) -> io::Result { diff --git a/auraed/src/vms/virtual_machines.rs b/auraed/src/vms/virtual_machines.rs index 887dab652..512ba833c 100644 --- a/auraed/src/vms/virtual_machines.rs +++ b/auraed/src/vms/virtual_machines.rs @@ -16,7 +16,6 @@ use std::{collections::HashMap, net::Ipv4Addr}; use anyhow::anyhow; use net_util::MacAddr; -use nix::libc; use tracing::error; use vmm_sys_util::{rand, signal::block_signal}; @@ -39,10 +38,6 @@ impl Default for VirtualMachines { impl VirtualMachines { /// Create a new instance of the virtual machines cache. pub fn new() -> Self { - unsafe { - let _ = libc::signal(libc::SIGCHLD, libc::SIG_IGN); - } - // Mask the signals handled by the Cloud Hyupervisor VMM so they only run on the dedicated signal handling thread for sig in &vmm::vm::Vm::HANDLED_SIGNALS { if let Err(e) = block_signal(*sig) {