fix(vmm): restore-path resilience — configurable timeout, per-VM partial-failure reporting, pre-restore orphan detection

[jrimmer]

Summary

Partially addresses #301. Three changes to stop a single child failure from dooming the whole restore batch, and to surface contention from orphaned firecracker processes before a restore is attempted.

Issue #301 remains open — the orphan-reap/gate and graceful-reconcile criteria are not yet satisfied. This PR narrows to the restore-path resilience subset (criteria #2 and #4) and documents the remaining gaps.

Problem

Intermittent failures trace back to the forkd controller's snapshot restore path:

1. The wait_for_sock timeout was hardcoded at 10s — under load it's too tight.
2. restore_many_with bailed on the first child failure, silently dropping already-spawned siblings.
3. Orphaned firecracker processes from a prior crash hold tap/netns resources and doom new restores, but were invisible until a restore failed.

Changes

1. Configurable per-child socket wait timeout (ForkOpts field)

• New ForkOpts::socket_wait_timeout_secs (default 10s, matching the historical budget) drives the wait_for_sock calls in restore_many_with.
• Daemon paths (create_sandbox, spawn_one_for_workspace) default to 30s, configurable via FORKD_SOCKET_WAIT_TIMEOUT_SECS env var.
• A warning fires when a child takes >80% of the budget to appear, so operators see contention before it pushes the next spawn over.
• wait_for_sock now bails when the socket path is a directory (not a Unix socket) — previously Path::exists() returned true for a directory, masking the failure.
• CLI paths keep the 10s default.

2. Per-VM partial-failure reporting (RestoreError/RestoreFailure/ForkChild)

• New types: RestorePhase (enum), RestoreFailure (struct), RestoreError (struct with failures: Vec<RestoreFailure>), ForkChild (struct with child_index + vm), implementing Display + std::error::Error where applicable.
• ForkResult.children is now Vec<ForkChild> — each surviving child carries its 1-based within-batch child_index so callers can map a surviving VM back to its resource slot (netns/cgroup at netns_offset + child_index). This preserves the child-to-VM mapping that a plain Vec<Vm> would lose when failures are filtered out.
• restore_many_with now collects per-child results in each phase instead of bailing on the first failure:
  • Spawn phase: failures recorded but remaining children still spawn; on any failure, spawned children are dropped (killing firecracker) and a RestoreError carrying every failure is returned.
  • Socket-wait phase: a timed-out child is recorded; on any timeout, all children are dropped and a RestoreError is returned. Concurrent socket wait: one thread per child, all with the SAME timeout so the batch is bounded by one deadline, not n × timeout.
  • Restore phase: each thread's result is collected; a panicked thread is recorded as a restore failure rather than panicking the caller.
• RestoreError names every failed child (index, phase, error, pid when known) so callers can report which specific child failed rather than a single bail that loses batch context.

3. Pre-restore orphan detection (scan_firecracker_orphans)

• New OrphanProcess struct + scan_firecracker_orphans(known_pids) function scans /proc for running firecracker processes whose PIDs are NOT in the caller's live_vms set.
• Linux impl: reads /proc/<pid>/comm (exact "firecracker" match), /proc/<pid>/cmdline, /proc/<pid>/stat (elapsed time).
• Non-Linux stub: returns empty vec (for dev-box compilation).
• Controller calls it before both create_sandbox and spawn_one_for_workspace restores via a warn_orphan_firecrackers helper that scopes the live_vms lock to just PID collection, dropping it before the /proc scan so the scan doesn't serialize concurrent sandbox operations.
• Exposed as forkd_orphan_firecrackers_detected_total metric so operators can alert on the precursor.
• Read-only — never kills anything. The scan is a point-in-time observation with a documented TOCTOU window.

Controller-level all-or-nothing semantics

The controller's create_sandbox handler is intentionally all-or-nothing: a sandbox requires all N children, so when restore_many_with returns partial success (some children restored, some failed), the controller drops the surviving children (kills their firecracker processes) and either retries the whole batch (if the failure is a transient "busy" condition) or returns a structured error. The per-child failure reporting from restore_many_with is used at the controller level for diagnostics and retry classification only — partial success is NOT exposed through the API.

This is a deliberate design choice, not a limitation to be fixed later: a half-populated sandbox is worse than no sandbox (it would hold resources but be unable to serve requests). Issue #301 tracks the broader partial-success-through-controller discussion; this PR does not close it.

