fix(s3): derive the delta upload's mtime instead of asking for it - #756
Merged
Conversation
`upload_delta_multipart` took `source_mtime` as a parameter while `upload_multipart_streaming`, one screen away, derives it from the local path. The executor already holds `local_path`, so the parameter existed only as a way to forget: a caller passing `None` leaves the rebuilt object without the mtime the original carried, silently, and from then on every sync that compares timestamps never finds it equal and re-uploads the whole file. That is the opposite of what a delta upload is for. The caller that would have had to remember does not exist yet. The adapter arm is the next tranche, which is exactly when this would have been found, on a real transfer, by a user whose sync stopped converging. Raised by the G Lane while checking whether their S3 work collided with this one. Nothing in sixteen green checks could have shown it: the code compiles either way, and no test asked what reached the wire. So the test asks. `a_delta_upload_carries_the_source_mtime_like_an_ordinary_one` asserts that `x-amz-meta-mtime` arrived on the CreateMultipartUpload with the source file's own value, not that a helper can compute one. Seen failing on a version that passes `None`, which is the shape the parameter allowed. Two limits from the appendix are now written down in the executor rather than left as silences. The rule that a ranged copy needs a source above 5 MB has no check because it cannot be reached: a baseline under 5 MB cannot produce a match as wide as the grid, so the planner refuses before an UploadPartCopy exists, and the argument is the reason for the absence rather than an excuse for it. And the appendix's request to verify the result with a full-object digest is not an omission to fill but a guardrail to restate: a multipart's object-level checksum is COMPOSITE, a hash of the part hashes with an `-N` suffix, the same shape the appendix already documents for the multipart ETag, and it is not comparable to a digest over the local file. A whole-object digest needs the full-object checksum type, which S3 offers for the CRC families and not for SHA256, unevenly across compatible backends. Measured alongside: on MinIO and R2 a multipart object is not hash-verifiable by a third-party client today. Signed-off-by: axpnet <45786925+axpnet@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughDelta multipart uploads now derive source mtime metadata from the local file. Mock S3 state captures the metadata, and tests cover metadata preservation, refusal behavior, failure cleanup, and live round trips. ChangesDelta multipart metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Delta multipart uploads now preserve the local file modification time in S3 metadata, preventing unnecessary later timestamp-based reuploads. The updated metadata and failure-path coverage shows no current merge-readiness risk. 🚥 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 |
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.
The defect
upload_delta_multiparttooksource_mtimeas a parameter.upload_multipart_streaming, one screen away in the same file, derives it from the local path withSelf::source_mtime_metadata(local_path).The executor already holds
local_path, since that is where it reads the PUT parts from, so the parameter existed only as a way to forget. A caller passingNoneleaves the rebuilt object without thex-amz-meta-mtimethe original carried, silently. From then on every sync that compares timestamps never finds that object equal and re-uploads the whole file, which is the exact outcome a delta upload exists to avoid.The caller that would have had to remember does not exist yet. The adapter arm is the next tranche, which is when this would have been found: on a real transfer, by a user whose sync stopped converging.
It came from the G Lane, while they were checking whether their own S3 work collided with #755. Nothing in that pull request's sixteen green checks could have shown it. The code compiles with the parameter,
create_multipart_uploadacceptsNoneby design, and no test asked what reached the wire.The test asks what reached the wire
a_delta_upload_carries_the_source_mtime_like_an_ordinary_oneasserts thatx-amz-meta-mtimearrived on theCreateMultipartUpload, carrying the source file's own value. Not that a helper can compute one, which is the version that would have passed either way.The mock backend now records the header on each create, so the absence is observable rather than inferred. Seen failing on a version where the executor passes
None, which is precisely the shape the parameter allowed:Two limits, written down instead of left silent
Both come from the appendix's constraint sheet, and neither is enforced in code. That is now stated in the executor with its reasoning, so the next reader does not have to decide whether it is an oversight.
A ranged copy needs a source object above 5 MB. There is no check because there is no way to reach the case: a baseline under 5 MB cannot produce a match as wide as the grid, which is at least 8 MiB, so the planner demotes every copy run and refuses the plan before a single
UploadPartCopyis built. The argument is the reason for the absence, not an excuse for it."Verify the result, do not assume it" is a guardrail to restate, not an omission to fill. The appendix asks for the multipart to be created with a checksum algorithm so S3 returns a full-object digest comparable against the local file. For a multipart the object-level checksum is COMPOSITE: a hash of the part hashes with an
-Nsuffix, which is the same shape the appendix itself already documents for the multipart ETag in04(b), and it is not comparable to a digest computed over the local file. A whole-object digest needs the full-object checksum type, which S3 offers for the CRC families and not for SHA256, with uneven support across S3-compatible backends.Measured alongside this, by the test station's rclone comparison bench on 300 MiB server-side copies:
with their own caveat attached: the bench asks for sha256 and rclone's S3 backend normally exposes the ETag, so that is partly a limit of the reading side. It is not proof that
x-amz-checksum-algorithmfails; it is evidence that a multipart object is not hash-verifiable by a third-party client today on either backend, which is why deferring closes nothing that currently works.On the yardstick, since the numbers will be quoted
The delta's saving should be cited against our own rsync path, not against rclone. rclone has no delta on S3 and resends the whole object, so "16 MiB against 1 GiB" measures that the other tool does not play, not how well this one does. The informative comparison is the wire ratio our native rsync achieves on the same file and the same edit, because that is what prices the aligned grid: near parity on an append, roughly 60 percent more wire on a middle edit that straddles two cells, and everything against nothing on an insertion at offset zero, which is Tier 1's declared blind spot. That gap is the field data the deferred Tier 2 decision rests on.
Gate
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings, andcargo test --lib s3with 180 tests passing and 3 ignored (the env-gated live lane), run on the branch merged overmainatbf15c887c.Summary by CodeRabbit