Repository navigation
fix(rootfs): key guest rootfs cache on the resolved guest binary - #1096
Merged
Merged
Conversation
The cache key came from BOXLITE_GUEST_HASH, a compile-time digest, but the
binary it describes is resolved at run time by find_binary, which ranks
BOXLITE_RUNTIME_DIR above the embedded runtime cache. A guest rebuilt after
boxlite was compiled kept the old key, so an existing cache entry was reused
and the VM booted the previous guest. The reconciliation guard that existed to
catch this only ran on a cache miss, and GC derived its retention suffix from
the same stale value.
GuestBinary becomes the single resolution point: one read feeds ELF validation
and the digest, and version_key, GC retention and the injected bytes all come
from that one value, so expected and actual cannot diverge. The guard and
BOXLITE_GUEST_HASH are removed rather than extended.
sha2 0.11 makes always hashing affordable — 0.10's portable backend costs
~690 ms debug / ~36 ms release on the 18 MB guest, which is why the
compile-time proxy existed. digest 0.11 drops LowerHex, so 16 digest sites
move to hex::encode.
Also reclaims base disks that no DB row and no backing chain claims. The
per-kind collectors iterate records, so a file whose row is gone is
unreclaimable; one real cache held 7 files against 1 row. The sweep is
filesystem-first, walks chains from both box overlays and from the bases
themselves, fails closed on a DB error, and skips files inside a grace window.
Also stamps the extracted runtime cache directory with the build's commit
(v{VERSION}-{COMMIT}-{HASH}) for provenance. Correctness still comes from the
manifest hash; the commit is deliberately kept out of the rootfs version key,
where it would split the cache for byte-identical guests.
📦 BoxLite review — looks good ·
|
Contributor
📝 WalkthroughWalkthroughThis PR upgrades sha2 and migrates digest formatting to hex::encode, introduces a process-wide GuestBinary type that resolves and validates the guest binary once for cache-key derivation, adds orphan base-disk garbage collection wired into recovery, and stamps embedded runtime cache directories with the build's git commit. ChangesHashing modernization
Guest binary identity refactor
Orphan base-disk garbage collection
Git commit provenance for cache naming
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuestRootfsManager
participant GuestBinary
participant guest_check
participant Filesystem
GuestRootfsManager->>GuestBinary: get()
GuestBinary->>Filesystem: read boxlite-guest bytes
GuestBinary->>guest_check: validate_guest_bytes(data, path)
guest_check-->>GuestBinary: validation result
GuestBinary->>GuestBinary: compute truncated SHA-256 id
GuestBinary-->>GuestRootfsManager: path() and id()
GuestRootfsManager->>GuestRootfsManager: derive cache version key from id
GuestRootfsManager->>Filesystem: inject resolved guest binary
sequenceDiagram
participant Recovery
participant BaseDiskManager
participant Database
participant Filesystem
Recovery->>BaseDiskManager: gc_orphans()
BaseDiskManager->>Filesystem: scan .ext4/.qcow2 base files
BaseDiskManager->>BaseDiskManager: referenced_backing_paths()
BaseDiskManager->>Database: lookup base-disk records
Database-->>BaseDiskManager: claimed or error
BaseDiskManager->>BaseDiskManager: skip referenced, claimed, or recent files
BaseDiskManager->>Filesystem: delete unclaimed settled files
BaseDiskManager-->>Recovery: reclaimed file count
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/boxlite/src/disk/base_disk.rs (1)
131-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
read_dir+ warn-on-real-failure pattern.The same
match fs::read_dir(path) { Ok(..) => .., Err(e) => { if path.exists() { warn!(..) } } }shape appears three times (boxes_dir scan and bases_dir scan inreferenced_backing_paths, and the bases_dir scan ingc_orphans). Extracting a small helper would remove the duplication and keep the "only warn when the path genuinely exists but is unreadable" semantics consistent in one place.♻️ Proposed helper
+ fn read_dir_or_warn(path: &Path, label: &str) -> Option<fs::ReadDir> { + match fs::read_dir(path) { + Ok(entries) => Some(entries), + Err(e) => { + if path.exists() { + tracing::warn!("GC: failed to read {} {}: {}", label, path.display(), e); + } + None + } + } + }Then each call site collapses to, e.g.:
- match fs::read_dir(boxes_dir) { - Ok(entries) => { ... } - Err(e) => { - if boxes_dir.exists() { - tracing::warn!("GC: failed to read boxes dir {}: {}", boxes_dir.display(), e); - } - } - } + if let Some(entries) = Self::read_dir_or_warn(boxes_dir, "boxes dir") { + for entry in entries.flatten() { ... } + }Also applies to: 158-173, 200-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/disk/base_disk.rs` around lines 131 - 155, Extract the repeated directory-reading and warning logic into a small helper near the relevant disk utilities, preserving the behavior of warning only when the directory still exists but cannot be read. Update the boxes_dir and both bases_dir scans in referenced_backing_paths and gc_orphans to use the helper while keeping each call site’s existing entry-processing logic unchanged.src/boxlite/src/runtime/rt_impl.rs (1)
1449-1455: 🩺 Stability & Availability | 🔵 TrivialOrphan base-disk GC only runs once, at process startup.
Since
gc_orphansis invoked solely fromrecover_boxes, orphaned base disks created by crashes or DB inconsistencies during a long-lived runtime's uptime won't be reclaimed until the next restart. For long-running deployments this could let disk usage grow between restarts. Worth considering a periodic sweep (e.g., on a timer or after N box removals) in a follow-up if disk pressure from orphaned bases proves to be an issue in practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/runtime/rt_impl.rs` around lines 1449 - 1455, Add a recurring orphan base-disk cleanup trigger beyond the startup-only call in recover_boxes, such as a timer or threshold after box removals, and reuse base_disk_mgr.gc_orphans with the existing boxes_dir and warning behavior. Ensure long-lived runtimes periodically reclaim orphaned base files without changing the existing startup recovery flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/boxlite/src/disk/base_disk.rs`:
- Around line 131-155: Extract the repeated directory-reading and warning logic
into a small helper near the relevant disk utilities, preserving the behavior of
warning only when the directory still exists but cannot be read. Update the
boxes_dir and both bases_dir scans in referenced_backing_paths and gc_orphans to
use the helper while keeping each call site’s existing entry-processing logic
unchanged.
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 1449-1455: Add a recurring orphan base-disk cleanup trigger beyond
the startup-only call in recover_boxes, such as a timer or threshold after box
removals, and reuse base_disk_mgr.gc_orphans with the existing boxes_dir and
warning behavior. Ensure long-lived runtimes periodically reclaim orphaned base
files without changing the existing startup recovery flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fab28222-5aaa-474b-81ed-cb0a9e4001bd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
src/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/src/disk/base_disk.rssrc/boxlite/src/images/archive/verifier.rssrc/boxlite/src/images/object.rssrc/boxlite/src/images/storage.rssrc/boxlite/src/images/store.rssrc/boxlite/src/litebox/archive.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/embedded.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/util/binary_finder.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/guest_binary.rssrc/boxlite/src/vmm/guest_check.rssrc/boxlite/src/vmm/mod.rssrc/shared/build.rssrc/shared/src/lib.rs
8 of 9 tasks
G4614
pushed a commit
to G4614/boxlite
that referenced
this pull request
Aug 3, 2026
…lite-ai#1096) ## Summary The guest rootfs cache key came from `BOXLITE_GUEST_HASH`, a compile-time digest, but the binary it describes is resolved at run time by `find_binary`, which ranks `BOXLITE_RUNTIME_DIR` above the embedded runtime cache. A guest rebuilt after boxlite was compiled kept the old key, so an existing cache entry was reused and the VM booted the previous guest. `GuestBinary` becomes the single resolution point, which makes that divergence unrepresentable. ## Changes - `GuestBinary` resolves, validates and identifies the guest binary once. One read feeds both the ELF check and the digest, and `version_key`, GC retention and the injected bytes all derive from that one value. - The reconciliation guard is deleted rather than extended. It only ran on a cache miss, so the cache-hit path — the silent one — was never covered. - `BOXLITE_GUEST_HASH` and its build-script emitter are removed, along with the now-dead `validate_guest_binary`. - `sha2` 0.10 → 0.11, which is what makes always-hashing affordable: 0.10's portable backend costs ~690 ms debug / ~36 ms release on the 18 MB guest. `digest` 0.11 drops `LowerHex`, so 16 digest sites move to `hex::encode`. - `BaseDiskManager::gc_orphans` reclaims base disks that no `base_disk` row and no backing chain claims. The per-kind collectors iterate DB records, so a file whose row is gone was unreclaimable; one real cache held 7 files against 1 row. - The extracted runtime cache directory is stamped with the build's commit for provenance. Correctness still comes from the manifest hash, and the commit is deliberately kept out of the rootfs version key. ## How to verify - `make test:unit:rust` - End to end: `make guest` to rebuild the guest without rebuilding boxlite, then start a box. The cache key moves and a new rootfs is built instead of the stale one being reused; starting again with an unchanged guest hits the cache. ## Risks / rollout - `gc_orphans` deletes files. It removes one only when no `base_disk` row of any kind names it *and* nothing backs onto it, walking chains from both box overlays and from the bases themselves. It fails closed on a database error and skips anything inside a 5-minute grace window. - Two tests (`id_tracks_the_bytes_on_disk`, `version_key_follows_the_guest_binary_on_disk`) cannot have a revert-and-watch-it-fail red side, because they call a symbol the fix introduces. That gap is recorded at both test sites; they were checked by mutation and by a booted VM instead. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automatic cleanup of unused base disk files, while protecting active and recently created data. - Added build commit information to runtime cache naming when available. - Improved guest runtime identification so cache updates reflect changes to the guest binary. - **Bug Fixes** - Recovery now attempts orphaned disk cleanup without interrupting the overall recovery process. - Improved validation and tracking of guest runtime binaries. - Standardized digest handling to preserve consistent image and cache verification. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The guest rootfs cache key came from
BOXLITE_GUEST_HASH, a compile-time digest, but the binary it describes is resolved at run time byfind_binary, which ranksBOXLITE_RUNTIME_DIRabove the embedded runtime cache. A guest rebuilt after boxlite was compiled kept the old key, so an existing cache entry was reused and the VM booted the previous guest.GuestBinarybecomes the single resolution point, which makes that divergence unrepresentable.Changes
GuestBinaryresolves, validates and identifies the guest binary once. One read feeds both the ELF check and the digest, andversion_key, GC retention and the injected bytes all derive from that one value.BOXLITE_GUEST_HASHand its build-script emitter are removed, along with the now-deadvalidate_guest_binary.sha20.10 → 0.11, which is what makes always-hashing affordable: 0.10's portable backend costs ~690 ms debug / ~36 ms release on the 18 MB guest.digest0.11 dropsLowerHex, so 16 digest sites move tohex::encode.BaseDiskManager::gc_orphansreclaims base disks that nobase_diskrow and no backing chain claims. The per-kind collectors iterate DB records, so a file whose row is gone was unreclaimable; one real cache held 7 files against 1 row.How to verify
make test:unit:rustmake guestto rebuild the guest without rebuilding boxlite, then start a box. The cache key moves and a new rootfs is built instead of the stale one being reused; starting again with an unchanged guest hits the cache.Risks / rollout
gc_orphansdeletes files. It removes one only when nobase_diskrow of any kind names it and nothing backs onto it, walking chains from both box overlays and from the bases themselves. It fails closed on a database error and skips anything inside a 5-minute grace window.id_tracks_the_bytes_on_disk,version_key_follows_the_guest_binary_on_disk) cannot have a revert-and-watch-it-fail red side, because they call a symbol the fix introduces. That gap is recorded at both test sites; they were checked by mutation and by a booted VM instead.Summary by CodeRabbit
New Features
Bug Fixes