From f0dfa8e59040af570563cae9a85217f88c102eae Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 21:44:30 -0700 Subject: [PATCH 1/8] feat(vmm): give every restored child its own rootfs backing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A snapshot's rootfs is one ext4, and Firecracker reopens that path verbatim for every child, so children of one tag share a filesystem. With the drive opened read-write — how every tag here was baked — two concurrent children are two guest kernels writing one filesystem with no coordinator: package files pick up other files' bytes, `/var/lib/dpkg` directory entries return EBADMSG, and the damage reads as a random build failure rather than a sandbox error. Firecracker cannot re-point a drive at load time (there is no `drive_overrides` in any release — checked in the v1.16.2 and v1.17.0 binaries), but it *can* re-point one on a restored VM via `PATCH /drives/{drive_id}`, and that call genuinely moves the device's storage: a write made in the guest afterwards lands in the new backing and not in the old file. So each child now gets its own reflink clone of the tag's rootfs and is re-pointed at it. The ordering matters and is the reason this is a three-call sequence rather than one. Loading with `resume_vm: false` leaves the guest un-executed, so nothing it does at boot — journal replay, `/var/log`, agent startup — can reach the shared base before the drive is re-pointed; measured, the base stays byte-identical through boot while the backing changes. Resuming after the re-point then hands the guest a disk that was always its own. **Fail closed.** A child whose rootfs could not be re-pointed is not resumed: it would run against the shared base, which is the defect being fixed. The failure propagates and the child is dropped (killing its Firecracker) rather than run unsafely. The backing lives in the child's work dir, so removing the dir reclaims it — a crash that skips that leaves the same class of straggler as the sockets already do. Snapshots with no recorded rootfs (daemon-side branches inherit the source's) keep the previous single-call behaviour. Requires Firecracker >= 1.15 for PATCH-on-restore, and a vmstate version match, so it is deployed together with an upgrade and a re-bake. Signed-off-by: jrimmer --- crates/forkd-vmm/src/lib.rs | 106 +++++++++++++++++++++++++++++++++++- 1 file changed, 103 insertions(+), 3 deletions(-) diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index e94a382..d850137 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -1440,6 +1440,66 @@ const CHILD_OOM_SCORE_ADJ: i32 = 500; /// to an attacker-pre-created file under `/tmp/`. const VMSTATE_ONLY_MEM_PLACEHOLDER: &str = "/dev/null/forkd-vmstate-only-mem-ignored"; +/// Where a restored child's own rootfs backing lives. +/// +/// A snapshot's rootfs is one file, and Firecracker reopens it verbatim for +/// every child, so without a per-child copy two concurrent children write one +/// filesystem with no coordinator. The backing is a reflink clone of the +/// snapshot's rootfs (see `chain::reflink_copy`), which costs nothing where the +/// filesystem supports it. +/// +/// It lives in the child's work dir so that removing the dir reclaims it. A +/// crash that skips that cleanup leaves a straggler beside the sockets, which is +/// the same class the startup sweep already collects. +fn child_rootfs_backing(work_dir: &Path, child_index: usize, tag_rootfs: &Path) -> PathBuf { + let stem = tag_rootfs + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("rootfs.ext4"); + work_dir.join(format!("child-{child_index}.{stem}")) +} + +/// Load a restored child **paused**, re-point its rootfs at `backing`, then +/// resume it. +/// +/// The order is load-bearing and was verified against Firecracker v1.17: +/// loading with `resume_vm: false` means the guest has not executed a single +/// instruction, so no boot-time write (journal replay, `/var/log`, agent +/// startup) can reach the shared base before the drive is re-pointed. Resuming +/// afterwards hands the guest a disk that is already its own — measured: the +/// base stays byte-identical through boot while the backing changes. +/// +/// **Fail closed.** A child whose rootfs could not be re-pointed must not be +/// resumed: it would run against the shared base, which is the corruption this +/// exists to prevent. Any failure here propagates, and the caller drops the +/// child (which kills its Firecracker) rather than run it unsafely. +fn load_paused_rebind_rootfs_and_resume( + sock: &Path, + load_body: &str, + backing: &Path, +) -> Result<()> { + api_call(sock, "PUT", "/snapshot/load", load_body) + .context("load child paused (resume_vm=false)")?; + // `drive_id` is required in the body — Firecracker rejects the override + // without it, which reads as a body error rather than a capability error. + api_call( + sock, + "PATCH", + "/drives/rootfs", + &serde_json::json!({"drive_id": "rootfs", "path_on_host": backing}).to_string(), + ) + .with_context(|| { + format!( + "re-point child rootfs at its own backing {} — refusing to resume a child \ + that would write the shared snapshot rootfs", + backing.display() + ) + })?; + api_call(sock, "PATCH", "/vm", r#"{"state":"Resumed"}"#) + .context("resume child after re-pointing its rootfs")?; + Ok(()) +} + fn api_call(sock: &Path, method: &str, path: &str, body: &str) -> Result<()> { api_call_with_timeout(sock, method, path, body, DEFAULT_API_TIMEOUT_SECS) } @@ -2316,10 +2376,43 @@ impl Snapshot { } } + // Give each child its own rootfs backing before anything is loaded, and + // fail the child — not the batch — if one cannot be materialised. The + // snapshot's rootfs is shared by every child of this tag, so a child + // that runs without its own copy is exactly the corruption this exists + // to prevent. Snapshots with no recorded rootfs (daemon-side branches + // inherit the source's) keep the previous behaviour. + let mut backings: Vec> = vec![None; children.len()]; + if let Some(tag_rootfs) = self.rootfs.clone() { + for (i, slot) in children.iter_mut().enumerate() { + let Some(c) = slot else { continue }; + let pid = c.pid; + let backing = child_rootfs_backing(work_dir, i + 1, &tag_rootfs); + match crate::chain::reflink_copy(&tag_rootfs, &backing) { + Ok(_) => backings[i] = Some(backing), + Err(e) => { + failures.push(RestoreFailure { + child_index: i + 1, + phase: RestorePhase::Restore, + pid: Some(pid), + error: format!("child rootfs backing {}: {e:#}", backing.display()), + }); + // Dropping the child kills its firecracker process. + *slot = None; + } + } + } + } + // Phase 2: parallel restore via threads. Each thread issues one // /snapshot/load PUT to its child's API socket. Body varies per // child only under MemfdShared (each child has its own memfd // path); for the File path, all children share the same JSON. + // + // With a backing the child is loaded PAUSED, re-pointed at that + // backing, and only then resumed, so no boot-time write can reach the + // shared base. Without one (no recorded rootfs) the previous + // single-call behaviour is preserved. let restore_start = Instant::now(); let mut handles: Vec<(usize, u32, thread::JoinHandle>)> = Vec::new(); for (i, slot) in children.iter().enumerate() { @@ -2328,6 +2421,8 @@ impl Snapshot { }; let sock = c.sock.clone(); let pid = c.pid; + let backing = backings[i].clone(); + let resume_on_load = backing.is_none(); let body = match &c.memfd { Some(region) => serde_json::json!({ "snapshot_path": &self.vmstate, @@ -2337,7 +2432,7 @@ impl Snapshot { "shared": true, }, "enable_diff_snapshots": opts.enable_diff_snapshots, - "resume_vm": true, + "resume_vm": resume_on_load, }) .to_string(), None => serde_json::json!({ @@ -2347,7 +2442,7 @@ impl Snapshot { "backend_type": "File", }, "enable_diff_snapshots": opts.enable_diff_snapshots, - "resume_vm": true, + "resume_vm": resume_on_load, }) .to_string(), }; @@ -2355,7 +2450,12 @@ impl Snapshot { i, pid, thread::spawn(move || -> Result<()> { - api_call(&sock, "PUT", "/snapshot/load", &body) + match backing { + Some(backing) => { + load_paused_rebind_rootfs_and_resume(&sock, &body, &backing) + } + None => api_call(&sock, "PUT", "/snapshot/load", &body), + } }), )); } From 4d4ba19e16f2de717001d58df75f2a29470b6ee5 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 21:45:28 -0700 Subject: [PATCH 2/8] docs: state the Firecracker version policy the per-child backings require MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vendored fork reads as mandatory today, which it is not: it carries the opt-in `MemBackendConfig::shared` field and `forkd-vmm` sends `shared` in exactly one place — the memfd path live-fork uses. Everything else, including the whole bake/restore/branch/exec surface, runs on stock Firecracker. Correcting that also gives somewhere to record the two version facts that now matter: - the tested baseline is v1.17.0, and per-child rootfs backings need **v1.15+**, because `PATCH /drives` on a restored VM is accepted by older builds but does not move the device's storage — a hard minimum, not a warning, since the failure mode is silent sharing rather than an error; - a vmstate is version-pinned, so upgrading invalidates every snapshot and means re-baking every tag. Adds the matching upgrade note and the feature entry to CHANGELOG. Signed-off-by: jrimmer --- CHANGELOG.md | 43 ++++++++++++++++++++++++++++++++++++ docs/VENDORED-FIRECRACKER.md | 6 ++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0059420..d0db86c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,49 @@ Versioning](https://semver.org/spec/v2.0.0.html) once it reaches ## Unreleased +### Upgrade note: Firecracker must be 1.15 or newer, and snapshots need re-baking + +Concurrent children of one snapshot now get their own rootfs backing (below), +which uses `PATCH /drives/{drive_id}` on a restored VM. That call is only +honoured from **Firecracker 1.15**; tested against **v1.17.0**. On older builds +the API accepts the request but the device keeps writing the original file, so +children would silently share the rootfs again — hence a hard minimum rather +than a warning. + +Because a vmstate is version-pinned, **upgrading Firecracker invalidates every +snapshot**: a new build refuses to load a vmstate written by an older one +(`Failed to load snapshot state from file`). Upgrading therefore means +re-baking every tag before its pool can serve again. + +Nothing else about the vendored fork changes: its MAP_SHARED patch is needed +only by `--live`, so a deployment that does not use live-fork can run stock +Firecracker. See docs/VENDORED-FIRECRACKER.md. + +### Per-child rootfs backings: concurrent children can no longer corrupt each other + +A snapshot's rootfs is one ext4 and Firecracker reopens that path verbatim for +every child, so children of a tag shared a filesystem. With the drive opened +read-write — how every tag was baked — two concurrent children were two guest +kernels writing one filesystem with no coordinator: package files picking up +other files' bytes, `EBADMSG` on `/var/lib/dpkg` entries, damage that reads as a +random build failure rather than a sandbox error. + +Each child now gets its own reflink clone of the tag's rootfs and is re-pointed +at it before it runs. The ordering is load-bearing: the child is loaded +`resume_vm: false`, re-pointed, and only then resumed, so no boot-time write +(journal replay, `/var/log`, agent startup) can reach the shared base first. +Measured on v1.17.0, the base stays byte-identical through boot while the +backing changes. + +A child whose rootfs cannot be re-pointed is **not** resumed — it would run +against the shared base — so the failure surfaces instead of drifting into +corruption. Backings live in the child's work dir and are reclaimed with it. + +Constraints worth knowing: a reflink clone is free only where the filesystem +supports it (XFS/btrfs, or ZFS 2.2+ block cloning); elsewhere it is a full copy. +Snapshots with no recorded rootfs (daemon-side branches inherit the source's) +keep the previous single-call behaviour. + ### Upgrade note: legacy sandbox rows block startup The controller now persists a boot identity (start time + boot id) for every diff --git a/docs/VENDORED-FIRECRACKER.md b/docs/VENDORED-FIRECRACKER.md index 0bcaa57..e49b97e 100644 --- a/docs/VENDORED-FIRECRACKER.md +++ b/docs/VENDORED-FIRECRACKER.md @@ -10,7 +10,11 @@ The fix is a ~44-line patch adding an opt-in `shared: bool` field to `MemBackend ## Where the fork lives -[**`deeplethe/firecracker`**](https://github.com/deeplethe/firecracker), branch [`forkd-v0.4-mem-backend-shared-v1.12`](https://github.com/deeplethe/firecracker/tree/forkd-v0.4-mem-backend-shared-v1.12) (matches the FC version forkd produces snapshots with). +[**`deeplethe/firecracker`**](https://github.com/deeplethe/firecracker), branch [`forkd-v0.4-mem-backend-shared-v1.12`](https://github.com/deeplethe/firecracker/tree/forkd-v0.4-mem-backend-shared-v1.12) (matches the version that branch was forked from). + +**The fork is needed only for `--live`.** It exists to carry the opt-in `shared: bool` on `MemBackendConfig`, and `forkd-vmm` sends `shared` in exactly one place — the memfd path live-fork uses. Every other restore path, and the whole bake/restore/branch/exec surface, runs on **stock Firecracker**. The tested baseline is **v1.17.0**, and the minimum for per-child rootfs backings (which rely on `PATCH /drives` on a restored VM) is **v1.15**. So if you do not use live-fork, run upstream Firecracker and ignore this document. + +Two things to know before upgrading the version: a vmstate is version-pinned, so a new build refuses to load snapshots written by an older one — an upgrade means re-baking every tag; and the API accepts `PATCH /drives` on builds older than 1.15 but does not move the device's storage, which is why the minimum is hard rather than advisory. - Forked from upstream `v1.12.0` tag. - Five commits on the branch (combined ~140 lines, all forkd-specific feature work): From b2c09c1b64c9519a44977924054e66dd504f577f Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:03:52 -0700 Subject: [PATCH 3/8] fix(vmm): reclaim a child's rootfs backing when the child dies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verifying the per-child backings on a live host showed the leak immediately: after killing two children, both 1.6 GiB copies were still on disk and the work dir was still there. Work dirs are not reclaimed on their own — the host had seven leftover ones holding only sockets and consoles, which was harmless until a backing moved in. The child owns its backing now, and `Drop` removes it alongside the socket and cgroup it already cleans up, so a kill reclaims the copy without depending on the caller's work-dir hygiene. Set from the backing pass rather than at construction, because the child has to exist before anything can own it. A controller crash can still strand a backing; that is the same class as the stale staging dirs and `.prev-*` backups, which are swept by their next use rather than by Drop. Signed-off-by: jrimmer --- crates/forkd-vmm/src/lib.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index d850137..53d1157 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -266,6 +266,12 @@ pub struct Vm { /// Drop. Phase 6's UFFD_WP arming will dup this fd to register a /// `userfaultfd` against the same VMA. pub memfd: Option, + /// This child's own writable rootfs backing, when it was restored from a + /// snapshot that recorded a rootfs (see `child_rootfs_backing`). Owned + /// here so it dies with the child: work dirs are not reclaimed on their + /// own, so a backing left inside one is a full rootfs copy that nothing + /// would ever collect. `None` for VMs booted directly. + backing: Option, } /// Accept one connection on an already-non-blocking `UnixListener`, @@ -1813,6 +1819,7 @@ impl Vm { netns: None, cgroup: None, memfd: None, + backing: None, }) } @@ -2137,6 +2144,11 @@ impl Drop for Vm { if let Some(cg) = &self.cgroup { cgroup::cleanup(cg); } + // Reclaim this child's writable rootfs copy. Work dirs outlive their + // VMs, so leaving it to the dir would leak a full rootfs per spawn. + if let Some(backing) = self.backing.take() { + let _ = std::fs::remove_file(&backing); + } } } @@ -2232,6 +2244,9 @@ impl Snapshot { netns, cgroup: None, memfd: None, + // Set by the backing pass below, once the child + // exists to own it. + backing: None, })); } Err(e) => { @@ -2389,7 +2404,13 @@ impl Snapshot { let pid = c.pid; let backing = child_rootfs_backing(work_dir, i + 1, &tag_rootfs); match crate::chain::reflink_copy(&tag_rootfs, &backing) { - Ok(_) => backings[i] = Some(backing), + Ok(_) => { + // The child owns it from here: Drop removes it, so a + // kill reclaims the copy instead of leaving a full + // rootfs in a work dir nothing sweeps. + c.backing = Some(backing.clone()); + backings[i] = Some(backing); + } Err(e) => { failures.push(RestoreFailure { child_index: i + 1, From 40a334125dbb295e27c76b47ae1ea7bbf38f3576 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:28:11 -0700 Subject: [PATCH 4/8] fix(controller): record the rootfs a daemon-baked snapshot booted from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore clones each child's writable backing from the snapshot's recorded rootfs, so a snapshot with no recorded path has nothing to clone and its children fall back to sharing one file. The CLI has always written the field; the daemon never did, which left daemon-created tags — `cp`/clone and preview branches — as the one place children still shared a rootfs. The path is the one the parent actually booted from, canonicalized, matching what the CLI records. Daemon-created branches already inherit `rootfs` from their head snapshot, so those are covered by this single site. Signed-off-by: jrimmer --- crates/forkd-controller/src/http.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 1b55999..a66154a 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -482,11 +482,22 @@ async fn create_snapshot( // Volumes via daemon snapshot API will land in a follow-up commit; // for now snapshots created through the daemon are volume-less. // Use the CLI's `forkd snapshot --volume` for tag-shared caches. - let snap = vm.snapshot_to( + let mut snap = vm.snapshot_to( snap_dir_for_task.join("vmstate"), snap_dir_for_task.join("memory.bin"), Vec::new(), )?; + // Record the rootfs this parent booted from. Restore gives every child + // its own reflink backing cloned from this path, and with no path there + // is nothing to clone, so children fall back to sharing one file — the + // daemon's snapshots were the last ones doing that. Daemon-created + // branches inherit the field from their head snapshot, so this single + // site covers them as well. + snap.rootfs = Some( + cfg.rootfs + .canonicalize() + .unwrap_or_else(|_| cfg.rootfs.clone()), + ); // Persist Snapshot metadata so subsequent forks read back the same // (possibly volume-bearing) snapshot description. let meta = serde_json::to_vec_pretty(&snap)?; From 08774e7aedfb35de450f830207632a3bc35182b4 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:40:00 -0700 Subject: [PATCH 5/8] fix(controller): reclaim rootfs backings a crashed controller stranded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A child killed while the controller is up has its backing removed by Vm::drop. The other order — controller crash, then startup reap — had nothing collecting them: the watchdog's stale-dir sweep keys on child-1.sock being absent, and it is present because the orphaned child held it until the reap killed it. The reap now reads each orphan's work dir from its own --api-sock while /proc still has it, and removes the child-* backings in that dir once the process is confirmed dead. Only the backings are removed, never the dir (the watchdog owns that), and only for a directory whose process this reap just reaped, so a live VM cannot be caught. Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 66 ++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index b1997ac..8ef4ce9 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -322,6 +322,65 @@ impl Registry { /// If some kills failed, those entries remain in the registry and /// the allocator may still collide with them — the caller should /// log the failure count and the operator should investigate. + /// The work dir a Firecracker process was started with, read from its own + /// `--api-sock` argument. `None` when the process is already gone or the + /// argument is unreadable — callers skip the reclaim rather than guess a + /// path, since removing the wrong directory would take a live VM's sockets + /// with it. + fn child_work_dir_from_cmdline(pid: u32) -> Option { + let raw = std::fs::read(format!("/proc/{pid}/cmdline")).ok()?; + let args: Vec = raw + .split(|b| *b == 0) + .filter(|s| !s.is_empty()) + .map(|s| String::from_utf8_lossy(s).into_owned()) + .collect(); + let sock = args + .iter() + .position(|a| a == "--api-sock") + .and_then(|i| args.get(i + 1))?; + Some(std::path::Path::new(sock).parent()?.to_path_buf()) + } + + /// Is `name` a restored child's rootfs backing (`child-.`)? + /// + /// The same dir also holds `child-.sock` and `child-.console`, and the + /// stem is the tag's rootfs filename — not always `rootfs.ext4` — so match + /// on shape rather than a fixed suffix. + fn is_child_backing(name: &str) -> bool { + let Some(rest) = name.strip_prefix("child-") else { + return false; + }; + let Some((index, ext)) = rest.split_once('.') else { + return false; + }; + !index.is_empty() + && index.chars().all(|c| c.is_ascii_digit()) + && !ext.is_empty() + && ext != "sock" + && ext != "console" + } + + /// Reclaim the rootfs backings a reaped child left behind. + /// + /// A child killed while the controller is up has its backing removed by + /// `Vm::drop`. This covers the other order — controller crash, then startup + /// reap — where nothing else collects them: the watchdog leaves the dir + /// alone because the socket is present, and the socket is present because + /// the orphan held it until now. Only backings in a directory belonging to + /// a process this reap has just confirmed dead are touched, so a live VM + /// cannot be caught; the dir itself is left to the watchdog's own sweep. + fn reclaim_child_backings(work_dir: &std::path::Path) { + let Ok(entries) = std::fs::read_dir(work_dir) else { + return; + }; + for entry in entries.flatten() { + let name = entry.file_name().to_string_lossy().into_owned(); + if Self::is_child_backing(&name) { + let _ = std::fs::remove_file(entry.path()); + } + } + } + pub(crate) fn kill_orphans(&self) -> Result { // One collected orphan: (sandbox id, pid, recorded start time, // recorded boot id). @@ -471,6 +530,10 @@ impl Registry { pid = pid, "killing orphaned Firecracker process on startup (pidfd signal, identity verified)" ); + // Read the child's work dir now: once the process is gone + // there is nothing left in /proc to read it from, and the + // rootfs backing inside would be stranded for good. + let orphan_work_dir = Self::child_work_dir_from_cmdline(pid); match pidfd_send_kill(pidfd) { Ok(()) => { // Wait for the process to actually exit (bounded). @@ -478,6 +541,9 @@ impl Registry { // hold netns/tap resources past the kill return. if wait_for_death(pid, std::time::Duration::from_secs(5)) { self.inner.lock().sandboxes.remove(&id); + if let Some(dir) = &orphan_work_dir { + Self::reclaim_child_backings(dir); + } killed += 1; } else { tracing::error!( From 93339431111a9ecfb489326ff584643d8a0b3913 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:48:04 -0700 Subject: [PATCH 6/8] test(controller): pin the backing predicate against the sock/console shapes The predicate selects what a startup reap deletes, and the work dir holds `child-.sock` and `child-.console` under the same prefix, while the stem is the tag's rootfs filename and so not always `rootfs.ext4`. Each assertion names the shape it is defending. Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 8ef4ce9..da43131 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -1204,6 +1204,28 @@ mod tests { ); } + /// The predicate decides what a startup reap deletes, and the work dir holds + /// sockets and consoles under the same `child-.` shape. The stem is the + /// tag's rootfs filename, so it is not always `rootfs.ext4`. + #[test] + fn child_backing_predicate_matches_backings_only() { + assert!(Registry::is_child_backing("child-1.rootfs.ext4")); + assert!(Registry::is_child_backing("child-12.python-3-12-slim.ext4")); + assert!( + !Registry::is_child_backing("child-1.sock"), + "sockets share the child-. shape and must survive the sweep" + ); + assert!( + !Registry::is_child_backing("child-1.console"), + "consoles share the child-. shape and must survive the sweep" + ); + assert!(!Registry::is_child_backing("memory-assembled.bin")); + assert!(!Registry::is_child_backing("child.rootfs.ext4")); + assert!(!Registry::is_child_backing("child-x.rootfs.ext4")); + assert!(!Registry::is_child_backing("child-1.")); + assert!(!Registry::is_child_backing("rootfs.ext4")); + } + #[test] #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_prunes_alive_pid_entries() { From 8af234e4478eef1a589f8a7fa0e4010f4798745b Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 23:16:13 -0700 Subject: [PATCH 7/8] fix(controller): sweep backings in directories no live Firecracker owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-kill reclaim could not cover a row retired *without* a kill. The crash simulation showed the ordinary restart shape: systemd kills the unit's cgroup, so the children die with the controller, the reap finds dead processes, and it prunes rather than kills — which is correct, but nothing then reaches `Vm::drop`, so nothing removes the backing. The watchdog leaves that directory alone because the socket file is still present. The sweep runs before the reap, when those children are already gone and their directories are unowned. Its gate is process ownership rather than file presence: a directory is skipped while any live Firecracker's `--api-sock` points inside it, and if any live Firecracker cannot be read the sweep removes nothing, because ownership can no longer be established. Leaking a backing costs disk; deleting a live VM's backing destroys that VM's disk mid-job. Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 62 ++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index da43131..8bbca31 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -381,7 +381,69 @@ impl Registry { } } + /// Remove backings left in directories no live Firecracker is using. + /// + /// Covers what the per-kill reclaim cannot: a row retired *without* a kill + /// because its process is already gone — the ordinary shape of a controller + /// restart, where systemd kills the unit's cgroup and takes the children with + /// it. Nothing reaches `Vm::drop` in that ordering, so nothing removes the + /// backing, and the watchdog leaves the directory alone because the socket + /// file is still there. + /// + /// The gate is process ownership, not file presence. A directory is skipped + /// while any live Firecracker's `--api-sock` points inside it, so a running + /// VM's backing cannot be caught; and if any live Firecracker cannot be read, + /// nothing is removed at all, because ownership can no longer be established. + /// Leaking a backing costs disk. Deleting a live VM's backing destroys that + /// VM's disk mid-job, so every ambiguity resolves toward keeping it. + fn sweep_stranded_backings() { + let mut owned = std::collections::HashSet::new(); + let Ok(pids) = std::fs::read_dir("/proc") else { + return; + }; + for entry in pids.flatten() { + let Ok(pid) = entry.file_name().to_string_lossy().parse::() else { + continue; + }; + let Ok(raw) = std::fs::read(format!("/proc/{pid}/cmdline")) else { + continue; + }; + if !raw.windows(12).any(|w| w == b"firecracker\0") { + continue; + } + match Self::child_work_dir_from_cmdline(pid) { + Some(dir) => { + owned.insert(dir); + } + // A live Firecracker whose directory we cannot read means we + // cannot say what is owned, so nothing is swept this time. + None => return, + } + } + let Ok(entries) = std::fs::read_dir(std::env::temp_dir()) else { + return; + }; + for entry in entries.flatten() { + if !entry.path().is_dir() { + continue; + } + let name = entry.file_name().to_string_lossy().into_owned(); + if !name.starts_with("forkd-daemon-") { + continue; + } + let dir = entry.path(); + if !owned.contains(&dir) { + Self::reclaim_child_backings(&dir); + } + } + } + pub(crate) fn kill_orphans(&self) -> Result { + // Sweep before the reap: in the restart case the children are already + // dead, so their rows are retired without a kill and `Vm::drop` never + // runs for them. Their directories are unowned by now, which is exactly + // what the sweep requires. + Self::sweep_stranded_backings(); // One collected orphan: (sandbox id, pid, recorded start time, // recorded boot id). type OrphanRow = (String, u32, Option, Option); From 60d0b8c54146c8c0c20af9ec64be4c827ba72fd8 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 23:27:50 -0700 Subject: [PATCH 8/8] test(controller): pin the predicate's approximate edges, not just its happy path The predicate already had the cases its callers rely on. These are the ones a reader would guess the other way, and two of them guard against a plausible future edit: - `child-1.sock.bak` and `child-1.SOCK` match, because the comparison is against everything after the first dot rather than a suffix. Nothing creates such a file; the assertions exist so the approximation is visible rather than assumed exact. - a multi-dot or non-ext4 stem (`child-1.tar.gz`, `child-1.rootfs.squashfs`) matches, because the stem is the tag's rootfs filename. Tightening this to `.ext4` would look like a hardening and would silently stop collecting backings for any tag whose rootfs is not an ext4. - the index is never parsed, so leading zeros and absurd values pass. - ASCII digits only: generalising to `is_numeric()` would accept unicode digits and start deleting names we never created. Every assertion was confirmed by running it, not by reading the predicate. Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 8bbca31..fe02521 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -1286,6 +1286,31 @@ mod tests { assert!(!Registry::is_child_backing("child-x.rootfs.ext4")); assert!(!Registry::is_child_backing("child-1.")); assert!(!Registry::is_child_backing("rootfs.ext4")); + + // Shape, not suffix: the comparison is against everything after the + // first dot, so these match and the sweep would delete them. Nothing + // creates such a file — the assertions are here so a reader knows the + // predicate is deliberately approximate rather than exact. + assert!(Registry::is_child_backing("child-1.sock.bak")); + assert!(Registry::is_child_backing("child-1.SOCK")); + + // The stem is the tag's rootfs filename, so a multi-dot or non-ext4 stem + // must keep matching: tightening this to `.ext4` would look like an + // improvement and would silently stop collecting those backings. + assert!(Registry::is_child_backing("child-1.tar.gz")); + assert!(Registry::is_child_backing("child-1.rootfs.squashfs")); + + // The index is never parsed, so leading zeros and absurd values pass. + assert!(Registry::is_child_backing("child-0.rootfs.ext4")); + assert!(Registry::is_child_backing("child-01.rootfs.ext4")); + assert!(Registry::is_child_backing("child-4294967296.rootfs.ext4")); + + // ASCII digits only. Generalising to `is_numeric()` would accept unicode + // digits and start deleting names we never created. + assert!(!Registry::is_child_backing("child-1\u{662}.rootfs.ext4")); + // Callers pass a file name, not a path. + assert!(!Registry::is_child_backing("child-1/rootfs.ext4")); + assert!(!Registry::is_child_backing("")); } #[test]