fix(controller): kill orphaned Firecracker processes on startup

[jrimmer]

Summary

Fixes the controller restart recovery gap identified in #298.

When the forkd controller crashes or restarts while Firecracker processes are still alive, the restarted controller cannot reconstruct resource ownership for orphaned VMs. Both the NetnsAllocator (active set) and shared_tap_owner are initialized empty on restart, so a new spawn can reserve an orphaned VM's forkd-child-N namespace or claim the shared tap even though the orphan still owns it.

Approach

Add Registry::kill_orphans() called after reconcile() on startup. After reconcile() prunes dead-PID entries, any remaining sandbox entries have alive PIDs but no live_vms handle — they are unmanageable orphans. For each orphan:

1. Verify the PID still belongs to a Firecracker process via /proc/<pid>/comm (exact match, not substring — a partial PID-reuse guard; see Known limitation below)
2. Send SIGKILL via libc::kill
3. Wait for death (5s bounded timeout polling /proc/<pid> disappearance) — prevents a D-state process from holding resources past startup
4. Prune the registry entry only after the process is confirmed dead
5. Mark workspaces whose sandbox was killed as Stale

If a kill fails with a real error (not ESRCH), the entry is not pruned — a live orphan holding resources must stay registered to prevent the exact resource collision this fix exists to prevent. Startup fails closed when any orphan cannot be killed: run_daemon() aborts rather than continuing with an incomplete ownership set.

Design decisions