Known limitations (issue #301 remains open)

1. Orphan detection is diagnostic-only: the pre-restore scan logs and counts orphans but does not reap or gate on them. A lifecycle-level orphan reap/gate path (kill or block on untracked firecracker processes before restore) is tracked in Controller restore-path resilience: stop mass-pruning sandboxes on restart and restore failures #301.

2. Graceful reconcile is tracked separately: the controller restart mass-prune issue (Controller restore-path resilience: stop mass-pruning sandboxes on restart and restore failures #301 criterion KSM directed hints for fork families #5) is addressed by fix(controller): kill orphaned Firecracker processes on startup #299 (startup orphan kill) and Close PID-reuse race in orphan-Firecracker termination (pidfd + durable identity) #304 (pidfd durable identity), not this PR.

3. Controller restart does not reattach surviving VMs: Controller restore-path resilience: stop mass-pruning sandboxes on restart and restore failures #301 criterion KSM directed hints for fork families #5 (a controller restart should not mass-prune alive VMs) requires reconcile() changes that distinguish "VM died" from "controller crashed with surviving VMs". This is out of scope for this PR.

Tests

• restore_error_display_lists_all_failures — Display lists every failure with phase, pid, and error.
• restore_error_single_failure_display — single Spawn-phase failure (no pid).
• restore_phase_eq_distinguishes_phases — phase enum equality.
• scan_firecracker_orphans_non_linux_returns_empty — stub contract on non-Linux.
• scan_firecracker_orphans_empty_known_set_non_linux — empty known set contract.
• orphan_process_struct_shape — OrphanProcess field stability.
• restore_many_partial_spawn_failure_keeps_siblings (ignored, requires Linux + KVM + root) — integration test: boots a parent VM, snapshots it, restores two children with per-child netns. Child 1's netns is pre-provisioned; child 2's is not, so ip netns exec fails at spawn. Asserts child 1 survives with child_index=1, child 2 fails at Spawn phase.

Out of scope

The concurrent-RW-mount-of-shared-rootfs issue (restore_many_with sends the same vmstate to all children) needs an immutable baseline + per-VM writable layer and is tracked as a separate architectural initiative. The prewarm (Phase 3) and memfd (Phase 1.5) paths retain their pre-existing early-bail behavior — they are host-resource failures rather than per-VM failures, and converting them is outside this change's scope.

[jrimmer]

Fix summary

Follow-up commit e3a3e5c addresses findings raised on this PR:

Correctness / dead code:

• Removed the dead sock_ok vec in the socket-wait loop — it was collected on the success path but only discarded via let _ = sock_ok. The success path proceeds with all children regardless.
• Removed the dead now_secs variable in scan_firecracker_orphans_impl — it was computed once at the top of the scan but never read; the per-orphan elapsed-time computation calls boot_relative_secs() itself.

Misleading comments:

• proc_clk_tck was a function that unconditionally returned Some(100); the unwrap_or(100) at the call site and the hz == 0 guard were unreachable. Replaced with a PROC_CLK_TCK const. Fixed the doc comment that incorrectly claimed libc would add a Linux-only dep — libc is already a forkd-vmm dependency.
• scan_firecracker_orphans doc claimed the scan was "one /proc readdir + per-PID /proc/<pid>/comm read" but the implementation also reads /proc/<pid>/cmdline and /proc/<pid>/stat per orphan. Corrected the doc to reflect the actual per-orphan I/O, and added a TOCTOU note: the scan is a point-in-time observation, and PIDs may be reused between scan and restore (the scan is logging-only and never gates a restore).
• Removed a dead cross-reference to a nonexistent kill_orphans path in state.rs.

Lock scoping / contention:

• The orphan scan previously held the live_vms Mutex across the entire /proc readdir + per-PID file reads + logging, serializing concurrent sandbox operations. Extracted a warn_orphan_firecrackers helper that scopes the lock to just PID collection, dropping it before the scan runs. This also deduplicates the scan between create_sandbox and spawn_one_for_workspace.

Env configurability:

• The daemon socket-wait timeout (30s) is now configurable via the FORKD_SOCKET_WAIT_TIMEOUT_SECS env var, so operators can tune without a redeploy.

The prewarm (Phase 3) and memfd (Phase 1.5) paths retain their pre-existing early-bail behavior — they are host-resource failures rather than per-VM failures, and converting them is outside this PR's scope.

[WaylandYang]

Thank you for improving error reporting and making the socket wait configurable; the structured per-child failures and lock scoping are useful changes. The implementation still does not provide the resilience claimed by this PR or fully close #301.

The socket waits are performed serially with a full timeout per child, so total delay can grow to n × timeout instead of a single batch deadline. On any spawn/socket/restore failure, all successfully restored siblings are still dropped, so the change reports partial failures but does not preserve partial success. Orphan detection is logging-only and neither cleans up nor gates a restore that is known likely to collide. Controller restart/reconciliation remains unchanged as well.

Please use a concurrent/shared-deadline wait, define and implement an explicit partial-success contract (or narrow the PR claim), and make orphan/restart handling actionable and race-safe before treating #301 as resolved. Add tests for multiple slow/mixed-success children and restart with a real orphaned Firecracker. Thanks for the contribution—the pieces here are useful, but the current behavior still matches the failure modes the PR says it removes.

[WaylandYang]

Thank you for adding concurrent socket waits, structured partial success, and an orphan metric. These are useful improvements, but the current head still does not satisfy the PR's resilience claim or #301's acceptance criteria, and it conflicts with main with no CI.

Blocking issues:

1. Orphan handling remains observation-only. Incrementing a metric does not reap or gate on an untracked Firecracker, so a restore known to be at risk still proceeds. Rebase with the startup recovery work where applicable, implement a safe cleanup/gating path for truly untracked runtime orphans, or narrow this PR and keep #301 open.
2. ForkResult.children compacts successful VMs while only failures retain child_index; callers cannot reliably map each surviving VM back to its requested child/resource slot. Return an indexed success type (for example { child_index, vm }) or an equivalent explicit mapping.
3. The controller still drops every successful sibling when any non-retryable partial failure occurs, contradicting the claim that one child no longer dooms the batch. Either expose/retain partial success through the controller API or explicitly define the controller as all-or-nothing and narrow the PR/issue-closing claim.
4. The integration test uses n = 2 with per_child_netns = false, a shared-TAP/shared-IP topology that #281/#300 reject and that can fail independently with EBUSY. It also pre-creates a directory, which satisfies the current Path::exists() socket wait and shifts the error to restore. Use a valid per-child-netns setup or an injected spawn/socket failure and assert the exact intended phase and original child mapping.

Please resolve the main conflicts, address these contract/test issues, keep #301 open until its restart/orphan criteria are actually complete, and add the README-required DCO sign-offs to every resulting commit. Thanks for the contribution; the partial-result machinery is promising, but its public semantics need to be unambiguous and tested in a valid topology.

[WaylandYang]

Re-reviewed the narrowed restore-path scope at the current head. Successful children retain their original child_index, the partial-failure integration setup now uses a valid per-child-netns topology, socket waits are concurrent, all callers handle ForkChild correctly, and the controller explicitly preserves all-or-nothing semantics while keeping issue #301 open. CI is green and I found no remaining blocker. Approved. Non-blocking follow-up: classify a partial batch as retryable only when all failures are busy, so a mixed permanent + busy result does not trigger avoidable whole-batch retries.

[WaylandYang]

#281 has now merged after a fresh latest-main CI run. I rechecked this branch against current main and #302 now has real content conflicts in crates/forkd-controller/src/http.rs and crates/forkd-controller/src/lib.rs.

Please rebase onto current main and preserve both sides of the controller integration:

• keep main/fix(controller): serialize concurrent spawns to avoid tap collision #281's shared-TAP claim/commit/RAII lifecycle, SharedTapBusy handling, and the three-part spawn result carrying ForkResult, NetnsReservation, and SharedTapClaim;
• keep fix(vmm): restore-path resilience — configurable timeout, per-VM partial-failure reporting, pre-restore orphan detection #302's concurrent socket waits, indexed partial results, controller all-or-nothing retry/error handling, configurable timeout, and orphan-detection metric/state.

In particular, please do not resolve the tuple/AppState conflicts by dropping either the TAP ownership fields or the orphan metric. Rerun the full CI suite and retain DCO sign-offs on every contributor commit. I did not find a new blocker in the VMM portion; this is an integration rebase required by the merge order, and I will re-review promptly once the rebased head is green.

[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]

Thanks for the rebase guidance — #302 is rebased onto dev (== main, 52f40da) and force-pushed. The 10 commits were squashed into one (f4119ec) so the #281-vs-#302 conflict in http.rs/lib.rs was resolved once, cleanly, with both sides preserved as you asked.

Both sides preserved

#281's shared-TAP lifecycle (kept in create_sandbox and the workspace spawn paths):

• SharedTapClaim struct + try_claim_shared_tap + release_shared_tap_if_owner
• tap_token_for_closure clone passed into spawn_blocking
• the 3-part spawn result: (ForkResult, Option<NetnsReservation>, Option<SharedTapClaim>)
• claim returned uncommitted, claim.commit() after live_vms.insert() in all three spawn paths (create_sandbox, create_workspace, resume_workspace)
• first_id_override/tap_token — the first child's sandbox id is the tap-lease token
• SharedTapBusy → 503, n>1 shared-TAP rejection

#302's restore-path resilience (kept):

• warn_orphan_firecrackers pre-restore scan (point-in-time, diagnostic-only, never kills) + orphan_firecrackers_detected atomic counter exposed as forkd_orphan_firecrackers_detected_total metric
• partial-success contract: ForkResult.children: Vec<ForkChild{vm, child_index}> + failures: Vec<RestoreFailure>; Ok(r) if r.failures.is_empty() returns the 3-part tuple, otherwise drops the partial set and retries on busy / surfaces RestoreError
• concurrent socket waits + socket_wait_timeout_secs: 30 for the daemon path
• last_err tracking across retry attempts

Conflict resolution detail (crates/forkd-controller/src/http.rs): the orphan scan block runs before spawn_blocking in both create_sandbox and spawn_one_for_workspace; the spawn tuple stays 3-part so tap_claim flows through to the commit-after-insert; the children loop destructures ForkChild (let vm = fc.vm;) and keeps first_id_override.take() for shared-tap ownership; test-state construction (test_state_with_cap, test_state_with_netns) initializes both shared_tap_owner and orphan_firecrackers_detected. lib.rs AppState gets both fields.

All commits DCO-signed. The branch is now on dev with a clean single-commit diff over dev (4 files: http.rs, lib.rs, main.rs, forkd-vmm/lib.rs). Could you re-review the rebased head? The original approve stood on the behavioral content; this rebase just integrates it with #281's merged shared-TAP stack.

[WaylandYang]

Thanks for resolving the #281 rebase and preserving both sides. The current head is not reviewable for approval yet because the required rust job fails under -D warnings: last_err is declared at crates/forkd-controller/src/http.rs:1398 and assigned at line 1439, but never read. Please either remove the dead tracking or, preferably, include the final restore error in the exhausted-retry response/log so the variable serves its diagnostic purpose. Push the fix and rerun CI; I will re-review the updated head.

[jrimmer]

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

What was addressed

last_err dead code (-D warnings failure): the busy-retry loop in crates/forkd-controller/src/http.rs now falls through on the final attempt rather than returning early, so the exhausted-retry error carries retry-exhaustion context into the API response/log. last_err is read at the post-loop Err(last_err.expect(...)).

Behavior preserved:

• Non-busy errors still return immediately (no retry, no backoff).
• Busy errors retry with the existing backoffs [50, 200, 800] ms.
• On exhaustion, the final busy error surfaces wrapped with "restore_many: exhausted all busy-retries" context — operators can now distinguish a transient contention storm from a permanent failure.
• last_err is always Some when the loop falls through (every fall-through path is a busy branch that just assigned it), so the expect cannot panic in normal control flow.
• No partial-success VMs are leaked on retry: drop(children) runs before every retry/return, and RestoreError holds only diagnostic failures (never live Vms).

Rebase

Rebased onto dev (77d3f5d), preserving both #281's shared-TAP lifecycle and #302's restore-path resilience:

• sync_guest_clocks: true (from fix(vmm): best-effort guest clock sync after snapshot restore (Option B) #300 on dev) + socket_wait_timeout_secs: daemon_socket_wait_timeout_secs() (from this PR) on both daemon spawn paths.
• ForkOpts/ForkResult carry both sides' fields (socket_wait_timeout_secs, sync_guest_clocks, clock_sync_ms, clock_sync_outcomes, failures, ForkChild).
• sync_guest_clocks now takes &[&Vm] and is passed the flattened live children (Vm is not Clone — it owns a Child), preserving the partial-success Vec<Option<Vm>> shape.

All commits DCO-signed. The rebased head is 1010559. Could you re-review?