From c03c8a931a3b0b4b26db5c433f6f2bfd488d2e19 Mon Sep 17 00:00:00 2001 From: EJ Campbell Date: Wed, 19 Aug 2026 21:18:54 -0700 Subject: [PATCH 1/2] Make PastaNetwork::cleanup fail closed on kill and PID-file failures cleanup logged a failed pasta kill and a failed PID-file removal at warn and returned Ok, so every caller was told teardown succeeded regardless. A surviving pasta keeps the tap device and the forwarded loopback ports bound, and cleanup_vm_verified (src/commands/common.rs) records network.cleanup() precisely so prepare can refuse to publish a snapshot while host resources remain; with the swallow, that leg was vacuous. Collect both failures and bail with the joined list, the same shape BridgedNetwork::cleanup already uses. kill() is start_kill + wait, so Ok from it confirms the process is reaped; an already-reaped child is reported as success, not an error (tokio start_kill on an exited child returns Ok since 1.44, tokio#7160). Callers all handle the Err: cleanup_vm_inner records it into CleanupFailures (src/commands/common.rs:1139) so cleanup_vm stays best effort and cleanup_vm_verified fails closed; the podman prepare error paths (src/commands/podman/mod.rs:1502, 1543, 1585, 1787) and the clone setup path (src/commands/snapshot.rs:184) log it and keep the original error. Fixes #800. Tested: cargo test --lib -p fcvm network::pasta Red on the unfixed code: cleanup_fails_closed_when_the_pid_file_cannot_be_removed ... FAILED panicked: cleanup must report the PID-file removal failure, not Ok: () Green with the fix: 37 passed; 0 failed. Reverting the fix turns the same test red again. Full lib suite: 476 passed; 0 failed. cargo fmt -p fcvm --check and cargo clippy --lib -p fcvm are clean. --- src/network/pasta.rs | 72 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/src/network/pasta.rs b/src/network/pasta.rs index 3755665d5..8e198c73c 100644 --- a/src/network/pasta.rs +++ b/src/network/pasta.rs @@ -1897,26 +1897,45 @@ impl NetworkManager for PastaNetwork { async fn cleanup(&mut self) -> Result<()> { info!(vm_id = %self.vm_id, "cleaning up pasta resources"); + let mut errors = Vec::new(); // `kill()` is start_kill + wait, so this stays correct whether or not // `start_kill_processes` already signalled pasta (re-signalling a not-yet-reaped // process is a no-op); when it did, the wait below has nothing left to wait for. + // `kill()` returning Ok also means the child was reaped, so propagating its + // error is the process-gone confirmation: a surviving pasta keeps the tap + // device and the forwarded loopback ports bound, and callers such as + // `cleanup_vm_verified` treat cleanup's Ok as proof those resources are gone. if let Some(mut process) = self.pasta_process.take() { if let Err(e) = process.kill().await { - warn!("failed to kill pasta: {}", e); + warn!(vm_id = %self.vm_id, error = %e, "failed to kill pasta"); + errors.push(format!("killing pasta: {}", e)); } } if let Some(ref pid_file) = self.pid_file { if pid_file.exists() { if let Err(e) = tokio::fs::remove_file(pid_file).await { - warn!("failed to remove pasta PID file: {}", e); + warn!(vm_id = %self.vm_id, error = %e, "failed to remove pasta PID file"); + errors.push(format!( + "removing pasta PID file {}: {}", + pid_file.display(), + e + )); } } } - info!(vm_id = %self.vm_id, "pasta cleanup complete"); - Ok(()) + if errors.is_empty() { + info!(vm_id = %self.vm_id, "pasta cleanup complete"); + Ok(()) + } else { + anyhow::bail!( + "pasta cleanup had {} error(s): {}", + errors.len(), + errors.join("; ") + ) + } } fn tap_device(&self) -> &str { @@ -1993,6 +2012,51 @@ impl NetworkManager for PastaNetwork { mod tests { use super::*; + /// A failed cleanup step must surface as an Err. Callers treat cleanup's + /// Ok as proof the host resources are gone: `cleanup_vm_verified` + /// (src/commands/common.rs) records the result so prepare can refuse to + /// publish a snapshot while pasta still holds the tap device and the + /// forwarded loopback ports, or while its PID file remains. + /// + /// A directory at the PID-file path makes `remove_file` fail with EISDIR + /// for every uid, so this needs neither root nor permission games. + #[tokio::test] + async fn cleanup_fails_closed_when_the_pid_file_cannot_be_removed() { + let dir = tempfile::tempdir().expect("creating tempdir"); + let mut network = PastaNetwork::new("cleanup-pidfile-test".into(), "tap0".into(), vec![]); + network.pid_file = Some(dir.path().to_path_buf()); + + let error = network + .cleanup() + .await + .expect_err("cleanup must report the PID-file removal failure, not Ok"); + assert!( + format!("{error:#}").contains("removing pasta PID file"), + "error must attribute the failed step: {error:#}" + ); + } + + /// An already-reaped pasta is not a cleanup failure. `kill()` returning Ok + /// is the process-gone confirmation (tokio's `start_kill` on a reaped + /// child returns Ok since 1.44), so cleanup must not fabricate an error + /// for a process that no longer exists. This pins the boundary of the + /// fail-closed contract: Err means "could not confirm gone", never + /// "was already gone". + #[tokio::test] + async fn cleanup_reports_success_for_an_already_reaped_pasta() { + let mut network = PastaNetwork::new("cleanup-reaped-test".into(), "tap0".into(), vec![]); + let mut child = tokio::process::Command::new("true") + .spawn() + .expect("spawning true"); + child.wait().await.expect("waiting for true"); + network.pasta_process = Some(child); + + network + .cleanup() + .await + .expect("a reaped pasta means the process is gone, which is cleanup success"); + } + /// A guest-answer wait that consumed its whole budget must not starve the /// port-forward wait that follows it. /// From 7b8cde7b95c9a89d75e6abec42cb13bbcce23308 Mon Sep 17 00:00:00 2001 From: EJ Campbell Date: Wed, 19 Aug 2026 22:16:07 -0700 Subject: [PATCH 2/2] Keep pasta cleanup evidence alive: retain the handle, drop exists() pre-checks, own state before the readiness wait Three defects in the pasta teardown path, all of the same shape: cleanup could report Ok, or lose its only means of retry, without having confirmed anything. - cleanup() gated PID-file removal on Path::exists(), which maps metadata errors (EACCES, ENOTDIR, ...) to false, so an unstattable path skipped the removal and cleanup returned Ok. remove_file now runs directly and only NotFound is treated as already clean. The stale-file removal at the top of start_pasta had the same pre-check and gets the same treatment. - cleanup() took the Child before kill(); on a kill error the handle was dropped with kill_on_drop off, leaving a possibly-live pasta that no later cleanup could signal, while the PID file was still removed. The handle is now restored on error and the PID file is only removed once the kill is confirmed. - start_pasta recorded self.pid_file and self.pasta_process only after wait_for_pid_file succeeded, so a pasta that wrote its PID file and died before readiness left the file unowned and the child unreapable, and cleanup() returned Ok. The new adopt_and_await_pasta records the PID path before the wait and stores the child on every path; the post_start retry loop already reaps a stored child between attempts. Red/green: the three new tests were run against the unfixed logic first (the helper was extracted with the original ordering preserved) and failed as expected, then passed after the fix: - cleanup_fails_closed_when_the_pid_path_cannot_be_checked (red: "cleanup must report the PID-file removal failure, not Ok: ()") - a_failed_kill_keeps_the_handle_and_the_pid_file (red: "the handle must survive a failed kill so the next cleanup can retry") - a_failed_readiness_wait_still_owns_the_child_and_pid_path (red: "the PID path must be recorded even when readiness fails", left: None) cleanup_treats_a_missing_pid_file_as_already_clean pins the NotFound boundary of the direct-removal path (green before and after by design, like the reaped-child boundary test). Tested: cargo test --lib -p fcvm (480 passed), cargo clippy --all-targets -p fcvm -- -D warnings (clean), cargo fmt -p fcvm -- --check (clean). --- src/network/pasta.rs | 203 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 187 insertions(+), 16 deletions(-) diff --git a/src/network/pasta.rs b/src/network/pasta.rs index 8e198c73c..1bd004317 100644 --- a/src/network/pasta.rs +++ b/src/network/pasta.rs @@ -1186,8 +1186,18 @@ impl PastaNetwork { } }; - if pid_file.exists() { - tokio::fs::remove_file(&pid_file).await?; + // Same rule as cleanup(): no exists() pre-check, because + // `Path::exists()` maps metadata errors to false and would silently + // keep a stale file that the readiness wait then mistakes for pasta + // being ready. Only NotFound means there was nothing to remove. + match tokio::fs::remove_file(&pid_file).await { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + return Err(e).with_context(|| { + format!("removing stale pasta PID file {}", pid_file.display()) + }) + } } let host_ipv6 = Self::detect_host_ipv6(); @@ -1416,13 +1426,33 @@ impl PastaNetwork { })); } - self.wait_for_pid_file(&mut child, &pid_file, &mut pid_file_watch) - .await?; + self.adopt_and_await_pasta(child, pid_file, &mut pid_file_watch) + .await + } + /// Take ownership of the spawned pasta and its PID path, then wait for + /// readiness. + /// + /// Both fields are recorded even when the wait fails: pasta can write its + /// PID file and exit before the wait returns, and cleanup() can only reap + /// the child and remove the file if `self` owns them on every error path. + /// The retry loop in `post_start` reaps `self.pasta_process` between + /// attempts for the same reason. + async fn adopt_and_await_pasta( + &mut self, + mut child: Child, + pid_file: PathBuf, + pid_file_events: &mut E, + ) -> Result<()> + where + E: crate::utils::DirEventSource, + { + self.pid_file = Some(pid_file.clone()); + let readiness = self + .wait_for_pid_file(&mut child, &pid_file, pid_file_events) + .await; self.pasta_process = Some(child); - self.pid_file = Some(pid_file); - - Ok(()) + readiness } /// Start an attempt-local stderr capture. @@ -1910,18 +1940,32 @@ impl NetworkManager for PastaNetwork { if let Err(e) = process.kill().await { warn!(vm_id = %self.vm_id, error = %e, "failed to kill pasta"); errors.push(format!("killing pasta: {}", e)); + // `kill_on_drop` is off for pasta, so dropping the handle here + // would leave a possibly-live process that nothing can signal + // again. Put it back so the caller's next cleanup() can retry. + self.pasta_process = Some(process); } } - if let Some(ref pid_file) = self.pid_file { - if pid_file.exists() { - if let Err(e) = tokio::fs::remove_file(pid_file).await { - warn!(vm_id = %self.vm_id, error = %e, "failed to remove pasta PID file"); - errors.push(format!( - "removing pasta PID file {}: {}", - pid_file.display(), - e - )); + // The PID file goes only after the kill is confirmed: while pasta may + // still be alive, the file is the on-disk record of that process. No + // exists() pre-check either: `Path::exists()` maps metadata errors + // (EACCES, ENOTDIR, ...) to false, which would skip the removal and + // report success without knowing whether the file remains. NotFound is + // the one removal error that already proves the goal state. + if errors.is_empty() { + if let Some(ref pid_file) = self.pid_file { + match tokio::fs::remove_file(pid_file).await { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + warn!(vm_id = %self.vm_id, error = %e, "failed to remove pasta PID file"); + errors.push(format!( + "removing pasta PID file {}: {}", + pid_file.display(), + e + )); + } } } } @@ -2036,6 +2080,133 @@ mod tests { ); } + /// `Path::exists()` maps metadata errors (EACCES, ENOTDIR, ...) to false, + /// so an exists() pre-check skips the removal for a PID path it could not + /// even stat, and cleanup reports Ok without knowing whether the file + /// remains. A regular file as the parent component makes both the stat and + /// the removal fail with ENOTDIR for every uid, so this needs neither root + /// nor permission games. + #[tokio::test] + async fn cleanup_fails_closed_when_the_pid_path_cannot_be_checked() { + let dir = tempfile::tempdir().expect("creating tempdir"); + let blocker = dir.path().join("not-a-directory"); + std::fs::write(&blocker, b"").expect("creating blocker file"); + let mut network = PastaNetwork::new("cleanup-enotdir-test".into(), "tap0".into(), vec![]); + network.pid_file = Some(blocker.join("pasta.pid")); + + let error = network + .cleanup() + .await + .expect_err("cleanup must report the PID-file removal failure, not Ok"); + assert!( + format!("{error:#}").contains("removing pasta PID file"), + "error must attribute the failed step: {error:#}" + ); + } + + /// An absent PID file is cleanup success, not an error: NotFound is the + /// one removal outcome that already proves the goal state. This pins the + /// boundary of the direct-removal path so it does not fail VMs whose PID + /// file was never written or was already collected. + #[tokio::test] + async fn cleanup_treats_a_missing_pid_file_as_already_clean() { + let dir = tempfile::tempdir().expect("creating tempdir"); + let mut network = PastaNetwork::new("cleanup-absent-test".into(), "tap0".into(), vec![]); + network.pid_file = Some(dir.path().join("never-written.pid")); + + network + .cleanup() + .await + .expect("NotFound means the PID file is already gone"); + } + + /// A failed kill must leave the handle in place. `kill_on_drop` is off for + /// pasta, so dropping the Child would leave a possibly-live pasta that + /// nothing can signal again, and a later cleanup() call could not retry. + /// The PID file must also survive the failed attempt: it is the on-disk + /// record that pasta may still hold the tap device and forwarded ports, + /// and it may only be removed once the process is confirmed gone. + /// + /// Failure vehicle: reap the child with waitpid(2) behind tokio's back. + /// The Child still believes the process exists, so kill() sends SIGKILL to + /// a freed PID and gets ESRCH. Reuse of that PID inside this test would + /// require a full pid-space wraparound between the waitpid and the kill, + /// so the signal cannot land on a live process. + #[tokio::test] + async fn a_failed_kill_keeps_the_handle_and_the_pid_file() { + let dir = tempfile::tempdir().expect("creating tempdir"); + let pid_file = dir.path().join("pasta.pid"); + std::fs::write(&pid_file, b"123\n").expect("writing PID file"); + + let child = tokio::process::Command::new("true") + .spawn() + .expect("spawning true"); + let pid = child.id().expect("child PID") as libc::pid_t; + let mut status = 0; + let reaped = unsafe { libc::waitpid(pid, &mut status, 0) }; + assert_eq!(reaped, pid, "waitpid must reap the child"); + + let mut network = PastaNetwork::new("cleanup-kill-fail-test".into(), "tap0".into(), vec![]); + network.pasta_process = Some(child); + network.pid_file = Some(pid_file.clone()); + + let error = network + .cleanup() + .await + .expect_err("an unconfirmed kill must fail cleanup"); + assert!( + format!("{error:#}").contains("killing pasta"), + "error must attribute the failed step: {error:#}" + ); + assert!( + network.pasta_process.is_some(), + "the handle must survive a failed kill so the next cleanup can retry" + ); + assert!( + pid_file.exists(), + "the PID file outlives an unconfirmed kill" + ); + } + + /// A readiness wait that fails still leaves real state behind: pasta can + /// write its PID file and exit before the wait returns, and the child + /// needs reaping either way. Both must be owned by `self` on the error + /// path, or cleanup() skips them and reports Ok while the PID file + /// remains on disk. + #[tokio::test] + async fn a_failed_readiness_wait_still_owns_the_child_and_pid_path() { + let dir = tempfile::tempdir().expect("creating tempdir"); + let pid_file = dir.path().join("pasta.pid"); + let mut child = Command::new("sh") + .arg("-c") + .arg("exit 7") + .spawn() + .expect("spawn exiting child"); + // Reap up front so the wait's child-exit arm fires deterministically + // instead of racing the PID-file deadline. + child.wait().await.expect("reap exited child"); + + let mut no_watch = None::; + let mut net = PastaNetwork::new("adopt-test".to_string(), "tap0".to_string(), vec![]); + let error = net + .adopt_and_await_pasta(child, pid_file.clone(), &mut no_watch) + .await + .expect_err("an exited pasta cannot become ready"); + assert!( + format!("{error:#}").contains("exited before becoming ready"), + "{error:#}" + ); + assert_eq!( + net.pid_file.as_deref(), + Some(pid_file.as_path()), + "the PID path must be recorded even when readiness fails, or cleanup cannot remove it" + ); + assert!( + net.pasta_process.is_some(), + "the child must be stored even when readiness fails, or cleanup cannot reap it" + ); + } + /// An already-reaped pasta is not a cleanup failure. `kill()` returning Ok /// is the process-gone confirmation (tokio's `start_kill` on a reaped /// child returns Ok since 1.44), so cleanup must not fabricate an error