Repository navigation
fix(microvm): reap the manager's whole process group on teardown - #207
Merged
Merged
Conversation
A failed integration-test run left a live runtime-manager holding :50054. Every
LATER run then could not bind and reported "Firecracker backend did not report
available" — a symptom that points at the backend, or at whatever was last
edited, rather than at the leaked process. It cost three runs during the harness
startup work and briefly looked like a code regression; the A/B that cleared it
only worked because the host was cleaned by hand in between.
`kill "${MANAGER_PID}"` was already there. It was not enough, in two ways that
compound: the manager does not reliably exit on SIGTERM, and its firecracker
children are reparented rather than killed when it does.
So the manager now starts via `setsid`, and cleanup signals the whole process
group — TERM, a bounded wait for a graceful exit, then KILL. Deliberately not a
host-wide `pkill -f firecracker`: that would kill unrelated VMs, and on the
usual dev host (a Lima VM named `firecracker-dev`) the pattern also matches the
caller's own shell command line and kills the session running the script.
The group id is read back with `ps -o pgid=` rather than assumed equal to the
captured pid: `setsid` execs in place when the caller is not already a group
leader but forks when it is, and only one of those keeps that pid.
Teardown then waits for the PORT, not the pid — the port is what the next run
needs back — and warns if it is still held, so the leak is reported by the run
that caused it. A matching pre-flight fails fast when the port is already taken,
naming the likely cause and the command to find it, so the misleading message
above cannot recur.
Verified on real KVM, all three paths:
- forced failure (corrupt rootfs, panics after the manager is up)
-> manager 0, firecracker 0, port free [previously: manager alive, port held]
- normal run -> 5/5 assertions, host clean afterwards; the TAP orphaned by the
failed run above went 1 -> 0, confirming the manager's startup
reconciliation handles the one thing group-kill cannot
- port pre-occupied -> fails immediately with the real cause, not a backend error
Contributor
🔎 Codex cross-audit (agent:audit)No Blocking findings. I audited only Verification performed:
I could not run the live Firecracker path in this read-only/no-KVM environment, so runtime validation still depends on the KVM CI job. |
compass-sdlc-bot
Bot
added
the
agent:reviewed-clean
Contributor
🔷 Gemini cross-audit (agent:audit-gemini)Independent Security & Correctness Audit of
|
github-actions
Bot
added
the
domain:infra
Contributor
🔎 Codex cross-audit (agent:audit)Findings
No Blocking issues found in the PR diff. Syntax checks passed; the CI-style non-interactive |
Contributor
🔷 Gemini cross-audit (agent:audit-gemini)🚨 BLOCKING REGRESSIONS & CORRECTNESS ISSUES1. Signal Storm / Shell Killing Itself in Non-Interactive Environments (CI)
2. Process Leaks in Interactive Environments (Local Dev Hosts)
3. Broken Startup & "Manager Exited Early" False Failures
💡 PROPOSED FIXTo safely isolate the manager and its children in their own process group, capture the PGID reliably, and prevent leaks across all environments, use a subshell to write the PGID to a PID file before # Start the manager in a new session and write its PGID (which is the child's PID) to a file
setsid sh -c 'echo "$$" > "${WORK}/manager.pid"; exec env \
RUNTIME_BACKEND=firecracker \
LANTERN_RUNTIME_BACKEND=firecracker \
LISTEN_ADDR="${MANAGER_LISTEN_ADDR}" \
... \
"${MANAGER_BIN}"' >"${MANAGER_LOG}" 2>&1 &
MANAGER_PID=$!
# Read the correct, active PGID and PID
for _ in $(seq 1 50); do
[ -s "${WORK}/manager.pid" ] && break
sleep 0.1
done
[ -s "${WORK}/manager.pid" ] || fail "manager did not start"
MANAGER_REAL_PID=$(cat "${WORK}/manager.pid")
MANAGER_PGID="${MANAGER_REAL_PID}"Then update the startup check and cleanup to reference
|
…nference
Both PR auditors flagged the first cut, and they were right. `setsid` behaves
two different ways depending on whether its caller is already a process group
leader, and the patch guessed at both:
- Caller NOT a leader (a script under CI, job control off): setsid execs in
place, so the leader keeps the pid `$!` returned — but only after setsid()
has actually run. Reading the pgid immediately after `&` is a race, and
losing it yields the SCRIPT'S OWN pgid. Cleanup would then have killed this
script's process group, taking the runner step with it. That is the very
self-kill the comment two lines above warns about for `pkill -f
firecracker`, reached from the other direction. My verification passed only
because the race happened to resolve favourably.
- Caller IS a leader (sourced from an interactive shell): setsid forks, `$!`
is the short-lived wrapper. The liveness check would have called it dead at
once ("manager exited early"), and the `ps` fallback would have signalled a
group the manager was never in — leaking exactly what this PR set out to fix.
So stop inferring it. The manager now launches behind a one-line wrapper that
publishes its own `$$` and then execs the manager. Inside that wrapper `$$` is
the session leader in BOTH cases, and the exec keeps the pid, so the published
value is the manager's pid and its process group id at once. The script waits
for that file — a handshake, with no race left to lose — and uses it for
liveness too.
Cleanup additionally refuses to signal a group equal to this script's own. The
handshake should make that unreachable; it is there because the cost of being
wrong is the whole run plus its diagnostics.
Verified on real KVM across both setsid paths and the failure path:
- job control OFF, normal run -> 5/5, host clean
- job control ON (fork path),
normal run -> 5/5, host clean [previously: aborted
immediately with "manager exited early"]
- job control ON, forced failure -> correct FAIL; manager 0, firecracker 0,
port free [previously: wrong group
signalled, manager leaked]
The refusal guard never fired in any run, which is the expected result.
Contributor
🛡️ Vuln scan — ❌ vulnerable dependency found |
dshakes
added
agent:audit
Contributor
🔎 Codex cross-audit (agent:audit)Blocking
Notes
|
Contributor
🔷 Gemini cross-audit (agent:audit-gemini)Blocking Correctness Regressions
Enhancements & Robustness
|
Second round of auditor findings on this PR, both correct. 1. Killing the manager's process group never reaped its VMs. The backend launches each firecracker with `pre_exec(libc::setsid())` (backends/firecracker.rs), so every VM leaves the manager's session before exec and survives a group kill. A run failing after Spawn but before the explicit Stop left a live VM behind — and because the manager itself DID die, the port check came back clean and the run reported a tidy teardown while the VM leaked. That is the same shape of false green this file keeps producing: the check passed because it was measuring the wrong thing. My own verification missed it for the same reason. The forced-failure case I tested panics the guest, and a panicking VM exits on its own, so firecracker was 0 for a reason that had nothing to do with the cleanup being correct. Cleanup now issues Stop through the manager FIRST, while it is still alive. That is also the only path that releases the VM's TAP, so it fixes the tap orphan the previous round left behind rather than deferring it to the next run's reconciliation. A uuid-scoped backstop (matching this run's vm_id in the api socket path) covers a VM that outlives the manager anyway — deliberately not `pkill -f firecracker`, which matches the usual dev host's Lima VM name and this script's own command line. 2. `MANAGER_PGID` was only assigned after the handshake, so a handshake that timed out while the manager was running leaked it — a regression against the old `MANAGER_PID=$!`, which covered exactly that window. The launch pid is now recorded before the handshake and used as the fallback, and cleanup re-reads the pgid file in case it was published late. Verified on real KVM by fault injection — a synthetic failure after a HEALTHY boot, which is the case that leaks (the panicking guest hid it): previous commit -> firecracker 1 LEAKED, tap 1, port free (false green) this commit -> firecracker 0, tap 0, port free Plus the three paths from the previous round still clean: job control on and off, and the forced-failure case.
dshakes
added
agent:audit
Contributor
🧪 QA — ❌ tests failing |
Contributor
🔎 Codex cross-audit (agent:audit)Blocking: none found. Findings:
Validation run:
|
Contributor
🔷 Gemini cross-audit (agent:audit-gemini)Audit Report:
|
Contributor
🛡️ Vuln scan — ❌ vulnerable dependency found |
Both non-blocking findings from the third audit round. The backstop matched `/run/firecracker/<vm_id>.sock`, which is the socket path only in the default configuration. Under jailer mode (FIRECRACKER_JAILER_PATH) the socket lives inside the jail root instead, so the match would miss and the VM would still leak — the exact case the backstop exists for. It now matches the vm_id anywhere in the argv of a process actually named `firecracker`, which covers both layouts. Scope is unchanged: the id is a uuid minted for this run, and restricting to `pgrep -x firecracker` means it cannot match another run, an unrelated hypervisor, or this script. Group kills pass `--` so a negative pgid can never be parsed as an option by a kill implementation that reads it that way. Verified on real KVM: fault injection after a healthy boot still leaves manager 0, firecracker 0, taps 0, port free; and a normal run is 5/5 with a clean host.
Contributor
🧪 QA — ❌ tests failing |
Contributor
🛡️ Vuln scan — ❌ vulnerable dependency found |
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.
What
A failed
integration-test.shrun left a liveruntime-managerholding:50054. Every later run then failed to bind and reported "Firecracker backend did not report available" — a symptom pointing at the backend, or at whatever was last edited, rather than at the leaked process.It cost three runs during the harness startup work and briefly looked like a code regression. The A/B that cleared it only worked because the host was cleaned by hand in between.
Why the existing kill wasn't enough
kill "${MANAGER_PID}"was already incleanup. Two things compound past it:SIGTERM.Change
setsid; cleanup signals the whole process group — TERM, bounded wait, then KILL.pkill -f firecracker: that would kill unrelated VMs, and on the usual dev host (a Lima VM namedfirecracker-dev) the pattern also matches the caller's own shell command line and kills the session running the script.ps -o pgid=rather than assumed equal to the captured pid —setsidexecs in place when the caller isn't already a group leader but forks when it is, and only one keeps that pid.Verification (real KVM, all three paths)
The TAP result also confirms the manager's startup reconciliation covers the one thing a group-kill cannot.