From 3a69b210e1d0447c55478bedff157b46964ec33f Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Mon, 13 Jul 2026 12:39:50 +0000 Subject: [PATCH 1/5] fix(PocketIC): Kill and reap the PocketIC server when all tests finish `start_server` used to spawn the PocketIC server, detach it into its own process group and discard the `Child` handle, so the server (and the canister-sandbox processes it re-execs) outlived the test process and shut down only on its TTL. A lingering server keeps the test action's inherited stdout/stderr pipes open past process exit, which caused issues on some CI runners that wait for those pipes to reach EOF. The spawned server is now transferred to a process-global registry. On unix, a `libc::atexit` callback runs once all subtests have finished, kills each server's whole process group (so the sandbox children are terminated too and release the inherited pipes) and reaps it. This also removes the `#[allow(clippy::zombie_processes)]` workaround. `start_server` now returns a `ServerHandle` instead of a `std::process::Child`; callers that terminated the server themselves should call `ServerHandle::kill_and_wait`. Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.lock | 1 + packages/pocket-ic/BUILD.bazel | 1 + packages/pocket-ic/CHANGELOG.md | 10 ++ packages/pocket-ic/Cargo.toml | 3 + packages/pocket-ic/src/lib.rs | 20 ++- packages/pocket-ic/src/server_process.rs | 181 +++++++++++++++++++++++ packages/pocket-ic/tests/unix.rs | 4 +- 7 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 packages/pocket-ic/src/server_process.rs diff --git a/Cargo.lock b/Cargo.lock index 5945998238af..481b99b85c45 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -20046,6 +20046,7 @@ dependencies = [ "ic-vetkeys", "icrc-ledger-types", "k256", + "libc", "maplit", "prost 0.13.5", "registry-canister", diff --git a/packages/pocket-ic/BUILD.bazel b/packages/pocket-ic/BUILD.bazel index 741c90f31ed5..b5de0461c6d5 100644 --- a/packages/pocket-ic/BUILD.bazel +++ b/packages/pocket-ic/BUILD.bazel @@ -20,6 +20,7 @@ rust_library( "@crate_index//:ic-certification", "@crate_index//:ic-management-canister-types", "@crate_index//:ic-transport-types", + "@crate_index//:libc", "@crate_index//:reqwest", "@crate_index//:schemars", "@crate_index//:semver", diff --git a/packages/pocket-ic/CHANGELOG.md b/packages/pocket-ic/CHANGELOG.md index 1747b80bad02..3cf64e54d10b 100644 --- a/packages/pocket-ic/CHANGELOG.md +++ b/packages/pocket-ic/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Changed +- `start_server` now returns a `ServerHandle` (instead of a `std::process::Child`). The + spawned PocketIC server is registered in a process-global registry and, on unix, is + automatically killed (its whole process group, so canister-sandbox children are + terminated too) and reaped when the test process exits — instead of being detached and + left to shut down on its TTL. This ensures the server no longer keeps the test action's + inherited stdout/stderr pipes open past process exit, which caused issues on some CI + runners. Callers that previously kept the `Child` to terminate the server themselves + should call `ServerHandle::kill_and_wait` instead. + ## 15.0.0 - 2026-06-26 diff --git a/packages/pocket-ic/Cargo.toml b/packages/pocket-ic/Cargo.toml index a4dfbe7b3d6e..4dc22dcbada6 100644 --- a/packages/pocket-ic/Cargo.toml +++ b/packages/pocket-ic/Cargo.toml @@ -48,6 +48,9 @@ tracing = { workspace = true } tracing-appender = { workspace = true } tracing-subscriber = { workspace = true } +[target.'cfg(unix)'.dependencies] +libc = { workspace = true } + [target.'cfg(windows)'.dependencies] wslpath = "0.0.2" diff --git a/packages/pocket-ic/src/lib.rs b/packages/pocket-ic/src/lib.rs index ba4bb9953eb3..fbffe4bd03c6 100644 --- a/packages/pocket-ic/src/lib.rs +++ b/packages/pocket-ic/src/lib.rs @@ -85,7 +85,7 @@ use std::{ fs::OpenOptions, net::{IpAddr, SocketAddr}, path::PathBuf, - process::{Child, Command}, + process::Command, sync::{Arc, mpsc::channel}, thread, thread::JoinHandle, @@ -101,6 +101,9 @@ use wslpath::windows_to_wsl; pub mod common; pub mod nonblocking; +mod server_process; + +pub use server_process::ServerHandle; const POCKET_IC_SERVER_NAME: &str = "pocket-ic-server"; @@ -2280,7 +2283,12 @@ pub struct StartServerParams { } /// Attempt to start a new PocketIC server. -pub async fn start_server(params: StartServerParams) -> (Child, Url) { +/// +/// The returned [`ServerHandle`] owns the spawned server process, which is registered in +/// a process-global registry and automatically killed and reaped (together with its +/// canister-sandbox children, on unix) once the test process exits. Callers that want to +/// terminate the server earlier can call [`ServerHandle::kill_and_wait`]. +pub async fn start_server(params: StartServerParams) -> (ServerHandle, Url) { let default_bin_dir = std::env::temp_dir().join(format!("{POCKET_IC_SERVER_NAME}-{LATEST_SERVER_VERSION}")); let default_bin_path = default_bin_dir.join("pocket-ic"); @@ -2389,11 +2397,13 @@ pub async fn start_server(params: StartServerParams) -> (Child, Url) { cmd.process_group(0); } - // TODO: SDK-1936 - #[allow(clippy::zombie_processes)] let child = cmd .spawn() .unwrap_or_else(|_| panic!("Failed to start PocketIC binary ({})", bin_path.display())); + // Transfer ownership of the server process to the process-global registry, which + // kills and reaps it (and its sandbox children) once all subtests have finished. + // See `server_process`. + let server_handle = server_process::register(child); loop { if let Ok(port_string) = std::fs::read_to_string(port_file_path.clone()) @@ -2404,7 +2414,7 @@ pub async fn start_server(params: StartServerParams) -> (Child, Url) { .parse() .expect("Failed to parse port to number"); break ( - child, + server_handle, Url::parse(&format!("http://{LOCALHOST}:{port}/")).unwrap(), ); } diff --git a/packages/pocket-ic/src/server_process.rs b/packages/pocket-ic/src/server_process.rs new file mode 100644 index 000000000000..14c3d4cb5f8d --- /dev/null +++ b/packages/pocket-ic/src/server_process.rs @@ -0,0 +1,181 @@ +//! Process-lifetime management for the spawned `pocket-ic-server`. +//! +//! Historically [`crate::start_server`] spawned the server, detached it into its own +//! process group (`process_group(0)`) and then discarded the [`Child`] handle, so the +//! server — and the canister-sandbox processes it re-execs — outlived the test process. +//! Under a remote-execution worker that only completes once the test action's +//! stdout/stderr pipes reach EOF, the lingering server (and its sandbox children, which +//! inherit those pipes) kept the pipe write-ends open, and the action failed with +//! `WaitDelay expired before I/O complete`. +//! +//! To fix this, every spawned server is transferred to a process-global registry. On +//! unix a `libc::atexit` callback — which runs once all `#[test]`s in the binary have +//! finished and the harness is exiting — kills each server's whole process *group* (so +//! the sandbox children die too and release the inherited pipes) and reaps it. This also +//! closes `SDK-1936` (properly reap the spawned child instead of leaking a zombie). + +use std::process::Child; +use std::sync::{Mutex, MutexGuard, OnceLock}; +#[cfg(unix)] +use std::time::{Duration, Instant}; + +/// A spawned `pocket-ic-server` candidate, owned exclusively by the [`registry`]. +struct ServerProc { + child: Child, + /// The server's process-group id. The client spawns the server with + /// `process_group(0)`, so the server is a group *leader* and `pgid == child.id()`. + /// The launcher/sandbox processes it re-execs inherit this group (they set no + /// `setpgid`/`setsid`), so signalling the group reaches all of them. + #[cfg(unix)] + pgid: libc::pid_t, +} + +/// The process-global set of spawned servers, indexed by the [`ServerHandle`] returned +/// to callers. A slot becomes `None` once its server has been reaped (by +/// [`ServerHandle::kill_and_wait`] or the exit-time reaper) so no server is signalled or +/// waited on twice. Slots are never removed, so a [`ServerHandle`]'s index stays valid. +fn registry() -> &'static Mutex>> { + static SERVERS: OnceLock>>> = OnceLock::new(); + SERVERS.get_or_init(|| { + // Install the exit-time reaper exactly once, when the first server is registered. + // It runs from `libc::exit()` after the libtest harness returns or calls + // `std::process::exit` — i.e. once every subtest has finished. This is *not* a + // signal-handler context, so locking a `Mutex`, sleeping and `waitpid` are all + // legal here; it only needs to be panic-free (see `reap_all`). + #[cfg(unix)] + // SAFETY: `reap_all` is an `extern "C"`, panic-free function and is registered + // exactly once (guarded by `OnceLock`). + unsafe { + libc::atexit(reap_all); + } + Mutex::new(Vec::new()) + }) +} + +fn lock_registry() -> MutexGuard<'static, Vec>> { + // A poisoned registry is fine to keep using: the data is a plain list of child + // handles and recovering it lets us still kill/reap the servers. + registry().lock().unwrap_or_else(|e| e.into_inner()) +} + +/// Transfers ownership of a freshly spawned `pocket-ic-server` [`Child`] to the +/// process-global registry and returns a [`ServerHandle`] referring to it. +/// +/// The registry keeps the child alive for the remainder of the process (so the server +/// stays up for the whole test run) and guarantees it is killed and reaped at process +/// exit, replacing the previous "spawn and leak" behaviour. +pub(crate) fn register(child: Child) -> ServerHandle { + #[cfg(unix)] + let pgid = child.id() as libc::pid_t; + let mut servers = lock_registry(); + servers.push(Some(ServerProc { + child, + #[cfg(unix)] + pgid, + })); + ServerHandle { + idx: servers.len() - 1, + } +} + +/// A handle to a `pocket-ic-server` spawned by [`crate::start_server`]. +/// +/// The server is owned by a process-global registry and is automatically killed and +/// reaped when the test process exits (on unix), so most callers can simply drop this +/// handle. Callers that want to terminate the server earlier — e.g. to test crash +/// recovery — can call [`ServerHandle::kill_and_wait`]. +pub struct ServerHandle { + idx: usize, +} + +impl ServerHandle { + /// Kills the server — its whole process group on unix, so the canister-sandbox + /// children are terminated too — and waits for it to be reaped, immediately. + /// + /// This is a *hard* kill (`SIGKILL` on unix): it does not give the server a chance + /// to shut down gracefully or checkpoint state. Idempotent with respect to the + /// exit-time reaper: the server is removed from the registry here, so it is not + /// signalled or waited on again at process exit. + pub fn kill_and_wait(&self) { + let taken = lock_registry().get_mut(self.idx).and_then(Option::take); + if let Some(mut proc) = taken { + #[cfg(unix)] + reap_one(&mut proc, Signal::Kill); + #[cfg(windows)] + { + let _ = proc.child.kill(); + let _ = proc.child.wait(); + } + } + } +} + +/// How aggressively [`reap_one`] terminates a server. +#[cfg(unix)] +#[derive(Clone, Copy)] +enum Signal { + /// Request a graceful shutdown first (`SIGTERM` drives the server's `ctrlc` handler, + /// which is built with the `termination` feature), escalating to `SIGKILL` if the + /// server outlives a short grace period. + TermThenKill, + /// Immediate, uncatchable `SIGKILL`. + Kill, +} + +/// `libc::atexit` callback: kill and reap every server still registered. +/// +/// Must be panic-free — unwinding across an `extern "C"` boundary is undefined +/// behaviour — hence the `catch_unwind`. +#[cfg(unix)] +extern "C" fn reap_all() { + let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let taken: Vec = { + let mut servers = lock_registry(); + servers.iter_mut().filter_map(Option::take).collect() + }; + for mut proc in taken { + reap_one(&mut proc, Signal::TermThenKill); + } + })); +} + +/// Signals a server's process group and reaps the direct server child. +/// +/// The child's launcher/sandbox descendants are terminated by the process-group signal +/// (their pipe write-ends close on *termination*, not on reaping) and are reaped by +/// init, since the test process cannot `waitpid` them (they are not its direct +/// children). The wait is bounded so the atexit handler can never hang process exit. +#[cfg(unix)] +fn reap_one(proc: &mut ServerProc, signal: Signal) { + let pgid = proc.pgid; + let pid = proc.child.id() as libc::pid_t; + + let first = match signal { + Signal::TermThenKill => libc::SIGTERM, + Signal::Kill => libc::SIGKILL, + }; + // A negative target signals the whole process group (server + launcher + sandboxes). + // `ESRCH` — e.g. a loser candidate whose group is already empty — is expected and + // harmless: the following `waitpid` still reaps the zombie. + unsafe { libc::kill(-pgid, first) }; + + let deadline = Instant::now() + Duration::from_millis(2_000); + loop { + let mut status: libc::c_int = 0; + // SAFETY: reaping our own direct child by pid. + match unsafe { libc::waitpid(pid, &mut status, libc::WNOHANG) } { + // Reaped (`r == pid`), or already gone / not our child (`-1` => `ECHILD`). + r if r == pid || r == -1 => return, + // Still running (`0`, or an unexpected value): wait, then escalate. + _ => { + if Instant::now() >= deadline { + unsafe { libc::kill(-pgid, libc::SIGKILL) }; + // Final blocking wait so the zombie is reaped before we return. + let _ = unsafe { libc::waitpid(pid, &mut status, 0) }; + return; + } + std::thread::sleep(Duration::from_millis(10)); + } + } + } +} diff --git a/packages/pocket-ic/tests/unix.rs b/packages/pocket-ic/tests/unix.rs index b7989f9e544c..5255c0f07071 100644 --- a/packages/pocket-ic/tests/unix.rs +++ b/packages/pocket-ic/tests/unix.rs @@ -71,7 +71,7 @@ fn test_canister_wasm() -> Vec { async fn resume_killed_instance_impl( incomplete_state: Option, ) -> Result<(), String> { - let (mut server, server_url) = start_server(StartServerParams::default()).await; + let (server, server_url) = start_server(StartServerParams::default()).await; let temp_dir = TempDir::new().unwrap(); let state = PocketIcState::new_from_path(temp_dir.path().to_path_buf()); @@ -125,7 +125,7 @@ async fn resume_killed_instance_impl( let time = pic.get_time().await; assert!(time >= now.into()); - server.kill().unwrap(); + server.kill_and_wait(); let (_, server_url) = start_server(StartServerParams::default()).await; let client = reqwest::Client::new(); From 0aca829bdba87277e4bb3b454d6669e577c7dbb1 Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Mon, 13 Jul 2026 12:42:03 +0000 Subject: [PATCH 2/5] trigger From c97a2d2165dd962312e4f1e2e56c6884bfa5791d Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Mon, 13 Jul 2026 12:54:18 +0000 Subject: [PATCH 3/5] fix(PocketIC): Address review comments - Handle EINTR in the reaper's waitpid loop by retrying instead of treating it as terminal, so an interrupting signal cannot let the server outlive process exit while still holding the inherited stdout/stderr pipes. Only non-EINTR errors (expected: ECHILD) are terminal. The final blocking wait after SIGKILL escalation also retries on EINTR. - Qualify the `start_server` doc comment: automatic kill+reap on process exit happens on unix only; on other platforms the server shuts down on its TTL. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/pocket-ic/src/lib.rs | 10 +++--- packages/pocket-ic/src/server_process.rs | 39 +++++++++++++++++++++--- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/packages/pocket-ic/src/lib.rs b/packages/pocket-ic/src/lib.rs index fbffe4bd03c6..74cb9dd6909c 100644 --- a/packages/pocket-ic/src/lib.rs +++ b/packages/pocket-ic/src/lib.rs @@ -2284,10 +2284,12 @@ pub struct StartServerParams { /// Attempt to start a new PocketIC server. /// -/// The returned [`ServerHandle`] owns the spawned server process, which is registered in -/// a process-global registry and automatically killed and reaped (together with its -/// canister-sandbox children, on unix) once the test process exits. Callers that want to -/// terminate the server earlier can call [`ServerHandle::kill_and_wait`]. +/// The returned [`ServerHandle`] owns the spawned server process. On unix it is +/// registered in a process-global registry and, once the test process exits, +/// automatically killed (its whole process group, so the canister-sandbox children are +/// terminated too) and reaped; on other platforms the server is not killed automatically +/// and shuts down on its TTL. Callers that want to terminate the server earlier can call +/// [`ServerHandle::kill_and_wait`]. pub async fn start_server(params: StartServerParams) -> (ServerHandle, Url) { let default_bin_dir = std::env::temp_dir().join(format!("{POCKET_IC_SERVER_NAME}-{LATEST_SERVER_VERSION}")); diff --git a/packages/pocket-ic/src/server_process.rs b/packages/pocket-ic/src/server_process.rs index 14c3d4cb5f8d..77edb5961035 100644 --- a/packages/pocket-ic/src/server_process.rs +++ b/packages/pocket-ic/src/server_process.rs @@ -164,14 +164,24 @@ fn reap_one(proc: &mut ServerProc, signal: Signal) { let mut status: libc::c_int = 0; // SAFETY: reaping our own direct child by pid. match unsafe { libc::waitpid(pid, &mut status, libc::WNOHANG) } { - // Reaped (`r == pid`), or already gone / not our child (`-1` => `ECHILD`). - r if r == pid || r == -1 => return, - // Still running (`0`, or an unexpected value): wait, then escalate. + // Reaped. + r if r == pid => return, + -1 => { + // `EINTR` (interrupted by a signal): retry — returning early here could + // let the server outlive process exit still holding the pipes, the exact + // failure this reaper prevents. Any other error is terminal; the expected + // one is `ECHILD` (the child was already reaped / is not ours). + if last_errno() == libc::EINTR { + continue; + } + return; + } + // Still running (`0`): wait, escalating to `SIGKILL` at the deadline. _ => { if Instant::now() >= deadline { unsafe { libc::kill(-pgid, libc::SIGKILL) }; // Final blocking wait so the zombie is reaped before we return. - let _ = unsafe { libc::waitpid(pid, &mut status, 0) }; + blocking_reap(pid); return; } std::thread::sleep(Duration::from_millis(10)); @@ -179,3 +189,24 @@ fn reap_one(proc: &mut ServerProc, signal: Signal) { } } } + +/// The calling thread's `errno`. Only meaningful immediately after a failed libc call. +#[cfg(unix)] +fn last_errno() -> libc::c_int { + std::io::Error::last_os_error().raw_os_error().unwrap_or(0) +} + +/// Blocking `waitpid` on a direct child, retrying on `EINTR` so a signal cannot leave the +/// child unreaped. +#[cfg(unix)] +fn blocking_reap(pid: libc::pid_t) { + let mut status: libc::c_int = 0; + loop { + // SAFETY: reaping our own direct child by pid. + let r = unsafe { libc::waitpid(pid, &mut status, 0) }; + if r == -1 && last_errno() == libc::EINTR { + continue; + } + return; + } +} From cecb54acd8f3d65a85387a6cabf647d801f65e02 Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Mon, 13 Jul 2026 13:30:45 +0000 Subject: [PATCH 4/5] fix(PocketIC): Address second round of review comments - Bound the reaper's wait so it can never hang process exit. The unbounded `blocking_reap` (waitpid with no timeout) is replaced by `reap_until`, which polls with WNOHANG until a deadline (retrying on EINTR). reap_one now signals, waits one bounded grace period, escalates to SIGKILL, then waits one more bounded window; if the child is still not reaped it is left for init (the pipes are already closed by the kill). - Update the reap_one doc to describe the bounded, best-effort reap instead of promising a guaranteed reap. - Check the `libc::atexit` return value and assert on failure, so a failure to register the reaper surfaces loudly instead of silently reintroducing the leaked-server problem. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/pocket-ic/src/server_process.rs | 96 +++++++++++++----------- 1 file changed, 51 insertions(+), 45 deletions(-) diff --git a/packages/pocket-ic/src/server_process.rs b/packages/pocket-ic/src/server_process.rs index 77edb5961035..3191085d8b23 100644 --- a/packages/pocket-ic/src/server_process.rs +++ b/packages/pocket-ic/src/server_process.rs @@ -43,10 +43,16 @@ fn registry() -> &'static Mutex>> { // signal-handler context, so locking a `Mutex`, sleeping and `waitpid` are all // legal here; it only needs to be panic-free (see `reap_all`). #[cfg(unix)] - // SAFETY: `reap_all` is an `extern "C"`, panic-free function and is registered - // exactly once (guarded by `OnceLock`). - unsafe { - libc::atexit(reap_all); + { + // SAFETY: `reap_all` is an `extern "C"`, panic-free function and is registered + // exactly once (guarded by `OnceLock`). + let rc = unsafe { libc::atexit(reap_all) }; + // A non-zero return means the handler was not registered, which would silently + // reintroduce the leaked-server problem — fail loudly instead. + assert_eq!( + rc, 0, + "failed to register the pocket-ic-server atexit reaper" + ); } Mutex::new(Vec::new()) }) @@ -139,12 +145,22 @@ extern "C" fn reap_all() { })); } -/// Signals a server's process group and reaps the direct server child. +/// Grace period given to each signal before escalating / giving up. +#[cfg(unix)] +const REAP_GRACE: Duration = Duration::from_millis(2_000); + +/// Signals a server's process group and makes a bounded, best-effort attempt to reap the +/// direct server child. /// /// The child's launcher/sandbox descendants are terminated by the process-group signal -/// (their pipe write-ends close on *termination*, not on reaping) and are reaped by -/// init, since the test process cannot `waitpid` them (they are not its direct -/// children). The wait is bounded so the atexit handler can never hang process exit. +/// (their pipe write-ends close on *termination*, not on reaping) and are reaped by init, +/// since the test process cannot `waitpid` them (they are not its direct children). +/// +/// The whole operation is time-bounded so it can never hang process exit: after the first +/// signal fails to reap the child within [`REAP_GRACE`] the group is `SIGKILL`ed and given +/// one more [`REAP_GRACE`] window. If the child is *still* not reaped it is left for init +/// to reap once we exit — what matters here (closing the inherited pipes) is already done +/// by the kill. #[cfg(unix)] fn reap_one(proc: &mut ServerProc, signal: Signal) { let pgid = proc.pgid; @@ -156,38 +172,15 @@ fn reap_one(proc: &mut ServerProc, signal: Signal) { }; // A negative target signals the whole process group (server + launcher + sandboxes). // `ESRCH` — e.g. a loser candidate whose group is already empty — is expected and - // harmless: the following `waitpid` still reaps the zombie. + // harmless: `reap_until` still reaps the zombie. unsafe { libc::kill(-pgid, first) }; - let deadline = Instant::now() + Duration::from_millis(2_000); - loop { - let mut status: libc::c_int = 0; - // SAFETY: reaping our own direct child by pid. - match unsafe { libc::waitpid(pid, &mut status, libc::WNOHANG) } { - // Reaped. - r if r == pid => return, - -1 => { - // `EINTR` (interrupted by a signal): retry — returning early here could - // let the server outlive process exit still holding the pipes, the exact - // failure this reaper prevents. Any other error is terminal; the expected - // one is `ECHILD` (the child was already reaped / is not ours). - if last_errno() == libc::EINTR { - continue; - } - return; - } - // Still running (`0`): wait, escalating to `SIGKILL` at the deadline. - _ => { - if Instant::now() >= deadline { - unsafe { libc::kill(-pgid, libc::SIGKILL) }; - // Final blocking wait so the zombie is reaped before we return. - blocking_reap(pid); - return; - } - std::thread::sleep(Duration::from_millis(10)); - } - } + if reap_until(pid, Instant::now() + REAP_GRACE) { + return; } + // Still alive after the grace period: force-kill the group and try once more. + unsafe { libc::kill(-pgid, libc::SIGKILL) }; + reap_until(pid, Instant::now() + REAP_GRACE); } /// The calling thread's `errno`. Only meaningful immediately after a failed libc call. @@ -196,17 +189,30 @@ fn last_errno() -> libc::c_int { std::io::Error::last_os_error().raw_os_error().unwrap_or(0) } -/// Blocking `waitpid` on a direct child, retrying on `EINTR` so a signal cannot leave the -/// child unreaped. +/// Polls `waitpid(WNOHANG)` for a direct child until it is reaped/gone or `deadline` +/// passes, retrying on `EINTR`. Returns `true` if the child was reaped or is already gone, +/// `false` if the deadline elapsed while it was still alive (so the caller can escalate). +/// +/// Bounded by `deadline` so it can never hang process exit; `EINTR` (interrupted by a +/// signal) is retried rather than treated as terminal, so an interruption cannot cause an +/// early return that leaves the server holding the pipes. The expected terminal error is +/// `ECHILD` (the child was already reaped / is not ours). #[cfg(unix)] -fn blocking_reap(pid: libc::pid_t) { - let mut status: libc::c_int = 0; +fn reap_until(pid: libc::pid_t, deadline: Instant) -> bool { loop { + let mut status: libc::c_int = 0; // SAFETY: reaping our own direct child by pid. - let r = unsafe { libc::waitpid(pid, &mut status, 0) }; - if r == -1 && last_errno() == libc::EINTR { - continue; + match unsafe { libc::waitpid(pid, &mut status, libc::WNOHANG) } { + r if r == pid => return true, + -1 if last_errno() == libc::EINTR => continue, + -1 => return true, + // Still running (`0`): wait unless the deadline has passed. + _ => { + if Instant::now() >= deadline { + return false; + } + std::thread::sleep(Duration::from_millis(10)); + } } - return; } } From 31db9d5855bcf5d918a4f78e5988c1cadb8651c1 Mon Sep 17 00:00:00 2001 From: Bas van Dijk Date: Mon, 13 Jul 2026 14:29:17 +0000 Subject: [PATCH 5/5] docs(PocketIC): Align server-lifetime doc comments with actual guarantees The kill+reap of the PocketIC server is unix-only and bounded/best-effort, but several doc comments still promised more. Adjust them to match: - `kill_and_wait`: no longer claims it waits for the server to be reaped "immediately"; it kills and makes a bounded, best-effort reap attempt. - `register` / `ServerHandle`: the exit-time kill+reap is unix-only and best-effort, not a guarantee; drop it from the "guarantees" wording. - `start_server`: the process is owned by the process-global registry; the returned `ServerHandle` only refers to it (dropping it does not stop the server), so it no longer implies drop/ownership semantics on the handle value. Doc-comment-only; no behavioural change. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/pocket-ic/src/lib.rs | 11 +++++----- packages/pocket-ic/src/server_process.rs | 28 ++++++++++++++---------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/packages/pocket-ic/src/lib.rs b/packages/pocket-ic/src/lib.rs index 74cb9dd6909c..7156c77c3c2c 100644 --- a/packages/pocket-ic/src/lib.rs +++ b/packages/pocket-ic/src/lib.rs @@ -2284,11 +2284,12 @@ pub struct StartServerParams { /// Attempt to start a new PocketIC server. /// -/// The returned [`ServerHandle`] owns the spawned server process. On unix it is -/// registered in a process-global registry and, once the test process exits, -/// automatically killed (its whole process group, so the canister-sandbox children are -/// terminated too) and reaped; on other platforms the server is not killed automatically -/// and shuts down on its TTL. Callers that want to terminate the server earlier can call +/// The spawned server process is owned by a process-global registry; the returned +/// [`ServerHandle`] only refers to it (dropping the handle does not stop the server). On +/// unix, once the test process exits, the server is killed (its whole process group, so +/// the canister-sandbox children are terminated too) and reaped on a bounded, best-effort +/// basis; on other platforms the server is not killed automatically and shuts down on its +/// TTL. Callers that want to terminate the server earlier can call /// [`ServerHandle::kill_and_wait`]. pub async fn start_server(params: StartServerParams) -> (ServerHandle, Url) { let default_bin_dir = diff --git a/packages/pocket-ic/src/server_process.rs b/packages/pocket-ic/src/server_process.rs index 3191085d8b23..c61a45ab4b76 100644 --- a/packages/pocket-ic/src/server_process.rs +++ b/packages/pocket-ic/src/server_process.rs @@ -68,8 +68,10 @@ fn lock_registry() -> MutexGuard<'static, Vec>> { /// process-global registry and returns a [`ServerHandle`] referring to it. /// /// The registry keeps the child alive for the remainder of the process (so the server -/// stays up for the whole test run) and guarantees it is killed and reaped at process -/// exit, replacing the previous "spawn and leak" behaviour. +/// stays up for the whole test run). On unix it is then killed (its process group) and +/// reaped at process exit on a bounded, best-effort basis, replacing the previous +/// "spawn and leak" behaviour; on other platforms the child is not reaped automatically +/// and shuts down on its TTL. pub(crate) fn register(child: Child) -> ServerHandle { #[cfg(unix)] let pgid = child.id() as libc::pid_t; @@ -86,22 +88,24 @@ pub(crate) fn register(child: Child) -> ServerHandle { /// A handle to a `pocket-ic-server` spawned by [`crate::start_server`]. /// -/// The server is owned by a process-global registry and is automatically killed and -/// reaped when the test process exits (on unix), so most callers can simply drop this -/// handle. Callers that want to terminate the server earlier — e.g. to test crash -/// recovery — can call [`ServerHandle::kill_and_wait`]. +/// The server is owned by a process-global registry. On unix it is killed and reaped +/// when the test process exits (on a bounded, best-effort basis), so most callers can +/// simply drop this handle. Callers that want to terminate the server earlier — e.g. to +/// test crash recovery — can call [`ServerHandle::kill_and_wait`]. pub struct ServerHandle { idx: usize, } impl ServerHandle { - /// Kills the server — its whole process group on unix, so the canister-sandbox - /// children are terminated too — and waits for it to be reaped, immediately. + /// Kills the server now — its whole process group on unix, so the canister-sandbox + /// children are terminated too — and makes a bounded, best-effort attempt to reap it. /// - /// This is a *hard* kill (`SIGKILL` on unix): it does not give the server a chance - /// to shut down gracefully or checkpoint state. Idempotent with respect to the - /// exit-time reaper: the server is removed from the registry here, so it is not - /// signalled or waited on again at process exit. + /// This is a *hard* kill (`SIGKILL` on unix): it does not give the server a chance to + /// shut down gracefully or checkpoint state. The reap is time-bounded, so on return + /// the process has been signalled (and, on unix, its whole group `SIGKILL`ed) but may + /// not yet be fully reaped. Idempotent with respect to the exit-time reaper: the + /// server is removed from the registry here, so it is not signalled or waited on again + /// at process exit. pub fn kill_and_wait(&self) { let taken = lock_registry().get_mut(self.idx).and_then(Option::take); if let Some(mut proc) = taken {