fix(PocketIC): Kill and reap the PocketIC server when all tests finish (experiment on RBE @ Namespace)#10760
Draft
basvandijk wants to merge 7 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>
- 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>
…jk/namespace-bazel-remote-execution-pocket-ic-fix
basvandijk
changed the base branch from
basvandijk/namespace-bazel-remote-execution
to
master
basvandijk
added
the
CI_RBE
basvandijk
changed the base branch from
master
to
basvandijk/namespace-bazel-remote-execution
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
1 participant
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.
WIP experiment: #10751 on top of #10579.