Skip to content

fix: shut pocket-ic down when the launcher fails to start - #79

Open
lwshang wants to merge 5 commits into
mainfrom
fix/orphaned-pocketic-on-startup-failure
Open

fix: shut pocket-ic down when the launcher fails to start#79
lwshang wants to merge 5 commits into
mainfrom
fix/orphaned-pocketic-on-startup-failure

Conversation

@lwshang

@lwshang lwshang commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Closes #76.

The leak

pocket-ic is spawned into its own process group and runs with --ttl 2592000 (30 days), so the launcher owns its lifecycle entirely — nothing else will ever clean it up. Only the happy path signalled it, so any failure after the spawn left the server and its sandbox children running orphaned for those 30 days. The Child handle escaped the async block only on success; on failure it was dropped with tokio's default kill_on_drop = false, so nothing signalled pocket-ic.

The fix

An RAII guard, as the issue suggested, constructed right at cmd.spawn() so every subsequent ? and panic is covered. On drop: SIGINT the process group (pocket-ic leads it, so its sandboxes go too), poll up to 5s, then SIGKILL the group unconditionally — that last part matters because the group leader can exit on SIGINT while children linger.

This replaces the happy-path-only cleanup rather than duplicating it, so one code path now covers the error return, the panic unwind, and the normal ctrl-c shutdown. It also let the sysinfo dependency go, whose only use was the old pid-only SIGINT.

Tests

The repo had none, so this adds the first one plus a cargo test CI job on ubuntu-24.04 and macos-15 (the platforms release.yml ships). tests/pocketic_cleanup.rs drives the real launcher binary against a fake pocket-ic — a small /bin/sh script — so it needs no pocket-ic download and runs in under a second. The fake records its own pid plus a background child's (standing in for the canister sandboxes, which share the server's process group but aren't the launcher's children), then fails the launcher's startup; the test asserts both are gone once the launcher has exited.

Against the unfixed launcher it reports the launcher exited but left pocket-ic server and sandbox child running. Against the fix it passes.

Verified against the real pocket-ic 15 server

Reproducing the issue's exact scenario — gateway port occupied, launcher panics with exit 101:

pre-fix (main) with fix
gateway port taken pocket-ic --ttl 2592000 left running nothing left behind
SIGTERM after startup exit 0, status dir removed, all 13 pocket-ic/sandbox processes gone
SIGINT after startup same, and the --state-dir state reloaded cleanly on the next run

Also in this PR

Two changelog/release-hygiene commits, easy to drop separately if you'd rather they went on their own:

  • docs(changelog) — release versions made misleading headers here: they track the bundled pocket-ic, and the date they embed is the dfinity/ic release date of that pocket-ic. The one existing entry showed it: --domain merged 2026-02-18 but sat under 12.0.0-2026-01-29-23-28.r1, dating it three weeks before it was written. Sections are now keyed by merge date, with the convention documented in the file (launcher behavior only; no section for a release that merely moved pocket-ic). Backfilled the five launcher-side changes since February — the file had gone untouched across 20 releases, so it silently claimed nothing had changed.
  • ci(release) — release bodies are empty today. --generate-notes fills in the merged-PR list, which is where the version→changes mapping lives now that the changelog is date-keyed.

Note for reviewers

Docker wasn't available on my machine, so the test only ran on macOS locally. The fake server is POSIX-clean (case/shift/$!/wait), so dash-as-/bin/sh on the Ubuntu runner should be fine — but CI is the first Linux run.

🤖 Generated with Claude Code

lwshang and others added 4 commits July 31, 2026 15:18
pocket-ic is spawned into its own process group and runs with a 30-day
--ttl, so the launcher owns its lifecycle entirely. Only the happy path
signalled it, so any failure after the spawn -- e.g. the HTTP gateway
failing to bind an already-used port, which panics with exit 101 -- left
the server and its sandbox children running orphaned for those 30 days.

Wrap the child in an RAII guard that signals the process group on drop:
SIGINT, up to 5s of grace, then SIGKILL for whatever is left in the
group. Because pocket-ic leads the group, that reaps its sandboxes too,
and running on Drop covers the error return, the panic unwind, and the
normal ctrl-c shutdown with one code path -- replacing the happy-path-only
cleanup along with its sysinfo dependency, whose only use was the
pid-only SIGINT.

