feat(archive): layered box export and import primitives - #1080
Open
G4614 wants to merge 40 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds digest-backed layered archive version 6 support. It adds file, directory, and shared-store exports, lazy layered imports, digest persistence and reuse, archive garbage collection, and required guest-freeze handling. SDK bindings expose the new export options. ChangesLayered archive export and import
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BoxImpl
participant BaseDiskManager
participant ArchiveBuilder
participant ImportRuntime
participant BoxRuntime
BoxImpl->>BaseDiskManager: capture and digest disk layers
BaseDiskManager->>ArchiveBuilder: publish manifest and layer objects
ArchiveBuilder-->>ImportRuntime: provide archive or directory objects
ImportRuntime->>BaseDiskManager: validate, reuse, or install layers
BaseDiskManager-->>ImportRuntime: return linked container layer
ImportRuntime->>BoxRuntime: transfer references and provision box
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
5 tasks
The guest rootfs disk is a thin COW overlay over the host-global guest rootfs cache, keyed by the bootstrap image plus guest binary version. It holds no user state, and clone and snapshot-restore already treat it as disposable: when it is absent, the next start recreates the overlay from the local cache. Export was the exception. It flattened the overlay into the archive, which both bloated every archive with a host-independent blob and stripped the backing reference, so an imported box would boot from the archived copy instead of the importing host's correctly-versioned cache. Export now omits it and import never installs one, even from an older archive. No archive version bump: the disk was already optional, since a never-started box exported without it, so old importers handle its absence and new importers ignore its presence. guest_disk_checksum stays on the manifest, always empty, so older importers still parse it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Export flattened the container disk's qcow2 chain into one image, so every archive carried a full copy of the image layer even though every box on a host is a COW child of the same one. v6 archives instead carry the chain as `layers/` blobs keyed by sha256, ordered base first, and an importer that already holds a layer skips its transfer entirely. Measured on a real box, a layered archive is the same size as a flattened one (12,578,096 vs 12,564,806 bytes): a short chain has almost no superseded data, and zstd erases the sparse image disk's holes. Export also no longer pays the flatten pass, and base digests are cached in the store so repeat exports do not re-hash immutable layers. Import resolves each layer against the local base store by digest, materializes only what is missing, and relinks children to paths it chose itself. The manifest carries digests and never paths, every blob is verified against its declared digest before anything points at it, and each relink is read back and checked — so a crafted archive still cannot aim a backing file at a host path of its choosing. Imported bases are ref-counted against the new box so the GC does not drop a layer the box reads through. Qcow2Helper::flatten keeps no caller but is retained: MAX_BACKING_CHAIN_DEPTH caps a chain at 8, and collapsing a chain is the compaction step that keeps clone-heavy lineages under that cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Inserting LayerFormat and ArchiveLayer split the manifest's version-history doc comment away from the struct, leaving it dangling — clippy's empty_line_after_doc_comments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unrelated to this PR's change: boxlite-ai#1084 landed an unformatted println! on main, so main's own Lint run is red and every PR merged against it inherits the failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G4614
force-pushed
the
feat/layered-archive
branch
from
July 29, 2026 11:29
03354a8 to
192d170
Compare
Five defects found reviewing the layered archive path, two of them exploitable. The deepest layer was never relinked nor validated, so its header's backing path — attacker-controlled data — survived verbatim into bases/. The chain is granted to the sandbox at start, so an archive could name any host file and have its bytes handed to the guest. That layer now goes through validate_no_backing_references. A layer already held locally was relinked to satisfy the incoming archive, rewriting a file other boxes and snapshots are backed by and silently re-pointing them at archive-supplied content. Reuse now requires that the local copy already sit on the parent the archive describes; otherwise a private copy is installed. Only freshly installed layers are ever relinked. A layer's digest covered its qcow2 header, which holds its parent's absolute local path. Layers therefore hashed differently on every host — cross-host dedup could never match — and the recorded digest went stale the moment import relinked the file, so re-exporting a box imported with a 3+ layer chain produced an archive only that host could read. Digests now name the canonical form, with backing_file_size and the path string blanked; backing_file_offset is kept, because it locates a reservation within the file that an importer needs in order to write the parent it picked. set_backing_file_path accepts that blanked reservation. Layers installed before a mid-way failure leaked permanently, since nothing collects a base with no dependents, and a layer sat unreferenced between installation and provisioning where a concurrent box rm would GC it. Each layer is now pinned to an import token as it lands; the token transfers to the box on success and collects on failure. A layer's declared format was written but never read, so a mislabelled layer reached relink and surfaced as a rebase error. verify_layer_format checks it against the blob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The image disk is the deepest layer of every chain and usually the largest, and it has no base_disk record, so digest_of returned None and export hashed it in full every single time. It is immutable and its path is derived from its image digest, so a sidecar file next to it is enough. Registering it as a base disk instead would have pulled it into try_gc_base's reach, and the image cache has its own lifecycle. This does not make the image layer dedup across hosts, and it cannot: mke2fs embeds a random filesystem UUID and creation timestamps, so two hosts building the ext4 for the same OCI image produce different bytes. Verified by building twice from one source tree with identical arguments — ceeecb83… vs fe5397e5…. Content addressing can only ever match the image layer within a single host. Skipping that layer entirely, by naming it with its image reference and letting the importer rebuild it the way the guest rootfs already works, is the only thing that would help across hosts — and it trades away the archive being self-contained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An archive is only worth having if it restores into a working box, so a failed guest freeze now abandons the export instead of quietly producing a lesser one. SIGSTOP pauses the vCPUs but leaves the guest's page cache unwritten, so without FIFREEZE the disk is crash-consistent — the equivalent of pulling the power cord. That archive looks exactly like a good one, and nothing in the manifest distinguishes them, which makes it worse than no archive: the failure surfaces at restore time, on data someone was relying on. The freeze was already attempted, but both an RPC error and the timeout only logged a warning and carried on; the `frozen` flag decided nothing beyond whether to thaw. Export now passes QuiescePolicy::RequireFrozen and the bracket refuses before SIGSTOP, so a doomed export costs neither a paused VM nor a copied disk. Clone and snapshot keep BestEffort — their output is a COW fork the caller boots immediately, not an artifact restored months later. The timeout goes from 5s to 30s. FIFREEZE does not fail under write load, it blocks until the filesystem flushes, so 5s turned a merely busy guest into a refusal. Verified with test_export_under_write_pressure, which exports while a background loop writes random 4KiB blocks: it passes with the freeze succeeding, and the refusal path is never reached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G4614
marked this pull request as ready for review
July 30, 2026 13:18
📦 BoxLite review — couldn't completepowered by BoxLite |
G4614
marked this pull request as draft
July 30, 2026 13:19
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
Comment on lines
+104
to
+118
| for base_id in &installed { | ||
| if let Err(e) = runtime | ||
| .base_disk_mgr | ||
| .store() | ||
| .add_ref(base_id, litebox.id().as_ref()) | ||
| { | ||
| tracing::warn!( | ||
| box_id = %litebox.id(), | ||
| base_disk_id = %base_id, | ||
| error = %e, | ||
| "Failed to record base disk ref for imported box" | ||
| ); | ||
| } | ||
| } | ||
| release_import_token(runtime, &token); |
There was a problem hiding this comment.
add_ref(base_id, litebox.id()) errors are only tracing::warn'd (not surfaced), then release_import_token unconditionally drops the same base's token ref; if that was its only ref, try_gc_base deletes the backing file the just-provisioned box's disk chain still depends on, breaking it on next start.
The bottom of every chain is the image's ext4, usually the largest layer, and it is the one layer content addressing can never reuse across hosts: mke2fs writes a random filesystem UUID and creation timestamps, so two hosts building the same image produce different bytes. Measured — two builds from one source tree with identical arguments hash differently. The image digest does match everywhere, being a hash of the OCI layer digests rather than of the built filesystem. Export now records it for whichever layer lives in the image cache, reading it back from the cache filename so export never has to reach a registry. An importer that already holds that image's disk uses its own copy and leaves the archived blob untouched. The archive still carries the blob, so it stays self-contained and an offline import keeps working. Dropping the blob entirely would save the transfer too, but only by making import depend on the image being pullable — a trade to make deliberately, and separately. The reused disk is returned without a base disk id: the image cache owns that file and manages its own lifecycle, so it must not be drawn into base-disk GC. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An `ExportOptions { as_directory: true }` export writes `manifest.json`
beside `layers/{hex}.zst` — one compressed object per layer, named by its
content — instead of one `.boxlite` file. The single file cannot be
backed up incrementally: it is opaque and changes completely between
exports. The directory can, and needs no protocol to do it: a layer two
exports share lands under the same name, so any mirror tool's existence
check (`aws s3 sync`, `mc mirror`, rsync) already skips everything the
destination holds. The sync tool is the negotiation.
Ordering makes an interrupted mirror safe: objects are written under a
temporary name and renamed, the manifest is written last, and a
re-export into the same directory leaves existing objects untouched —
verified by mtime in a_reexport_into_the_same_directory_skips_existing_objects.
Import reads the directory in place: no up-front extraction, each object
unpacked only when the host actually wants that layer. A layer already
held locally is never even opened — proven by handing the importer a
mirror whose already-held object is garbage bytes, which must not and
does not fail (a_layer_the_host_already_holds_is_never_read_from_the_directory).
Python (`ExportOptions(as_directory=True)`) and Node
(`{ asDirectory: true }`) expose the flag. REST refuses it: the wire
format is one HTTP body, and refusing beats silently handing back a
single file to a caller who asked for a mirrorable directory.
Real-VM round trip: export as directory, import, boot — passes as the
suite's tenth test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It verifies an existing capability (an archive is a file any S3 client can move) against a live MinIO, which does not belong in this change's scope. The file stays local; whether it becomes its own PR is a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A store is `archives/<name>.json` manifests over one `layers/` pool —
the layout the user-facing sketch always wanted: N archives sharing
every layer they have in common, so the pool grows by what is new, not
by what is exported. Publishing is `ExportOptions { as_directory: true,
archive_name: Some(..) }` with the store root as destination; importing
is handing the manifest's path to the ordinary import. The manifests are
the references: deleting an archive is deleting its manifest, and
`ArchiveStore::gc` sweeps what no manifest names.
The sweep fails closed. A manifest that cannot be parsed aborts it,
because its references are unknown and anything deleted might be them.
Only content-named `.zst` objects and stale `.partial` staging files are
candidates — a README in the pool directory is not the sweeper's to
take, and a single-archive directory (root `manifest.json`) refuses to
open as a store at all, since its layers would all look orphaned.
Publish races sweep, and the classic failure is a manifest committed
over objects a concurrent gc just deleted — silently, discovered at
restore time. Two mechanisms close it: unreferenced objects younger than
the grace period are never swept (a publish writes objects before its
manifest, so in-flight work looks orphaned), and after its manifest
lands the publisher re-checks every object it named and rewrites any
that a sweep took in the window.
Real-VM round trip: publish into a store, import from the manifest path,
boot, then remove + gc empties the pool — the suite's eleventh test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
11 tasks
Main migrated digest formatting to hex::encode while this branch was in
flight — sha2 0.11's output type no longer implements LowerHex, so the
merge left CanonicalLayer::digest as the one remaining `{:x}` and every
CI clippy job red. Aligned it, gave main's new base-disk test the digest
field this branch added, and satisfied the two lints the workspace-wide
clippy adds over the package-level run this branch had been validated
with: do_export_finalize's dest/as_directory pair becomes an ExportDest
enum (too_many_arguments), and extract_layer_object moves above the test
module (items_after_test_module).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # src/boxlite/src/litebox/clone_export.rs
Thirty seconds was enough for a busy guest but not for a busy host: with four VMs booting in parallel, the running-box export test flaked on a freeze timeout turned refusal, while a lone run passed. An export is not latency-sensitive — the timeout exists to bound a wedged or agentless guest, not to keep a busy one on schedule — so the ceiling doubles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thirty seconds was enough for a busy guest but not for a busy host: with four VMs booting in parallel, the running-box export test flaked on a freeze timeout turned refusal, while a lone run passed. An export is not latency-sensitive — the timeout exists to bound a wedged or agentless guest, not to keep a busy one on schedule — so the ceiling doubles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G4614
marked this pull request as ready for review
July 31, 2026 06:35
Export flattened the container disk's qcow2 chain into one image, so every archive carried a full copy of the image layer even though every box on a host is a COW child of the same one. v6 archives instead carry the chain as `layers/` blobs keyed by sha256, ordered base first, and an importer that already holds a layer skips its transfer entirely. Measured on a real box, a layered archive is the same size as a flattened one (12,578,096 vs 12,564,806 bytes): a short chain has almost no superseded data, and zstd erases the sparse image disk's holes. Export also no longer pays the flatten pass, and base digests are cached in the store so repeat exports do not re-hash immutable layers. Import resolves each layer against the local base store by digest, materializes only what is missing, and relinks children to paths it chose itself. The manifest carries digests and never paths, every blob is verified against its declared digest before anything points at it, and each relink is read back and checked — so a crafted archive still cannot aim a backing file at a host path of its choosing. Imported bases are ref-counted against the new box so the GC does not drop a layer the box reads through. Qcow2Helper::flatten keeps no caller but is retained: MAX_BACKING_CHAIN_DEPTH caps a chain at 8, and collapsing a chain is the compaction step that keeps clone-heavy lineages under that cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Inserting LayerFormat and ArchiveLayer split the manifest's version-history doc comment away from the struct, leaving it dangling — clippy's empty_line_after_doc_comments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five defects found reviewing the layered archive path, two of them exploitable. The deepest layer was never relinked nor validated, so its header's backing path — attacker-controlled data — survived verbatim into bases/. The chain is granted to the sandbox at start, so an archive could name any host file and have its bytes handed to the guest. That layer now goes through validate_no_backing_references. A layer already held locally was relinked to satisfy the incoming archive, rewriting a file other boxes and snapshots are backed by and silently re-pointing them at archive-supplied content. Reuse now requires that the local copy already sit on the parent the archive describes; otherwise a private copy is installed. Only freshly installed layers are ever relinked. A layer's digest covered its qcow2 header, which holds its parent's absolute local path. Layers therefore hashed differently on every host — cross-host dedup could never match — and the recorded digest went stale the moment import relinked the file, so re-exporting a box imported with a 3+ layer chain produced an archive only that host could read. Digests now name the canonical form, with backing_file_size and the path string blanked; backing_file_offset is kept, because it locates a reservation within the file that an importer needs in order to write the parent it picked. set_backing_file_path accepts that blanked reservation. Layers installed before a mid-way failure leaked permanently, since nothing collects a base with no dependents, and a layer sat unreferenced between installation and provisioning where a concurrent box rm would GC it. Each layer is now pinned to an import token as it lands; the token transfers to the box on success and collects on failure. A layer's declared format was written but never read, so a mislabelled layer reached relink and surfaced as a rebase error. verify_layer_format checks it against the blob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The image disk is the deepest layer of every chain and usually the largest, and it has no base_disk record, so digest_of returned None and export hashed it in full every single time. It is immutable and its path is derived from its image digest, so a sidecar file next to it is enough. Registering it as a base disk instead would have pulled it into try_gc_base's reach, and the image cache has its own lifecycle. This does not make the image layer dedup across hosts, and it cannot: mke2fs embeds a random filesystem UUID and creation timestamps, so two hosts building the ext4 for the same OCI image produce different bytes. Verified by building twice from one source tree with identical arguments — ceeecb83… vs fe5397e5…. Content addressing can only ever match the image layer within a single host. Skipping that layer entirely, by naming it with its image reference and letting the importer rebuild it the way the guest rootfs already works, is the only thing that would help across hosts — and it trades away the archive being self-contained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An archive is only worth having if it restores into a working box, so a failed guest freeze now abandons the export instead of quietly producing a lesser one. SIGSTOP pauses the vCPUs but leaves the guest's page cache unwritten, so without FIFREEZE the disk is crash-consistent — the equivalent of pulling the power cord. That archive looks exactly like a good one, and nothing in the manifest distinguishes them, which makes it worse than no archive: the failure surfaces at restore time, on data someone was relying on. The freeze was already attempted, but both an RPC error and the timeout only logged a warning and carried on; the `frozen` flag decided nothing beyond whether to thaw. Export now passes QuiescePolicy::RequireFrozen and the bracket refuses before SIGSTOP, so a doomed export costs neither a paused VM nor a copied disk. Clone and snapshot keep BestEffort — their output is a COW fork the caller boots immediately, not an artifact restored months later. The timeout goes from 5s to 30s. FIFREEZE does not fail under write load, it blocks until the filesystem flushes, so 5s turned a merely busy guest into a refusal. Verified with test_export_under_write_pressure, which exports while a background loop writes random 4KiB blocks: it passes with the freeze succeeding, and the refusal path is never reached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bottom of every chain is the image's ext4, usually the largest layer, and it is the one layer content addressing can never reuse across hosts: mke2fs writes a random filesystem UUID and creation timestamps, so two hosts building the same image produce different bytes. Measured — two builds from one source tree with identical arguments hash differently. The image digest does match everywhere, being a hash of the OCI layer digests rather than of the built filesystem. Export now records it for whichever layer lives in the image cache, reading it back from the cache filename so export never has to reach a registry. An importer that already holds that image's disk uses its own copy and leaves the archived blob untouched. The archive still carries the blob, so it stays self-contained and an offline import keeps working. Dropping the blob entirely would save the transfer too, but only by making import depend on the image being pullable — a trade to make deliberately, and separately. The reused disk is returned without a base disk id: the image cache owns that file and manages its own lifecycle, so it must not be drawn into base-disk GC. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An `ExportOptions { as_directory: true }` export writes `manifest.json`
beside `layers/{hex}.zst` — one compressed object per layer, named by its
content — instead of one `.boxlite` file. The single file cannot be
backed up incrementally: it is opaque and changes completely between
exports. The directory can, and needs no protocol to do it: a layer two
exports share lands under the same name, so any mirror tool's existence
check (`aws s3 sync`, `mc mirror`, rsync) already skips everything the
destination holds. The sync tool is the negotiation.
Ordering makes an interrupted mirror safe: objects are written under a
temporary name and renamed, the manifest is written last, and a
re-export into the same directory leaves existing objects untouched —
verified by mtime in a_reexport_into_the_same_directory_skips_existing_objects.
Import reads the directory in place: no up-front extraction, each object
unpacked only when the host actually wants that layer. A layer already
held locally is never even opened — proven by handing the importer a
mirror whose already-held object is garbage bytes, which must not and
does not fail (a_layer_the_host_already_holds_is_never_read_from_the_directory).
Python (`ExportOptions(as_directory=True)`) and Node
(`{ asDirectory: true }`) expose the flag. REST refuses it: the wire
format is one HTTP body, and refusing beats silently handing back a
single file to a caller who asked for a mirrorable directory.
Real-VM round trip: export as directory, import, boot — passes as the
suite's tenth test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It verifies an existing capability (an archive is a file any S3 client can move) against a live MinIO, which does not belong in this change's scope. The file stays local; whether it becomes its own PR is a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Main migrated digest formatting to hex::encode while this branch was in
flight — sha2 0.11's output type no longer implements LowerHex, so the
merge left CanonicalLayer::digest as the one remaining `{:x}` and every
CI clippy job red. Aligned it, gave main's new base-disk test the digest
field this branch added, and satisfied the two lints the workspace-wide
clippy adds over the package-level run this branch had been validated
with: do_export_finalize's dest/as_directory pair becomes an ExportDest
enum (too_many_arguments), and extract_layer_object moves above the test
module (items_after_test_module).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thirty seconds was enough for a busy guest but not for a busy host: with four VMs booting in parallel, the running-box export test flaked on a freeze timeout turned refusal, while a lone run passed. An export is not latency-sensitive — the timeout exists to bound a wedged or agentless guest, not to keep a busy one on schedule — so the ceiling doubles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
G4614
force-pushed
the
feat/layered-archive
branch
from
July 31, 2026 07:34
8fd2fe6 to
69a4e6f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/boxlite/src/runtime/import.rs (1)
625-627: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
#[cfg(test)]attribute.Line 625 and Line 626 both apply
#[cfg(test)]tolayered_install_tests.🧹 Proposed fix
#[cfg(test)] -#[cfg(test)] mod layered_install_tests {🤖 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/import.rs` around lines 625 - 627, Remove the duplicated #[cfg(test)] attribute immediately before the layered_install_tests module, leaving a single conditional-compilation attribute for the test module.
🤖 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.
Inline comments:
In `@src/boxlite/src/runtime/import.rs`:
- Around line 406-428: Validate every layer digest using the existing
image-digest validation logic during manifest validation, before any layer
consumer can use it; also validate the digest at the start of resolve_layer
before find_by_digest. Ensure malformed values, including non-SHA-256 or
incorrect-length strings and path traversal, are rejected before
layer_entry_name, filesystem access, or reuse lookup.
- Around line 667-703: Update the test
a_failed_box_ref_keeps_the_import_token_refs to stage and install at least two
layers, ensuring install_layers produces a non-empty installed collection with a
base disk and token reference. Keep the trigger, handoff_import_refs call, and
assertions unchanged so the failure path is actually exercised.
---
Nitpick comments:
In `@src/boxlite/src/runtime/import.rs`:
- Around line 625-627: Remove the duplicated #[cfg(test)] attribute immediately
before the layered_install_tests module, leaving a single
conditional-compilation attribute for the test module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3fa6de3-bd74-47a6-a67d-038fd2948c35
📒 Files selected for processing (20)
sdks/node/lib/native-contracts.tssdks/node/src/snapshot_options.rssdks/python/src/snapshot_options.rssrc/boxlite/src/db/base_disk.rssrc/boxlite/src/db/migration/mod.rssrc/boxlite/src/db/migration/v6_to_v7.rssrc/boxlite/src/db/migration/v9_to_v10.rssrc/boxlite/src/db/schema.rssrc/boxlite/src/disk/base_disk.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/disk/qcow2.rssrc/boxlite/src/litebox/archive.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/clone_export.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/import.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/tests/clone_export_import.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- sdks/node/lib/native-contracts.ts
- src/boxlite/src/disk/mod.rs
- src/boxlite/src/rootfs/guest.rs
- src/boxlite/src/db/migration/v9_to_v10.rs
- src/boxlite/src/db/schema.rs
- src/boxlite/src/db/base_disk.rs
- sdks/python/src/snapshot_options.rs
- src/boxlite/src/db/migration/v6_to_v7.rs
- src/boxlite/src/db/migration/mod.rs
- src/boxlite/src/disk/base_disk.rs
- src/boxlite/src/litebox/box_impl.rs
- src/boxlite/src/litebox/archive.rs
- src/boxlite/src/litebox/clone_export.rs
- sdks/node/src/snapshot_options.rs
- src/boxlite/src/disk/qcow2.rs
- src/boxlite/src/runtime/options.rs
- src/boxlite/src/rest/litebox.rs
- src/boxlite/src/runtime/rt_impl.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/boxlite/src/disk/base_disk.rs (2)
452-465: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the
digest_ofcontract comment with its return value.The comment says unregistered paths return
None, but Line 465 returnsSomefromsidecar_digest. Document that unregistered image and rootfs paths can return a sidecar-backed digest.🤖 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 452 - 465, Update the documentation for digest_of to state that unregistered image backing-file and raw-rootfs paths may return a sidecar-backed digest, while retaining the existing None behavior when no digest is available. Keep the implementation unchanged, including the sidecar_digest call in the unregistered-path branch.
423-440: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFilter digest reuse to
CloneBaserecords.
BaseDiskStore::find_by_digestmatches onlydigestand returns the existing record kind, so aCloneBaseinstallation can reuse a matchingSnapshotorRootfsrecord. Limit reuse to compatibleCloneBaserecords beforeadd_ref; otherwise import can attach a container layer to a user-owned snapshot or cache-backed rootfs.🤖 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 423 - 440, Update the digest lookup and reuse flow in BaseDiskStore::find_by_digest so only records with BaseDiskKind::CloneBase are eligible for reuse before add_ref. Preserve the existing behavior for matching CloneBase records, while ensuring Snapshot and Rootfs records are excluded from import reuse.
🤖 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.
Outside diff comments:
In `@src/boxlite/src/disk/base_disk.rs`:
- Around line 452-465: Update the documentation for digest_of to state that
unregistered image backing-file and raw-rootfs paths may return a sidecar-backed
digest, while retaining the existing None behavior when no digest is available.
Keep the implementation unchanged, including the sidecar_digest call in the
unregistered-path branch.
- Around line 423-440: Update the digest lookup and reuse flow in
BaseDiskStore::find_by_digest so only records with BaseDiskKind::CloneBase are
eligible for reuse before add_ref. Preserve the existing behavior for matching
CloneBase records, while ensuring Snapshot and Rootfs records are excluded from
import reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6baadfd-e150-4b44-b41b-7b7f3356a7fd
📒 Files selected for processing (2)
src/boxlite/src/disk/base_disk.rssrc/boxlite/src/runtime/import.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/boxlite/src/runtime/import.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/boxlite/src/runtime/import.rs (1)
660-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
#[cfg(test)].The attribute appears twice on
layered_install_tests. The second one has no effect.🧹 Proposed cleanup
-#[cfg(test)] #[cfg(test)] mod layered_install_tests {🤖 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/import.rs` around lines 660 - 661, Remove the duplicated #[cfg(test)] attribute above layered_install_tests, leaving a single test-configuration attribute and preserving the test module or function unchanged.
🤖 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/runtime/import.rs`:
- Around line 660-661: Remove the duplicated #[cfg(test)] attribute above
layered_install_tests, leaving a single test-configuration attribute and
preserving the test module or function unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7d1a434-40a8-40fc-a7ba-177e7a9fb5c2
📒 Files selected for processing (20)
sdks/node/lib/native-contracts.tssdks/node/src/snapshot_options.rssdks/python/src/snapshot_options.rssrc/boxlite/src/db/base_disk.rssrc/boxlite/src/db/migration/mod.rssrc/boxlite/src/db/migration/v6_to_v7.rssrc/boxlite/src/db/migration/v9_to_v10.rssrc/boxlite/src/db/schema.rssrc/boxlite/src/disk/base_disk.rssrc/boxlite/src/disk/mod.rssrc/boxlite/src/disk/qcow2.rssrc/boxlite/src/litebox/archive.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/clone_export.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/import.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/tests/clone_export_import.rs
🚧 Files skipped from review as they are similar to previous changes (19)
- src/boxlite/tests/clone_export_import.rs
- sdks/node/lib/native-contracts.ts
- sdks/node/src/snapshot_options.rs
- src/boxlite/src/db/schema.rs
- src/boxlite/src/db/migration/mod.rs
- src/boxlite/src/db/migration/v9_to_v10.rs
- src/boxlite/src/rootfs/guest.rs
- src/boxlite/src/runtime/options.rs
- src/boxlite/src/db/migration/v6_to_v7.rs
- sdks/python/src/snapshot_options.rs
- src/boxlite/src/rest/litebox.rs
- src/boxlite/src/db/base_disk.rs
- src/boxlite/src/disk/mod.rs
- src/boxlite/src/litebox/box_impl.rs
- src/boxlite/src/litebox/archive.rs
- src/boxlite/src/disk/qcow2.rs
- src/boxlite/src/disk/base_disk.rs
- src/boxlite/src/runtime/rt_impl.rs
- src/boxlite/src/litebox/clone_export.rs
# Conflicts: # sdks/node/lib/native-contracts.ts # sdks/node/src/snapshot_options.rs # sdks/python/src/snapshot_options.rs # src/boxlite/src/disk/base_disk.rs # src/boxlite/src/disk/qcow2.rs # src/boxlite/src/litebox/archive.rs # src/boxlite/src/litebox/box_impl.rs # src/boxlite/src/litebox/clone_export.rs # src/boxlite/src/rest/litebox.rs # src/boxlite/src/runtime/import.rs # src/boxlite/src/runtime/options.rs # src/boxlite/tests/clone_export_import.rs
G4614
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/litebox/archive.rs (1)
308-329: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGive each writer a unique staging name.
The staging path depends only on the digest. Two exports that publish the same layer into one directory or store therefore share
layers/<hex>.zst.partial.File::createtruncates the file that the other writer still holds, so the surviving writer can rename a zero-padded staging file onto the content-addressed name. The bad object then persists, because theobject.exists()fast path stops any later export from rewriting it.Use a per-writer suffix, for example a UUID or the process id.
gcalready sweeps stale.partialfiles, so abandoned staging files are still reclaimed. Apply the same treatment to the.{name}.json.partialmanifest staging path inbuild_store_archive.🐛 Proposed fix
- let staging = object.with_extension("zst.partial"); + let staging = object.with_extension(format!("zst.{}.partial", uuid::Uuid::new_v4()));🤖 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/litebox/archive.rs` around lines 308 - 329, Update the staging paths in the layer export loop and build_store_archive so each writer uses a unique suffix, such as a UUID or process ID, rather than deriving the path solely from the digest or manifest name. Preserve the final content-addressed object and manifest paths, and ensure both layer .zst.partial files and .json.partial manifest files use the per-writer staging name before atomic publication.
🧹 Nitpick comments (5)
src/boxlite/tests/minio_backup_roundtrip.rs (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a unique object key per run.
The key is fixed, and the bucket is shared. Two runs against the same MinIO overwrite one object, so a run can download another run's archive or observe the key already deleted. Add a unique component, for example a UUID or the box name plus a timestamp.
🤖 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/tests/minio_backup_roundtrip.rs` at line 94, Update the object key construction in the MinIO roundtrip test to include a unique per-run component, such as a generated UUID or timestamp combined with the box name. Preserve the existing bucket and archive suffix while ensuring concurrent or repeated runs never reuse the same key.src/boxlite/src/litebox/archive.rs (1)
273-284: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPublish the directory manifest by rename.
build_layered_directorywritesmanifest.jsonin place. A concurrent reader, a mirroring tool, or an import that runs during a re-export into the same directory can read a truncated manifest.build_store_archivealready avoids this with a staging file plusmove_file. Use the same sequence here.♻️ Proposed change
let manifest_path = output_dir.join(MANIFEST_FILENAME); - std::fs::write(&manifest_path, manifest_json).map_err(|e| { + let staging = output_dir.join(".manifest.json.partial"); + std::fs::write(&staging, manifest_json).map_err(|e| { BoxliteError::Storage(format!( "Failed to write {}: {}", - manifest_path.display(), + staging.display(), e )) })?; + move_file(&staging, &manifest_path)?;🤖 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/litebox/archive.rs` around lines 273 - 284, Update the manifest-writing flow in build_layered_directory to write manifest.json to a staging file first, then publish it by renaming the staging file with the existing move_file helper, matching build_store_archive. Preserve the current manifest path and BoxliteError::Storage error context while ensuring readers only observe the completed manifest.sdks/node/src/snapshot_options.rs (2)
62-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert propagation of the new export fields.
The test supplies
Nonefor both fields and discards the converted values. It cannot detect a droppedarchive_name, an incorrectas_directorydefault, or a mismatch in the option dependency. Add a valid directory-store case and assert both converted fields. Add invalid-combination coverage after the invariant is defined.🤖 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 `@sdks/node/src/snapshot_options.rs` around lines 62 - 65, Update the test around JsExportOptions to use a valid directory-store configuration, assert both converted as_directory and archive_name values, and verify the option dependency is preserved. After defining the invariant, add coverage for invalid field combinations so conversion rejects or handles them as specified.
25-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse one enforced contract for
archiveNameandasDirectory.Both public boundaries allow the fields independently. The Rust conversion defaults
as_directorytofalsewhile retainingarchive_name. Decide whetherarchiveNamerequiresasDirectory: trueor implies directory export, then enforce that rule consistently.
sdks/node/src/snapshot_options.rs#L25-L35: validate the combination before constructingExportOptions, or update the native contract if name-only store selection is intentional.sdks/node/lib/native-contracts.ts#L363-L375: encode the dependency in the TypeScript shape or update its documentation to match native behavior.Verify the downstream rule with:
#!/bin/bash set -euo pipefail rg -n -C 8 'archive_name|as_directory|ExportDest::Store|ExportOptions|validate|TryFrom' \ src/boxlite/src sdks/node/src sdks/node/lib🤖 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 `@sdks/node/src/snapshot_options.rs` around lines 25 - 35, The archiveName/asDirectory contract is inconsistent across the public boundaries. In sdks/node/src/snapshot_options.rs lines 25-35, inspect downstream ExportOptions handling and enforce the chosen rule before constructing ExportOptions—either require asDirectory when archiveName is set or make archiveName imply directory export. In sdks/node/lib/native-contracts.ts lines 363-375, encode the same dependency in the TypeScript contract or document the intentional native behavior; keep both boundaries consistent.sdks/node/lib/native-contracts.ts (1)
363-375: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the REST capability boundary.
JsBoxlite.rest()returns the sameJsBoxinterface, but this cohort rejects directory exports through REST. The new descriptions do not state this limitation. Add a runtime-specific note forasDirectoryandarchiveNameso remote callers know these options are unsupported.🤖 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 `@sdks/node/lib/native-contracts.ts` around lines 363 - 375, Update the documentation comments for JsExportOptions.asDirectory and archiveName to explicitly state that these options are unsupported when using JsBoxlite.rest() or REST callers. Preserve the existing descriptions and clarify that the limitation applies to the REST runtime only.
🤖 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.
Inline comments:
In `@src/boxlite/tests/minio_backup_roundtrip.rs`:
- Around line 48-59: Update the sha256 helper to use an available SHA-256 digest
command, preferring the platform-supported tool such as sha256sum when shasum is
unavailable. Check the command exit status and require a non-empty parsed
digest, failing the test rather than returning an empty string when execution or
output validation fails.
---
Outside diff comments:
In `@src/boxlite/src/litebox/archive.rs`:
- Around line 308-329: Update the staging paths in the layer export loop and
build_store_archive so each writer uses a unique suffix, such as a UUID or
process ID, rather than deriving the path solely from the digest or manifest
name. Preserve the final content-addressed object and manifest paths, and ensure
both layer .zst.partial files and .json.partial manifest files use the
per-writer staging name before atomic publication.
---
Nitpick comments:
In `@sdks/node/lib/native-contracts.ts`:
- Around line 363-375: Update the documentation comments for
JsExportOptions.asDirectory and archiveName to explicitly state that these
options are unsupported when using JsBoxlite.rest() or REST callers. Preserve
the existing descriptions and clarify that the limitation applies to the REST
runtime only.
In `@sdks/node/src/snapshot_options.rs`:
- Around line 62-65: Update the test around JsExportOptions to use a valid
directory-store configuration, assert both converted as_directory and
archive_name values, and verify the option dependency is preserved. After
defining the invariant, add coverage for invalid field combinations so
conversion rejects or handles them as specified.
- Around line 25-35: The archiveName/asDirectory contract is inconsistent across
the public boundaries. In sdks/node/src/snapshot_options.rs lines 25-35, inspect
downstream ExportOptions handling and enforce the chosen rule before
constructing ExportOptions—either require asDirectory when archiveName is set or
make archiveName imply directory export. In sdks/node/lib/native-contracts.ts
lines 363-375, encode the same dependency in the TypeScript contract or document
the intentional native behavior; keep both boundaries consistent.
In `@src/boxlite/src/litebox/archive.rs`:
- Around line 273-284: Update the manifest-writing flow in
build_layered_directory to write manifest.json to a staging file first, then
publish it by renaming the staging file with the existing move_file helper,
matching build_store_archive. Preserve the current manifest path and
BoxliteError::Storage error context while ensuring readers only observe the
completed manifest.
In `@src/boxlite/tests/minio_backup_roundtrip.rs`:
- Line 94: Update the object key construction in the MinIO roundtrip test to
include a unique per-run component, such as a generated UUID or timestamp
combined with the box name. Preserve the existing bucket and archive suffix
while ensuring concurrent or repeated runs never reuse the same key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2288fb9-e22a-4e53-b04a-e722d8e8d50d
📒 Files selected for processing (13)
sdks/node/lib/native-contracts.tssdks/node/src/snapshot_options.rssdks/python/src/snapshot_options.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/archive.rssrc/boxlite/src/litebox/archive_store.rssrc/boxlite/src/litebox/clone_export.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/runtime/import.rssrc/boxlite/src/runtime/options.rssrc/boxlite/tests/clone_export_import.rssrc/boxlite/tests/minio_backup_roundtrip.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/boxlite/src/rest/litebox.rs
- src/boxlite/src/runtime/options.rs
- src/boxlite/src/litebox/clone_export.rs
- src/boxlite/src/runtime/import.rs
- sdks/python/src/snapshot_options.rs
G4614
changed the title
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.
Box archives now export and import the container disk as content-addressed layers with single-file or mirrorable directory output, export metadata, and C/Go SDK bindings for the new primitives.
Test plan:
cargo fmt --all -- --checkcargo check -p boxlite --no-default-featuresBOXLITE_DEPS_STUB=1 cargo test -p boxlite --no-default-features --lib— 994/994 passedBOXLITE_DEPS_STUB=1 cargo clippy -p boxlite -p boxlite-c --no-default-features --all-targets -- -D warningsBOXLITE_DEPS_STUB=1 cargo test -p boxlite-c --no-default-featuresgo test ./...fromsdks/gousing a locally copied stublibboxlite.agit diff --checkcargo nextest run -p boxlite --features krun --test clone_export_import --profile vmnot rerun after narrowing scope; needs Linux/native validation