fix(snapshot): repair ext4 rootfs before boot to prevent re-snapshot corruption

[jrimmer]

Summary

Closes #296. Eliminates ext4 rootfs corruption from unclean VM shutdowns by treating the rootfs as an immutable baseline — never mounted read-write — and booting each VM from a per-snapshot reflink clone.

Problem

When forkd snapshot (or forkd from-image) boots a parent VM from an ext4 rootfs, the VM runs read-write and writes to the filesystem (journal entries, atime updates, apt installations). When the snapshot is complete, vm.kill() SIGKILLs the firecracker process without giving the guest a chance to unmount the ext4 filesystem cleanly.

The rootfs ext4 file is left on disk with uncommitted journal transactions and potentially corrupted metadata. On the next forkd snapshot --rootfs <same file>, the guest kernel's ext4 driver replays the dirty journal, which can produce severe corruption: EBADMSG errors, missing directories, binaries showing as "data" file type.

The previous approach (PR #295 v1) ran e2fsck -fy on the rootfs before each boot to repair the dirty journal. The reviewer flagged a TOCTOU race: the /proc/*/fd scan that guarded the e2fsck call is point-in-time, so another VM could open the rootfs during repair — potentially worse corruption than the original bug.

Solution: Immutable Baseline + Reflink Clone

The rootfs file provided via --rootfs is the immutable baseline — it is NEVER mounted read-write. Before each boot:

1. forkd creates a reflink copy of the baseline rootfs (via ioctl(FICLONE), instant on btrfs/xfs/overlayfs; falls back to a full streamed copy on ext4/tmpfs) in the snapshot directory at <snapshot_dir>/rootfs.ext4.
2. The VM boots from the clone (read-write ext4). The guest writes to the clone, not to the baseline.
3. After vm.kill(), the clone is left dirty — but the baseline stays clean (it was never written to).
4. The clone persists as the snapshot's rootfs (needed for restores — Firecracker re-opens the rootfs from the path stored in the vmstate).
5. The next forkd snapshot --rootfs <baseline> boots from a fresh clone of the still-clean baseline — no e2fsck needed, no TOCTOU race.

Why this is better than e2fsck

	e2fsck-on-boot (v1)	Immutable baseline (v2)
TOCTOU race	Yes — /proc/*/fd scan is point-in-time	No — baseline is never written to
Corruption risk	e2fsck on online FS = catastrophic	None — baseline is always clean
Per-snapshot rootfs	No (shared, dirty after kill)	Yes (per-snapshot clone, isolated)
Performance	e2fsck scan on every boot	Reflink clone is instant (CoW, no data copied)
Non-reflink FS	Works (e2fsck is FS-independent)	Full copy fallback (slower but correct)

Implementation

• pub fn reflink_copy(src, dst) in forkd-vmm/src/chain.rs — exposes the existing FICLONE + stream-copy fallback (previously private as copy_base_memory, used for memory.bin chain copies).
• snapshot_cmd in forkd-cli/src/main.rs — when the rootfs is ext4 (read-write), creates a reflink clone at <snapshot_dir>/rootfs.ext4 before booting. Boots from the clone and records its path in snap.rootfs.
• Read-only rootfs (squashfs) is unchanged — it's already immutable.

Limitations

• On non-reflink filesystems (ext4 without the reflink feature flag, tmpfs), the clone falls back to a full copy. For large rootfs (e.g., 24 GiB), this is slow. The forkd doctor command already warns about non-reflink hosts. A future improvement could use fallocate + copy_file_range for partial copying, or recommend btrfs/xfs for production deployments.
• The clone consumes storage proportional to the VM's writes (CoW: only modified blocks are allocated). On btrfs/xfs, this is typically small (journal + atime + apt cache ≈ tens of MB).

[WaylandYang]

The e2fsck result handling currently continues into the VM with a filesystem that was not successfully verified or repaired.

Only exit statuses composed of bits 1/2 (corrected / reboot-needed after correction), plus 0, are successful here. Status 4 means errors remain uncorrected, and 8/16/32/128 mean operational error, usage error, cancellation, or shared-library error. The current code merely warns for bit 4 and misreports 8/16/32/128 as repaired, then boots the image anyway. A signal-terminated e2fsck (status.code() == None) is also converted to -1 and allowed through. That defeats the integrity guarantee this patch is intended to add.

Please return an error before Vm::boot for any status outside 0/1/2/3 and for signal termination, including useful stdout/stderr in the diagnostic. Given that a dirty image can produce EBADMSG and broken binaries, a missing e2fsck should also fail closed for this path (or require an explicit opt-out rather than silently continuing). Extracting the status classification into a helper would make the full bitmask easy to unit-test.

[WaylandYang]

There is a deeper correctness problem here than the e2fsck status handling: the snapshot flow does not give restored VMs independent writable disks.

Firecracker's persisted block-device state records the backing-file path and read-only flag, and restore reopens a writable device at that saved path. Snapshot::restore_many_with(n) sends the same vmstate to every child without cloning or overlaying the rootfs, so multiple children can concurrently mount the same ext4 image as an exclusive RW block device. Older snapshots also continue to reference that mutable file, allowing disk state to diverge from their saved memory/device state.

Running e2fsck before boot cannot make this safe—and without proving exclusive ownership, it may repair a filesystem while another live VM is using it. Making /tmp tmpfs only removes one source of writes; the rest of the root filesystem remains writable. Also, this patch changes the CLI snapshot path, while the daemon snapshot endpoint still boots RW and kills the VM without this preparation.

Please establish an immutable baseline plus an independent per-VM writable layer (reflink/copy/overlay or equivalent), and only run fsck while exclusive access is guaranteed. Add coverage for two simultaneous children, restoring an older snapshot after disk mutation, and the daemon snapshot path.

[jrimmer]

Update: e2fsck exit-code handling corrected, fail-closed semantics, daemon path covered

The e2fsck repair has been extracted into a shared forkd_vmm::fsck module with proper exit-code classification and fail-closed semantics.

Fixes applied

Exit-code classification rewritten:

• Exit codes 8/16/32/128 (operational/usage/cancellation/shared-lib errors) were misreported as "repaired" and the VM booted anyway. Now classified correctly via a classify_e2fsck_exit(Option<i32>) -> E2fsckStatus enum and abort the snapshot.
• Exit code 4 (uncorrectable errors) only warned then booted a known-corrupt filesystem. Now aborts with an error.
• Signal-terminated e2fsck (status.code() == None) was mapped to -1 via unwrap_or(-1) and coincidentally hit the bit-4 branch. Now explicitly detected as Signaled and fails closed.
• Only exit codes 0/1/2/3 are treated as boot-safe; all others abort.

Fail-closed on missing e2fsck binary:

• Previously warned and continued (fail-open), booting an unchecked rootfs. Now returns an error requiring e2fsprogs to be installed.

120-second timeout added:

• Command::output() was replaced with spawn() + try_wait() polling loop. A hung e2fsck on a large/corrupt rootfs no longer blocks the snapshot indefinitely.

Both stdout and stderr surfaced on failure:

• Previously only stderr was shown on the uncorrectable path. Now both streams are included in the error message for operator diagnostics.

18 table-driven unit tests:

• The classification logic is now a pure function (classify_e2fsck_exit) with unit tests covering all documented exit codes (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 16, 32, 64, 128, None, -1) and boot-safety assertions. Cross-platform — no firecracker or e2fsck binary needed.

Daemon POST /v1/snapshots endpoint now covered:

• The daemon's create_snapshot had the same boot-RW + SIGKILL pattern but no e2fsck. Now calls the same shared forkd_vmm::fsck::repair_ext4_rootfs helper inside the spawn_blocking closure before Vm::boot.

ext4 detection consistency:

• CLI was case-sensitive (== "ext4"), daemon was case-insensitive (eq_ignore_ascii_case). The daemon path now uses case-insensitive detection for the e2fsck gate, matching build_snapshot_boot_config's existing behavior.

Exit-code comment completed:

• The inline comment omitted 32 (cancellation) and 128 (shared-lib error). Now documented in the E2fsckStatus enum.

Architectural note

The deeper concurrent-RW-mount concern (multiple restore_many_with children mounting the same ext4 file as RW block device) is not addressed by this PR — e2fsck before parent boot cannot make concurrent RW mounts safe. That requires an immutable baseline + per-VM writable layer (reflink/copy/overlay) and is tracked separately.

[WaylandYang]

The exit-code classification and daemon call site fix the earlier narrow findings, but two correctness blockers remain:

1. repair_ext4_rootfs pipes stdout and stderr, polls try_wait until the child exits, and only then drains the pipes. If e2fsck writes enough output to fill either pipe, the child blocks waiting for the parent to read while the parent waits for the child to exit; the 120-second timer then misreports a hung check. Drain both streams concurrently (or use a timeout implementation around wait_with_output) and include the captured output on timeout/failure.
2. There is still no exclusive-ownership proof before running e2fsck. A snapshot restore can keep the same RW ext4 backing file mounted by one or more live VMs, while an older snapshot and a new snapshot operation reference that same mutable path. Running e2fsck -fy against an online filesystem can itself corrupt it, and multiple restored children still share one writable block image.

At minimum, fsck must acquire lifecycle/exclusive ownership that prevents any live VM from using the rootfs. The complete #296 fix needs an immutable baseline plus per-VM writable clone/overlay (and coverage for simultaneous children and restoring an older snapshot after mutation). If this PR is intentionally reduced to a safe fsck helper, please narrow its claims and keep #296 open; it cannot currently close the root cause.

[jrimmer]

Fix: concurrent pipe draining + narrowed scope

Finding 1 (pipe deadlock): repair_ext4_rootfs piped stdout+stderr but polled try_wait() until the child exited, then drained the pipes. If e2fsck wrote enough output to fill a pipe buffer (64 KiB on Linux), the child blocked waiting for the parent to read while the parent waited for the child to exit — a deadlock. The 120-second timeout then misreported a hung check.

Fix: stdout and stderr are now drained concurrently in background threads using mpsc::channel. The main thread polls try_wait() with the timeout while the drain threads keep the pipes flowing. The drain threads' output is collected after the child exits (with a 2-second grace period). This eliminates the deadlock — e2fsck can produce arbitrary output without blocking.

Finding 2 (concurrent-RW-mount / exclusive ownership): This PR does NOT provide exclusive-ownership proof before running e2fsck. A snapshot restore can keep the same RW ext4 backing file mounted by one or more live VMs. Running e2fsck -fy against an online filesystem can itself corrupt it.

Scope narrowing: This PR is intentionally reduced to a safe fsck helper for the snapshot-creation path. The complete #296 fix requires an immutable baseline rootfs with per-VM writable layers (reflink/copy/overlay) and exclusive-ownership proof before running e2fsck — that is a separate architectural initiative. The PR body and commit message now state this explicitly. #296 remains open.

All 18 existing fsck unit tests pass on Linux, and the controller crate compiles clean.

[jrimmer]

Update: exclusive-ownership guard before e2fsck

repair_ext4_rootfs_with_timeout now checks whether any live process holds the rootfs file open before running e2fsck. Running e2fsck -fy against an ext4 concurrently mounted RW by a live VM can cause silent catastrophic corruption — worse than the dirty-journal issue being repaired.

The guard scans /proc/*/fd/* symlinks and compares them to the canonicalized rootfs path. If any PIDs are found holding the file, the function bails with an error listing the PIDs and instructing the operator to stop all VMs using the rootfs.

This is a point-in-time check: a VM could open the file after the scan passes. The complete fix for concurrent RW mounts (immutable baseline + per-VM writable layer) remains tracked separately in issue #296.

Tests (all verified passing on Linux):

• rootfs_in_use_non_linux_returns_empty: non-Linux stub returns empty
• rootfs_in_use_detects_open_fd (Linux only): opens a temp file, verifies rootfs_in_use detects the current process's PID
• rootfs_in_use_empty_for_unheld_file (Linux only): verifies empty result for a file nobody holds
• repair_refuses_on_in_use_rootfs: verifies the in-use check handles missing files without crashing

[WaylandYang]

Thank you for fixing the pipe deadlock and e2fsck exit-code handling. Two correctness guarantees in the current PR are still not implemented strongly enough to run e2fsck safely.

First, scanning /proc/*/fd is only a point-in-time observation. After the scan reports no holders, another VM/process can open the rootfs before or during e2fsck; no lifetime lock or ownership lease prevents that TOCTOU race. Online e2fsck against a concurrently writable filesystem can corrupt it. Please tie exclusive rootfs ownership to the VM/image lifecycle (or perform repair only on an offline private copy) and hold that exclusion for the complete check/repair operation.

Second, the timeout path returns immediately without including the captured stdout/stderr promised by the API, which removes the diagnostics needed to distinguish timeout from repair failures.

Please add a race regression test demonstrating that a new holder cannot appear while repair owns the image, plus timeout-output coverage. This should not close #296 until that lifecycle-level exclusion exists. Thanks again for the contribution.

[WaylandYang]

Thank you for replacing online e2fsck with a clone-based design. I re-reviewed the current head and three correctness/portability blockers remain.

1. An existing cache rootfs produced or dirtied by an older forkd version is reused solely because the path exists, then cloned as the supposedly immutable baseline. Add a cache schema/version migration, offline validation, or rebuild path before trusting legacy entries.
2. snapshot_cmd removes snap_dir/rootfs.ext4 before clone, boot, and snapshot succeed. Re-running a tag can destroy the last usable snapshot, and when the input rootfs equals that path it unlinks its own source. Stage under a distinct temporary path, reject or safely handle src == dst, and atomically publish only after success.
3. The clone now lives inside the snapshot directory, but SNAPSHOT_FILES already puts rootfs.ext4 in the main archive while emit_rootfs_sidecar packages the same file again. The sidecar manifest also records the packing host snapshot absolute path as target_path. This duplicates a potentially huge image and makes pull placement host/path dependent. Use one rootfs transport and a portable content-addressed destination.

Please add upgrade/dirty-cache, same-tag failure, src == dst, and pack/unpack portability regression tests. Thanks for the contribution; the immutable-baseline direction is sound once these lifecycle edges are closed.

[WaylandYang]

Repository branch flow has moved to dev for daily integration and main for tested promotions. I retargeted this PR to dev; the diff is unchanged because dev was fast-forwarded to the same commit as main before the switch.

[jrimmer]

Thanks for the careful re-review. All three blockers from the 18:28 round are closed in 4862d26 (pushed just now), plus the four requested regression tests. Could you re-review the head?

Blocker 1 — cache versioning (legacy/dirty-cache trust).
Each built rootfs now gets a .cache-meta.json sidecar recording schema_version + sha256 + image + size + forkd version. validate_cached_rootfs trusts a cache entry only when (a) the meta exists, (b) schema_version == ROOTFS_CACHE_SCHEMA_VERSION (=1), and (c) the live sha256 still matches. Legacy entries with no meta (older forkd), schema mismatches (cache migration), or sha mismatches (truncation / partial write / mutation by a prior snapshot that mounted the baseline RW) force a rebuild — the baseline is never cloned from an untrusted cache. Wired into both from_image_cmd and run_cmd cache-hit paths; write_rootfs_cache_meta is called after every build.

Tests:

• validate_cached_rootfs_rejects_legacy_entry_without_meta (upgrade/dirty-cache)
• validate_cached_rootfs_rejects_wrong_schema_version (upgrade/dirty-cache)
• validate_cached_rootfs_rejects_sha_mismatch_after_mutation (dirty-cache)
• validate_cached_rootfs_accepts_fresh_valid_entry
• validate_cached_rootfs_misses_on_missing_file

Blocker 2 — atomic snapshot staging (same-tag failure + src==dst).
snapshot_cmd removed snap_dir/rootfs.ext4 before clone/boot/snapshot succeeded, so re-running a tag destroyed the last usable snapshot and (src==dst) could unlink its own source. The entire new snapshot (rootfs clone + vmstate + memory.bin + snapshot.json) is now built under a distinct staging dir (<snap_dir>.staging-<pid>) and only published via publish_snapshot() after boot + warmup + snapshot + metadata write all succeed. publish_snapshot does a safe two-step shuffle: move the old snap_dir aside, rename staging into place (the commit point), then drop the old; on commit-point failure the old snapshot is restored from the aside, so a crash at any point leaves either the new OR the old snapshot, never neither. A src==dst guard rejects cloning the baseline into the snapshot's own rootfs.ext4 path (canonical-path comparison). The recorded snap.rootfs is re-pointed at the final snap_dir/rootfs.ext4 after publish so pull placement doesn't depend on the transient staging path.

Tests:

• publish_snapshot_atomically_replaces_existing (same-tag failure)
• publish_snapshot_into_nonexistent_snap_dir
• publish_snapshot_preserves_existing_when_staging_missing (same-tag failure recovery — the old snapshot survives a failed re-run)

Blocker 3 — portable rootfs transport (dedup + content-addressed).
The rootfs was shipped twice — tarred into the pack via SNAPSHOT_FILES AND emitted as a .rootfs.zst sidecar — duplicating a potentially huge image. pack() now records rootfs.ext4 in the manifest files list (for integrity accounting + list_local) but skips appending it to the tar body when a portable sidecar is emitted, so there is ONE rootfs transport. RootfsRef.target_path is now a PORTABLE relative filename (e.g. "rootfs.ext4") instead of the packing host's absolute path; satisfy_rootfs resolves it against the destination snapshot dir (absolute paths from legacy packs still work). unpack_into now returns the dest snapshot dir (and unpack_chain_into the head link's dest) so the relative target_path can be resolved on pull. The content address is the sha256 (sidecar name + integrity check); target_path is only the in-snap-dir filename.

