fix(PocketIC): Kill and reap the PocketIC server when all tests finish#10751
Draft
basvandijk wants to merge 5 commits into
Draft
fix(PocketIC): Kill and reap the PocketIC server when all tests finish#10751basvandijk wants to merge 5 commits into
basvandijk wants to merge 5 commits into
Conversation
`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>
Contributor
There was a problem hiding this comment.
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
atexitreaper on Unix to terminate the server process group andwaitpid-reap the server process. - Updates
start_serverto return aServerHandle(breaking change) and updates callers that explicitly terminate the server. - Documents the API change in the
CHANGELOGand addslibcdependency 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>
- 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>
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
start_serverused to spawn the PocketIC server, detach it into its own process group (process_group(0)) and discard theChildhandle. 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 theTODO: 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 only consider an action complete once those pipes reach EOF, this caused spurious failures.
Change
server_process.rs) that owns theChildexclusively.libc::atexitcallback runs once all subtests in the test binary have finished (i.e. the harness is exiting) and, for each registered server:SIGTERMto the whole process group — so the launcher/sandbox children die too and release the inherited pipes — escalating toSIGKILLafter a short, bounded grace period;waitpid-reaps the direct child.#[allow(clippy::zombie_processes)]workaround is removed.start_servernow returns aServerHandleinstead of astd::process::Child. Callers that previously kept theChildto terminate the server themselves should callServerHandle::kill_and_wait(seetests/unix.rs).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) — noted under## UnreleasedinCHANGELOG.md; warrants a major version bump at release time.Testing
rustfmt,clippy --all-features -p pocket-ic(strict flags), andbazel buildall clean. Ran the pocket-ic package suites and thepocket_ic_serverintegration tests (incl. the gateway / live-mode / bitcoind / dogecoind cases that spawn long-lived servers and sidecars) — all pass, and every test binary exits cleanly, confirming the atexit reaper runs without hanging.🤖 Generated with Claude Code