fix(SDK-1936): Kill and reap the PocketIC server when all tests finish by basvandijk · Pull Request #10751 · dfinity/ic

basvandijk · 2026-07-13T12:40:31Z

What

start_server used to spawn the PocketIC server, detach it into its own process group (process_group(0)) and discard the Child handle. As a result the server — and the canister-sandbox processes it re-execs, which inherit the server's stdio — outlived the test process and shut down only on its TTL (this was the TODO: SDK-1936).

A lingering server keeps the test action's inherited stdout/stderr pipe write-ends open past process exit. On some CI runners that don't do proper PID namespacing (like on RBE runners @ Namespace) that only consider an action complete once those pipes reach EOF, this caused spurious failures.

Change

Every spawned server is transferred to a process-global registry (server_process.rs) that owns the Child exclusively. This covers servers spawned implicitly through PocketIc/PocketIcBuilder, not just direct start_server callers.
On unix, a libc::atexit callback runs when the process exits normally (for a test binary: once all subtests have finished and the harness is exiting) and, for each registered server:
- sends SIGTERM to the whole process group — so the launcher/sandbox children die too and release the inherited pipes — escalating to SIGKILL after a short grace period. All groups are signalled up front and reaped against one shared deadline, so process exit is delayed by at most ~2×2s in total, no matter how many servers are still running;
- waitpid-reaps the direct child;
- removes the killed server's port file (the server only cleans it up itself on graceful shutdown), so a stale reuse: true port file can't hand a later process with a recycled pid a dead URL.
- The cleanup does not run if the process terminates abnormally (killed by a signal, abort); the server then falls back to shutting down on its TTL, as before.
start_server now returns a ServerHandle instead of a std::process::Child:
- ServerHandle::kill_and_wait — terminate the server now (see tests/unix.rs);
- ServerHandle::pid — the OS pid of the spawned process (replaces Child::id);
- ServerHandle::detach — deliberately opt back into the old detached behaviour when the server must outlive the spawning process (it is then bounded by its TTL again).
Registration opportunistically cleans up candidates that already exited (e.g. reuse: true candidates that lost the port-file race), reaping their zombies instead of accumulating them for the lifetime of the process — this also closes SDK-1936's zombie leak during long runs, not just at exit — and the #[allow(clippy::zombie_processes)] workaround is removed.
fork() is handled: a forked child never kills the parent's servers (the reaper checks the registry's owner pid), and a forked child that spawns servers of its own adopts the registry so they are still cleaned up at its exit.

This is gated behind #[cfg(unix)]; Windows behaviour is unchanged (the server is still bounded by its TTL there).

API impact

Breaking change to the public start_server (return type) plus the new ServerHandle type — documented under ## Unreleased in CHANGELOG.md with migration guidance; warrants a major version bump at release time.

Testing

Two new regression tests in tests/unix.rs pin the new behaviour by re-invoking the test binary as a helper that spawns a server and exits:

server_is_killed_when_process_exits asserts the helper's stdout reaches EOF (i.e. the server and its sandbox children released the inherited pipes — exactly the CI hang this PR fixes) and that the server pid is gone afterwards;
detached_server_survives_process_exit asserts a detach()ed server outlives the helper.

rustfmt, clippy --all-features --all-targets -p pocket-ic (strict flags), and bazel build all clean. Ran the pocket-ic package suites, the pocket_ic_server integration tests (incl. the gateway / live-mode / bitcoind / dogecoind cases that spawn long-lived servers and sidecars), and the full set of ~148 bazel test targets depending on the pocket-ic crate (NNS/SNS integration tests, ledger, rosetta, …) — all pass, and every test binary exits cleanly, confirming the atexit reaper runs without hanging.

Builds and tests involving pocket-ic also succeed on the RBE cluster @ Namespace.

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes pocket-ic test-runner hangs caused by spawned pocket-ic-server (and its sandbox descendants) outliving the test process and keeping inherited stdout/stderr pipes open. It introduces process-lifetime management for the server on Unix by tracking spawned servers in a global registry and reaping them at process exit, and updates the public API accordingly.

Changes:

Introduces a process-global server registry and an atexit reaper on Unix to terminate the server process group and waitpid-reap the server process.
Updates start_server to return a ServerHandle (breaking change) and updates callers that explicitly terminate the server.
Documents the API change in the CHANGELOG and adds libc dependency wiring.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/pocket-ic/tests/unix.rs	Updates test to use `ServerHandle::kill_and_wait` instead of `Child::kill`.
packages/pocket-ic/src/server_process.rs	Adds global registry + Unix `atexit` reaper to kill process groups and reap server children.
packages/pocket-ic/src/lib.rs	Switches `start_server` return type to `ServerHandle` and registers spawned server in registry.
packages/pocket-ic/CHANGELOG.md	Notes the breaking API change and new lifecycle behavior (Unix).
packages/pocket-ic/Cargo.toml	Adds `libc` as a Unix-only dependency.
packages/pocket-ic/BUILD.bazel	Adds `libc` to Bazel deps for the crate.
Cargo.lock	Records the `libc` dependency addition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- 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) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 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) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

…tees 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) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

