Repository navigation
Conversation
Defect found while checking what a consumer needs: branching a restored child is no longer durable
Measured on a v1.17 host with this branch deployed: Two things make it worse than a stale path. The clone's This matters beyond a corner case: The fix belongs here: on branch, materialise the child's rootfs into the new tag — a reflink clone of the backing, recorded as the new tag's I would not merge this PR without it. I have not written it yet, and I am marking it here rather than leaving it for whoever reads the diff. |
The clone path was dead: wrong FICLONE ioctl numberWhile checking what a consumer needed I measured the per-child cost on a ZFS pool and got a full copy — 12291 MiB of pool growth for one 12 GiB child, when both the source and the destination were on the same pool and that pool demonstrably clones (
ENOTTY is in the list this function treats as "this filesystem does not support reflink", so it falls through to a full streamed copy — on every filesystem, silently, including the ones where cloning works. That is why the number being wrong was invisible: the failure path is the same as the legitimate one. Two consequences beyond this PR: a restored child cost a full copy of the rootfs (measured above), and the bake path's "reflink preferred" baseline clone was a full copy too. Corrected in this PR; |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks. Pausing on a baked-in drive and re-pointing it with PATCH /drives before resume is the right mechanism, and the evidence table is exactly what this needed. Not merging yet. Beyond the branch-durability blocker you already flagged, here is what I found.
Already landed separately: the FICLONE fix you described (0x40209409 → 0x40049409) was never pushed to this branch; the diff doesn't touch chain.rs. It is in dev now as #322, credited to you and verified on an XFS reflink=1 loopback (old number → ENOTTY, new → shared extents). Please rebase onto dev.
Blocking
- Branch durability (your comment): a branch taken from a restored child has to materialise the child's backing into the new tag and record it as that tag's
rootfs. - No runtime Firecracker version gate. The CHANGELOG says
PATCH /drivesis accepted but doesn't move storage on FC < 1.15, so on an older binary children silently share the base again. That is the exact corruption this PR fixes, now with no signal. A documented minimum isn't enough; this needs to fail closed. Options: checkfirecracker --versionwhen the controller/CLI starts, orGET /vm/configafter the PATCH and refuse to resume unless the drive'spath_on_hostis the backing. - Read-only rootfs snapshots pay a full copy per child for nothing. The CLI read-only (squashfs) bake records
snap.rootfs = cfg.rootfs, and restore clones wheneverself.rootfs.is_some(). Every child of a read-only tag now copies the squashfs and PATCHes a drive that can never be written.Snapshotdoesn't persist the drive's read-only flag (your ask #2 on #317), so the fix probably starts there: record it at bake and skip backings for read-only drives. - Cost on filesystems without reflink. On ext4 (the default on most hosts, including our test box) each child is a full streamed copy of the rootfs. The copies run sequentially in the loop before any load, so
fork -n Ncosts N × rootfs in both disk and spawn latency, with no warning. At minimum: say once per restore that the fallback happened, gate on free space (theavailable_byteshelper from #316), and run the copies in the per-child threads. Whether ext4 hosts should get an explicit opt-in/refusal instead of silent full copies is a product call worth making in this PR.
Non-blocking
sweep_stranded_backingsscansstd::env::temp_dir()forforkd-daemon-*. It matches where the controller creates work dirs only while both use the sameTMPDIR, so please derive both from one helper.- The upgrade note (re-bake every tag when moving to FC ≥ 1.15) is a release-level decision. We'll schedule it with the release rather than let it land silently in
dev.
I can't exercise the KVM path on my side (the test host has no KVM), so your measured evidence plus a rerun after these changes will be what we rely on.
0x40209409 encodes a 32-byte payload; FICLONE is _IOW(0x94, 9, int) = 0x40049409. The kernel answered ENOTTY, which the fallback reads as no reflink, so every reflink_copy streamed a full copy on every filesystem. Reported by @jrimmer in deeplethe#321. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the thorough review, and for landing the FICLONE fix as #322. A correction first: my earlier comment said the FICLONE constant was "corrected in this PR". It wasn't — that commit never reached this branch. #322 is the fix; I'll rebase onto Plan for this PR:
I'd like to split 4 (cost without reflink) into a follow-up PR, since it carries the product call you raised (silent full copy vs. opt-in vs. refusal on ext4). My proposal there: warn once per restore on fallback, gate on The non-blocking items (one helper for the work-dir root; the upgrade note belonging to the release) I'll take here. I'll rerun the evidence table on KVM after the changes and post the results. |
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 <jason@rimmer.net>
…uire 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 <jason@rimmer.net>
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 <jason@rimmer.net>
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 <jason@rimmer.net>
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 <jason@rimmer.net>
…shapes The predicate selects what a startup reap deletes, and the work dir holds `child-<n>.sock` and `child-<n>.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 <jason@rimmer.net>
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 <jason@rimmer.net>
… 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 <jason@rimmer.net>
…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.
…ease 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.
60d0b8c to
3f98a20
Compare
|
Rebased onto A correction that removes blocker 2's premiseThe "Firecracker ≥ 1.15, else
So there is no version floor and no re-bake: the upgrade note is gone from CHANGELOG and What changed
Evidence:
|
| check | result |
|---|---|
| two concurrent children of one writable tag each write their own backing | TICK…24 → …34 in each backing |
| the tag's rootfs through that | byte-identical |
| branch a child with the detour; source after resume | back on its own backing (…36 → …46) |
| the branch's rootfs after the source resumes | unchanged, holds the snapshot instant (…35) |
| kill the source | its backing is reclaimed |
| restore the branch after its source is gone | loads; the child continues from …35 on its own backing; branch rootfs unwritten |
| control: same branch without the detour, kill source, restore | PUT /snapshot/load returned 400 … Failed to restore devices … No such file — the defect reproduced |
| read-only tag, two children | both restored, no backing made, nothing to detour |
Not covered yet
- This ran on aarch64 KVM through
forkd-vmmdirectly, without netns, so multi-child batches ran as two single-child batches of the same tag. The controller's BRANCH/suspend handlers are covered by build, clippy and unit tests, not yet by a KVM run through the daemon. I'll rerun the full table through the controller on our x86 host (v1.17.0) and post it here before this leaves draft. - The two
forkd-vmmtests that fail on this machine (apply_diff_copies_only_data_regions,assemble_chain_memory_produces_correct_bytes) fail identically ondev: they assume 4 KiB pages and this host has 16 KiB.
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.
|
CI note: bd01919 fixes a flaky test from #314, not part of this change. The |
…nk probe, REST smoke spawn (#3) * feat(cli): doctor readiness gate — --json, FC version pin, netns floor, reflink workdir probe, REST smoke spawn forkd doctor today is a human diagnostic (18 checks, glyph report). A serving pool needs a GATE: a cpu-sbx node is ready only when every blocking check is green AND the node can actually fork. - --json: machine-readable {ready, checks[]} output; exit code is the gate (0 = no FAIL). WARN never blocks, same as the human path. - --expected-fc-version: exact pin compare (snapshot-family rule: restore only on the exact FC version x CPU family x guest kernel; '>=1.10' is too loose for a homogeneous pool). - --min-netns N: fan-out >1 needs one forkd-child-* netns per concurrent child; fewer = FAIL. - reflink workdir check (new, always on): per-child rootfs backing (PR deeplethe#321) is FICLONE on XFS (~6 ms/fork) but a full stream copy on ext4 (~265 s/fork for a 24G base). Probes ioctl(FICLONE) in TMPDIR. - --smoke-tag TAG [--smoke-port P]: REST-spawn one sandbox through the live daemon (per_child_netns), optionally TCP-probe envd (:49983), always delete. Proves restore+fork e2e. tempfile moves dev->runtime deps (doctor uses it on the probe path). * fix(cli): doctor smoke spawn probes envd via the child netns The guest of a per-child-netns sandbox lives at 10.42.0.2 INSIDE its netns; from the root netns the probe got 'No route to host'. Probe now setns()es via 'ip netns exec' in a helper child (never in-process) and retries for 10s to cover envd/socat bind lag — same contract as lane-02's init-loop.sh. Non-Linux gets a plain-connect stub. Also derive Default for DoctorOpts (clippy derivable_impls). * feat(cli): doctor warns on ARP gc_thresh defaults that starve big netns pools Each per-child netns carries its own ARP neighbor table. The Linux defaults (gc_thresh1/2/3 = 128/512/1024) overflow once a pool does n~100 REST fan-out spawns: every subsequent spawn then stalls in guest-net bring-up with 'neighbour: arp_cache: neighbor table overflow!' in dmesg (observed live on the Phase 0 C3 node, forkd-p0-1: POST /v1/sandboxes >55s stall while GETs stayed instant). WARN, not FAIL: a fresh node passes until it fans out hard, and the fix is one sysctl — but a serving pool must raise it at bootstrap. * fix(cli): doctor tests — Check::warn/fail take a hint argument * fix(cli): doctor review fixes — serde_json spawn body, timeout on netns probe, assert serialized readiness CodeRabbit PR#3: - smoke spawn body built with serde_json::json! (a --smoke-tag with quotes/backslashes previously produced malformed JSON and could inject extra fields like n/memory_limit_mib). - the in-netns TCP probe is argv-wrapped in `timeout 3` (the kernel's SYN retry window is ~2 min; without the cap a dropped-SYN guest blocked the readiness gate for minutes past the 10 s deadline) and the address is passed as a positional parameter ($1), never interpolated into the shell text. - json gate test asserts the serialized {"ready":…} value (via the extracted json_output helper) instead of re-testing the predicate. * fix(cli): smoke spawn FAILS when cleanup DELETE fails (CodeRabbit PR#3) A leaked probe sandbox on a 'passing' node defeats the gate: it holds a netns and a firecracker slot. The cleanup closure now checks the DELETE result and downgrades the Check to fail on transport error or non-2xx. * fix(cli): netns probe used the colon /dev/tcp form — bash requires /dev/tcp/HOST/PORT 'exec 3<>/dev/tcp/10.42.0.2:49983' is not valid bash /dev/tcp syntax; it fails with 'No such file or directory' even when the guest answers. Live diagnosis on forkd-p0-1: colon form always fails, slash form (/dev/tcp/10.42.0.2/49983) connects in the same child. The address is a parse-validated SocketAddr, so building the probe text from it carries no injection surface.
…n on the Ubuntu 22.04 sandbox hosts (#6) * fix(vmm): best-effort guest clock sync after snapshot restore (Option B) After snapshot restore, the guest's KVM clock reflects the snapshot's saved time, not the host's current wall time. For snapshots created minutes or hours ago, the guest clock starts in the past, causing TLS certificate validation failures, incorrect timestamps, and token expiry issues inside the guest. This adds Phase 2.5 to restore_many_with: after the parallel restore PUT completes, poll each child's guest agent (forkd-agent.py on port 8888) until it responds, then exec `date -s @<host_unix_ts>` to set the guest clock to the host's current wall time. All children in a batch are synced in parallel. This is the cross-platform, cross-kernel-version alternative to the `clocksource=kvm-clock` boot arg (PR deeplethe#289), which is x86-only and forward-only (requires re-baking existing snapshots). This approach works on any architecture, any Linux kernel, and any existing snapshot without re-baking. Best-effort and non-fatal: if the guest agent doesn't come up within 10 seconds, or `date` is unavailable, the sandbox remains usable with a stale clock. Failures are logged as warnings. References deeplethe#288 (stale sandbox VM clock problem). Signed-off-by: jrimmer <jrimmer@users.noreply.github.com> Signed-off-by: jrimmer <jason@rimmer.net> * fix: clippy doc_lazy_continuation in sync_guest_clocks doc comment Signed-off-by: jrimmer <jason@rimmer.net> * test(vmm): add integration test for guest clock sync + fix exec args Fix a bug in sync_guest_clocks: the exec args were ["-c", cmd] which causes subprocess.run to try executing a program named "-c" (fails with FileNotFoundError). Changed to ["sh", "-c", cmd] so the date command runs through the shell as intended. The bug was silently masked because clock sync is best-effort (failures logged as warnings, not propagated) and the clocksource=kvm-clock boot arg (deeplethe#289) was correcting the clock on modern kernels. Add sync_guest_clocks_corrects_wrong_clock integration test: - Boots a fresh VM via Vm::boot - Deliberately sets the guest clock to 2001-09-09 (old_ts=1000000000) - Calls sync_guest_clocks on the single VM - Verifies the guest clock is corrected to host wall time (within 5s) - Verifies the corrected time is NOT the mock old time This isolates the scripting solution: even if KVM_CLOCK_REALTIME already corrected the clock, the test forces a wrong clock and verifies the sync function fixes it. The mock clock underneath is fine — we're testing the mechanism (date -s via guest agent exec), not the environment (kernel clock behavior). The test is #[ignore] (requires Linux + KVM + firecracker + rootfs) and #[cfg(target_os = "linux")]. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): per-child timestamp, opt-in clock sync, reject n>1 shared-IP Three findings addressed: 1. Host timestamp captured before polling agent — a child that became ready near the 10-second deadline was set ~10 seconds in the past. Fix: capture SystemTime::now() per child AFTER ping succeeds, immediately before the date -s exec. This minimizes the gap between timestamp capture and clock correction. 2. restore_many_with unconditionally performed the agent protocol — custom/no-agent snapshots added up to 10 seconds to every restore even though failure is only a warning. Fix: clock sync is now opt-in via ForkOpts::sync_guest_clocks (default false). The daemon's spawn paths (create_sandbox, spawn_one_for_workspace) set it to true because all forkd images contain forkd-agent. CLI paths keep it false — the CLI doesn't know whether the image has an agent. 3. For n>1 with per_child_netns=false, every worker contacted the same 10.42.0.2:8888 — the shared IP/MAC topology is fundamentally conflicting. Fix: restore_many_with now rejects n>1 with per_child_netns=false with a clear error message directing callers to use per_child_netns=true for multi-child spawns. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): expose per-child clock sync outcomes for fail-closed callers sync_guest_clocks now returns (u128, Vec<ClockSyncOutcome>) instead of just u128. ClockSyncOutcome is an enum with Synced{child_index} and Failed{child_index, error} variants, exposed via a new ForkResult::clock_sync_outcomes field. Callers (e.g. the controller's create_sandbox) can inspect the per-child outcomes to decide whether to fail-closed or degrade when some children have unsynchronized clocks. The controller now logs clock_sync_failures alongside the existing timing fields. Also fixes the unused child_index warning from the thread closure (the index is used in the collection loop, not inside the thread). Tests: - clock_sync_outcome_construction_and_matching: verifies enum discriminants, field access, and the filter-for-failures pattern callers use to decide fail-closed behavior. - sync_guest_clocks_corrects_wrong_clock (#[ignore] integration): now also asserts the outcome vec contains exactly one Synced entry for the single child. - fork_result_has_clock_sync_ms_field: updated to include the new clock_sync_outcomes field. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): clippy unused-enumerate-index + behavioral clock sync tests Two CI/coverage gaps from the review: 1. Clippy (rust CI was red): the sync_guest_clocks child loop used .enumerate() and discarded the index (clippy::unused_enumerate_index). Drop the .enumerate() and iterate children directly. 2. Behavioral coverage for the three review asks: - multi-child address/mode: restore_many_with_rejects_multi_child_shared_ip unit test proves n>1 with per_child_netns=false is rejected before any Firecracker spawn or filesystem side effect. - no-agent/disabled-sync latency: fork_opts_sync_guest_clocks_defaults_disabled unit test proves the opt-in default keeps sync off (no added latency for custom/no-agent images). - delayed agent readiness (timestamp freshness): sync_guest_clocks_delayed_agent_readiness (#[ignore] integration) boots a VM, calls sync_guest_clocks immediately (no pre-wait) so it must poll for the not-yet-ready agent, then asserts the guest clock matches host wall time within 3s — a stale pre-poll timestamp would drift by the poll duration. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): deterministic delayed-agent-readiness test for clock sync WaylandYang's final review round (round 5, 2026-08-14) flagged the delayed-agent-readiness integration test as non-deterministic: it called sync_guest_clocks immediately and only checked the final drift plus an upper elapsed bound, so it never guaranteed that agent readiness was actually delayed beyond the accepted 3-second drift. If the agent came up quickly, the old pre-poll-timestamp implementation still passed the test. Extract the timestamp-placement logic from sync_guest_clocks into sync_one_child_clock(ping, now, set_clock), which accepts injectable readiness-probe / timestamp-source / clock-setter closures. The production call site wires these to ping_at/ping_in_netns, SystemTime::now, and the date -s exec; the test wires them to a deterministic probe that fails for 4 seconds (> the 3s accepted drift) then succeeds. The new unit test sync_one_child_clock_timestamp_fresh_after_delayed_ping: 1. asserts the induced poll delay (4s) exceeds the accepted drift (3s), so the freshness assertion is meaningful, 2. asserts the timestamp passed to set_clock was captured AFTER ping succeeded (now_called_at >= probe_ready_at, freshness_gap < 3s) -- a pre-poll-timestamp implementation would fail by ~4s, 3. asserts set_clock was actually invoked with that fresh timestamp. No VM, guest agent, or KVM is required -- it runs in the unit-test suite on every platform. The end-to-end mechanism (real agent + date -s) is still covered by the #[ignore] integration test sync_guest_clocks_corrects_wrong_clock. Verified on Linux (rust 1.83, x86_64): cargo clippy clean for forkd-vmm/controller/cli/uffd; cargo test -p forkd-vmm --lib passes 48 passed, 0 failed, 3 ignored. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): address ce-code-review on clock sync refactor Apply the three cross-reviewer findings from the ce-code-review pass (5 reviewers: correctness, testing, maintainability, reliability, adversarial): 1. Timeout error dropped the agent address (P2, safe_auto) — flagged by correctness, testing, and adversarial. In multi-child restores the operator could not tell which child's agent failed to come up. Now wrapped at the call site: `.map_err(|e| e.context(format!( "guest agent at {addr} did not come up")))`. 2. No unit test for the timeout path (P2, manual) — flagged by testing, reliability, and adversarial. Only the #[ignore] integration tests exercised it, and the 10s production timeout made a dedicated unit test impractical. The timeout is now an injected parameter on sync_one_child_clock, and sync_one_child_clock_times_out_when_probe_ never_succeeds uses a 500ms injected timeout to verify the bail message and that now()/set_clock are never called on the timeout path. 3. No unit test for set_clock failure propagation (P2, manual) — flagged by testing, reliability, and adversarial. sync_one_child_clock_propagates_set_clock_failure verifies a set_clock error propagates verbatim and that now() was still called (timestamp captured before the failed set). Acknowledged (P3 advisory, no action): the 4s induced delay makes the freshness test the slowest non-ignored unit test; wall-clock second granularity in the freshness assertions is robust given the 4s>3s headroom and the compile-time invariant guard. Verified on Linux (rust 1.83, x86_64): cargo clippy clean for forkd-vmm/controller/cli/uffd; cargo test -p forkd-vmm --lib passes 50 passed, 0 failed, 3 ignored. Signed-off-by: jrimmer <jason@rimmer.net> * style: cargo fmt on clock sync refactor Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): run Registry::reconcile under a single lock reconcile() took K+4 separate registry-mutex acquisitions for a pass that prunes K stale sandboxes: one lock per removed id, plus one each for the stale scan, the live-id snapshot, and the workspace walk. Two handler calls interleaved between the scan and the per-id removal could make the reported prune count diverge from what was actually removed. Collect the stale ids, remove them, and walk the workspaces for the Running -> Stale transition all under one `self.inner.lock()` acquisition. `pid_alive` is a `/proc` stat and takes no lock we care about, so holding the registry mutex across it is fine. flush() behavior is unchanged: it still runs once, after the lock is dropped, only when something changed. Adds coverage for reconcile()'s prune-count and workspace-transition behavior (previously untested): a dead sandbox is pruned and its Running workspace flips to Stale with live_sandbox_id cleared; a sandbox with no recorded pid is left alone and its workspace stays Running; a Suspended workspace is never touched; and a no-op pass does not rewrite state.json. Fixes deeplethe#310 Signed-off-by: Amir Fathi <amirfathi.me@gmail.com> * fix(vmm): restore-path resilience for issue deeplethe#301 (squashed) Squash of 10 commits on fix/restore-path-resilience for rebasing onto current dev (which now contains deeplethe#281's shared-TAP lifecycle). Original commits: - 858f63b fix(vmm): restore-path resilience for issue deeplethe#301 - ce3d800 fix(vmm): apply review fixes for deeplethe#301 - 0d73362 fix(vmm): make DEFAULT_SOCKET_WAIT_SECS pub for cross-crate use - e820848 fix(controller): use Vm::pid() accessor, not private field - 1ddf40c fix(controller): drop needless borrow in workspace orphan scan call - ca52820 fix(vmm): partial-success contract + concurrent socket wait for restore_many_with - 9e863de fix(controller): expose orphan-firecracker detection as a metric - 2aca840 test(vmm): integration test for partial-success restore - 9342e7a test(vmm): accept Restore or SocketWait phase in partial-success test - f2751b9 fix(vmm): review fixes for deeplethe#301 — indexed success, valid test topology, docs Signed-off-by: jrimmer <jason@rimmer.net> * style: cargo fmt on rebase resolution Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): preserve original child slot in clock-sync outcomes Review deeplethe#302: restore_many_with flattening children via children::flatten() renumbered ClockSyncOutcome.child_index, so a survivor past a failed hole (spawn/socket-wait) was reported as 0..n instead of its original 1-based within-batch slot, breaking the resource correlation that ForkChild/RestoreFailure promise. - sync_guest_clocks now takes &[(usize, &Vm)] where the usize is the original 1-based within-batch slot; outcomes echo that slot in both ClockSyncOutcome::Synced/Failed and the tracing child spans. - Add live_children_with_indices(): the single index-preserving mapping (enumerate + filter_map, i+1) used by restore_many_with, extracted so the invariant is unit-testable without KVM. - Regression: live_children_with_indices_preserves_survivor_slot_ across_hole proves [None,Some,None,Some] -> slots [2,4], not [0,1]. - Update the single-child clock-sync integration test to pass slot 1 and assert child_index == 1. Signed-off-by: jrimmer <jason@rimmer.net> * style: fix clippy doc-lazy-continuation in sync docs Map a line directly following the #Cons list was parsed as an unindented list continuation (-D warnings / clippy::doc-lazy-continuation). Separate it with a blank doc line. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): kill orphaned Firecracker processes on startup Issue deeplethe#298: when the controller crashes or restarts while Firecracker processes are still alive, the restarted controller cannot reconstruct resource ownership for orphaned VMs. Both the NetnsAllocator (active set) and shared_tap_owner are initialized empty, so a new spawn can reserve an orphaned VM's netns index or claim the shared tap even though the orphan still owns it. Fix: add `Registry::kill_orphans()` called after `reconcile()` on startup. After reconcile() prunes dead-PID entries, any remaining sandbox entries have alive PIDs but no live_vms handle — they are unmanageable orphans. kill_orphans() kills each orphan (after verifying the PID belongs to a Firecracker process via /proc/<pid>/comm to guard against PID reuse), prunes the registry entry, and marks workspaces whose sandbox was killed as Stale. After this, the NetnsAllocator (empty active set) and shared_tap_owner (None) are safe: no orphaned VM holds a netns index or the shared tap. Closes deeplethe#298 Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): address ce review findings for orphan recovery Address all P0 and P1 findings from the 7-reviewer ce-* panel: P0: kill_pid failure no longer prunes the registry entry. ESRCH (process already dead from TOCTOU race) is distinguished from real errors (EPERM etc.). On ESRCH, the entry is safely pruned. On real errors, the entry is kept in the registry to prevent the exact resource collision issue P1 fixes: - Wait for process death after SIGKILL (5s bounded timeout polling /proc/<pid> disappearance). A D-state process stuck on I/O can hold netns/tap resources past the kill return; if it doesn't exit within the timeout, the registry entry is kept. - pid_is_firecracker uses exact match (== "firecracker") instead of substring match to avoid false-positive kills of unrelated processes. - Separate KillOrphansResult struct (killed, pruned_stale, kill_failed) replaces the misleading single usize counter. The startup log now reports all three counts. - Extract mark_stale_workspaces() helper to eliminate the ~22-line duplicated workspace stale-marking block between reconcile() and kill_orphans(). - kill_orphans is pub(crate) with a doc invariant: "MUST only be called before any Vm handle is held" — prevents future post-startup misuse. - Defensive pid <= 1 guard in pid_is_firecracker (defense-in-depth against corrupted state.json). - ESRCH downgraded from error! to debug! (benign TOCTOU race). - SAFETY comment added on unsafe libc::kill. New tests: pid:None entries skipped, multiple workspaces same sandbox, persistence to disk after kill_orphans, reconcile+kill_orphans integration. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): abort startup on kill_failed, document PID-reuse limit WaylandYang round 4 found that kill_orphans correctly keeps registry entries on EPERM/timeout (kill_failed > 0), but run_daemon only logs the count and continues. The NetnsAllocator active set and shared_tap_owner start empty, so the next spawn can reuse the failed orphan's netns index or tap lease — the exact collision deeplethe#298 prevents. Fix: run_daemon now aborts startup when kill_failed > 0, forcing operator intervention rather than silently admitting conflicting spawns. The error message names the count and tells the operator to kill the orphans manually and restart. Also updated the pid_is_firecracker doc comment to be honest about the PID-reuse limitation: the comm check is a defense-in-depth heuristic, not complete PID-reuse protection. A robust fix would use pidfd signaling plus durable identity validation (process start time recorded with the registry entry at spawn time, compared on startup). That is tracked as a follow-up. Added test: kill_orphans_result_kill_failed_contract verifies the KillOrphansResult contract (kill_failed == 0 on empty registry, so baseline startup does NOT abort). Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): extract check_orphan_kill_result for testable abort decision The startup abort logic was inline in run_daemon, making it impossible to unit-test the kill_failed > 0 → bail decision path. Extracted into pub(crate) check_orphan_kill_result(&KillOrphansResult) -> Result<()> so the abort contract is directly testable. Tests: - check_orphan_kill_result_aborts_on_kill_failure: verifies that kill_failed=1 returns Err with "aborting startup" message, and kill_failed=0 returns Ok even with killed/pruned entries. - check_orphan_kill_result_error_contains_count: verifies the error message contains the exact kill_failed count so operators know how many orphans need manual intervention. The existing kill_orphans_result_kill_failed_contract test (zero-baseline) is retained as documentation of the baseline contract. The new tests exercise the actual abort decision logic. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): remove orphaned last_err assignment after rebase The dead-last_err cleanup dropped the declaration and the unreachable expect, but the `last_err = Some(e)` assignment (guarded by the tap/cgroup-busy warn block on main) survived the rebase and referenced a now-missing binding. Remove it — the loop already returns on the final attempt, so the assignment is dead. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): durable process identity for orphan recovery Replace the comm=="firecracker" check with durable process identity verification and pidfd signaling, addressing review r6 (WaylandYang) and the ce-code-review security findings. The comm check was spoofable (prctl PR_SET_NAME) and could not distinguish two Firecracker processes sharing a PID over time — a same-name PID-reuse attack would kill a legitimate new Firecracker, and on a multi-tenant host a cross-tenant DoS was possible. The TOCTOU window between the check and kill(2) was also acknowledged. Changes: 1. SandboxInfo::proc_starttime (serde default) — persists the process start time (field 22 of /proc/<pid>/stat, clock ticks since boot) captured at VM registration. Backward-compatible: old state.json files without this field deserialize to None. 2. read_proc_starttime — robust parse of /proc/<pid>/stat that handles the parenthesized comm field (field 2, can contain spaces/parens) by splitting at the last ')' and indexing field 22 in the remainder. 3. process_identity_matches — compares the live start time against the recorded one. Returns Match / PidReuse / Dead / Unknown. PidReuse means the original process exited and the PID was recycled — prune the stale entry without killing. Unknown (no recorded start time, /proc unreadable, or off-Linux) fails closed. 4. pidfd_open + pidfd_send_kill — signal through a pidfd opened AFTER the identity check, closing the TOCTOU window: the fd is pinned to the verified process, so a PID reuse between check and signal cannot redirect the kill. Uses raw libc::syscall with SYS_pidfd_open / SYS_pidfd_send_signal (Linux 5.3+; libc 0.2.x exposes the syscall numbers but not named fns). 5. comm_is_firecracker — retained as a secondary confirmation after the primary start-time check passes, to catch corrupted state.json. Not a security boundary. 6. kill_orphans reworked: Match → pidfd kill + wait_for_death; PidReuse → prune stale (no kill); Dead → prune stale; Unknown → fail closed (keep entry, increment kill_failed so run_daemon aborts startup). Tests updated to reflect the new fail-closed semantics: - kill_orphans_prunes_alive_pid_entries now uses a mismatched start time (u64::MAX) to trigger PidReuse → prune, not the old None path. - kill_orphans_fails_closed_on_unknown_identity (new): alive PID with proc_starttime: None → fail closed, entry kept, kill_failed=1. - kill_orphans_prunes_same_name_pid_reuse_without_killing (new regression): the same-name Firecracker PID-reuse attack the reviewer flagged — a mismatched start time must prune without killing, even if comm were "firecracker". - All other kill_orphans tests updated to use mismatched start times where they previously relied on the comm-check prune path. Verified on Linux (rust 1.83, x86_64) via Docker: clippy clean for forkd-controller; cargo test --all-features passes 104 unit + 8 integration tests, 0 failed. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): close pidfd TOCTOU + ce-code-review r6 follow-ups Address ce-code-review findings (security P1, testing P1, adversarial P2/P3, plus the cross-reviewer coverage gaps) on the durable-identity rework from 2e275fe. 1. Residual TOCTOU window (security P1, adversarial ADV-1): pidfd closes only the open→signal window, NOT the check→open window. The original could exit and the PID could be reused by another Firecracker between process_identity_matches (which reads /proc/<pid>/stat) and pidfd_open. Fix: after pidfd_open succeeds, re-read the live start time and compare to the recorded one. A mismatch means the PID was reused between the check and the open → prune without killing (PidReuse). The pidfd pins the process at open time, so /proc/<pid> reflects the pinned process. This closes the check→open window; the comm check is now a pure sanity check, not a load-bearing defense. Design comment corrected to not overstate the guarantee. 2. Off-Linux comment/behavior mismatch (adversarial ADV-2): the module comment claimed off-Linux "prunes without killing" but Unknown actually fails closed (keeps entry, increments kill_failed, run_daemon aborts). Fixed the comment to state the real behavior. 3. Cross-platform test guards (adversarial ADV-3): the PidReuse- asserting tests (mismatched starttime → pruned_stale) now carry #[cfg(target_os = "linux")] because off-Linux returns Unknown (kill_failed), not PidReuse. The cross-platform fail-closed test (proc_starttime: None → Unknown) stays ungated. 4. Match-arm coverage (testing P1, +reliability/security/adversarial): new kill_orphans_match_arm_kills_verified_child_via_pidfd spawns a real child, records its true start time (Match), and asserts the secondary comm_is_firecracker check fails closed for a non-firecracker process — proving the Match arm is entered and the comm gate works as defense-in-depth. (Killing a real Firecracker would require a Firecracker binary; the comm gate is the testable boundary here.) 5. start-time parser unit test (testing P2): read_proc_starttime now has a direct test (parses our own /proc/<pid>/stat, rejects pid<=1, returns None for nonexistent PID). 6. Backward-compat serde test (testing safe_auto): new proc_starttime_defaults_to_none_when_absent_in_old_state_json deserializes a hand-written pre-r6 state.json (no proc_starttime key) and asserts #[serde(default)] yields None — the contract that lets existing deployments upgrade without wiping state. Verified on Linux (rust 1.83, x86_64) via Docker: clippy clean for forkd-controller; cargo test --all-features passes 107 unit + 8 integration tests, 0 failed (3 new tests pass). Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): add SIGKILL success-path regression + correct misleading comments Review r8 (2026-08-20) found that kill_orphans_match_arm_kills_verified_child_via_pidfd did not do what its name claimed: it spawned 'sleep' (comm='sleep'), so the secondary comm_is_firecracker check failed closed and the irreversible success path (pidfd_send_kill -> wait_for_death -> registry removal) was never executed. - Rename the existing test to kill_orphans_match_arm_comm_mismatch_fails_closed so its name and doc accurately describe the comm-gate fail-closed branch it covers. - Add kill_orphans_match_arm_kills_verified_firecracker_via_pidfd, which spawns a disposable process whose /proc/<pid>/comm is literally 'firecracker' (a copy of 'sleep' renamed to 'firecracker'), records its true start time, and asserts killed == 1, kill_failed == 0, pruned_stale == 0, and the registry entry is removed — exercising the full irreversible pidfd kill path. Also correct two misleading comments flagged in the same review: - SandboxInfo::proc_starttime claimed legacy None state may use comm-only verification; the implementation fails closed (keeps the entry, increments kill_failed) and never does comm-only kill. - IdentityCheck::Unknown claimed the caller chooses between a comm-only kill or prune; kill_orphans keeps the entry and aborts startup via kill_failed. Signed-off-by: jrimmer <jason@rimmer.net> * style: cargo fmt on new firecracker-named kill test Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): persist boot id + fail closed on pid-less entries Addresses review deeplethe#299 (both blockers) and rebases onto dev with deeplethe#312's single-lock reconcile preserved. Blocker 1 — cross-reboot false Match (PID+starttick collision): proc_starttime is ticks since boot, only unique within one host boot. The registry persists across reboots, so after a reboot an unrelated Firecracker can share both the numeric PID and the boot-relative start tick of a recorded entry, and a bare starttime check would SIGKILL the wrong process. Persist /proc/sys/kernel/random/boot_id at registration (SandboxInfo.boot_id) and gate the identity check on it: - different boot id -> PidReuse (prune WITHOUT signaling; the old process cannot still exist across a reboot) - missing recorded boot id (legacy state.json) -> Unknown (fail closed) - live boot id unreadable -> Unknown (fail closed) New read_boot_id() helper; recorded at all three production VM registration sites in http.rs. Blocker 2 — pid: None entries recreated the deeplethe#298 collision risk: Such rows were skipped by both reconcile and kill_orphans, so startup succeeded with an empty allocator/shared-tap ownership while a live VM may still hold those resources. kill_orphans now counts retained rows without a PID into KillOrphansResult.unresolved, and check_orphan_kill_result aborts startup on unresolved > 0 (fail closed). Rebase: preserved deeplethe#312's single-lock reconcile (whole in-memory pass under one registry lock, including inline workspace-stale marking); kept mark_stale_workspaces only for kill_orphans' use after pruning. Tests: - kill_orphans_does_not_kill_across_boot_id_mismatch: matching PID + real start time but a DIFFERENT recorded boot id -> pruned without signaling (killed==0, pruned_stale==1). - kill_orphans_counts_pid_none_entries_as_unresolved (replaces kill_orphans_skips_pid_none_entries): pid:None rows surface as unresolved and block startup via check_orphan_kill_result. - check_orphan_kill_result_aborts_on_unresolved: startup-decision regression. - Existing kill-path tests set boot_id to the current boot id where they exercise the Match/PidReuse path; the legacy fail-closed test keeps boot_id: None. Signed-off-by: jrimmer <jason@rimmer.net> * fix(controller): resolve use-after-move in busy-retry warn (E0382) The rebase onto post-deeplethe#302 dev merged both sides of the busy-retry block textually but not semantically: in the Err(e) arm, last_err = Some(e) (and the final_attempt variant Some(e.context(...))) both move e, then the trailing tracing::warn!(error = %e, ...) borrowed the moved value. deeplethe#302 had moved that warn into the else branch ahead of the assignment. The arm already computes let msg = format!("{e:#}") for busy-classification, so use error = %msg in the warn to avoid re-borrowing e after the moves. * test(controller): make orphan-kill SIGKILL regression robust to CI orphan-reaping kill_orphans_match_arm_kills_verified_firecracker_via_pidfd spawned its firecracker-named 'sleep' via sh -c '<bin> 30 & echo $!' and relied on the backgrounded process being reparented to init, so wait_for_death would see /proc/<pid> disappear right after SIGKILL. On the GitHub Actions runner that detached child is reaped almost immediately (diagnostic showed state=Z ppid=1 at t=0ms, dead by 5ms), so process_identity_matches returned Dead, kill_orphans pruned it, and the test failed with pruned_stale=1 instead of killed=1. Spawn the process as a DIRECT child the test owns (the runner cannot reap an owned, non-orphaned child), and reap it via a small reaper thread that calls Child::wait(). wait_for_death (polling /proc/<pid>) then observes the SIGKILL reaping and the Match arm reports killed=1 as intended. * fix(controller): distinct legacy-row diagnostics + off-Linux gate skip (review deeplethe#299 r9) Legacy state.json rows (no recorded boot identity via serde defaults) took the generic Unknown path on first restart after upgrade and aborted startup with an EPERM-style message describing neither cause nor fix. - IdentityCheck::LegacyUnverifiable: missing recorded starttime OR boot id is reported distinctly from a genuinely unverifiable environment. - KillOrphansResult.legacy_unverified counts legacy rows separately from kill_failed; run_daemon logs include the new counter. - check_orphan_kill_result emits a dedicated upgrade diagnostic (kill the recorded PID manually or remove the entry once verified dead), distinct from the kill-failure and pid-less-row messages. - Off-Linux, identity can never be verified, so the gate skipped any registry row and aborted dev-box startup; the abort decision is now extracted into a pure cross-platform orphan_abort_reason(), the Linux gate fails closed on it, and the off-Linux gate logs the counts and allows startup (review deeplethe#299 non-blocking). - Corrected the wrong pidfd comment: /proc/<pid> reflects whoever currently holds the PID; the post-open starttime re-read is what makes the reuse check safe. - kill_orphans folds its two entry lock acquisitions into one guard, matching the deeplethe#312 single-lock invariant. Tests: legacy rows count as legacy_unverified (not kill_failed); abort distinctness (legacy vs kill-failure diagnostics); Linux-gated gate wiring test; existing abort-contract tests retargeted to the pure decision function so they run on every platform. * fix(controller): fail spawn loudly when boot id is unreadable at registration read_boot_id() returning None at registration was recorded silently, producing a sandbox row that can never pass startup identity verification — bricking every subsequent controller start until state.json was hand-edited (review deeplethe#299). registration_boot_id() treats an unreadable boot id as a spawn-blocking error. Both production registration paths call it BEFORE any VM work: the /v1/fork handler returns a 500 up front, and spawn_one_for_workspace bails before snapshot lookup. Off-Linux it returns Ok(None) so dev-box spawns keep working. * fix(controller): type alias for orphan row to satisfy clippy type_complexity * fix(vmm): immutable baseline rootfs — reflink clone before boot (closes deeplethe#296) The rootfs corruption issue (deeplethe#296) was caused by vm.kill() SIGKILLing firecracker without a clean ext4 unmount, leaving the rootfs dirty. The previous approach (PR deeplethe#295) ran e2fsck -fy before each boot to repair the dirty journal — but the reviewer flagged a TOCTOU race: the /proc/*/fd scan is point-in-time, so another VM could open the rootfs during e2fsck. The immutable-baseline approach eliminates the race entirely: 1. The original rootfs is the IMMUTABLE BASELINE — never mounted RW. 2. Before each boot, forkd creates a reflink copy (FICLONE ioctl, instant on btrfs/xfs/overlayfs; falls back to full copy on ext4/tmpfs) in the snapshot directory. 3. The VM boots from the clone and writes to it; the baseline stays clean. 4. After vm.kill(), the clone persists as the snapshot's rootfs (needed for restores — Firecracker re-opens the rootfs from the path 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. The reflink copy is exposed as pub fn reflink_copy in chain.rs (wraps the existing copy_base_memory which already has FICLONE + stream fallback). The snapshot_cmd function in forkd-cli creates the clone at <snapshot_dir>/rootfs.ext4 and records its path in snap.rootfs. Signed-off-by: jrimmer <jason@rimmer.net> * fix(snapshot): cache versioning, atomic staging, portable rootfs transport Review deeplethe#295 r6 (WaylandYang 2026-08-14): three correctness/portability blockers on the immutable-baseline clone design. All three closed, plus the four requested regression tests. Blocker 1 — cache versioning (crates/forkd-cli/src/main.rs): A cached rootfs produced or dirtied by an older forkd version was reused solely because the path exists, then cloned as the supposedly immutable baseline. Now each built rootfs gets a `.cache-meta.json` sidecar recording schema_version + sha256 + image + size + forkd version. `validate_cached_rootfs` trusts a cache entry only when the meta exists, the schema version matches ROOTFS_CACHE_SCHEMA_VERSION (=1), and the live sha256 still matches. Legacy entries (no meta), schema mismatches, or sha mismatches (truncation/mutation) force a rebuild. Wired into both from_image_cmd and run_cmd cache-hit paths; write_rootfs_cache_meta is called after every build. Blocker 2 — atomic snapshot staging (snapshot_cmd + publish_snapshot): 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. Blocker 3 — portable rootfs transport (crates/forkd-cli/src/hub.rs): The rootfs was shipped TWICE — tarred into the pack via SNAPSHOT_FILES AND emitted as a content-addressed .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 so the relative target_path can be resolved; unpack_chain_into returns the head link's dest. Tests (crates/forkd-cli/src/main.rs): - 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 - 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) Signed-off-by: jrimmer <jason@rimmer.net> * style: cargo fmt + fix stray brace in run_cmd cache validation Rebase resolution left a stray closing brace after the validate_cached_rootfs match block in run_cmd (the original if/else's trailing brace was not removed). cargo fmt the rebased diff. Signed-off-by: jrimmer <jason@rimmer.net> * fix(cli): qualify sha256_file calls with hub:: prefix write_rootfs_cache_meta and validate_cached_rootfs called sha256_file without the hub:: prefix; the function lives in forkd-cli::hub, not main.rs. This was a compile error (E0425 cannot find function sha256_file) on Linux clippy CI. Signed-off-by: jrimmer <jason@rimmer.net> * fix(cli): clippy redundant closure in destination fallback unwrap_or_else(|| std::path::PathBuf::new()) -> unwrap_or_else(std::path::PathBuf::new) Signed-off-by: jrimmer <jason@rimmer.net> * fix(snapshot): 4 blockers from review deeplethe#295 (2026-08-22) Rebased onto dev (d2d238a, incl. deeplethe#312). Addresses all four review blocks on the staging/transport flow while keeping cache-versioning and failure-rollback. Blocker 1 — FC-visible rootfs path unstable across publish: Firecracker serializes the drive path_on_host INTO the binary vmstate and REOPENS it on restore (no PUT /drives override is accepted before /snapshot/load). Previously snapshot_cmd cloned rootfs to staging_dir/rootfs.ext4 and booted the VM from it, then publish_snapshot renamed the whole dir to snap_dir — leaving the vmstate's recorded path nonexistent, so the first restore after publish failed. Fix: clone+boot from the STABLE final path snap_dir/rootfs.ext4 from the start; only vmstate/memory/snapshot.json are staged. publish_snapshot is replaced by publish_snapshot_metadata, which renames the 3 metadata files into snap_dir (snapshot.json LAST = commit marker) and never moves the rootfs. Tests: metadata-replace-keeps-rootfs, into-nonexistent-snap_dir, preserves-existing-on-missing-metadata. Blocker 2 — pack/unpack sidecar never placable: 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. Fix: when a portable sidecar is emitted, retain rootfs.ext4 out of manifest.files (sidecar carries sha integrity). Test: pack_unpack_roundtrip_with_sidecar_rootfs. Blocker 3 — src==dst guard compared staging path, not final: extracted to rootfs_clone_into_self(src, snap_dir) comparing canonical against the FINAL snap_dir/rootfs.ext4, so re-snapshotting a tag whose baseline is its own rootfs.ext4 is rejected. Test: same-tag regression. Blocker 4 — path traversal via RootfsRef.target_path: satisfy_rootfs now REQUIRES a safe single-component relative filename (no absolute / no separators / no ..), rejecting malicious ../../../ or legacy absolute paths (fail-closed; writes would otherwise land outside snap_dir under sudo). Test: satisfy_rootfs_rejects_unsafe_target_paths. Signed-off-by: jrimmer <jason@rimmer.net> * fix(snapshot): re-bake rollback + validated absolute rootfs placement (deeplethe#295 r9) Addresses the 2026-08-22T20:28 review round on PR deeplethe#295. Blocker 1 — a failed re-bake destroyed the previously published tag: the old rootfs was remove_file'd before the VM booted, so any later failure (boot timeout, snapshot error, publish error, interrupt) left old metadata frozen against a replaced, dirtied rootfs (deeplethe#296 EBADMSG). The previous rootfs is now renamed aside to <tag>.rootfs.ext4.prev-<pid> and a RootfsRollback guard restores it on every failure path (Drop = rollback, covers ?-propagation and panic unwinding); the backup is deleted only after publish_snapshot_metadata commits the new snapshot. Blocker 2 — snap_dir-relative target_path broke cross-host restore: Firecracker freezes the drive path into the vmstate and reopens it verbatim at /snapshot/load, so placement must not depend on the destination snap_dir. manifest.rootfs.target_path records the absolute path again (un-breaking published packs whose absolute target_path the r6 code rejected outright), and satisfy_rootfs validates it against forkd-managed roots — the forkd data dir, its snapshots/ tree, and the rootfs cache (, default /var/cache/forkd) — with traversal/backslash rejection and existing-prefix canonicalization (symlink escape hardening) before any privileged write. A missing or unfetchable sidecar is now a hard error: a warn-and-continue produced a snapshot that could never restore. Nits: duplicated src==dst comment line removed; unpack_chain_into tail bails instead of returning an empty PathBuf that would resolve target_path against the CWD under sudo. Tests: RootfsRollback armed/disarmed unit test; satisfy_rootfs rejection matrix updated for the absolute-path contract plus placement and sha-dedup assertions; cross-snap-dir verbatim placement round-trip (reviewer's blocker-2 regression ask); validate_rootfs_target_path unit contract; the #[ignore] KVM regression extended with a failed-re-bake rollback and a cross-tag sidecar placement scenario (FORKD_TEST_*). Signed-off-by: jrimmer <jason@rimmer.net> * fix(snapshot): clippy needless-question-mark + restore opts rebuild in KVM test Signed-off-by: jrimmer <jason@rimmer.net> * test(snapshot): rollback restores content at the published path, not absence Signed-off-by: jrimmer <jason@rimmer.net> * test(snapshot): surface per-child restore failures instead of swallowing them Signed-off-by: jrimmer <jason@rimmer.net> * fix(snapshot): repoint staged snapshot.json at final paths on publish The staging-flow snapshot.json was serialized with staging-dir vmstate/ memory paths, and publish_snapshot_metadata renamed the files without fixing up the paths inside — so every staged snapshot published since the atomic-staging change recorded a vmstate path in a directory that is deleted right after publish. Restore then fails with 'Failed to open snapshot file' (caught by the first real-KVM run of the snapshot_stable_rootfs_publishes_and_restores regression on a KVM host; CI could never see it because the test is #[ignore]-gated). publish_snapshot_metadata now rewrites the staged snapshot.json's vmstate/memory keys to the 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. Publish unit tests now stage real JSON (as snapshot_cmd does) and assert the repoint; the KVM regression asserts per-child restore failures are empty so a dead child can no longer hide behind an Ok. Signed-off-by: jrimmer <jason@rimmer.net> * fix(cli): sweep every rootfs .prev backup on re-bake A re-bake parks the previous published rootfs as `<tag>.rootfs.ext4.prev-<pid>`. The pid belongs to the run that parked it, so the same-pid check could only ever clean up after a run whose pid happened to match. A `kill -9` between the preserve-rename and the publish stranded the file under a pid no later run would look for — several GB that nothing collected — while the tag was left with no `rootfs.ext4` until someone renamed the backup back by hand. Sweep every `<tag>.rootfs.ext4.prev-*` sibling instead. When the published rootfs is missing, the newest backup is renamed back into place; the remaining ones are scratch from runs that never published and are dropped best-effort — a stale file that cannot be removed no longer aborts the bake, since the preserve-rename would overwrite it anyway. Also corrects the `satisfy_rootfs` doc comment, which still described the missing-sidecar case as a warning when it is now a `bail!`. Both are follow-ups from the deeplethe#295 approval. Signed-off-by: jrimmer <jason@rimmer.net> * fix(vmm): reap the Firecracker child on every failed boot `Vm::boot` spawned Firecracker and then drove `/boot-source`, `/drives/rootfs`, `/entropy` and `/actions` with a bare `std::process::Child` in hand. `Child` has no kill-on-drop, so any `?` in that window — `wait_for_sock` timing out, or an API call failing after the process was up — returned an error and abandoned a *live* Firecracker. The orphan keeps the sandbox's tap device and rootfs fd open, and the tap name is frozen into the vmstate, so every later spawn of that snapshot dies inside Firecracker with `Open tap device failed` before it can create its API socket. Callers see `socket .../child-N.sock never appeared` and the snapshot stays unusable until the orphan is killed by hand — the permanent-failure mode in deeplethe#301. Own the child in a `PendingFirecracker` guard that reaps it on every early return and hands it to the `Vm` only on success. `restore_many_with` already wrapped its children immediately, so `boot` was the only window in the crate that could orphan a process with no handle left. Two unit tests cover the drop path (the pid must be gone, not just untracked) and the take path (the handed-over child must still be live). Signed-off-by: jrimmer <jason@rimmer.net> * fix(cli): gate bakes on free space, and always clean up the staging dir Both halves of one failure mode: a bake that runs out of space does not stop, it produces a *corrupt* artifact — a rootfs whose package files carry other files' bytes, or a truncated `memory.bin`. Inside a guest that surfaces much later as `uname: option requires an argument`, `Exec format error`, or `EBADMSG` on a `/var/lib/dpkg` entry, so it reads like a broken build rather than a broken image. - `forkd parent build` (conversion) and `forkd snapshot` (bake) check free space on the target filesystem before the first write and refuse below a 5 GiB reserve (`FORKD_MIN_FREE_GIB` overrides). Conversion is the first multi-GB write of the pipeline, and the snapshot dir holds the clone plus a fresh `memory.bin`. The probe stays advisory: if `statvfs` cannot run, warn and continue rather than block work on a broken measurement. `doctor::available_bytes(path)` is extracted from the check's hardcoded-path statvfs so both callers share one probe. - `forkd snapshot` removes its staging dir on every exit. Only the success path did, so any `?` after the volatile artifacts were written — boot timeout, snapshot error, publish error, interrupt — left a fully written `memory.bin` (GBs) beside the snapshot dir that nothing ever collected. Tests cover the guard (removal, and tolerance of an already-removed dir — the success path renames files out and a stale sweep may beat it) and the probe's ancestor walk, which is what makes it usable before the snapshot dir exists. Signed-off-by: jrimmer <jason@rimmer.net> * test(cli): tolerate free-space drift between the two statvfs probes Parallel tests write to the same filesystem between the two calls, so exact equality flakes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): restore the .prev backup unless the tag was republished A rootfs.ext4 at the published path is not evidence the tag is whole: the interrupted run clones its fresh rootfs there right after parking the backup and then boots and writes to it for the whole warmup. The sweep discarded the backup in that case, leaving the old metadata frozen against a dirtied, unpublished clone (deeplethe#296) with the last good rootfs deleted. Decide by the publish commit marker instead: restore the newest backup unless snapshot.json is newer than the backup's park ctime. Backups whose pid is still running a bake are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(vmm): use the real FICLONE ioctl number 0x40209409 encodes a 32-byte payload; FICLONE is _IOW(0x94, 9, int) = 0x40049409. The kernel answered ENOTTY, which the fallback reads as no reflink, so every reflink_copy streamed a full copy on every filesystem. Reported by @jrimmer in deeplethe#321. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style: rustfmt the FICLONE derivation test Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(cli): name the backup tuple for clippy::type_complexity Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(vmm): compare FICLONE without a cast Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(release): build the release binaries against jammy (glibc 2.35) — ubuntu-latest's glibc 2.39 binary dies on the sandbox host VMs (#5) LIVE refusal (urun dev-usw2 2026-10-15, cpu-sbx node boot, stage forkd-daemon): the freshly-installed forkd-controller crash-looped with /usr/local/forkd/forkd-controller: /lib/x86_64-linux-gnu/libc.so.6: version GLIBC_2.38 not found (required by forkd-controller) The release workflow builds on ubuntu-latest (24.04, glibc 2.39) while the sandbox host VM image is Ubuntu 22.04 (glibc 2.35) — a binary built on newer glibc can never run there. The binaries job now builds inside an ubuntu:22.04 container (all-TLS via rustls; no openssl native deps), keeping every other step identical. The docker image job is untouched (it runs the binary inside its own container image). --------- Signed-off-by: jrimmer <jrimmer@users.noreply.github.com> Signed-off-by: jrimmer <jason@rimmer.net> Signed-off-by: Amir Fathi <amirfathi.me@gmail.com> Co-authored-by: jrimmer <jason@rimmer.net> Co-authored-by: WaylandYang <145302500+WaylandYang@users.noreply.github.com> Co-authored-by: Amir Fathi <amirfathi.me@gmail.com> Co-authored-by: WaylandYang <wayland0916@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Fixes the defect in #317 — concurrent children of one tag sharing a writable rootfs — and carries the version policy that comes with it.
The mechanism, and why it is this one
Neither released Firecracker has
drive_overrides: I checked the v1.16.2 and v1.17.0 binaries, not the source, and neither contains the string. So the load-time override from #5774 is not available in a released build.What a released Firecracker does have is
PATCH /drives/{drive_id}on a restored VM, and it moves the device's storage rather than just its config. Measured: after re-pointing a child's rootfs, a write made inside the guest landed in the new backing and not in the original file.So each child gets its own reflink clone of the tag's rootfs and is re-pointed at it before it runs:
The ordering is load-bearing, and step 2 is why. Loading paused 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.Fail closed. A child whose rootfs cannot be re-pointed is not resumed. It would run against the shared base, which is the defect, so the failure surfaces and the child is dropped (killing its Firecracker) rather than run unsafely.
Two API details worth having:
PATCH /drivesrequiresdrive_idin the body (without it a 400 reads as a body error, not a capability error), andPUT /drivesto add a drive post-restore fails withPCI is not enabledsince forkd bootspci=off— irrelevant here, relevant if anyone wants per-child extra devices later.Evidence
child-1: AAA=1 BBB=0,child-2: AAA=0 BBB=1)DELETESIGKILLthe controller, let systemd restart it: 2 backings → 0POST /v1/snapshotsnow writesrootfsintosnapshot.json; daemon branches inherit itThe crash case is interesting and shaped the sweep. On a restart, systemd kills the unit's cgroup, so the children die with the controller; the reap finds dead processes and correctly prunes rather than kills, so nothing reaches
Vm::dropand nothing used to remove the backing — while the watchdog skips the directory because its socket file is still present. A sweep now runs before the reap, when those directories are unowned.Its gate is process ownership, not file presence: a directory is skipped while any live Firecracker's
--api-sockpoints inside it, and if any live Firecracker cannot be read, the sweep removes nothing. Leaking a backing costs disk; deleting a running VM's backing destroys its disk mid-job, so every ambiguity resolves toward keeping it.The per-kill reclaim is kept alongside the sweep rather than replaced: it covers a process verified ours and confirmed dead at kill time, which the pre-reap sweep cannot see.
Consequences to plan for
PATCH /driveswithout moving storage. That was secondhand and wrong: measured on stock v1.5.1, v1.7.0, v1.10.1, v1.12.1, v1.14.1 and v1.17.0, a restored VM's writes move to the new file and/vm/configreports it. Existing snapshots keep loading; nothing needs re-baking. Every re-point is still verified by reading/vm/configback, and a child is not resumed unless it reports its own backing.--live. It carriesMemBackendConfig::shared, andforkd-vmmsendssharedin exactly one place — the memfd path live-fork uses. Everything else runs on stock Firecracker.std::env::temp_dir()-derived (now from one helper shared with the sweep). On a host where/tmpis tmpfs that charges a child's rootfs copy to RAM;TMPDIRrelocates it./dev/vdb:VolumeSpec.read_onlydefaults tofalseand the path is frozen, so children of a tag carrying one all open the same file. Out of scope here; worth its own issue.Not covered by the evidence
Both crash runs exercised the cgroup-kill flavour, because the children were spawned without a memory limit and so died with the controller. The other flavour — Firecracker outliving the controller, e.g. children in their own per-VM cgroup — exercises the kill path, which is verified through a plain
DELETEbut not yet through a crash.Snapshots with no recorded rootfs keep the previous behaviour. Daemon-baked snapshots, branches and workspace suspends all record one now.