fix(cli): sweep every rootfs .prev backup on re-bake - #314
WaylandYang merged 6 commits into
Conversation
Sorry, something went wrong.
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>
|
I let down the honoring the the PR number: 🥧. I'm now sad there isn't a pi symbol in the emoji set. |
Sorry, something went wrong.
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>
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # crates/forkd-cli/src/main.rs
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks. Sweeping by prefix is the right idea, but the "is rootfs.ext4 present?" test broke the most common crash case, so I pushed a fix to this branch rather than bouncing it back.
The problem. After the preserve-rename, snapshot_cmd immediately reflink-copies a fresh clone to snap_dir/rootfs.ext4 and boots from it for the whole warmup. A kill -9 anywhere from the clone to publish (by far the widest part of the window) leaves:
- a rootfs.ext4 that is an unpublished, dirtied clone,
- old vmstate/snapshot.json still published against the backup,
- the backup itself.
The sweep saw "tag is whole" and deleted the backup. That destroyed the last good rootfs and left the #296 mismatch (old metadata against a dirtied rootfs) with no way back. RootfsRollback::drop handles the same state the other way: it restores the backup over the clone.
The fix (e416661). Decide by the publish commit marker instead of by presence. A run writes snapshot.json after it parks the backup, and rename(2) stamps the backup's ctime. So:
- if snapshot.json mtime is not newer than the newest backup's ctime, the tag was never republished, and the backup is renamed back over whatever is at the published path;
- otherwise the run published and died before deleting its backup, and the backup is discarded;
- if it was published but rootfs.ext4 is missing, or a backup can't be dated, nothing is touched and a warning is printed;
- a backup whose pid is still alive belongs to a concurrent bake of the tag and is left alone.
New tests cover the warmup-crash case (backup restored over the clone) and the live-pid case. The existing discard test now includes a post-park snapshot.json. I also merged dev (after #315/#316) and resolved the conflicts.
Verified on Linux (Ubuntu 22.04, stable Rust in Docker): fmt, clippy -D warnings, and cargo test --all all green, together with #315/#316/#322.
Sorry, something went wrong.
Follow-ups from the #295 approval — the two items marked "pick up separately".
1. satisfy_rootfs doc comment
The comment still described a missing sidecar as a warning it "warns (does not fail)"; the function now bail!s. Comment corrected to match the code.
2. Stale .prev cleanup only saw the current pid
The backup is parked as <tag>.rootfs.ext4.prev-<pid>, and the entry check only looked for the current run's pid. A kill -9 between the preserve-rename and the publish therefore stranded the file under a pid no later run would look for, and — because the crash landed after the rename — the tag was left with no rootfs.ext4 at all until someone renamed the backup back by hand. The cleanup could not act on the window it acknowledged.
The sweep is now by prefix over every <tag>.rootfs.ext4.prev-* sibling:
Ordering is by mtime — rename(2) preserves it, so a backup carries the timestamp of the bake that produced its rootfs — with the filename breaking ties so the choice never depends on readdir order.
One deliberate behaviour change: a stale backup that cannot be removed no longer aborts the bake. The old ? turned a hygiene failure into a failed bake, and the subsequent preserve-rename overwrites the path regardless.
Three unit tests: restore-the-newest, discard-when-the-tag-is-whole, and tag scoping + a snapshot dir whose parent does not exist yet.
cargo fmt --check clean, cargo clippy --all-targets --all-features -D warnings clean, cargo test -p forkd-cli green (56 passed, 1 ignored). The #[ignore]d KVM test was not run for this change — the new paths are pure filesystem logic.