From f8660d4cb81d3844f22af3508fa43709942d3205 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 21:44:30 -0700 Subject: [PATCH 01/11] 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 ce822ef..0d425d2 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) } @@ -2355,10 +2415,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() { @@ -2367,6 +2460,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, @@ -2376,7 +2471,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!({ @@ -2386,7 +2481,7 @@ impl Snapshot { "backend_type": "File", }, "enable_diff_snapshots": opts.enable_diff_snapshots, - "resume_vm": true, + "resume_vm": resume_on_load, }) .to_string(), }; @@ -2394,7 +2489,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 08868c235dd4f7ff91fd052d2f2c85797ba1596f Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 21:45:28 -0700 Subject: [PATCH 02/11] 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 ae048e6..579fdb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,49 @@ Versioning](https://semver.org/spec/v2.0.0.html) once it reaches chain-assembly base copy and the bake's rootfs baseline clone streamed a full copy even on btrfs, XFS, and ZFS 2.2+. Found by @jrimmer in #321. +### 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 01be496f249f5462a0fe8db94170b6f310566fbf Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:03:52 -0700 Subject: [PATCH 03/11] 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 0d425d2..6342766 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`, @@ -1852,6 +1858,7 @@ impl Vm { netns: None, cgroup: None, memfd: None, + backing: None, }) } @@ -2176,6 +2183,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); + } } } @@ -2271,6 +2283,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) => { @@ -2428,7 +2443,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 c9c525e1647ba9d09f04810f515d20a97691ca8d Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:28:11 -0700 Subject: [PATCH 04/11] 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 065e0cb7bb80d783e294deb2bfce3199af77adea Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:40:00 -0700 Subject: [PATCH 05/11] 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 6eac60f5e80ebaae1f4edc9d3c4cd3f37b3b51cb Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 22:48:04 -0700 Subject: [PATCH 06/11] 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 f0f335bb10fb0b8e76458d8d2f8fe4782b0d1c30 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 23:16:13 -0700 Subject: [PATCH 07/11] 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 44b399949ca3d536a79b5f707746f258264f5b7b Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 23:27:50 -0700 Subject: [PATCH 08/11] 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] From e0ad1a892695dc12977f9d43e8c75b37b5fa09fc Mon Sep 17 00:00:00 2001 From: Jason Rimmer Date: Mon, 28 Sep 2026 14:18:37 -0700 Subject: [PATCH 09/11] fix(vmm,controller): verify every rootfs re-point, and give branches their own rootfs Three defects in per-child backings, found in review and on a KVM host. Branches did not own their rootfs. A snapshot of a restored child recorded the child's backing, a file in a work dir that dies with the child, so the new tag stopped loading once its source was gone ("Failed to restore from snapshot ... No such file"), and until then every child restored from it wrote the source's live disk. BRANCH (full, diff and live) and workspace suspend now point the paused source's drive at a clone the new tag owns for the length of the snapshot, record it as the tag's rootfs, and point the drive home before the source resumes. A source whose drive cannot be pointed home is killed rather than resumed onto the tag's disk. A PATCH was trusted on its 204. Every re-point now reads GET /vm/config back and fails unless Firecracker reports the new path, so a child is never resumed on the shared base on the strength of an accepted request. Read-only rootfs was copied per child for nothing. Whether the drive is writable is now read from the restored vmstate itself, so read-only tags, including ones baked before this, share the file without a copy. The copies move into the per-child restore threads, so they run in parallel. The restore work-dir root and its prefix come from one helper, shared by the spawn path and the startup sweep. --- crates/forkd-controller/src/http.rs | 66 +++- crates/forkd-controller/src/state.rs | 18 +- crates/forkd-vmm/src/lib.rs | 467 +++++++++++++++++++++++---- 3 files changed, 474 insertions(+), 77 deletions(-) diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index a66154a..fc05929 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -490,9 +490,9 @@ async fn create_snapshot( // 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. + // daemon's snapshots were the last ones doing that. Branches and + // workspace suspends record a clone of their own (see + // `Vm::detour_rootfs_into`). snap.rootfs = Some( cfg.rootfs .canonicalize() @@ -1370,7 +1370,10 @@ async fn create_sandbox( // ran concurrently (e.g. two branches of the same source). Mix the // netns offset in so concurrent batches get distinct work_dirs. let work_dir = - std::env::temp_dir().join(format!("forkd-daemon-{tag_for_work_dir}-o{netns_offset}")); + crate::state::work_dir_root().join(format!( + "{}{tag_for_work_dir}-o{netns_offset}", + crate::state::WORK_DIR_PREFIX + )); // v0.5 chain assembly. If `chain` is set, hash-verify each // parent against the recorded parent_content_hash (this is // the foot-gun guard committed to in the design doc), then @@ -1986,7 +1989,7 @@ async fn branch_sandbox( // our mmap. #[cfg(target_os = "linux")] if live_mode { - let (live_pause_ms, worker) = run_live_branch_setup( + let (live_pause_ms, worker, branch_rootfs) = run_live_branch_setup( &vm, &snap_dir_for_task, &dst_mem, @@ -2018,7 +2021,7 @@ async fn branch_sandbox( volumes: source_volumes, parent_tag: None, parent_content_hash: None, - rootfs: None, + rootfs: branch_rootfs, }); } #[cfg(not(target_os = "linux"))] @@ -2049,6 +2052,15 @@ async fn branch_sandbox( let pause_start = std::time::Instant::now(); vm.pause()?; + // A restored child's rootfs is its own backing, which dies + // with it. Point the drive at a copy this tag owns for the + // snapshot, so the vmstate records a durable path, and back + // before the source resumes (see `Vm::detour_rootfs_into`). + let rootfs_detour = vm + .detour_rootfs_into(&snap_dir_for_task) + .context("give the branch its own rootfs")?; + let branch_rootfs = rootfs_detour.as_ref().map(|d| d.path().to_path_buf()); + // Phase 1a measurement hook: take a Diff snapshot first // (captures pages dirtied since restore; clears the dirty // bitmap). Discarded after metrics. The subsequent Full @@ -2104,6 +2116,9 @@ async fn branch_sandbox( diff_snap.physical_size_bytes, diff_snap.logical_size_bytes, )); + if let Some(detour) = rootfs_detour { + detour.finish()?; + } let resume_result = vm.resume(); pause_ms = Some(pause_start.elapsed().as_millis() as u64); @@ -2149,16 +2164,20 @@ async fn branch_sandbox( volumes: diff_snap.volumes, parent_tag: None, parent_content_hash: None, - rootfs: None, + rootfs: branch_rootfs, } } else { - let snap = vm.snapshot_to( + let mut snap = vm.snapshot_to( snap_dir_for_task.join("vmstate"), snap_dir_for_task.join("memory.bin"), // Inherit volumes from the source snapshot so grandchildren // re-attach the same persistent disks the source had. source_volumes, )?; + if let Some(detour) = rootfs_detour { + detour.finish()?; + } + snap.rootfs = branch_rootfs; // resume() may fail after a successful snapshot. The snapshot file // is intact and usable; the source sandbox is in an unknown state // (most likely still paused). We log and continue rather than @@ -2618,7 +2637,7 @@ fn run_live_branch_setup( dst_mem: &std::path::Path, source_volumes: Vec, id_for_log: &str, -) -> anyhow::Result<(u64, LiveBranchWorker)> { +) -> anyhow::Result<(u64, LiveBranchWorker, Option)> { use std::os::fd::AsRawFd; let memfd = vm.memfd_handle().ok_or_else(|| { @@ -2710,12 +2729,23 @@ fn run_live_branch_setup( // resume. PauseGuard's Drop resumes on early return. let pause_start = std::time::Instant::now(); let pause_guard = vm.pause_guard()?; + // Declared after the pause guard so an early return drops it first: the + // drive goes home before the guard resumes the source. The clone runs + // inside the pause window — near-free with reflink, a full rootfs copy + // without it — because the copy has to match the vmstate's instant. + let rootfs_detour = vm + .detour_rootfs_into(snap_dir) + .context("give the branch its own rootfs")?; + let branch_rootfs = rootfs_detour.as_ref().map(|d| d.path().to_path_buf()); vm.snapshot_vmstate_only( snap_dir.join("vmstate"), dst_mem.to_path_buf(), source_volumes, ) .context("vmstate-only snapshot during live BRANCH")?; + if let Some(detour) = rootfs_detour { + detour.finish()?; + } pause_guard.resume().context("resume after vmstate dump")?; let pause_ms = pause_start.elapsed().as_millis() as u64; @@ -2731,6 +2761,7 @@ fn run_live_branch_setup( mmap_guard, wp_branch, }, + branch_rootfs, )) } @@ -3227,6 +3258,12 @@ async fn suspend_workspace( None }; vm.pause()?; + // As in `branch_sandbox`: the state snapshot must own its rootfs, + // not record the live child's backing, which dies with the child. + let rootfs_detour = vm + .detour_rootfs_into(&snap_dir_for_task) + .context("give the workspace state its own rootfs")?; + let state_rootfs = rootfs_detour.as_ref().map(|d| d.path().to_path_buf()); let snap = if diff_mode { let diff_path = std::env::temp_dir().join(format!( "forkd-ws-diff-{}-{}.bin", @@ -3238,6 +3275,9 @@ async fn suspend_workspace( diff_path.clone(), Vec::new(), )?; + if let Some(detour) = rootfs_detour { + detour.finish()?; + } vm.resume()?; pause_ms = Some(pause_start.elapsed().as_millis() as u64); if let Some(h) = cp_handle { @@ -3252,14 +3292,18 @@ async fn suspend_workspace( volumes: diff_snap.volumes, parent_tag: None, parent_content_hash: None, - rootfs: None, + rootfs: state_rootfs, } } else { - 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(), )?; + if let Some(detour) = rootfs_detour { + detour.finish()?; + } + snap.rootfs = state_rootfs; vm.resume()?; pause_ms = Some(pause_start.elapsed().as_millis() as u64); snap diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index fe02521..10a35f4 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -39,6 +39,20 @@ pub struct Registry { path: PathBuf, } +/// The directory a restore's work dir is named for, under [`work_dir_root`]. +/// One helper for both ends: the spawn path creates these, and the startup +/// sweep reclaims the rootfs backings stranded in them, so the two must agree +/// on where to look even when `TMPDIR` changes. +pub(crate) const WORK_DIR_PREFIX: &str = "forkd-daemon-"; + +/// Where restore work dirs live: the controller's temp dir. A child's rootfs +/// backing lives in its work dir, so on a host where `/tmp` is tmpfs, point +/// `TMPDIR` at the snapshot root's filesystem — both so the backings are not +/// charged to RAM and so the reflink clone has both ends on one filesystem. +pub(crate) fn work_dir_root() -> PathBuf { + std::env::temp_dir() +} + impl Registry { pub fn load_or_init(path: impl Into) -> Result { let path: PathBuf = path.into(); @@ -420,7 +434,7 @@ impl Registry { None => return, } } - let Ok(entries) = std::fs::read_dir(std::env::temp_dir()) else { + let Ok(entries) = std::fs::read_dir(work_dir_root()) else { return; }; for entry in entries.flatten() { @@ -428,7 +442,7 @@ impl Registry { continue; } let name = entry.file_name().to_string_lossy().into_owned(); - if !name.starts_with("forkd-daemon-") { + if !name.starts_with(WORK_DIR_PREFIX) { continue; } let dir = entry.path(); diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index 6342766..6590ce6 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -1465,8 +1465,57 @@ fn child_rootfs_backing(work_dir: &Path, child_index: usize, tag_rootfs: &Path) work_dir.join(format!("child-{child_index}.{stem}")) } -/// Load a restored child **paused**, re-point its rootfs at `backing`, then -/// resume it. +/// The rootfs drive as the running Firecracker reports it: its host path and +/// whether it is read-only. Read from `GET /vm/config`, which on a restored VM +/// reflects the vmstate — so the read-only flag is the one the snapshot was +/// baked with, whatever `snapshot.json` does or does not record. +fn rootfs_drive(sock: &Path) -> Result<(PathBuf, bool)> { + let raw = api_request(sock, "GET", "/vm/config", "", DEFAULT_API_TIMEOUT_SECS)?; + let config: serde_json::Value = + serde_json::from_str(&raw).context("parse GET /vm/config response")?; + let drive = config["drives"] + .as_array() + .and_then(|drives| drives.iter().find(|d| d["drive_id"] == "rootfs")) + .context("GET /vm/config lists no drive with drive_id `rootfs`")?; + let path = drive["path_on_host"] + .as_str() + .context("rootfs drive has no path_on_host")?; + Ok((PathBuf::from(path), drive["is_read_only"] == true)) +} + +/// Re-point the rootfs drive at `path`, then read the configuration back and +/// fail unless Firecracker reports `path` as the drive's host path. +/// +/// The read-back is what makes this fail closed rather than trust a 204: a +/// build that accepted the PATCH without taking it would otherwise leave the +/// child on the old file with nothing to say so. Measured on stock v1.5.1, +/// v1.7.0, v1.10.1, v1.12.1, v1.14.1 and v1.17.0: every one moves the guest's +/// writes to the new file on a restored VM and reports the new path. +fn repoint_rootfs(sock: &Path, path: &Path) -> Result<()> { + // `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": path}).to_string(), + )?; + let (reported, _) = rootfs_drive(sock).context("read back the rootfs drive after PATCH")?; + if reported != path { + bail!( + "Firecracker accepted PATCH /drives/rootfs but still reports {} as the rootfs, \ + not {}", + reported.display(), + path.display() + ); + } + Ok(()) +} + +/// Load a restored child **paused**, give it its own clone of `tag_rootfs` at +/// `backing`, re-point its rootfs there, then resume it. Returns whether a +/// backing was made: a read-only rootfs cannot be written, so it is shared as +/// is and costs no copy. /// /// 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 @@ -1482,28 +1531,26 @@ fn child_rootfs_backing(work_dir: &Path, child_index: usize, tag_rootfs: &Path) fn load_paused_rebind_rootfs_and_resume( sock: &Path, load_body: &str, + tag_rootfs: &Path, backing: &Path, -) -> Result<()> { +) -> 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() - ) - })?; + let (_, read_only) = rootfs_drive(sock).context("read the restored child's rootfs drive")?; + if !read_only { + crate::chain::reflink_copy(tag_rootfs, backing) + .with_context(|| format!("child rootfs backing {}", backing.display()))?; + repoint_rootfs(sock, backing).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(()) + Ok(!read_only) } fn api_call(sock: &Path, method: &str, path: &str, body: &str) -> Result<()> { @@ -1524,6 +1571,18 @@ fn api_call_with_timeout( body: &str, timeout_secs: u64, ) -> Result<()> { + api_request(sock, method, path, body, timeout_secs).map(|_| ()) +} + +/// `api_call_with_timeout`, returning the response body for the callers that +/// read one (`GET /vm/config`). +fn api_request( + sock: &Path, + method: &str, + path: &str, + body: &str, + timeout_secs: u64, +) -> Result { let mut stream = UnixStream::connect(sock).with_context(|| format!("connect {}", sock.display()))?; let timeout = Duration::from_secs(timeout_secs); @@ -1574,11 +1633,11 @@ fn api_call_with_timeout( ) })?; + let body_start = response + .find("\r\n\r\n") + .map(|i| i + 4) + .unwrap_or(response.len()); if !(200..300).contains(&status_code) { - let body_start = response - .find("\r\n\r\n") - .map(|i| i + 4) - .unwrap_or(response.len()); bail!( "firecracker API {} {} returned {}: {}", method, @@ -1587,7 +1646,7 @@ fn api_call_with_timeout( response[body_start..].trim() ); } - Ok(()) + Ok(response[body_start..].to_string()) } /// True if `buf` contains a complete HTTP response (headers + body up to @@ -1884,6 +1943,103 @@ impl Vm { } } +impl Vm { + /// Give a paused VM's rootfs a durable home for the snapshot about to be + /// taken of it: clone this child's own backing into `dir`, and point the + /// drive there so the vmstate records a path the new snapshot owns. + /// + /// Without this, a snapshot of a restored child records the child's + /// backing — a file in a work dir that dies with the child — so the new + /// tag stops loading once the child is gone, and every child restored from + /// it would write the source's live disk in the meantime. + /// + /// The VM must stay paused until the returned detour is + /// [`finish`](RootfsDetour::finish)ed, which points the drive back at the + /// child's own backing. Returns `None` for a VM with no backing of its own + /// (booted directly, or restored from a snapshot that recorded no rootfs, + /// or a read-only rootfs): its drive already points at a durable file. + pub fn detour_rootfs_into(&self, dir: &Path) -> Result>> { + let Some(home) = self.backing.clone() else { + return Ok(None); + }; + // `child-.`; the snapshot keeps the tag's name. + let name = home + .file_name() + .and_then(|n| n.to_str()) + .and_then(|n| n.split_once('.')) + .map(|(_, rest)| rest.to_string()) + .unwrap_or_else(|| "rootfs.ext4".to_string()); + let path = dir.join(name); + crate::chain::reflink_copy(&home, &path) + .with_context(|| format!("clone child rootfs into {}", path.display()))?; + // Built before the PATCH so that a failed PATCH still points the drive + // home on drop — Firecracker may have taken it before the read-back. + let detour = RootfsDetour { + vm: self, + home, + path, + finished: false, + }; + repoint_rootfs(&self.sock, &detour.path).context("point rootfs at the snapshot's copy")?; + Ok(Some(detour)) + } +} + +/// A paused VM whose rootfs drive points at a snapshot's copy instead of its +/// own backing. See [`Vm::detour_rootfs_into`]. +/// +/// Resuming the VM in this state would have it write the snapshot's rootfs, so +/// pointing it home is fail-closed: if that cannot be verified, the VM's +/// Firecracker is killed rather than left to be resumed onto another tag's +/// disk. Dropping an unfinished detour does the same, which covers every early +/// return between the detour and the resume. +pub struct RootfsDetour<'a> { + vm: &'a Vm, + home: PathBuf, + path: PathBuf, + finished: bool, +} + +impl RootfsDetour<'_> { + /// Where the snapshot's copy of the rootfs is — the path to record as its + /// `rootfs`. + pub fn path(&self) -> &Path { + &self.path + } + + /// Point the drive back at the VM's own backing. On error the VM has been + /// killed. + pub fn finish(mut self) -> Result<()> { + self.finished = true; + self.go_home() + } + + fn go_home(&self) -> Result<()> { + repoint_rootfs(&self.vm.sock, &self.home).map_err(|e| { + // SAFETY: kill(2) on a pid we spawned; no memory is touched. + unsafe { + libc::kill(self.vm.pid as libc::pid_t, libc::SIGKILL); + } + e.context(format!( + "point rootfs back at the child's own backing {} — the child was killed \ + rather than resumed onto the snapshot's copy {}", + self.home.display(), + self.path.display() + )) + }) + } +} + +impl Drop for RootfsDetour<'_> { + fn drop(&mut self) { + if !self.finished { + if let Err(e) = self.go_home() { + tracing::warn!(error = %format!("{e:#}"), "rootfs detour: could not return home"); + } + } + } +} + /// RAII guard that holds a VM paused. Drop calls `vm.resume()` unless /// the guard was [`commit`](Self::commit)ted (used when the caller has /// already explicitly resumed and doesn't want the implicit retry). @@ -2430,37 +2586,21 @@ 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. + // Every child of a tag that recorded a rootfs is assigned its own + // backing path up front, and owns it from here: Drop removes it, so a + // failed or killed child reclaims the copy instead of leaving a full + // rootfs in a work dir nothing sweeps. The copy itself is made in the + // child's restore thread below, once the loaded vmstate has said + // whether the drive is writable at all. Snapshots with no recorded + // rootfs keep the previous behaviour. + let tag_rootfs = self.rootfs.clone(); let mut backings: Vec> = vec![None; children.len()]; - if let Some(tag_rootfs) = self.rootfs.clone() { + if let Some(tag_rootfs) = &tag_rootfs { 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(_) => { - // 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, - phase: RestorePhase::Restore, - pid: Some(pid), - error: format!("child rootfs backing {}: {e:#}", backing.display()), - }); - // Dropping the child kills its firecracker process. - *slot = None; - } - } + let backing = child_rootfs_backing(work_dir, i + 1, tag_rootfs); + c.backing = Some(backing.clone()); + backings[i] = Some(backing); } } @@ -2469,12 +2609,13 @@ impl Snapshot { // 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. + // With a backing the child is loaded PAUSED, given its copy, + // re-pointed at it, and only then resumed, so no boot-time write can + // reach the shared base; the copies run in these threads, in parallel. + // 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(); + let mut handles: Vec<(usize, u32, thread::JoinHandle>)> = Vec::new(); for (i, slot) in children.iter().enumerate() { let Some(c) = slot else { continue; @@ -2482,6 +2623,7 @@ impl Snapshot { let sock = c.sock.clone(); let pid = c.pid; let backing = backings[i].clone(); + let tag_rootfs = tag_rootfs.clone(); let resume_on_load = backing.is_none(); let body = match &c.memfd { Some(region) => serde_json::json!({ @@ -2509,19 +2651,28 @@ impl Snapshot { handles.push(( i, pid, - thread::spawn(move || -> Result<()> { - match backing { - Some(backing) => { - load_paused_rebind_rootfs_and_resume(&sock, &body, &backing) - } - None => api_call(&sock, "PUT", "/snapshot/load", &body), + thread::spawn(move || -> Result { + match (backing, tag_rootfs) { + (Some(backing), Some(tag_rootfs)) => load_paused_rebind_rootfs_and_resume( + &sock, + &body, + &tag_rootfs, + &backing, + ), + _ => api_call(&sock, "PUT", "/snapshot/load", &body).map(|()| false), } }), )); } for (i, pid, h) in handles { match h.join() { - Ok(Ok(())) => {} + Ok(Ok(true)) => {} + // A read-only rootfs is shared as is; there is no backing to own. + Ok(Ok(false)) => { + if let Some(c) = children[i].as_mut() { + c.backing = None; + } + } Ok(Err(e)) => { failures.push(RestoreFailure { child_index: i + 1, @@ -2891,6 +3042,194 @@ mod tests { let _ = std::fs::remove_file(&sock); } + /// A stand-in for Firecracker's API socket: answers `GET /vm/config` with + /// one rootfs drive, and records every request. `honour_patch: false` + /// models a build that answers `PATCH /drives` with 204 and keeps the old + /// file — the case the read-back exists to catch. + #[cfg(target_os = "linux")] + struct FakeFirecracker { + sock: PathBuf, + requests: std::sync::Arc>>, + } + + #[cfg(target_os = "linux")] + impl FakeFirecracker { + fn start(name: &str, rootfs: &str, read_only: bool, honour_patch: bool) -> Self { + use std::io::BufRead; + use std::os::unix::net::UnixListener; + let sock = std::env::temp_dir().join(format!( + "forkd-fakefc-{name}-{}-{}.sock", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + )); + let listener = UnixListener::bind(&sock).unwrap(); + let requests = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let log = requests.clone(); + let mut path_on_host = rootfs.to_string(); + thread::spawn(move || { + for stream in listener.incoming() { + let Ok(stream) = stream else { return }; + let mut reader = std::io::BufReader::new(stream); + let mut line = String::new(); + if reader.read_line(&mut line).is_err() { + continue; + } + let request_line = line.trim().to_string(); + let mut len = 0usize; + loop { + let mut h = String::new(); + reader.read_line(&mut h).unwrap(); + if h == "\r\n" || h.is_empty() { + break; + } + if let Some(v) = h.to_ascii_lowercase().strip_prefix("content-length:") { + len = v.trim().parse().unwrap(); + } + } + let mut body = vec![0u8; len]; + reader.read_exact(&mut body).unwrap(); + let body: serde_json::Value = + serde_json::from_slice(&body).unwrap_or(serde_json::Value::Null); + let (method, path) = request_line + .split_once(' ') + .map(|(m, rest)| (m, rest.split(' ').next().unwrap_or(""))) + .unwrap(); + log.lock().unwrap().push(format!("{method} {path}")); + let reply = match (method, path) { + ("GET", "/vm/config") => serde_json::json!({"drives": [{ + "drive_id": "rootfs", + "path_on_host": path_on_host, + "is_read_only": read_only, + }]}) + .to_string(), + ("PATCH", "/drives/rootfs") => { + if honour_patch { + path_on_host = body["path_on_host"].as_str().unwrap().to_string(); + } + String::new() + } + _ => String::new(), + }; + let status = if reply.is_empty() { + "204 No Content" + } else { + "200 OK" + }; + let mut stream = reader.into_inner(); + let _ = write!( + stream, + "HTTP/1.1 {status}\r\nContent-Length: {}\r\n\r\n{reply}", + reply.len() + ); + } + }); + Self { sock, requests } + } + + fn requests(&self) -> Vec { + self.requests.lock().unwrap().clone() + } + } + + #[cfg(target_os = "linux")] + impl Drop for FakeFirecracker { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.sock); + } + } + + #[test] + #[cfg(target_os = "linux")] + fn repoint_rootfs_accepts_a_drive_that_moved() { + let fc = FakeFirecracker::start("moved", "/snap/rootfs.ext4", false, true); + repoint_rootfs(&fc.sock, Path::new("/work/child-1.rootfs.ext4")).unwrap(); + assert_eq!( + rootfs_drive(&fc.sock).unwrap(), + (PathBuf::from("/work/child-1.rootfs.ext4"), false) + ); + } + + #[test] + #[cfg(target_os = "linux")] + fn repoint_rootfs_fails_closed_when_patch_is_not_taken() { + let fc = FakeFirecracker::start("ignored", "/snap/rootfs.ext4", false, false); + let err = repoint_rootfs(&fc.sock, Path::new("/work/child-1.rootfs.ext4")).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("still reports /snap/rootfs.ext4"), + "error should name the path Firecracker kept: {msg}" + ); + } + + #[test] + #[cfg(target_os = "linux")] + fn restore_never_resumes_a_child_whose_rootfs_did_not_move() { + let dir = + std::env::temp_dir().join(format!("forkd-rebind-noresume-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let base = dir.join("rootfs.ext4"); + std::fs::write(&base, b"base").unwrap(); + let backing = dir.join("child-1.rootfs.ext4"); + let fc = FakeFirecracker::start("noresume", base.to_str().unwrap(), false, false); + let r = load_paused_rebind_rootfs_and_resume(&fc.sock, "{}", &base, &backing); + assert!(r.is_err(), "a child left on the shared base must fail"); + let requests = fc.requests(); + assert!( + !requests.iter().any(|r| r == "PATCH /vm"), + "must not resume a child still pointed at the shared base: {requests:?}" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[cfg(target_os = "linux")] + fn restore_gives_a_writable_rootfs_its_own_backing() { + let dir = + std::env::temp_dir().join(format!("forkd-rebind-writable-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let base = dir.join("rootfs.ext4"); + std::fs::write(&base, b"base").unwrap(); + let backing = dir.join("child-1.rootfs.ext4"); + let fc = FakeFirecracker::start("writable", base.to_str().unwrap(), false, true); + let used = load_paused_rebind_rootfs_and_resume(&fc.sock, "{}", &base, &backing).unwrap(); + assert!(used); + assert_eq!(std::fs::read(&backing).unwrap(), b"base"); + assert_eq!( + fc.requests(), + [ + "PUT /snapshot/load", + "GET /vm/config", + "PATCH /drives/rootfs", + "GET /vm/config", + "PATCH /vm" + ] + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[cfg(target_os = "linux")] + fn restore_shares_a_read_only_rootfs_without_copying() { + let dir = + std::env::temp_dir().join(format!("forkd-rebind-readonly-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let base = dir.join("rootfs.squashfs"); + std::fs::write(&base, b"base").unwrap(); + let backing = dir.join("child-1.rootfs.squashfs"); + let fc = FakeFirecracker::start("readonly", base.to_str().unwrap(), true, true); + let used = load_paused_rebind_rootfs_and_resume(&fc.sock, "{}", &base, &backing).unwrap(); + assert!(!used); + assert!(!backing.exists(), "a read-only rootfs must not be copied"); + assert_eq!( + fc.requests(), + ["PUT /snapshot/load", "GET /vm/config", "PATCH /vm"] + ); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn wait_for_sock_returns_immediately_when_path_exists() { let sock = std::env::temp_dir().join(format!( From 3f98a20eba205c669b1b63aa8ce68bae6c0b0c19 Mon Sep 17 00:00:00 2001 From: Jason Rimmer Date: Mon, 28 Sep 2026 14:18:37 -0700 Subject: [PATCH 10/11] docs: drop the Firecracker 1.15 floor; re-pointing works on every release measured The floor rested on a claim that PATCH /drives on a restored VM is accepted but does not move storage before v1.15. Measured on stock v1.5.1, v1.7.0, v1.10.1, v1.12.1, v1.14.1 and v1.17.0: every one moves the guest's writes to the new file and reports the new path in /vm/config. So no upgrade, and no re-bake, is needed for per-child backings, and the upgrade note goes with it. --- CHANGELOG.md | 69 +++++++++++++++++++++--------------- docs/VENDORED-FIRECRACKER.md | 4 +-- 2 files changed, 42 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 579fdb7..e39bce2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,24 +14,6 @@ Versioning](https://semver.org/spec/v2.0.0.html) once it reaches chain-assembly base copy and the bake's rootfs baseline clone streamed a full copy even on btrfs, XFS, and ZFS 2.2+. Found by @jrimmer in #321. -### 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 @@ -42,20 +24,49 @@ 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 +at it with `PATCH /drives/rootfs` 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. + +**Fail closed.** After the PATCH, forkd reads `GET /vm/config` back and resumes +the child only if Firecracker reports the child's own backing as its rootfs. A +child whose rootfs cannot be re-pointed and verified 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. +**No Firecracker upgrade is required.** Re-pointing a drive on a restored VM +works on every stock release measured — v1.5.1, v1.7.0, v1.10.1, v1.12.1, +v1.14.1 and v1.17.0 (aarch64): the guest's writes move to the new file and +`/vm/config` reports it. So existing snapshots keep loading and nothing needs +re-baking. + +**Read-only rootfs is shared, not copied.** Whether the drive is writable is +read from the restored vmstate itself (`/vm/config`), so a read-only (squashfs) +tag — including one baked before this change — costs no copy per child. + +**Branches and workspace suspends own their rootfs.** A snapshot of a restored +child used to record the child's backing, a file in a work dir that dies with +the child: the new tag failed to load (`Failed to restore from snapshot … No +such file`) once its source was gone, and until then every child restored from +it wrote the source's live disk. The paused source's drive is now pointed at a +clone the new tag owns for the length of the snapshot, recorded as the tag's +`rootfs`, and pointed home before the source resumes; a source whose drive +cannot be pointed home is killed rather than resumed onto the tag's disk. This +covers full, diff and live BRANCH and `POST /v1/workspaces/:name/suspend`. + 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. +supports it (XFS/btrfs, or ZFS 2.2+ block cloning); elsewhere it is a full copy +per child, made in parallel in the children's restore threads. A live BRANCH +makes its clone inside its pause window, so on a filesystem without reflink the +pause includes a full rootfs copy. Snapshots with no recorded rootfs keep the +previous single-call behaviour. + +Restore work dirs, and so the backings in them, live under the controller's +temp dir. On a host where `/tmp` is tmpfs, set `TMPDIR` to a directory on the +snapshot root's filesystem, so backings are not charged to RAM and the clone +has both ends on one filesystem. ### Upgrade note: legacy sandbox rows block startup diff --git a/docs/VENDORED-FIRECRACKER.md b/docs/VENDORED-FIRECRACKER.md index e49b97e..db77a79 100644 --- a/docs/VENDORED-FIRECRACKER.md +++ b/docs/VENDORED-FIRECRACKER.md @@ -12,9 +12,9 @@ The fix is a ~44-line patch adding an opt-in `shared: bool` field to `MemBackend [**`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. +**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**; per-child rootfs backings rely on `PATCH /drives` on a restored VM, which moves the device's storage on every stock release measured back to v1.5.1. 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. +One thing to know before changing the version: a vmstate is version-pinned, so a new build may refuse to load snapshots written by an older one — plan to re-bake tags when you upgrade. - Forked from upstream `v1.12.0` tag. - Five commits on the branch (combined ~140 lines, all forkd-specific feature work): From bd0191949fb004592d7a589497671ab3a6e688d4 Mon Sep 17 00:00:00 2001 From: Jason Rimmer Date: Mon, 28 Sep 2026 14:22:42 -0700 Subject: [PATCH 11/11] test(cli): park sweep-test backups under pids that can never be live The .prev rootfs sweep leaves a backup alone while the pid in its name is a running process, since it belongs to a concurrent bake. The tests named their "dead" backups prev-1000 and prev-2000, which are often live pids on a CI runner, so the sweep skipped them and four tests failed depending on what the runner happened to be running. Pointing the constant at pid 1 reproduces the exact failures. Use pids above PID_MAX_LIMIT (2^22), which the kernel never assigns. --- crates/forkd-cli/src/main.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index a3434a5..cc9aab6 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -4830,6 +4830,14 @@ mod tests { .unwrap(); } + /// Pids the sweep tests park backups under, standing for bakes that have + /// exited. They must never name a live process, or the sweep leaves the + /// backup alone as a concurrent bake's: small made-up pids like 1000 are + /// often live on a CI runner. The kernel never hands out a pid above + /// `PID_MAX_LIMIT` (2^22 on 64-bit), so these cannot be. + const DEAD_PID_A: u32 = 4_194_305; + const DEAD_PID_B: u32 = 4_194_306; + /// Review #295 approval follow-up (2026-09-02): a `kill -9` between /// the preserve-rename and the publish strands the backup under the /// dead run's pid, leaving the tag with no `rootfs.ext4`. The sweep @@ -4839,8 +4847,12 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let snap_dir = tmp.path().join("tag"); std::fs::create_dir_all(&snap_dir).unwrap(); - let older = tmp.path().join("tag.rootfs.ext4.prev-1000"); - let newer = tmp.path().join("tag.rootfs.ext4.prev-2000"); + let older = tmp + .path() + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_A}")); + let newer = tmp + .path() + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_B}")); write_with_mtime(&older, b"OLD-ROOTFS", 1_700_000_000); // Ordering is by park time (ctime), so park the newer one later. std::thread::sleep(std::time::Duration::from_millis(20)); @@ -4871,8 +4883,12 @@ mod tests { std::fs::create_dir_all(&snap_dir).unwrap(); let live = snap_dir.join("rootfs.ext4"); std::fs::write(&live, b"CURRENT-GOOD").unwrap(); - let a = tmp.path().join("tag.rootfs.ext4.prev-1000"); - let b = tmp.path().join("tag.rootfs.ext4.prev-2000"); + let a = tmp + .path() + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_A}")); + let b = tmp + .path() + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_B}")); write_with_mtime(&a, b"STALE-A", 1_700_000_000); write_with_mtime(&b, b"STALE-B", 1_700_000_900); let now = std::time::SystemTime::now() @@ -4905,7 +4921,9 @@ mod tests { let snap_dir = tmp.path().join("tag"); std::fs::create_dir_all(&snap_dir).unwrap(); write_with_mtime(&snap_dir.join("snapshot.json"), b"{}", 1_700_000_000); - let backup = tmp.path().join("tag.rootfs.ext4.prev-1000"); + let backup = tmp + .path() + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_A}")); write_with_mtime(&backup, b"LAST-PUBLISHED", 1_700_000_000); std::fs::write(snap_dir.join("rootfs.ext4"), b"UNPUBLISHED-DIRTY-CLONE").unwrap(); @@ -4959,11 +4977,11 @@ mod tests { let held = tmp .path() .join("snapshots") - .join("tag.rootfs.ext4.prev-1000"); + .join(format!("tag.rootfs.ext4.prev-{DEAD_PID_A}")); let other = tmp .path() .join("snapshots") - .join("other.rootfs.ext4.prev-1000"); + .join(format!("other.rootfs.ext4.prev-{DEAD_PID_A}")); std::fs::write(&held, b"MINE").unwrap(); std::fs::write(&other, b"OTHER-TAG").unwrap();