From bde979ab7596fc2b7135605e30fbb6ed528c30dc Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 13:54:41 -0700 Subject: [PATCH 1/2] fix(cli): gate bakes on free space, and always clean up the staging dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both halves of one failure mode: a bake that runs out of space does not stop, it produces a *corrupt* artifact — a rootfs whose package files carry other files' bytes, or a truncated `memory.bin`. Inside a guest that surfaces much later as `uname: option requires an argument`, `Exec format error`, or `EBADMSG` on a `/var/lib/dpkg` entry, so it reads like a broken build rather than a broken image. - `forkd parent build` (conversion) and `forkd snapshot` (bake) check free space on the target filesystem before the first write and refuse below a 5 GiB reserve (`FORKD_MIN_FREE_GIB` overrides). Conversion is the first multi-GB write of the pipeline, and the snapshot dir holds the clone plus a fresh `memory.bin`. The probe stays advisory: if `statvfs` cannot run, warn and continue rather than block work on a broken measurement. `doctor::available_bytes(path)` is extracted from the check's hardcoded-path statvfs so both callers share one probe. - `forkd snapshot` removes its staging dir on every exit. Only the success path did, so any `?` after the volatile artifacts were written — boot timeout, snapshot error, publish error, interrupt — left a fully written `memory.bin` (GBs) beside the snapshot dir that nothing ever collected. Tests cover the guard (removal, and tolerance of an already-removed dir — the success path renames files out and a stale sweep may beat it) and the probe's ancestor walk, which is what makes it usable before the snapshot dir exists. Signed-off-by: jrimmer --- CHANGELOG.md | 20 +++++ crates/forkd-cli/src/doctor.rs | 76 +++++++++++++----- crates/forkd-cli/src/main.rs | 141 ++++++++++++++++++++++++++++++++- 3 files changed, 215 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00594206..7f10072c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,26 @@ 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. +### Bakes refuse to start on a nearly full disk, and clean up after themselves + +Running out of space mid-write does not fail cleanly — it leaves a +*corrupt* artifact: a rootfs whose package files contain other files' +bytes, or a truncated `memory.bin`. Inside a guest that surfaces much +later as `uname: option requires an argument`, `Exec format error`, or +`EBADMSG` on a `/var/lib/dpkg` entry, which reads like a broken build +rather than a broken image. `forkd parent build` (conversion) and +`forkd snapshot` now check free space on the target filesystem first and +refuse with an explicit error below a 5 GiB reserve +(`FORKD_MIN_FREE_GIB` to override). The check is advisory: if `statvfs` +cannot run, it warns and continues rather than blocking work on a broken +measurement. + +`forkd snapshot` also removes its staging dir on every exit now, not only +on success. A failure after the volatile artifacts were written — boot +timeout, snapshot error, publish error, interrupt — used to leave a fully +written `memory.bin` (GBs) beside the snapshot dir that nothing ever +collected. + ### Rootfs sidecar placement: recorded absolute path, validated Packs record the rootfs sidecar's target as the vmstate-frozen ABSOLUTE diff --git a/crates/forkd-cli/src/doctor.rs b/crates/forkd-cli/src/doctor.rs index fb79d966..b78e01b6 100644 --- a/crates/forkd-cli/src/doctor.rs +++ b/crates/forkd-cli/src/doctor.rs @@ -519,6 +519,59 @@ fn check_docker_daemon() -> Check { } } +/// Free bytes on the filesystem holding `path` (`statvfs` `f_bavail` +/// × `f_frsize`) — the space actually available to a non-root writer. +/// +/// `path` need not exist: the call walks up to the nearest existing +/// parent, so callers can gate a directory they are about to create. +/// Shared by `forkd doctor` and the bake preflight +/// (`require_free_space` in `main.rs`), which is the point of taking a +/// path argument — the doctor's own hardcoded snapshot dir is not the +/// only filesystem a bake writes to. +/// +/// Returns `Err` on Unix when no ancestor exists or `statvfs` fails. +#[cfg(unix)] +pub(crate) fn available_bytes(path: &std::path::Path) -> std::io::Result { + use std::os::unix::ffi::OsStrExt; + let mut probe = path.to_path_buf(); + while !probe.exists() { + match probe.parent() { + Some(p) if p.as_os_str() != probe.as_os_str() => probe = p.to_path_buf(), + _ => { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("no existing ancestor of {}", path.display()), + )) + } + } + } + let c_path = std::ffi::CString::new(probe.as_os_str().as_bytes()).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("path contains NUL: {}", probe.display()), + ) + })?; + let mut buf: libc::statvfs = unsafe { std::mem::zeroed() }; + let rc = unsafe { libc::statvfs(c_path.as_ptr(), &mut buf) }; + if rc != 0 { + return Err(std::io::Error::last_os_error()); + } + Ok((buf.f_bavail as u64).saturating_mul(buf.f_frsize as u64)) +} + +/// Non-Unix stub: there is no portable free-space call here, and +/// reporting a number would be worse than reporting none. +#[cfg(not(unix))] +pub(crate) fn available_bytes(path: &std::path::Path) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + format!( + "free-space check is Unix-only ({} not probed)", + path.display() + ), + )) +} + fn check_snapshot_dir_space() -> Check { #[cfg(unix)] { @@ -531,30 +584,15 @@ fn check_snapshot_dir_space() -> Check { .map(std::path::PathBuf::from) .unwrap_or_else(|| home.join(".local/share")); let dir = xdg.join("forkd/snapshots"); - // statvfs needs a path that exists; walk up to an existing parent. - let mut probe = dir.clone(); - while !probe.exists() { - match probe.parent() { - Some(p) if p.as_os_str() != probe.as_os_str() => probe = p.to_path_buf(), - _ => return Check::warn("snapshot dir space", "no path to stat", ""), - } - } - use std::os::unix::ffi::OsStrExt; - let c_path = match std::ffi::CString::new(probe.as_os_str().as_bytes()) { - Ok(c) => c, - Err(_) => return Check::warn("snapshot dir space", "bad path", ""), + let avail_bytes = match available_bytes(&dir) { + Ok(bytes) => bytes, + Err(e) => return Check::warn("snapshot dir space", format!("statvfs failed: {e}"), ""), }; - let mut buf: libc::statvfs = unsafe { std::mem::zeroed() }; - let rc = unsafe { libc::statvfs(c_path.as_ptr(), &mut buf) }; - if rc != 0 { - return Check::warn("snapshot dir space", "statvfs failed", ""); - } - let avail_bytes = (buf.f_bavail as u64).saturating_mul(buf.f_frsize as u64); let avail_gib = avail_bytes as f64 / 1024.0 / 1024.0 / 1024.0; if avail_gib >= 5.0 { Check::pass( "snapshot dir space", - format!("{avail_gib:.1} GiB free at {}", probe.display()), + format!("{avail_gib:.1} GiB free at {}", dir.display()), ) } else if avail_gib >= 1.0 { Check::warn( diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 58232022..146b8559 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -2185,6 +2185,12 @@ fn parent_build_cmd( // The build script does sudo internally for mount/chroot. Run as // current user; the user is expected to run `sudo forkd ...` once // for the whole pipeline (kvm + netns + bind mount all need root). + // Conversion is the first multi-GB write of the pipeline: it unpacks a + // full image into a fresh ext4, and an ENOSPC partway through yields a + // rootfs with foreign content in package files (a swapped + // `/usr/bin/uname`, `EBADMSG` on `/var/lib/dpkg` entries) that passes + // every "does it run" check. Refuse before the first byte. + require_free_space(&out, "convert an image to a rootfs")?; let mut cmd = std::process::Command::new("bash"); cmd.arg(&script) .arg(&image) @@ -2746,6 +2752,57 @@ fn eval_cmd(target: String, child: Option, code: Vec) -> Result< Ok(()) } +/// Free-space reserve a bake must have before it starts writing. Matches +/// `forkd doctor`'s "recommended ≥5 GiB for warmed parent rootfs + +/// memory.bin". Override with `FORKD_MIN_FREE_GIB` (e.g. on a host whose +/// artifacts are known to be small). +const DEFAULT_MIN_FREE_GIB: f64 = 5.0; + +fn min_free_gib() -> f64 { + std::env::var("FORKD_MIN_FREE_GIB") + .ok() + .and_then(|v| v.parse::().ok()) + .filter(|g| *g >= 0.0) + .unwrap_or(DEFAULT_MIN_FREE_GIB) +} + +/// Refuse to start a step that writes gigabytes when its target +/// filesystem is nearly full. +/// +/// Running out of space partway through does not fail cleanly: the +/// artifact is left *corrupt* rather than obviously incomplete — a +/// half-written ext4 or a truncated `memory.bin` — and that surfaces +/// much later as random guest breakage (a swapped `/usr/bin/uname`, a +/// `EBADMSG` on a directory entry, an `Exec format error`) that reads +/// like a broken build rather than a broken image. One clear error here +/// is worth a lot of downstream debugging. +/// +/// The probe is advisory by design: if `statvfs` cannot run, warn and +/// continue rather than block work on a broken measurement. +fn require_free_space(path: &std::path::Path, what: &str) -> Result<()> { + const GIB: f64 = 1024.0 * 1024.0 * 1024.0; + let reserve = min_free_gib() * GIB; + match crate::doctor::available_bytes(path) { + Ok(avail) if (avail as f64) < reserve => bail!( + "refusing to {what}: only {:.1} GiB free on the filesystem holding {} \ + (need {:.1} GiB).\n A write that runs out of space leaves a corrupt \ + artifact, not an incomplete one — free space and retry, or lower the \ + reserve with FORKD_MIN_FREE_GIB.", + avail as f64 / GIB, + path.display(), + min_free_gib(), + ), + Ok(_) => Ok(()), + Err(e) => { + eprintln!( + " note: could not check free space for {} ({e})", + path.display() + ); + Ok(()) + } + } +} + #[allow(clippy::too_many_arguments)] // mirrors the CLI flag surface 1-to-1 fn snapshot_cmd( tag: Option, @@ -2850,6 +2907,10 @@ fn snapshot_cmd( // was working around: since the baseline is never written to, it // never has uncommitted journal transactions or dirty metadata. let snap_dir = snapshot_dir(&tag); + // A bake writes the cloned rootfs plus a fresh `memory.bin` here; on a + // nearly full filesystem the result is a corrupt snapshot rather than a + // failed command, so stop before the first write. + require_free_space(&snap_dir, "bake a snapshot")?; // Distinct staging dir beside the target. The pid suffix keeps // concurrent runs from colliding; the `staging-` prefix keeps it // out of SNAPSHOT_FILES / list_local enumeration. @@ -2860,6 +2921,13 @@ fn snapshot_cmd( std::fs::remove_dir_all(&staging_dir) .with_context(|| format!("remove stale staging dir {}", staging_dir.display()))?; } + // Sweep the staging dir on every exit from here on. Only the success + // path used to remove it, so any `?` after the files were written — + // boot timeout, snapshot error, publish error, interrupt — left a + // fully-written `memory.bin` (GBs) behind, and nothing ever collected + // it. Declared before `rootfs_rollback` so the two unwind in reverse: + // the rootfs is restored first, then the staging shell is removed. + let _staging_guard = StagingDirGuard(staging_dir.clone()); // src == dst guard (review #295 blocker 3 / 2026-08-22): reject // cloning the baseline into the snapshot's own final rootfs path. See @@ -3093,9 +3161,9 @@ fn snapshot_cmd( ), } } - // Drop the staged-only dir (now empty of the files we renamed, or - // holding only a leftover on a partial failure). - let _ = std::fs::remove_dir_all(&staging_dir); + // The staged-only dir is removed by `_staging_guard` on scope exit — + // on success it holds nothing but the shell the files were renamed out + // of, and on failure it must not survive at all. eprintln!(" published snapshot → {}", snap_dir.display()); // Parent VM is dead and the snapshot lives under data_dir; work_dir @@ -3139,6 +3207,25 @@ fn cleanup_workdir(work_dir: &std::path::Path) { } } +/// Removes a bake's staging dir when it goes out of scope. +/// +/// The volatile artifacts (`vmstate`, `memory.bin`, `snapshot.json`) are +/// written here and renamed into the snapshot dir at publish, so the dir +/// is scratch in every outcome. Only the success path used to remove it, +/// which meant any failure after the files were written — boot timeout, +/// snapshot error, publish error, interrupt — left a fully written +/// `memory.bin` behind for good. Holds the path, not a handle, so the +/// successful rename of the files out of the dir is unaffected. +struct StagingDirGuard(std::path::PathBuf); + +impl Drop for StagingDirGuard { + fn drop(&mut self) { + // Best-effort and NotFound-tolerant: a successful publish may have + // already emptied it, and a failure may have removed it. + let _ = std::fs::remove_dir_all(&self.0); + } +} + /// Rollback guard for re-baking an existing tag (review #295 blocker 1, /// 2026-08-22 r9). /// @@ -4496,6 +4583,54 @@ mod tests { ); } + /// The staging guard is the only thing between a failed bake and a + /// multi-GB `memory.bin` left on disk forever: only the success path + /// used to clean up. + #[test] + fn staging_dir_guard_removes_the_dir_on_drop() { + let tmp = tempfile::tempdir().unwrap(); + let staging = tmp.path().join("tag.staging-4242"); + std::fs::create_dir_all(&staging).unwrap(); + std::fs::write(staging.join("memory.bin"), b"GBS-OF-GUEST-RAM").unwrap(); + { + let _guard = StagingDirGuard(staging.clone()); + assert!(staging.join("memory.bin").exists()); + } + assert!( + !staging.exists(), + "dropping the guard must remove the staging dir and its contents" + ); + } + + /// Drop must tolerate a dir that is not there: the success path may + /// have removed it, and a stale sweep may have beaten the guard to it. + #[test] + fn staging_dir_guard_tolerates_a_missing_dir() { + let tmp = tempfile::tempdir().unwrap(); + let staging = tmp.path().join("never-created.staging-4242"); + { + let _guard = StagingDirGuard(staging.clone()); + } + assert!(!staging.exists()); + } + + /// The free-space probe must work for a path that does not exist yet — + /// both bake gates run before the directory is created — and must + /// report the same filesystem as an existing path under it. + #[cfg(unix)] + #[test] + fn available_bytes_walks_up_to_an_existing_ancestor() { + let tmp = tempfile::tempdir().unwrap(); + let deep = tmp.path().join("a/b/c"); + let bytes = crate::doctor::available_bytes(&deep).expect("statvfs via an ancestor"); + assert!(bytes > 0, "a real filesystem must report free bytes"); + assert_eq!( + bytes, + crate::doctor::available_bytes(tmp.path()).expect("statvfs"), + "a missing descendant must resolve to the same filesystem as its ancestor" + ); + } + /// Review #295 r9 blocker 1: the rollback guard must restore the /// preserved previous rootfs and remove the partial clone when the /// bake fails (drop while armed), and must NOT touch anything when From 8120ff1cc5a8372def3f4fb6e74f7b1ed50a9d5b Mon Sep 17 00:00:00 2001 From: WaylandYang Date: Mon, 14 Sep 2026 12:54:12 +0800 Subject: [PATCH 2/2] test(cli): tolerate free-space drift between the two statvfs probes Parallel tests write to the same filesystem between the two calls, so exact equality flakes. Co-Authored-By: Claude Opus 5 --- crates/forkd-cli/src/main.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 146b8559..90d33c17 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -4624,10 +4624,13 @@ mod tests { let deep = tmp.path().join("a/b/c"); let bytes = crate::doctor::available_bytes(&deep).expect("statvfs via an ancestor"); assert!(bytes > 0, "a real filesystem must report free bytes"); - assert_eq!( - bytes, - crate::doctor::available_bytes(tmp.path()).expect("statvfs"), - "a missing descendant must resolve to the same filesystem as its ancestor" + // Not assert_eq: parallel tests write to the same filesystem between + // the two probes, so allow drift rather than flake. + let ancestor = crate::doctor::available_bytes(tmp.path()).expect("statvfs"); + assert!( + bytes.abs_diff(ancestor) < 1024 * 1024 * 1024, + "a missing descendant must resolve to the same filesystem as its ancestor \ + ({bytes} vs {ancestor})" ); }