From 28c37251d3009d592aa610abfed2fbeb9df85e8d Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 13:49:38 -0700 Subject: [PATCH] fix(vmm): reap the Firecracker child on every failed boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Vm::boot` spawned Firecracker and then drove `/boot-source`, `/drives/rootfs`, `/entropy` and `/actions` with a bare `std::process::Child` in hand. `Child` has no kill-on-drop, so any `?` in that window — `wait_for_sock` timing out, or an API call failing after the process was up — returned an error and abandoned a *live* Firecracker. The orphan keeps the sandbox's tap device and rootfs fd open, and the tap name is frozen into the vmstate, so every later spawn of that snapshot dies inside Firecracker with `Open tap device failed` before it can create its API socket. Callers see `socket .../child-N.sock never appeared` and the snapshot stays unusable until the orphan is killed by hand — the permanent-failure mode in #301. Own the child in a `PendingFirecracker` guard that reaps it on every early return and hands it to the `Vm` only on success. `restore_many_with` already wrapped its children immediately, so `boot` was the only window in the crate that could orphan a process with no handle left. Two unit tests cover the drop path (the pid must be gone, not just untracked) and the take path (the handed-over child must still be live). Signed-off-by: jrimmer --- CHANGELOG.md | 15 ++++++ crates/forkd-vmm/src/lib.rs | 94 +++++++++++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0059420..2175c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,21 @@ snapshot is fully published, so a tag always holds a consistent src == dst guard comment duplication and makes the chain-unpack tail bail instead of resolving an empty path. +### A failed boot no longer orphans its Firecracker process + +`Vm::boot` spawned Firecracker and then issued the `/boot-source`, +`/drives/rootfs`, `/entropy` and `/actions` calls with a bare +`std::process::Child` in hand. `Child` has no kill-on-drop, so any `?` in +that window — including `wait_for_sock` timing out — returned an error and +abandoned a *live* Firecracker. The orphan holds the sandbox's tap device +and rootfs fd, and the tap name is frozen into the vmstate, so every later +spawn of that snapshot dies inside Firecracker with `Open tap device +failed` before it can create its API socket; callers see +`socket .../child-N.sock never appeared` and the snapshot stays unusable +until the orphan is killed by hand. The child is now owned by a +`PendingFirecracker` guard that reaps it on every early return and hands +it over to the `Vm` only on success. + ### Rootfs sidecar placement: recorded absolute path, validated Packs record the rootfs sidecar's target as the vmstate-frozen ABSOLUTE diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index e94a382..ce822ef 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -1580,6 +1580,45 @@ fn wait_for_sock(sock: &Path, timeout: Duration) -> Result<()> { ) } +/// Owns a just-spawned Firecracker until a [`Vm`] takes it over. +/// +/// `std::process::Child` has no kill-on-drop, so every `?` between +/// `spawn_firecracker` and the `Ok(Vm { .. })` at the end of [`Vm::boot`] +/// used to abandon a *live* Firecracker: `wait_for_sock` timing out, or +/// any of the `/boot-source`, `/drives`, `/entropy`, `/actions` calls +/// failing after the process was up. The orphan keeps the tap device and +/// the rootfs fd open, and the tap name is frozen into the vmstate, so +/// every later spawn of that snapshot dies inside Firecracker with +/// `Open tap device failed` before it can create its API socket — which +/// the parent reports as `socket .../child-N.sock never appeared`, for +/// good. Mirrors the `WorkDirGuard` idiom in the test module. +struct PendingFirecracker(Option); + +impl PendingFirecracker { + fn new(child: Child) -> Self { + Self(Some(child)) + } + + fn pid(&self) -> u32 { + self.0.as_ref().expect("child is owned until taken").id() + } + + /// Hand the running child to the `Vm`; dropping the guard afterwards + /// no longer reaps it. + fn take(mut self) -> Child { + self.0.take().expect("child is owned until taken") + } +} + +impl Drop for PendingFirecracker { + fn drop(&mut self) { + if let Some(mut child) = self.0.take() { + let _ = child.kill(); + let _ = child.wait(); + } + } +} + fn spawn_firecracker(sock: &Path, console: &Path) -> Result { spawn_firecracker_in(None, sock, console) } @@ -1649,8 +1688,8 @@ impl Vm { let _ = std::fs::remove_file(&sock); let _ = std::fs::remove_file(&console); - let proc = spawn_firecracker(&sock, &console)?; - let pid = proc.id(); + let proc = PendingFirecracker::new(spawn_firecracker(&sock, &console)?); + let pid = proc.pid(); wait_for_sock(&sock, Duration::from_secs(10))?; @@ -1746,7 +1785,7 @@ impl Vm { )?; Ok(Vm { - proc, + proc: proc.take(), pid, sock, console, @@ -2652,6 +2691,55 @@ mod tests { } } + fn spawn_sleeper() -> Child { + Command::new("sleep") + .arg("30") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sleep") + } + + fn proc_exists(pid: u32) -> bool { + Path::new(&format!("/proc/{pid}")).exists() + } + + /// A boot that fails after Firecracker is up must reap it. Before the + /// guard, the `?` in `wait_for_sock` — and each API call after it — + /// dropped a bare `Child`, leaving a live Firecracker holding the tap + /// device. That orphan is what makes every later spawn of the snapshot + /// fail with `socket .../child-N.sock never appeared` (issue #301). + #[test] + fn pending_firecracker_drop_reaps_the_owned_pid() { + let child = spawn_sleeper(); + let pid = child.id(); + assert!(proc_exists(pid), "sleeper must be alive before the drop"); + { + let pending = PendingFirecracker::new(child); + assert_eq!(pending.pid(), pid, "pid() must report the owned child"); + } // the failed-boot path: dropped without take() + assert!( + !proc_exists(pid), + "dropping the guard must reap pid {pid}, not abandon it" + ); + } + + /// `take()` is the success path: the `Vm` owns the process from then on, + /// so the guard must leave it running. + #[test] + fn pending_firecracker_take_hands_over_a_live_child() { + let mut child = PendingFirecracker::new(spawn_sleeper()).take(); + let pid = child.id(); + assert!( + proc_exists(pid), + "the child must still be alive after take() — the Vm owns it now" + ); + let _ = child.kill(); + let _ = child.wait(); + assert!(!proc_exists(pid), "cleanup must reap the handed-over child"); + } + #[test] fn kernel_supports_kvm_clock_realtime_parses_versions() { assert!(!kernel_supports_kvm_clock_realtime("5.10.0-foo"));