fix(controller): kill orphaned Firecracker processes on startup

crates/forkd-controller/src/api.rs

@@ -306,6 +306,20 @@ pub struct SandboxInfo {
     pub guest_addr: String,
     pub created_at_unix: u64,
     pub pid: Option<u32>,
+    /// Process start time in clock ticks since boot (field 22 of
+    /// `/proc/<pid>/stat`), captured at VM registration. Used on
+    /// controller restart to detect PID reuse: if the recorded PID now
+    /// points to a process with a different start time, the original
+    /// Firecracker has exited and the PID was recycled — we must prune
+    /// the stale registry entry rather than kill an unrelated process.
+    ///
+    /// `#[serde(default)]` keeps existing `state.json` files loadable
+    /// (entries written before this field existed deserialize to
+    /// `None`, which is treated as "identity unknown — fail closed:
+    /// keep the entry and increment `kill_failed` rather than risk
+    /// killing an unidentifiable process").
+    #[serde(default)]
+    pub proc_starttime: Option<u64>,
     pub memory_limit_mib: Option<u64>,
     /// Set to true once any BRANCH (Full or Diff) has been taken from
     /// this sandbox. Diagnostic flag — phase 1d (v0.3.1) lifted the

crates/forkd-controller/src/http.rs

@@ -38,7 +38,7 @@ use crate::api::{
     ExecResponse, SandboxInfo, SnapshotInfo, SnapshotInfoDetail, SuspendWorkspaceRequest,
     VersionResponse, WorkspaceInfo, WorkspaceStatus,
 };
-use crate::state::Registry;
+use crate::state::{read_proc_starttime, Registry};
 use forkd_vmm::ClockSyncOutcome;
 
 const API_VERSION: &str = "v1";
@@ -1349,6 +1349,12 @@ async fn create_sandbox(
                         return Err(e);
 
                     }
+                    tracing::warn!(
+                        attempt = attempt + 1,
+                        next_backoff_ms = backoffs_ms[attempt],
+                        error = %e,
+                        "restore_many: tap/cgroup busy, retrying"
+                    );
                 }
             }
         }
@@ -1406,6 +1412,7 @@ async fn create_sandbox(
                 guest_addr: "10.42.0.2:8888".to_string(),
                 created_at_unix: now,
                 pid: Some(vm.pid()),
+                proc_starttime: read_proc_starttime(vm.pid()),
                 memory_limit_mib: req.memory_limit_mib,
                 has_branched: false,
                 last_branch_memory_path: None,
@@ -2833,6 +2840,7 @@ fn spawn_one_for_workspace(
         guest_addr: "10.42.0.2:8888".to_string(),
         created_at_unix: unix_now(),
         pid: Some(vm.pid()),
+        proc_starttime: read_proc_starttime(vm.pid()),
         memory_limit_mib,
         has_branched: false,
         last_branch_memory_path: None,
@@ -3449,6 +3457,7 @@ mod tests {
             guest_addr: "10.42.0.2:8888".to_string(),
             created_at_unix: unix_now(),
             pid: None,
+            proc_starttime: None,
             memory_limit_mib: None,
             has_branched: false,
             last_branch_memory_path: None,
@@ -4493,6 +4502,7 @@ mod tests {
                 guest_addr: "127.0.0.1:1".into(),
                 created_at_unix: 1,
                 pid: Some(99999999),
+                proc_starttime: None,
                 memory_limit_mib: None,
                 has_branched: false,
                 last_branch_memory_path: None,

crates/forkd-controller/src/lib.rs

@@ -80,6 +80,25 @@ fn unauthenticated_non_loopback(bind: SocketAddr, token_file: Option<&Path>) ->
     token_file.is_none() && !bind.ip().is_loopback()
 }
 
+/// Post-`kill_orphans` startup decision: abort if any orphan could
+/// not be killed (EPERM, D-state timeout, etc.). The NetnsAllocator
+/// active set and `shared_tap_owner` start empty, so a new spawn could
+/// reuse the still-alive orphan's netns index or tap lease — the exact
+/// collision #298 is meant to prevent. Returning `Err` here causes
+/// `run_daemon` to exit before binding the HTTP listener, so no spawns
+/// can be admitted.
+pub(crate) fn check_orphan_kill_result(orphans: &crate::state::KillOrphansResult) -> Result<()> {
+    if orphans.kill_failed > 0 {
+        anyhow::bail!(
+            "aborting startup: {} orphaned Firecracker process(es) could not be killed \
+             (EPERM or did not exit within timeout); refusing to accept spawns that may \
+             collide with still-alive orphans. Kill them manually and restart.",
+            orphans.kill_failed
+        );
+    }
+    Ok(())
+}
+
 /// Bring up the controller daemon. Blocks until the listener exits.
 /// SIGTERM and SIGINT trigger a graceful shutdown; SIGHUP reopens the
 /// configured audit log after external rotation.
@@ -91,6 +110,37 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> {
         tracing::info!(pruned, "reconciled stale sandbox entries on startup");
     }
 
+    // Kill orphaned Firecracker processes left over from a previous
+    // controller instance (issue #298). After reconcile() prunes
+    // dead-PID entries, any remaining entries have alive PIDs but no
+    // live_vms handle — they are unmanageable orphans. Kill them and
+    // prune the registry entries so the NetnsAllocator (empty active
+    // set) and shared_tap_owner (None) start clean.
+    let orphans = registry.kill_orphans()?;
+    if orphans.killed > 0 {
+        tracing::info!(
+            killed = orphans.killed,
+            pruned_stale = orphans.pruned_stale,
+            kill_failed = orphans.kill_failed,
+            "killed orphaned Firecracker processes on startup"
+        );
+    } else if orphans.pruned_stale > 0 || orphans.kill_failed > 0 {
+        tracing::warn!(
+            pruned_stale = orphans.pruned_stale,
+            kill_failed = orphans.kill_failed,
+            "orphan recovery: some entries pruned as stale or kill failed"
+        );
+    }
+
+    // Fail closed: if any orphan could not be killed (EPERM, D-state
+    // timeout, etc.), the controller cannot guarantee a clean resource
+    // state. The NetnsAllocator active set and shared_tap_owner start
+    // empty, so a new spawn could reuse the still-alive orphan's netns
+    // index or tap lease — the exact collision #298 is meant to prevent.
+    // Abort startup so the operator intervenes rather than silently
+    // admitting conflicting spawns. (review #299)
+    check_orphan_kill_result(&orphans)?;
+
     let audit = AuditSink::open(&cfg.audit_log)
         .with_context(|| format!("open audit log {}", cfg.audit_log.display()))?;
     tracing::info!(audit_log = %audit.path().display(), "audit log open");