Add the repo's first test, plus a cargo test CI job on Linux and macOS.
The test drives the real launcher binary against a fake pocket-ic server,
so it needs no pocket-ic download and runs in under a second: the fake
records its own pid and a background child's, then fails the launcher's
startup, and the test asserts both are gone once the launcher has exited.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Release versions make misleading changelog headers in this repo. They
track the bundled pocket-ic, and the date they embed is the dfinity/ic
release date of that pocket-ic -- nothing to do with when the launcher
itself changed. The one existing entry showed the failure directly: the
--domain flag merged 2026-02-18 but was filed under
12.0.0-2026-01-29-23-28.r1, dating it three weeks before it was written.

Key sections by merge date instead, and document the convention in the
file: launcher behavior only, and no section for a release that merely
moved pocket-ic. What a given release contains is a question for its
release notes, which are version-indexed and generated.

Also backfill the launcher-side changes since February. The file had gone
untouched across 20 releases, so it silently claimed nothing had changed
since the --domain flag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Release bodies are empty today, so nothing records what went into a
weekly release. --generate-notes fills in the merged-PR list for free,
which is where the version-to-changes mapping lives now that CHANGELOG.md
is keyed by date.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The repo's branch ruleset requires the `lint:required` and `build:required`
checks. Renaming the aggregate to `ci:required` left `lint:required` with
nothing to report it, which blocks the merge, so keep the pinned name and
note why it no longer matches what the job gates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a lifecycle leak where pocket-ic (spawned in its own process group with a 30-day --ttl) could be left orphaned when the launcher fails or panics after spawning it, and adds CI/test coverage plus release/changelog hygiene to make the behavior verifiable and easier to track.

Changes:

  • Introduces an RAII PocketIcProcess guard in src/main.rs to signal the pocket-ic process group on all exit paths (success, error, panic), removing the previous happy-path-only cleanup and dropping the sysinfo dependency.
  • Adds the first integration test (tests/pocketic_cleanup.rs) that runs the real launcher against a fake pocket-ic shell script and asserts both the server and a background “sandbox” process are cleaned up after launcher failure.
  • Updates CI to run cargo test (Ubuntu + macOS) and improves release notes generation; refreshes the changelog convention/content.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/main.rs Replaces happy-path-only cleanup with a drop-guard that shuts down the pocket-ic process group across all exit paths.
tests/pocketic_cleanup.rs Adds an integration test that reproduces the failure-after-spawn scenario and asserts no pocket-ic processes remain.
.github/workflows/ci.yml Adds a cargo test job (Ubuntu/macOS) and ensures aggregate gating covers both lint and test.
.github/workflows/release.yml Enables gh release create --generate-notes so release bodies include merged-PR notes.
Cargo.toml Removes sysinfo; enables nix signal feature needed for process group signalling.
Cargo.lock Updates lockfile to reflect dependency changes (notably removing sysinfo and transitive deps).
CHANGELOG.md Switches to date-keyed sections and backfills recent launcher-behavior changes.
Suppressed comments (1)

.github/workflows/ci.yml:39

  • This comment describes rust-cache behavior, but the workflow doesn't use rust-cache. Consider revising it to match what the step actually accomplishes (ensuring only the pinned toolchain is present / reducing disk usage).
      # rust-cache folds every installed toolchain's version into its cache key,
      # and the runner image's bundled `stable` drifts as the image updates, which
      # would miss the cache every run. Leave only the pinned toolchain.

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

Comment thread .github/workflows/ci.yml Outdated
The caching rationale referred to rust-cache, which does not appear in the
file: setup-rust-toolchain runs Swatinem/rust-cache itself, gated on a
`cache` input that defaults to true. Name that so the reasoning can be
checked from what the workflow shows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang
lwshang marked this pull request as ready for review July 31, 2026 19:56
@lwshang
lwshang requested a review from a team as a code owner July 31, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pocket-ic left orphaned when launcher exits on the error/panic path

2 participants