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"));