Conversation
When a multipart upload fails, the writer sends AbortMultipartUpload to delete the stored parts. The success answer to that request is 204 No Content (AWS service model, MinIO). The rollback registered its callback as S3Callback::Upload, and that arm of on_response accepts only 200. So the handler saw each 204 as a failure and sent the abort again: 1 + retry requests for each failed upload, 4 with the default. Register the rollback as S3Callback::Delete. That arm accepts 200 and 204 as success. It reports 404 as NotFound, and the handler stops there too: the upload id is already gone. Every other failure still retries.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 3 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 3 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 4 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
StatusReproduced on
To run the repro: This PR replaces #33682, which a cleanup of stale pull requests closed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that Callback::Delete in simple_request.rs still treats 200 as success (so the pre-fix 200 behavior is unchanged), that the Failure arm keeps the same retry-and-deref sequence as before, and that the missing abort after a failed CompleteMultipartUpload is pre-existing and not introduced here.
Extended reasoning...
The diff is a two-line callback-variant swap plus a matching enum change in on_rollback_multi_part_request, and the new test pins 204/404/500 abort responses via a loopback stub. The only open question is the policy choice in the NotFound arm (already raised inline); the rest of the change preserves existing behavior for 200 and for all non-404 failures, and the deref_(this) calls remain on every terminal path.
Only a 404 with the code NoSuchUpload says the upload id is gone. A proxy in front of the store can answer 404 too, and that answer says nothing about the upload. Such a 404 retries like any other failure, as it did before.
|
The second commit (e6c2af7) applies the optional suggestion from the review. The rollback now stops only on a 404 with the code |
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the earlier inline concern: a 404 now only stops the rollback when the parsed <Code> is exactly NoSuchUpload, and every other 404 (including the bare gateway 404 that error_with_body maps to NoSuchKey) falls through to the same retry loop as Failure.
What was reviewed:
- Refcount balance in
on_rollback_multi_part_request: each terminal arm callsderef_exactly once, and the retry arm hands the ref to the re-issued request unchanged from the previous code. - The
Deletearm ofon_responsein simple_request.rs accepts 200/204 and routes 404 througherror_with_body, so theNoSuchUploadmatch is reachable only with a real XML error code. - The new
it.eachcovers 204, 404 NoSuchUpload, plain-text 404, and 500 with exact request-order assertions in a fresh subprocess against a loopback stub.
Extended reasoning...
Overview
The PR changes one callback registration in src/runtime/webcore/s3/multipart.rs (S3Callback::Upload → S3Callback::Delete) and rewrites the match in on_rollback_multi_part_request to take S3DeleteResult. Success and a 404 whose parsed code is NoSuchUpload stop immediately; any other 404 or failure decrements options.retry and re-issues the abort. A describe.concurrent block with four it.each cases is added to test/js/bun/s3/s3.test.ts.
Security risks
None identified. The change only affects how many times a best-effort AbortMultipartUpload is re-sent after a failed upload. No credential handling, signing, or user-controlled parsing changes; the NoSuchUpload comparison is a slice equality on bytes already parsed by the existing XML error parser.
Level of scrutiny
Moderate. The function is a raw-pointer callback that owns a ref on MultiPartUpload, so the refcount balance per arm needed checking: both terminal arms deref_ once, and the retry arm returns without deref because the re-issued request inherits the ref — identical to the pre-existing structure. The second commit (e6c2af7) changed the NotFound handling from unconditional stop to code-gated stop, which was the substance of the prior inline comment; I verified in simple_request.rs:239-245 that a 404 with no XML <Code> is reported as NoSuchKey, so a proxy 404 does not match the NoSuchUpload arm and still retries.
Other factors
The test spawns a subprocess per case with a loopback Bun.serve stub, drains stdout/stderr/exited concurrently, asserts exact request order before exit code, and uses port: 0. The 204 and 404-NoSuchUpload cases fail on the base (4 aborts vs 1), and the proxy-404 and 500 cases pin the retry behavior that remains. No CODEOWNER covers these paths, and the only prior objection (mine) was addressed in code rather than by thread resolution. The doc comment above the function ("if it fails we will retry, if it still we just deinit") still describes the behavior accurately.
needless_pass_by_value is denied in the workspace. The callback type fixes the by-value signature, so the retry arm binds the error, as the handler did before.
There was a problem hiding this comment.
LGTM — the follow-up commits address the 404 concern from the earlier review: the rollback now stops only on Success or a 404 whose parsed <Code> is NoSuchUpload, and every other 404 or failure still goes through the existing retry loop.
What was reviewed:
Callback::Deleterouting insimple_request.rs: 200|204 →Success, 404 →error_with_body(NotFound)(which substitutesNoSuchKeyonly when no<Code>is present, so a bare proxy 404 cannot match theNoSuchUploadarm), everything else →Failure.- Ref balance in
on_rollback_multi_part_request: each terminal arm derefs exactly once; the retry arm re-issues the request and returns without deref, matching the pre-existing shape. - The
it.eachtest covers 204,NoSuchUpload, bare 404, and 500 with exact request sequences and runs in a subprocess against a loopback stub, with proxy env vars cleared.
Extended reasoning...
Overview
The PR changes one callback registration in src/runtime/webcore/s3/multipart.rs (S3Callback::Upload → S3Callback::Delete for the AbortMultipartUpload rollback) and rewrites the match in on_rollback_multi_part_request to take an S3DeleteResult. Success and a NotFound carrying code NoSuchUpload deref immediately; any other NotFound or Failure decrements retry and re-issues the abort, dereffing only when retries are exhausted. A new describe.concurrent block in test/js/bun/s3/s3.test.ts pins the number of abort requests for four stub responses.
Security risks
None. The change affects only the response handling of a best-effort cleanup request against an endpoint the user already configured; it does not alter signing, credentials, or any user-facing result. The NoSuchUpload match is a byte-exact comparison against the parsed XML code, so a response body cannot widen the stop condition beyond that single value.
Level of scrutiny
Moderate. It is a small, self-contained change in the S3 client, but it involves a raw-pointer callback with manual refcount handling. I traced Callback::Delete through on_response, error_with_body, and not_found in simple_request.rs to confirm the status → variant mapping and the NoSuchKey fallback; the handler's ref discipline is unchanged from the base (one deref per terminal path, none on the retry path). The concern raised in the previous review — treating every 404 as terminal — was addressed in commit e6c2af7 by narrowing the stop condition to NoSuchUpload, and the third commit is a clippy-driven binding rename with no behavior change.
Other factors
The test exercises the behavior through a spawned child against an in-process Bun.serve({ port: 0 }) stub, drains stdout/stderr/exit concurrently, asserts the exact request sequence via toEqual, and clears proxy env vars so the loopback endpoint is reached directly. The 204 and NoSuchUpload cases fail on the base (4 aborts instead of 1), and the bare-404 and 500 cases pin the retained retry behavior. No CODEOWNERS entry covers the changed paths, and there is no outstanding third-party objection in the timeline.
|
Updated 8:39 AM PT - Sep 17th, 2026
✅ @robobun, your commit 189e1de8cc274d87778e84a4827781beac7f22fb passed in 🧪 To try this PR locally: bunx bun-pr 43099That installs a local version of the PR into your bun-43099 --bun |
|
Closing: #41688 landed this fix. On main the rollback completes through the I checked this with the stub from this PR against a debug build of main (6d504dd), with One difference remains. Main treats every 404 answer to the abort as final. This PR retried a 404 that has no |
1 participant
Problem
AbortMultipartUpload(DELETE ?uploadId=) to delete the stored parts. The success answer is204 No Content(sources in Notes). Bun reads the 204 as a failure and sends the abort again:1 + retryrequests, 4 with the defaultretry: 3.rollback_multi_part_request(multipart.rs:848) registersS3Callback::Upload. That arm ofon_response(simple_request.rs:344) accepts only 200.Fix
S3Callback::Delete. That arm (simple_request.rs:321) accepts 200 and 204, and reports 404 asNotFound.on_rollback_multi_part_requeststops onSuccessand on a 404 with the codeNoSuchUpload: the upload id is gone. Every other failure still retries (403, 5xx, network error, a 404 from a proxy).test/js/bun/s3/s3.test.ts, "s3 multipart upload rollback". Without the fix, the 204 andNoSuchUploadcases fail with 4abortrequests. The 500 and proxy 404 cases pin the retries that remain. Also ran the other files intest/js/bun/s3/(details in Notes).Background
S3File.writer()andS3Client.write()go multipart when the data reachespartSize:CreateMultipartUpload, oneUploadPartper part, thenCompleteMultipartUpload.MultiPartUpload::failrejects the caller's promise, then starts the rollback.execute_simple_s3_requesttakes aCallbackenum. The variant selects which statuseson_responsecounts as success, and the handler's result type.Notes
History. This is #33682 again. A cleanup of stale pull requests closed that PR on 2026-09-13 without a review of the fix. It passed CI on every lane that ran. The test moved from a new file into
s3.test.ts.Measured. Local stub, every part fails with 500,
retry: 3.DELETErequests sent:NoSuchUploadSources for 204.
AbortMultipartUploadhas"responseCode":204, andNoSuchUploadis its only modeled error (botocore service-2.json, API reference).AbortMultipartUploadHandlerends withwriteSuccessNoContent(w)(object-multipart-handlers.go). Since do not return an error in AbortMultipartUpload() minio/minio#18135 it answers 204 for an unknown upload id too, so MinIO answers each repeated abort with 204 again.404 handling. The first commit stopped on every 404. The review on this PR pointed out two facts. A proxy in front of the store can answer 404 while the upload still exists.
error_with_bodyreports a 404 without a<Code>asNoSuchKey, so the handler cannot tell that case from a store answer by the variant alone. The second commit stops only onNoSuchUpload, the one error that the AWS model lists for this operation. Any other 404 retries, as it did before this PR.Not changed, on purpose.
multipart.rs:284,:410,:731) still retry on every failure, 403 included. A retry policy by error class is a separate change.CompleteMultipartUploadanswered with 404 stays a plain failure.CreateMultipartUploadstill registersS3Callback::Download. Its success status is 200, and that arm accepts 200.S3Callbackregistration whose variant rejects the success status of its operation.Suites. I ran all 18 test files in
test/js/bun/s3/on the debug build. All pass except one test ins3-list-objects.test.ts: "Should fall back to NoSuchKey for a 404 whose has no usable" times out at 5000 ms when the whole file runs on a debug build. It passes alone. #38357 documents that timeout on main and fixes it. This PR does not touchlist().Related. #33681, #33686, #34045 and #35875 touched the same rollback and retry code. The same cleanup closed them. Open #41688 and #42590 add new
rollback_multi_part_request()call sites. Each new call site sends the same repeated aborts until this lands.Conflict with #40252. That PR rewrites
on_rollback_multi_part_requestand still registersS3Callback::UploadwithS3UploadResult. When the two conflict, keepS3Callback::DeleteandS3DeleteResult. A resolution that takes the other side compiles and drops the fix. The test in this PR then fails.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file