All commits are DCO-signed and the branch is on dev (== main). cargo clippy -p forkd-cli --target x86_64-unknown-linux-gnu -- -D warnings is clean (LSP-verified; the forkd-vmm Linux-only build errors on macOS are pre-existing cfg(target_os = "linux") gates, not from this PR — CI is the source of truth for the green build). The pack/unpack portability round-trip is covered by the existing pack_unpack_roundtrip / pack_v2_then_unpack_recreates_all_chain_links tests in hub.rs plus the new cache-validation tests above.

[WaylandYang]

Thanks for addressing the three lifecycle blockers in 4862d26. GitHub currently reports the branch as conflicting with dev, so it cannot construct the merge commit and no CI checks run for this head. Please rebase onto current dev, resolve the conflicts without dropping the cache validation, atomic staging, or portable single-rootfs transport changes, and rerun the full CI suite. The existing changes-requested review remains in place until the rebased head can be reviewed.

[jrimmer]

Rebased onto current dev (77d3f5d), resolving the conflicts without dropping any of the three lifecycle fixes. The rebased head is 79b280e.

Preserved from 4862d26:

• Cache versioning (blocker 1): .cache-meta.json sidecar with schema_version + sha256; validate_cached_rootfs trusts a cache entry only when the meta exists, the schema matches ROOTFS_CACHE_SCHEMA_VERSION (=1), and the live sha256 still matches. Legacy/dirty/mismatched entries force a rebuild.
• Atomic snapshot staging (blocker 2): snapshot_cmd builds the entire new snapshot under a distinct staging-<pid> dir and publishes via publish_snapshot (two-step shuffle, old snapshot restored on commit-point failure). src == dst guard rejects cloning the baseline into the snapshot's own rootfs.ext4 path.
• Portable single-rootfs transport (blocker 3): pack() records rootfs.ext4 in the manifest files list for integrity accounting but skips appending it to the tar body when a portable sidecar is emitted (one rootfs transport). RootfsRef.target_path is now a portable relative filename; satisfy_rootfs resolves it against the destination snapshot dir.

