chore: refactor rootfs direct provider and add checksums #1543
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
dobrac
force-pushed
the
fix/rootfs-direct-provider
branch
6 times, most recently
from
7fc7ee9 to
6110ab9
Compare
dobrac
changed the title
dobrac
force-pushed
the
fix/rootfs-direct-provider
branch
from
6110ab9 to
5f6a4fa
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // We will use this to handle base builds that are already diffs. | ||
| // The passed slice p must start as empty, otherwise we would need to copy the empty values there. | ||
| if *buildID == uuid.Nil { | ||
| isEmpty, err := header.IsEmptyBlock(p[n:int64(n)+readLength], int64(b.header.Metadata.BlockSize)) |
There was a problem hiding this comment.
this might not be needed, it's just check to be sure that the block is empty
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| if !isEmpty { | ||
| return 0, fmt.Errorf("block is not empty") | ||
| } |
There was a problem hiding this comment.
Bug: Empty block check fails for non-blockSize reads
The IsEmptyBlock function compares the input slice against a pre-allocated empty buffer of exactly blockSize bytes using bytes.Equal. However, the slice passed here has length readLength (from p[n:int64(n)+readLength]), which can differ from blockSize when reads span multiple mappings or aren't block-aligned. When readLength != blockSize, bytes.Equal will return false due to the length mismatch, incorrectly reporting non-empty blocks and causing spurious errors even when the slice contains all zeros.
| if wrongCount > 10 { | ||
| return fmt.Errorf("too many block checksum mismatches") | ||
| } |
| } | ||
| } | ||
|
|
||
| if !bytes.Equal(checksums.Checksum[:], headerChecksums.Checksum[:]) { |
There was a problem hiding this comment.
do you even need to do this if you do block checksums check?
| m, err := builder.Build() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error building diff metadata: %w", err) | ||
| } | ||
|
|
||
| return m, nil | ||
| } |
There was a problem hiding this comment.
You can do the same as in the Cache, also Build() can't return error, so that can be also simplified
| m, err := builder.Build() | |
| if err != nil { | |
| return nil, fmt.Errorf("error building diff metadata: %w", err) | |
| } | |
| return m, nil | |
| } | |
| return builder.Build() | |
| } |
Note
Introduce BLAKE3-based rootfs checksums with optional verification, refactor direct/NBD providers and cache export to a unified diff metadata builder, and wire verification into sandbox lifecycle.
Provider.Verifyand wire into sandbox create/resume; pass full config to providers.DirectProviderto mmap the rootfs file, stream diff viaheader.NewDiffMetadataBuilder, and optionally attach computed checksums.NBDProviderto compute and optionally verify BLAKE3 checksums (per-block and aggregate); include checksums in exported diffs.CalculateChecksumsReader).header.Headerandheader.DiffMetadatawithChecksums; propagate throughToDiffHeader.DiffMetadataBuilderinExportToDiff; removeMarkAllAsDirty.phases.Run; simplify base phase by removing provision symlink.ROOTFS_CHECKSUM_VERIFICATIONflag to config.Written by Cursor Bugbot for commit 160574d. This will update automatically on new commits. Configure here.