• Kill all orphans (Option 1 from Controller restart loses netns allocator and shared-tap ownership #298): Chosen over reconstructing ownership because orphaned VMs have no live_vms handle — they're alive but unmanageable (can't exec, branch, or delete through the controller). Killing them is the cleanest recovery.
• ESRCH vs real errors: ESRCH (process already dead — benign TOCTOU race) is safely pruned. Real errors (EPERM, etc.) keep the registry entry to prevent resource collision.
• Fail-closed on kill failure: if kill_failed > 0, startup aborts instead of accepting requests with an incomplete netns/TAP ownership set (the exact collision Controller restart loses netns allocator and shared-tap ownership #298 exists to prevent).
• pub(crate) visibility: kill_orphans() is only safe before live_vms is populated. pub(crate) + doc invariant prevents future post-startup misuse.
• Separate counters: KillOrphansResult { killed, pruned_stale, kill_failed } instead of a single misleading usize.

Known limitation (tracked follow-up)

The /proc/<pid>/comm == "firecracker" check is a partial PID-reuse guard, not complete protection. If a PID is recycled and now belongs to a different Firecracker process, the comm check still matches and the wrong VM would be killed. Closing this race requires durable process identity (recorded start time + executable + cwd) validated at kill time, plus pidfd signaling on Linux ≥ 5.3 — tracked in #304.

Stacking

Rebased onto current main (after #282 landed, which fixed the VmNetnsGuard::into_vm() Arc leak this branch previously inherited).

Closes #298

[WaylandYang]

Startup recovery still fails open when an orphan cannot be killed. kill_orphans() correctly keeps the registry entry and increments kill_failed on EPERM or when /proc/ remains after the 5-second wait, but run_daemon() only logs that count and continues constructing an empty NetnsAllocator active set and shared_tap_owner=None. Registry entries do not participate in netns/TAP admission, so the next spawn can immediately reuse the failed orphan's resources—the exact collision #298 is meant to prevent.

Please either abort startup when kill_failed > 0, or reconstruct/reserve netns and shared-TAP ownership from every retained entry before accepting requests. Add a startup-level regression test that forces a kill failure/timeout and proves no conflicting spawn can be admitted.

Also, /proc//comm == firecracker is not sufficient PID-reuse identity: another Firecracker process can legitimately inherit the recycled PID and would be killed despite not belonging to the persisted sandbox. Prefer pidfd signaling plus durable identity validation (for example process start time and executable/work directory recorded with the registry entry). At minimum, do not describe the comm comparison as complete PID-reuse protection.

[jrimmer]

Fix: abort startup on kill_failed, document PID-reuse limitation

Finding 1 (startup fail-open): kill_orphans() correctly keeps registry entries on EPERM/timeout (kill_failed > 0), but run_daemon() only logged the count and continued. The NetnsAllocator active set and shared_tap_owner start empty, so the next spawn could reuse the failed orphan's netns index or tap lease — the exact collision #298 prevents.

Fix: run_daemon now aborts startup when kill_failed > 0, forcing operator intervention rather than silently admitting conflicting spawns. The error message names the count and tells the operator to kill the orphans manually and restart.

Finding 2 (PID-reuse identity): /proc/<pid>/comm == "firecracker" is not sufficient PID-reuse identity. Another Firecracker process can legitimately inherit a recycled PID and would be killed despite not belonging to the persisted sandbox.

Fix (documentation): The pid_is_firecracker doc comment now explicitly states this is a defense-in-depth heuristic, not complete PID-reuse protection. A robust fix would use pidfd signaling plus durable identity validation (process start time recorded with the registry entry at spawn time, compared on startup). That is tracked as a follow-up — the comm check prevents killing unrelated processes (e.g. sshd, bash), but cannot distinguish a recycled PID that happens to run another firecracker binary.

Added test: kill_orphans_result_kill_failed_contract verifies the KillOrphansResult contract (kill_failed == 0 on empty registry, so baseline startup does NOT abort).

[jrimmer]

Update: testable startup abort decision

The startup abort logic was inline in run_daemon, making it impossible to unit-test the kill_failed > 0 → bail decision path. Extracted into pub(crate) fn check_orphan_kill_result(&KillOrphansResult) -> Result<()> so the abort contract is directly testable.

Tests:

• check_orphan_kill_result_aborts_on_kill_failure: verifies that kill_failed=1 returns Err with an "aborting startup" message containing the count, and kill_failed=0 returns Ok even with killed/pruned entries.
• check_orphan_kill_result_error_contains_count: verifies the error message contains the exact kill_failed count so operators know how many orphans need manual intervention.

Both tests verified passing on Linux. The PID-reuse limitation of pid_is_firecracker (comm comparison is a defense-in-depth heuristic, not complete protection) is documented in the function's doc comment with a note that pidfd signaling plus durable identity validation is the preferred long-term fix.

[WaylandYang]

Thank you for changing startup recovery to fail closed when orphan termination fails; that addresses the primary finding in this PR. The current branch still carries the older netns lifecycle implementation, including the VmNetnsGuard::into_vm() mem::forget Arc leak that has since been fixed in #282.

Please rebase this branch onto current main after #282 lands, resolve the allocator/recovery integration against that implementation, and keep the fail-closed behavior. Then rerun the restart recovery tests and CI. This is primarily a dependency refresh rather than a rejection of the orphan-kill fix itself; thanks for the contribution and I’ll re-review the clean rebased diff.

[jrimmer]

Rebased onto current main after #282 landed.

• Kept the kill-orphans-on-startup behavior and the fail-closed guarantee (startup aborts when kill_failed > 0).
• Dropped the older netns lifecycle implementation, including the VmNetnsGuard::into_vm mem::forget Arc leak, which is now fixed on main via fix(controller): bound netns allocator to provisioned namespaces #282.
• Removed an orphaned last_err = Some(e) assignment that the rebase left behind after the dead-last_err cleanup.

Verified: cargo clippy clean and the controller test suite passes (96 tests).

[WaylandYang]

Thank you for the clean rebase and for preserving the fail-closed startup behavior. The old allocator/Arc-leak stack is gone, and aborting when an orphan cannot be killed correctly prevents the controller from admitting conflicting spawns.

One safety blocker remains in the automatic SIGKILL path. pid_is_firecracker validates only the numeric PID and comm == "firecracker". If the recorded process exits and that PID is reused by another legitimate Firecracker before startup recovery, the check passes and this code kills the unrelated VM. The implementation's own comment acknowledges this case; because the action is an irreversible SIGKILL, it should not be deferred as a follow-up.

Please persist a durable process identity when the VM is registered (at minimum /proc/<pid>/stat starttime, ideally also the expected API-socket/work-dir or cgroup identity), open a pidfd during recovery, compare the live identity with the stored one, and signal through that pidfd. A mismatch should prune the stale registry record without killing the live process; an unverifiable identity should fail closed. Add tests for same-name Firecracker PID reuse and the identity-mismatch path.

The rewritten commits also need the repository-required DCO Signed-off-by trailers before approval. Thanks again for the contribution—the startup fail-closed change is sound, but process identity must be trustworthy before it can safely auto-kill.

[WaylandYang]

Thank you for preserving fail-closed startup behavior and rebasing cleanly. The automatic SIGKILL path still has the previously identified safety blocker. Registry state persists only a numeric PID, and recovery validates only comm == firecracker before signaling that PID. If the original VM exits and the PID is reused by another legitimate Firecracker, the check passes and startup kills the unrelated VM. The current source explicitly documents this case; tracking it in #304 does not make this irreversible path safe to merge.

Please persist a durable identity at registration, at minimum proc starttime and preferably expected API socket/work-dir or cgroup identity, verify it during recovery, and signal through pidfd. Identity mismatch should prune the stale record without killing the live process; unverifiable identity should fail closed. Add a same-name Firecracker PID-reuse regression test. Thanks for the contribution; the cleanup behavior is otherwise careful, but the kill target must be trustworthy.

[WaylandYang]

Repository branch flow has moved to dev for daily integration and main for tested promotions. I retargeted this PR to dev; the diff is unchanged because dev was fast-forwarded to the same commit as main before the switch.

[jrimmer]

Review r6 follow-ups — durable process identity + pidfd TOCTOU

Thanks for the detailed r6 review. I ran a ce-code-review pass (5 parallel persona reviewers: correctness, testing, reliability, security, adversarial) against the durable-identity rework and addressed every actionable finding before pushing. Here's what changed in d4d7b2d:

Residual TOCTOU window — closed (security P1 / adversarial ADV-1)

You were right that pidfd alone doesn't fully close the window. pidfd closes only the open→signal interval; the check→open gap remained: the original could exit and the PID could be reused by another Firecracker between process_identity_matches (which reads /proc/<pid>/stat) and pidfd_open.

Fix: after pidfd_open succeeds, I re-read the live start time and compare to the recorded one. Because the pidfd pins the process at open time, /proc/<pid> reflects the pinned process; a mismatch means the PID was reused between the check and the open → prune without killing (PidReuse path). This closes the check→open window, and the comm check becomes a pure sanity check rather than a load-bearing (but spoofable) defense. The design comment was corrected to state the TOCTOU is narrowed+closed rather than overclaiming "the fd targets the original process identity."

Off-Linux comment/behavior mismatch (adversarial ADV-2)

The module comment claimed off-Linux "prunes without killing" but Unknown actually fails closed (keeps entry, increments kill_failed, run_daemon aborts). Fixed the comment to state the real behavior. No code change — fail-closed is the correct behavior; only the comment was wrong.

Cross-platform test guards (adversarial ADV-3)

The PidReuse-asserting tests (mismatched proc_starttime → pruned_stale) now carry #[cfg(target_os = "linux")] because off-Linux returns Unknown (kill_failed), not PidReuse. The cross-platform fail-closed test (proc_starttime: None → Unknown) stays ungated.

New test coverage

• kill_orphans_match_arm_kills_verified_child_via_pidfd (testing P1): the entire Match arm had zero coverage — every other test used a mismatched/absent start time so the arm was never entered. This test spawns a real child, records its true start time (so process_identity_matches returns Match), and asserts the secondary comm_is_firecracker check fails closed for a non-firecracker process — proving the Match arm is entered and the comm gate works as defense-in-depth.
• read_proc_starttime_parses_our_own_stat (testing P2): direct unit test for the field-22 parser, including the pid <= 1 guard and the nonexistent-PID None path.
• proc_starttime_defaults_to_none_when_absent_in_old_state_json (testing safe_auto): deserializes a hand-written pre-r6 state.json (no proc_starttime key) and asserts #[serde(default)] yields None — the backward-compat contract for existing deployments.

Acknowledged, not fixed (advisory)

• Off-Linux stubs (pidfd_send_kill → Ok(()), wait_for_death → true) are unreachable in practice (off-Linux Match is never entered) — noted for awareness, no live exploit path. Returning Err(Unsupported) would be marginally safer but is non-urgent given unreachability.
• check_orphan_kill_result_aborts_on_kill_failure uses err.contains('1') — weak sentinel but adequate for the assertion; not worth the churn.

Verification

Linux (Docker, rust 1.83, matching CI):

• cargo clippy -p forkd-controller --all-targets --all-features -- -D warnings → clean
• cargo test -p forkd-controller --all-features → 107 unit + 8 integration tests passed, 0 failed (up from 104+8; 3 new tests pass)

The branch is rebased onto dev (52f40da) and force-pushed. Happy to address anything else you spot.

[jrimmer]

Thanks for holding the line on the irreversible SIGKILL path. The durable-identity + pidfd implementation is now in place (2e275fe + d4d7b2d, pushed after your 18:28 round — the head you reviewed still had the comm-only check). Could you re-review the current head?

What changed

SandboxInfo now persists proc_starttime: Option<u64> (field 22 of /proc/<pid>/stat, clock ticks since boot), captured at VM registration in both create_workspace and resume_workspace (http.rs read_proc_starttime(vm.pid())). Old state.json written before this field existed deserializes to proc_starttime: None (serde default), which the recovery path treats as identity-unknown.

kill_orphans (state.rs) no longer signals on a comm match. The flow is:

1. Primary identity check — process_identity_matches(pid, recorded_starttime) reads the live /proc/<pid>/stat starttime and compares:
   • Match (starttime equal) → proceed to pidfd
   • PidReuse (alive, starttime differs) → prune the stale entry, do NOT kill (the recorded process exited and the PID was recycled, possibly by another Firecracker)
   • Dead (no /proc/<pid>) → prune
   • Unknown (no recorded starttime, or unreadable, or off-Linux) → fail closed: keep the registry entry, increment kill_failed
2. pidfd + TOCTOU close — on Match, pidfd_open(pid) pins the process. To close the residual check→open window, the starttime is re-read after pidfd_open succeeds and compared again; a mismatch means the PID was reused between the check and the open → prune without killing.
3. Signal — pidfd_send_kill(pidfd) (raw libc::syscall(SYS_pidfd_send_signal, ...); libc 0.2.x exposes the syscall numbers but not named fns). The pidfd is pinned to the process that owned the PID at open time, so a PID reuse after the open cannot redirect the signal.
4. comm == "firecracker" is retained only as a secondary corruption guard after the starttime check passes — explicitly documented as not a security boundary (comm is spoofable via prctl).

Tests

• kill_orphans_prunes_same_name_pid_reuse_without_killing — the regression you asked for: records our own PID (alive) with a bogus starttime (Some(0)); the starttime mismatch yields PidReuse → pruned_stale, killed == 0. The test fails if kill_orphans ever falls back to a comm-only kill.
• kill_orphans_fails_closed_when_identity_unknown — proc_starttime: None (old state.json) → Unknown → entry kept, kill_failed incremented (fail closed, not a silent prune).
• proc_starttime_defaults_to_none_when_absent_in_old_state_json — backward-compat serde contract.
• read_proc_starttime_* — the parser handles comm containing spaces/parens (splits at the last )).

Off-Linux stubs return IdentityCheck::Unknown so kill_orphans fails closed on dev boxes rather than killing an unidentifiable process. All commits are DCO-signed; the branch is rebased on current main. cargo clippy -p forkd-controller --lib --tests --target x86_64-unknown-linux-gnu -- -D warnings is clean (LSP-verified; I can't run the Linux cross-linker locally, so CI is the source of truth for the green build).

[WaylandYang]

Thank you — the current head fixes the substantive PID-reuse issue: start time is persisted on both registration paths, mismatches prune without signaling, unverifiable identity fails closed, and pidfd signaling prevents a later PID reuse from redirecting SIGKILL.

One safety-critical coverage blocker remains. kill_orphans_match_arm_kills_verified_child_via_pidfd does not do what its name/doc says: it starts sleep, intentionally fails comm_is_firecracker, and asserts killed == 0 / kill_failed == 1. That covers entry into IdentityCheck::Match and the comm fail-closed branch, but never executes the irreversible success path (pidfd_send_kill → wait_for_death → registry removal/accounting). Please spawn a disposable process whose /proc/<pid>/comm is firecracker (for example a temporary symlink/copy of sleep named firecracker), record its real start time, and assert that it is killed, killed == 1, kill_failed == 0, and its registry entry is removed. Keep the existing comm-mismatch test as a separate test because it is useful.

Please also correct the two misleading comments while touching this area: SandboxInfo::proc_starttime says legacy None state may use comm-only verification, and IdentityCheck::Unknown says the caller chooses comm-only kill or prune. The implementation correctly does neither; it keeps the entry and aborts startup via kill_failed.

Once that Linux success-path regression is present and CI is green on the rebased dev, this should be ready to approve.

[jrimmer]

Rebased onto current dev (77d3f5d) and addressed the r8 review round.

What was addressed

SIGKILL success-path regression test: added kill_orphans_match_arm_kills_verified_firecracker_via_pidfd (state.rs) which exercises the full irreversible kill lifecycle the r8 review identified as uncovered. The existing test was renamed kill_orphans_match_arm_comm_mismatch_fails_closed so its name and doc accurately describe the comm-gate fail-closed branch it covers (kept as a separate test per the review).

The new test:

1. Copies sleep to a TempDir-resident file named firecracker so /proc/<pid>/comm == "firecracker" (comm is the executable basename, truncated to 15 chars).
2. Double-forks via sh -c '... & echo $!' so the child is reparented to init (PID 1), not held as a child of the test process. This mirrors production: real Firecracker orphans are NOT children of the controller, so when kill_orphans SIGKILLs them, init reaps them and /proc/<pid> disappears. A child of the test process would linger as a zombie (held until the test reaps), causing wait_for_death to time out and report kill_failed instead of killed — so the double-fork is load-bearing for the test's correctness.
3. Sanity-asserts /proc/<pid>/comm == "firecracker", records the TRUE start time so process_identity_matches returns Match, inserts a registry entry.
4. Asserts killed == 1, kill_failed == 0, pruned_stale == 0, and the registry entry is removed — exercising pidfd_open → start-time re-verification → comm_is_firecracker (passes) → pidfd_send_kill → wait_for_death → sandboxes.remove.
5. Defense-in-depth cleanup signals the detached PID directly (no-op if already reaped by init).

Corrected misleading comments:

• SandboxInfo::proc_starttime (api.rs): the None → IdentityCheck::Unknown path does NOT do comm-only verification — process_identity_matches returns Unknown immediately and kill_orphans never calls comm_is_firecracker on it. Comment now says "identity unknown — fail closed: keep the entry and increment kill_failed rather than risk killing an unidentifiable process."
• IdentityCheck::Unknown (state.rs): the doc claimed the caller "chooses between a conservative comm-only kill or a prune-without-kill." kill_orphans does neither — it keeps the entry and increments kill_failed (aborting startup via check_orphan_kill_result). Comment now describes the actual fail-closed behavior.

Rebase

Rebased onto dev (77d3f5d), preserving the durable-identity + pidfd work and dev's ClockSyncOutcome import. All commits DCO-signed. The rebased head is ce0c39c. Could you re-review?