All commits DCO-signed. Could you re-review the rebased head?

[WaylandYang]

Thanks for the substantial rework, but the current head is not safe to merge yet. I found three blocking correctness issues in the new staging/transport flow:

1. The VM is booted with staging_dir/rootfs.ext4, so Firecracker serializes that host path into the binary vmstate. publish_snapshot then renames the staging directory to snap_dir, making the embedded path nonexistent. Updating only Snapshot.rootfs in snapshot.json does not rewrite the Firecracker vmstate; restore_many_with sends only vmstate + memory and cannot override the drive path. A newly created RW snapshot can therefore fail its first restore after successful publication. Please keep the FC-visible rootfs at a stable path or add an explicit restore-time relocation mechanism, and add a Linux/KVM regression that snapshots, confirms staging is gone, then restores from the published tag.

2. hub::pack still records rootfs.ext4 in manifest.files but skips appending it to the tar when a sidecar exists. hub::unpack verifies every manifest.files entry before satisfy_rootfs is called, so it hashes the missing extracted rootfs.ext4 and fails. The sidecar can never be placed. Remove the sidecar-owned rootfs from the tar-file verification set (or defer that entry's verification until after sidecar placement) and add a real pack→unpack→sidecar round-trip test proving the tar contains one transport and unpack succeeds.

3. The advertised src==dst guard compares the source against staging_dir/rootfs.ext4, not the final snap_dir/rootfs.ext4. Passing the existing tag's own snap_dir/rootfs.ext4 therefore passes the guard; publication then deletes the original baseline with the old directory and leaves the newly booted/dirtied clone in its place. Compare against the final published rootfs path and add the exact same-tag regression.

Also please validate RootfsRef.target_path before snap_dir.join(target): an untrusted manifest can currently supply ../../... (or an absolute legacy path), and sidecar placement will write outside the snapshot directory—especially dangerous because these commands are commonly run via sudo. New portable refs should be constrained to a safe relative filename; legacy absolute behavior needs an explicit safe migration policy rather than unconditional writes.

CI is green, but these paths are not exercised by the current unit tests. Please keep the cache-versioning and failure-rollback work; those portions look sound.

[jrimmer]

Rebased onto current dev (d2d238a, includes #312) and addressed all three blocking issues plus the path-traversal note. Cache-versioning and failure-rollback are preserved.

Blockers addressed

1. FC-visible rootfs path disappeared across publish (staging → snap_dir rename). Firecracker serializes the block-device path_on_host into the binary vmstate and reopens it on /snapshot/load (no PUT /drives override is accepted on restore). The old flow booted from staging_dir/rootfs.ext4, then renamed the whole dir, leaving the recorded path nonexistent — first restore after publish failed. Now snapshot_cmd clones/boots from the stable final path snap_dir/rootfs.ext4 from the start; only vmstate/memory.bin/snapshot.json are staged. publish_snapshot is replaced by publish_snapshot_metadata, which pre-verifies all three metadata files exist and renames them into snap_dir (snapshot.json last = commit marker), never moving the rootfs. A fresh clone over an existing tag's snap_dir/rootfs.ext4 removes the stale file first (after the src==dst guard) to avoid reflink_copy's create_new(true) EEXIST.

2. pack/unpack sidecar could never be placed. pack listed rootfs.ext4 in manifest.files but omitted it from the tar body; unpack verified every declared file before satisfy_rootfs, hashing the missing extracted rootfs and failing. Now when a portable sidecar is emitted, rootfs.ext4 is retained out of manifest.files (sidecar carries sha integrity). Regression: pack_unpack_roundtrip_with_sidecar_rootfs.

3. src==dst guard compared against the staging path, not the final published path. Extracted to rootfs_clone_into_self(src, snap_dir), comparing canonical against the FINAL snap_dir/rootfs.ext4; re-snapshotting a tag whose baseline is its own rootfs.ext4 is rejected. Regression: rootfs_clone_into_self_rejects_same_tag_rootfs.

4. Path traversal via RootfsRef.target_path. satisfy_rootfs now requires a safe single-component relative filename (no absolute / no separators / no ..); ../../... or absolute legacy paths are rejected fail-closed (would otherwise write outside the snapshot dir under sudo). Regression: satisfy_rootfs_rejects_unsafe_target_paths.

Tests also: publish_snapshot_metadata_replaces_metadata_keeps_rootfs, _into_nonexistent_snap_dir, _preserves_existing_when_metadata_missing (asserts no torn vmstate/memory on the missing-metadata path), plus an #[ignore] Linux+KVM regression snapshot_stable_rootfs_publishes_and_restores that boots from the stable rootfs, snapshots to staging, publishes metadata, confirms staging is cleared, and restores from the published tag.

All commits DCO-signed. Head is 6893488. Could you re-review?

[WaylandYang]

Blockers 2, 3 and 4 are properly fixed and I verified each in the code, not just the summary. pack now withholds rootfs.ext4 from manifest.files exactly when a sidecar is emitted, so unpack's verification loop no longer hashes a file that was deliberately left out of the tar. rootfs_clone_into_self canonicalizes against the final snap_dir/rootfs.ext4 (canonicalizing the parent so a not-yet-existing snap_dir still compares correctly). satisfy_rootfs validates target_path before any join, requiring a non-empty single-component relative name, which closes the ../../absolute write-outside-snap_dir hole. The cache-versioning and staged-metadata rollback work is sound, and publish_snapshot_metadata pre-verifying all three files before the first rename is the right shape.

Two blocking issues remain, both in the blocker-1 rework.

1. A failed re-bake now destroys the previously published tag.

main.rs:2787-2799: when snap_dir/rootfs.ext4 already exists, it is remove_filed and a fresh baseline is reflink-cloned into that exact published path — before the VM boots. Everything after that point can fail (boot timeout, guest-agent wait, the Firecracker snapshot call), and the only cleanup is remove_dir_all(&staging_dir). snap_dir is then left holding the old snapshot.json/vmstate/memory.bin next to a rootfs that has been replaced by a different baseline and dirtied by the failed boot. Restoring that tag afterwards gives the vmstate a filesystem it was never frozen against — the exact EBADMSG/broken-binary symptom #296 is about, now reachable without any crash, just a failed re-snapshot. There is no --force gate on overwriting an existing tag either.

This is the same "removes the previous rootfs.ext4 before the replacement is known-good" concern from the earlier round; the immutable-baseline rework relocated it rather than resolving it. The cheapest fix that keeps the stable FC-visible path: rename the existing rootfs aside (rootfs.ext4.prev-<pid>) instead of deleting it, restore it on every failure path, and delete it only after publish_snapshot_metadata succeeds. Requiring --force to re-bake an existing tag would be a reasonable additional guard. Please add a regression that fails the snapshot after the clone and asserts the tag still restores.

2. Making target_path snap_dir-relative breaks cross-host restore — a regression against dev.

The fact that motivated blocker 1 — Firecracker serializes the block-device host path into the binary vmstate, and nothing on the restore path can override it — applies to unpack/pull too, and nothing in this PR rewrites the vmstate (rewrite_snapshot_paths only touches the vmstate and memory string keys inside snapshot.json). On dev today, RootfsRef.target_path is the absolute path FC reopens and satisfy_rootfs reproduces it verbatim on the target host, which is precisely why it was absolute. This PR changes it to a bare filename resolved against the destination snap_dir — and snap_dir is data_dir()/snapshots/<tag>, which differs whenever:

• the pack is unpacked with --tag <other> (explicitly supported for single-link packs), or
• data_dir() differs between the packing and pulling host — root's /var/lib/forkd versus a user's ~/.local/share/forkd, or a different XDG_DATA_HOME.

In those cases the sidecar is placed at the new snap_dir, the vmstate still names the packing host's path, and the first fork fails with the cryptic block-device error satisfy_rootfs's own doc comment says it exists to prevent. satisfy_rootfs now also rejects legacy absolute target_paths outright, so already-published packs stop working rather than degrading.

The doc on RootfsRef.target_path in hub.rs:117-122 still reads "Absolute path Firecracker reopens at restore", which is now false — that contradiction is the tell that the two fixes are in tension. Either keep an FC-visible path that is reproducible across hosts and validate it (a constrained absolute path under a known root, rejecting traversal), or introduce an explicit restore-time relocation step and make the pack format declare it. Whichever you pick, please add a round-trip regression that unpacks into a different snap_dir than the one packed from and restores.

Evidence gap. snapshot_stable_rootfs_publishes_and_restores is #[ignore] and requires FORKD_TEST_ROOTFS; this repo has no other ignored tests and no KVM job, so blocker 1's fix currently has no automated proof at all. Please attach the output of running it on a KVM host. It also only covers the same-host, same-tag case, so it would not catch either issue above.

Nits: main.rs:2748-2749 has a duplicated/truncated pair of comment lines left over from an edit. unpack_chain_into ends with destinations.last().cloned().unwrap_or_else(PathBuf::new); the empty case is unreachable today, but if it ever fired, satisfy_rootfs would resolve rootfs.ext4 against the process CWD — under sudo. bail! is the safer tail.

Note that #302 merged as e2fd1a6e, so this needs a rebase onto current dev regardless.

[jrimmer]

Review r9 (2026-08-22T20:28) follow-ups — both blockers, nits, and the evidence gap

All addressed on head 29138ad (rebased onto current dev — includes #302 as e2fd1a6 — and rust CI is green).

Blocker 1: a failed re-bake no longer destroys the published tag

The previous rootfs is now renamed aside to <tag>.rootfs.ext4.prev-<pid> instead of being deleted before boot, and a RootfsRollback guard (Drop = rollback) restores it on every failure path: clone error, boot timeout, snapshot failure, publish failure, ?-propagation, and panic unwinding. The guard is disarmed only after publish_snapshot_metadata commits; the backup is then deleted (best-effort, never fatal). The tag now always holds a consistent (rootfs, vmstate, memory.bin, snapshot.json) tuple — the #296 EBADMSG pairing is unreachable through re-bake failure.

On the suggested --force gate: I've held off for now, since re-running snapshot on an existing tag is the documented recovery flow when a bake produced a broken snapshot, and this rollback removes the data-loss reason for gating it. Happy to add --force if you'd still prefer the explicit opt-in — say the word and it's a small change.

Blocker 2: target_path is absolute again — validated against forkd-managed roots

I took your option (A): keep the FC-visible absolute path and validate it. The r6 snap-dir-relative design was the actual regression — Firecracker reopens the vmstate-frozen path at /snapshot/load, so placement cannot depend on the destination snap_dir (different --tag or data dir mis-placed the sidecar), and it rejected already-published packs outright. Now:

• emit_rootfs_sidecar records the packing host's absolute path verbatim (legacy packs work again; no format break).
• satisfy_rootfs resolves the target through validate_rootfs_target_path: non-empty, absolute, no ../backslash components, and lexically inside a forkd-managed root — the forkd data dir, its snapshots/ tree, or the rootfs cache ($FORKD_RUN_CACHE, default /var/cache/forkd) — so sudo forkd unpack/pull can no longer be aimed at /etc/cron.d (your blocker-4 concern is preserved, generalized beyond snap_dir). When the containing root exists, the longest existing prefix must canonicalize inside the root's canonical form, closing symlink-escape writes.
• A missing or unfetchable sidecar is now a hard error instead of a warn-and-continue — the resulting snapshot could never restore, so failing at pull time with the sidecar name is the actionable moment.
• Placement is fully snap_dir-independent, so sha-based dedup works host-wide again (a chain of packs sharing a base fetches it once) — this also restores the behavior documented in docs/HUB.md, which I've updated (the doc had always specified recorded-absolute placement; the code was the outlier).

Nits

Duplicated src == dst comment line removed; unpack_chain_into tail now bail!s instead of returning an empty PathBuf (which would have resolved target_path against the CWD under sudo).

Tests (52 unit + the KVM regression)

• rootfs_rollback_guard_restores_previous_rootfs_on_failure: armed drop swaps the partial clone back to the previous rootfs; disarmed drop is a no-op.
• satisfy_rootfs_rejects_unsafe_target_paths: rejection matrix updated for the absolute contract (relative/r6-format rejected too), plus placement + sha-dedup assertions for an in-roots absolute path.
• rootfs_target_path_places_verbatim_regardless_of_snap_dir: the round-trip you asked for — placement lands at the recorded path even when the destination snap dir differs.
• validate_rootfs_target_path_allows_managed_roots_only: unit contract for the root allowlist.
• snapshot_stable_rootfs_publishes_and_restores (#[ignore], KVM) extended: after restore it parks the published rootfs aside, dirties a partial clone, drops an armed guard, asserts rollback restored the rootfs, and restores again; then places a sidecar from a different snap_dir and asserts the bytes landed at the recorded path.

Evidence gap — KVM run output

Attaching the output of snapshot_stable_rootfs_publishes_and_restores from a KVM host in a follow-up comment (needs FORKD_TEST_KERNEL/FORKD_TEST_ROOTFS + /dev/kvm; I don't want to gate this round's review on it landing first, but it's coming).

[jrimmer]

KVM evidence: snapshot_stable_rootfs_publishes_and_restores passing on real hardware

Run on a dedicated KVM host (Proxmox VM, Linux 6.17.13-2-pve x86_64, /dev/kvm present, firecracker on PATH, rustc 1.97.1), branch head 7a73fa3, release build:

FORKD_TEST_KERNEL=/var/lib/forkd/kernels/vmlinux \
FORKD_TEST_ROOTFS=/var/cache/forkd/dev-base-rootfs.ext4 \  # 4 GiB ext4
cargo test -p forkd-cli --bin forkd --release \
  snapshot_stable_rootfs_publishes_and_restores -- --ignored --nocapture

running 1 test
    re-bake failed — restored previous rootfs from /tmp/forkd-snapshot-stable-rootfs-1748489/snap.rootfs.ext4.prev-x
==> placing rootfs → /tmp/forkd-snapshot-stable-rootfs-1748489/snap/rootfs.ext4 (from /tmp/forkd-snapshot-stable-rootfs-1748489/216908786ceb14ee.rootfs.zst)
✓ rootfs ready at /tmp/forkd-snapshot-stable-rootfs-1748489/snap/rootfs.ext4
test tests::snapshot_stable_rootfs_publishes_and_restores ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 53 filtered out; finished in 16.08s

All three phases exercised on real Firecracker + KVM: publish→restore of the published tag, the failed-re-bake rollback (guard drop restored the previous rootfs; the post-rollback restore succeeded), and cross-snap_dir sidecar placement (--tag-override simulation placed bytes at the recorded absolute path, then a verified restore).

Running it on KVM caught two real bugs CI never could

You were right to insist on this — the #[ignore]d test had never actually been executed, and it failed immediately:

1. publish_snapshot_metadata left staging-dir paths inside the published snapshot.json. The staged metadata is serialized with <staging>/vmstate / <staging>/memory.bin paths, and publish renamed the files without re-pointing the keys — so every snapshot published since the atomic-staging change recorded a vmstate path in a directory deleted moments later. Restore failed with Load snapshot error: Failed to open snapshot file: No such file or directory. Fixed in cf6693d: publish rewrites the staged snapshot.json's vmstate/memory keys to the final snap_dir paths before installing it (still snapshot.json-last, so the commit marker is never a half-fixed file). This is safe precisely because vmstate/mem are /snapshot/load request arguments, unlike the drive path frozen into the binary vmstate (blocker 1's constraint) — and it's why the test now asserts snap.vmstate == snap_dir/vmstate after load_snapshot_meta.

2. The test itself was restoring from the in-memory Snapshot (stale staging paths) instead of reloading the published metadata — i.e. it wasn't exercising the production forkd fork --tag path (load_snapshot_meta) at all, which is why the first bug was invisible. The test now reloads from the published snapshot.json before restoring, and asserts per-child failures are empty so a dead child can't hide behind an Ok (restore is partial-success by contract — the first run returned "0 children" with the error silently tucked away).

Publish unit tests were updated to stage real JSON (as snapshot_cmd does) and assert the re-point; both fixes are in the CI-green head 7a73fa3.

[WaylandYang]

Both blockers are resolved, and I checked the implementation rather than the summary.

Rollback guard. The previous rootfs is renamed aside before the clone and the guard is armed at that moment, so every failure between there and publish — including ?-propagation and unwinding — restores it; disarm() happens only after publish_snapshot_metadata returns, and the backup delete is best-effort afterwards. Removing the partial clone before the restore-rename is the right ordering. The tag now always holds a consistent tuple, which is what #296 needed.

Absolute target_path with root validation. This is the right call: placement cannot depend on the destination snap_dir when Firecracker reopens the vmstate-frozen path verbatim, and the relative format also rejected already-published packs. validate_rootfs_target_path covers the sudo concern better than my original scoping did — lexical containment in a managed root, then canonicalizing the longest existing prefix to catch a symlink planted inside one. Worth noting for the record that Path::starts_with compares componentwise, so /var/cache/forkd-evil does not slip past the /var/cache/forkd root; that is the failure mode this kind of check usually has, and it is not present here.

On the KVM run. This is exactly why I asked for it, and the outcome makes the point better than I did: an #[ignore]d test that has never been executed is not evidence. It caught publish_snapshot_metadata leaving staging paths in the published snapshot.json — which means every snapshot published since the atomic-staging change could not restore, through a code path CI cannot reach — and it caught the test restoring from the in-memory Snapshot rather than through load_snapshot_meta, which is why the first bug was invisible. Re-pointing vmstate/memory before the renames is correct and safe for the reason you give: those are /snapshot/load arguments, unlike the drive path frozen into the binary vmstate.

On --force: agreed, don't add it. Re-running snapshot on an existing tag is the documented recovery flow, and the rollback removes the data-loss reason for gating it. A prompt there would sit in front of the exact operation someone reaches for when a bake went wrong.

Approving. Two things to pick up separately, neither worth holding this:

1. satisfy_rootfs's doc comment still says it "warns (does not fail) when the sidecar can't be found" — that is now a bail!. One line, and the new behavior is the right one.
2. The stale-prev cleanup only removes the backup matching the current pid, so a kill -9 between the preserve-rename and publish leaves <tag>.rootfs.ext4.prev-<other-pid> — potentially several GB — that nothing ever collects, while the tag itself has no rootfs.ext4 until someone renames it back by hand. The doc acknowledges the window; the cleanup just cannot act on it. Globbing <tag>.rootfs.ext4.prev-* on entry and, when rootfs.ext4 is missing, restoring the newest backup instead of only deleting a same-pid one would close it.

One mechanical thing before this can land: #299 merged as 041c904e and both PRs added a section under CHANGELOG "Unreleased", so this now conflicts there. It is the only conflict — everything else merges clean — and both sections should survive. Rebase and I will merge, or say the word and I will resolve that one file on the branch myself.