… API Implements the suggestions from the code review of this PR: - Add ServerHandle::pid() (replaces the lost Child::id capability), ServerHandle::detach() (deliberately restore the old outlive-the-process behaviour) and #[derive(Debug)]. - Guard the atexit reaper against running in fork()ed children, which inherit the registry and would kill the parent's live servers. - Kill servers registered after the exit-time drain (by threads that outlive main) immediately instead of leaking them past the reaper. - Reap in parallel at exit: signal all process groups up front and poll against one shared deadline, bounding exit latency at ~2x2s total instead of per server. - Remove the port file of servers killed by kill_and_wait or the exit-time reaper (which skip the server's own graceful cleanup), so a stale reuse port file cannot hand a later process with a recycled pid a dead URL. - Opportunistically clean up already-exited server candidates on registration (via a non-reaping waitid(WNOWAIT) peek, group SIGKILL for surviving group members, then reap), so reuse-loser zombies no longer accumulate for the lifetime of long-lived processes. - Add regression tests for the exit-time reaper: a helper re-invocation of the test binary spawns a server and exits; the outer test asserts the inherited pipes are released (EOF) and the server is gone, plus the detach() counterpart asserting survival. - Document the exact semantics (normal-exit-only cleanup, blocking bounds, reuse caveats on all handle methods, stale-pid hazards) and rewrite the CHANGELOG entry with an Added section and full migration guidance. - Rename stale _child bindings in tests/tests.rs to _server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

- Adopt the registry in fork()ed children that spawn their own servers: the first post-fork registration disowns the inherited (parent-owned) entries and updates owner_pid, so the inherited exit-time reaper cleans up exactly the child's servers while never touching the parent's. - Read the spawn-server helper mode via std::env::var instead of var_os for clarity. - Drop the redundant libc dev-dependency: integration tests already compile against the crate's unix libc dependency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

mraszyk · 2026-07-16T10:16:19Z

+//! 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`.


If inheriting the pipes is the issue, then an easier solution would be to do

cmd.stdin(std::process::Stdio::null()); cmd.stdout(std::process::Stdio::null()); cmd.stderr(std::process::Stdio::null());

when spawning the PocketIC server in start_server and let the pipes be not inherited. CC @basvandijk

Although that would likely hide all server output so it has its trade-off.

I checked the PocketIC server output and it basically consists of canister logs produced during the test run so it might be a reasonable trade-off to turn them off on CI.

Note that Namespace just implemented proper sandboxing so we no longer strictly need this.

I do remember we used to have many stray pocket-ic-server zombies on our Darwin runners. So in general I think it's good that a test kills all processes it started. However this does add quite a bit of complexity which might be overkill.

@nmattia what's your opinion?

github-actions Bot added the fix label Jul 13, 2026

basvandijk requested a review from Copilot July 13, 2026 12:40

Copilot started reviewing on behalf of basvandijk July 13, 2026 12:41 View session

basvandijk added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Jul 13, 2026

trigger

0aca829

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread packages/pocket-ic/src/server_process.rs Outdated

Comment thread packages/pocket-ic/src/lib.rs Outdated

basvandijk requested a review from Copilot July 13, 2026 12:54

Copilot started reviewing on behalf of basvandijk July 13, 2026 12:54 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread packages/pocket-ic/src/server_process.rs Outdated

Comment thread packages/pocket-ic/src/server_process.rs Outdated

Comment thread packages/pocket-ic/src/server_process.rs Outdated

basvandijk requested a review from Copilot July 13, 2026 13:30

Copilot started reviewing on behalf of basvandijk July 13, 2026 13:32 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread packages/pocket-ic/src/server_process.rs Outdated

Comment thread packages/pocket-ic/src/server_process.rs Outdated

Comment thread packages/pocket-ic/src/lib.rs Outdated

basvandijk requested a review from Copilot July 13, 2026 14:29

Copilot started reviewing on behalf of basvandijk July 13, 2026 14:29 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

basvandijk marked this pull request as ready for review July 13, 2026 15:40

basvandijk requested a review from a team as a code owner July 13, 2026 15:40

github-actions Bot added the @core-protocol label Jul 13, 2026

basvandijk mentioned this pull request Jul 13, 2026

fix(PocketIC): Kill and reap the PocketIC server when all tests finish (experiment on RBE @ Namespace) #10760

Draft

basvandijk changed the title ~~fix(PocketIC): Kill and reap the PocketIC server when all tests finish~~ fix(SDK-1936): Kill and reap the PocketIC server when all tests finish Jul 13, 2026

basvandijk requested a review from Copilot July 15, 2026 10:33

Copilot started reviewing on behalf of basvandijk July 15, 2026 10:33 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread packages/pocket-ic/tests/unix.rs

Comment thread packages/pocket-ic/src/server_process.rs

Comment thread packages/pocket-ic/Cargo.toml

basvandijk requested a review from Copilot July 15, 2026 10:57

Copilot started reviewing on behalf of basvandijk July 15, 2026 10:57 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

mraszyk reviewed Jul 16, 2026

View reviewed changes

Uh oh!

Conversation

basvandijk commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Change

API impact

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mraszyk Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

mraszyk Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

mraszyk Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

basvandijk Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

basvandijk commented Jul 13, 2026 •

edited

Loading