feat(s3): delta multipart executor with copy-source if-match guard (T3) - #755
Merged
Conversation
T3 of APPENDIX-S3-DELTA-UPLOAD, on top of the T1/T2 planner in providers/s3_delta_plan.rs: - upload_part_copy_internal gains if_match, sent as x-amz-copy-source-if-match (quotes restored at the send site; stat strips them). The plain server-side copy passes None and keeps its historical behaviour. - upload_delta_multipart, a sibling of server_side_copy_multipart: the plan is computed at the door (a refusal sends no request at all), JoinSet fan-out at effective_upload_concurrency(), copy parts pinned to the baseline ETag, put parts read from the local file, best-effort abort on every error path, completion via complete_multipart_upload_internal unchanged. - The executor retries CreateMultipartUpload once on NetworkError: the live MinIO lane caught hyper IncompleteMessage on a stale pooled keep-alive connection right after the baseline traffic. - Mocked-HTTP unit tests assert the wire, not the planner; env-gated ignored live lane (AEROFTP_S3_DELTA_LIVE_*) covers middle edit, append, and the insertion-at-0 blind spot refusal against MinIO. Signed-off-by: axpnet <45786925+axpnet@users.noreply.github.com>
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
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 |
✅ 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. |
This was referenced Sep 8, 2026
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
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.
What
T3 of the S3 delta upload appendix (
[OPEN]-APPENDIX-S3-DELTA-UPLOAD), on top of the T1/T2 planner inproviders/s3_delta_plan.rs. Two things, nothing else:upload_part_copy_internalgainsif_match: Option<&str>, sent asx-amz-copy-source-if-match. The header did not exist anywhere ins3.rsbefore. The existing caller (server_side_copy_multipart) passesNoneand keeps its historical behaviour. The header needs the quoted wire form; a HEAD response carries the quotes,statstrips them, so both forms are accepted and the quotes are restored at the send site.upload_delta_multipart, a sibling ofserver_side_copy_multipart: JoinSet fan-out ateffective_upload_concurrency(), ETag collection, sort by part number, best-effort abort on every error path.DeltaPart::Copydispatchesupload_part_copy_internalpinned to the baseline ETag;DeltaPart::Putreads its byte range from the local file and dispatchesupload_part_internal. Completion goes throughcomplete_multipart_upload_internalunchanged.One deliberate deviation from the sketch in
07-executive-plan.md: the plan is computed at the door (fromtotal_size,matches,grid) rather than received as a&[DeltaPart]. A caller holding a refused plan must not be able to spend a single request on it, and computing inside the executor is what makes the door testable: a refusal returns before evenCreateMultipartUploadis sent.Not included, deliberately: the baseline digest cache, the S3 arm in
try_delta_transfer_with_progress, the per-endpoint ranged-copy probe cache. Those are T4.Why the guard is not optional
x-amz-copy-sourceis resolved at each UploadPartCopy call. A concurrent overwrite of the baseline between two parts would silently assemble an object from two versions. Pinning every copy part withx-amz-copy-source-if-matchturns that race into412 Precondition Failed(06 R1). A 412 is a fallback signal for the caller, not a hard error; the mapping lives in T4, the executor surfaces the status in the error text.Return contract
Ok(Some(wire_bytes)): success; wire bytes are the PUT parts only (COPY parts are requests, not bytes), for the caller's wire ratio.Ok(None): the planner refused the shape. The caller falls back to the ordinary upload; no request has been sent at all.Err(_): the multipart was aborted best-effort first. A 412 arrives on this path.Test evidence (fail-first)
Unit, mocked HTTP (axum), asserting what the executor DID on the wire, never only what the planner decided:
delta_refused_plan_sends_no_requests_at_all: the door, not the guard. A refused plan (empty match list, and an overlapping one) costs zero requests, CreateMultipartUpload included.delta_copy_parts_carry_if_match_and_only_changed_bytes_travel: the copy part carries the exactx-amz-copy-source-rangeand the quoted baseline ETag; the put part carries exactly the edited bytes; the CompleteMultipartUpload body lists parts sorted (the mock holds the copy response until the put has arrived, so an unsorted body is deterministic, not a timing coin toss); progress reaches exactly the wire total.delta_412_from_the_guard_aborts_and_never_completes: 412 surfaces with the status in the message, the upload is aborted, CompleteMultipartUpload never runs.delta_short_local_file_aborts_instead_of_completing: a local file shorter than the planned put range fails the read and aborts.Mutation battery, one broken rule at a time, each run to red first:
delta_copy_parts_carry_if_match_and_only_changed_bytes_traveldelta_refused_plan_sends_no_requests_at_alldelta_412_from_the_guard_aborts_and_never_completesdelta_copy_parts_...(deterministic ordering mock)delta_copy_parts_...delta_copy_parts_...delta_copy_parts_...Measured: 7, caught: 7, not measured: 3 (the JoinError panic arm; the pre-existing 200-with-error XML path in
upload_part_copy_internal; the create-retry branch, which needs a fault-injecting server).M4 is worth a line of its own: its first version stayed green because the ordering relied on a 250 ms sleep, a coin toss under load. The mock now holds the copy response until the put arrives, and the mutation went red on every run after that.
Live lane (MinIO in Docker)
New env-gated,
#[ignore]d live tests, modelled onaerorsync/live_tests.rs(AEROFTP_S3_DELTA_LIVE_*, with a REQUIRED flag so a mandatory job cannot pass by skipping):Ok(None)) and the ordinary-upload fallback lands byte-identical.Seen failing on defective code: with the executor ignoring the match list (the silent no-op of 06 R3), the middle-edit test goes red on the wire bound with the full 1 GiB uploaded.
The live lane also caught a real defect outside the planner: the first CreateMultipartUpload after the heavy baseline transfer failed with hyper
IncompleteMessageon a stale pooled keep-alive connection. The executor now retries the create once onNetworkError; replaying a create is safe because the failed attempt never reached the server.Gate
All green on the rebased base (
origin/maina90f613d6):cargo fmt --all --checkcargo test --lib s3: 179 passed, 3 ignored (the live lane)cargo clippy --all-targets -- -D warnings: exit 0npm run typecheck,i18n:validate,i18n:diacritics,i18n:untranslated,check:provider-inventorynpm run test:unit: 910 passedNo change to
src-tauri/Cargo.toml, no new dependency.