Repository navigation
s3: one result channel per streaming upload, so a failed fetch("s3://") upload is reported once - #45242
s3: one result channel per streaming upload, so a failed fetch("s3://") upload is reported once#45242robobun wants to merge 2 commits into
Conversation
A native stream that failed before anything read it keeps the failure for its first consumer. `upload_stream` checked for that before it started an upload: it took the failure out of the stream and rejected. The stream was left unlocked with no failure, so it read as complete and empty. A second `S3Client.write`, `s3file.write` or `Bun.write` of the same stream resolved 0 and stored an empty object under the key. Remove the check for a `Bytes` source. The native attach in the same function already handles a stored failure the way `wire_native_sink` does for every other sink: it locks the stream, ends it with the failure and fails the upload. The early check made that code unreachable. The first write still rejects with the stream's failure and sends no request.
`upload_stream` had two result channels: the promise it returns and the
caller's callback. fetch() is the only caller with a callback, and it
drops the promise.
- The check that refuses a used stream ran before the callback was
stored. It settled only the promise, so `fetch("s3://", { body })`
never settled when an option getter had used the body stream.
- Every failed upload also rejected the dropped promise: an unhandled
rejection beside the 500 response that fetch resolved with.
With a callback, `upload_stream` now creates no promise and sends every
outcome to the callback, once, a refusal included. The callback gets the
origin of a failure. fetch rejects with the error that the body stream
produced, as it does for an http: URL. Every other failure still
resolves the 500 response.
The checks for an `Invalid` source and for a `File` source with a stored
error are removed. Neither can fire: nothing gives `upload_stream` an
`Invalid` source, and a `FileReader` does not keep an error in
`pending` (#41420).
|
Updated 4:16 AM PT - Oct 14th, 2026
✅ @robobun, your commit b5a346369b254deba9700b9403514f3b22e50e8d passed in 🧪 To try this PR locally: bunx bun-pr 45242That installs a local version of the PR into your bun-45242 --bun |
|
Reproduction (release build of main 3d6c7bc, a local setTimeout(async () => {
try {
const res = await fetch("s3://backup.tar", { method: "PUT", body: stream, s3 });
console.log("handled response:", res.status, res.statusText);
} catch (e) {
console.log("caught:", String(e));
}
setTimeout(() => console.log("still running"), 50);
}, 1);
// main: handled response: 500 AccessDenied, then "S3Error: Access Denied", exit code 1
// this PR: handled response: 500 AccessDenied, then "still running", exit code 0The hang: PR: #45242 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughS3 streaming uploads now deliver typed outcomes through a callback or returned promise. Fetch rejects for upload refusal and body-stream errors, while other S3 failures retain the 500 response path. Tests cover these outcomes and disturbed streams across S3 write APIs and fetch entry points. ChangesS3 Streaming Uploads
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: using one result channel per S3 streaming upload to report failed fetch uploads once. |
| Description check | |
The description explains the problem, the fix, trade-offs, and verification results. It does not use the template headings, but it provides the required information for both sections. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes user-visible fetch("s3://", { body: stream }) semantics (a body-stream error now rejects where it resolved a 500) and the description explicitly asks for a maintainer decision on that, a human look is still worthwhile.
What was reviewed:
- The
done: UploadStreamDonesingle-channel delivery: bothhandle_reject_streamandresolvetake it viamem::replace, so the callback-first-then-task.failordering inhandle_reject_streamlands onDeliveredand cannot fire the fetch callback (and free its context) twice;_pump_ref/_upload_refbalances are unchanged. - The
is_disturbedrefusal with a callback returnsundefinedwhile fetch returns its own pre-captured promise; the Blob.rs callers passNoneand keep the promise path. - The deleted per-source pending-error checks: the native ByteStream path still takes the stored error after attaching (
take_pending_error) and routes it throughhandle_reject_stream, so a failed stream no longer reads as empty on a second write (covered by the "twice" fixture rows). - Tests exercise
USE_SYSTEM_BUN-failable cases: refused-stream matrix across fetch entry points, body-failure matrix with exact outcomes, exit code 0 and no unhandled rejections.
Extended reasoning...
The change reworks how S3 streaming uploads report their outcome in src/runtime/webcore/s3/client.rs (replacing end_promise/callback fields with a single UploadStreamDone enum and adding the S3UploadStreamResult variants Refused/BodyFailed) and updates the fetch("s3://") callback in src/runtime/webcore/fetch.rs to reject on body-stream errors, plus ~500 lines of new tests. It touches refcounted native code with manual heap ownership but no auth, crypto, or injection surface beyond the existing S3 credential handling, which is unchanged. The bug hunt ran dry with no findings and the delivery-once invariant, refcount balance, and removed early-return paths check out. Deferring rather than approving because the PR intentionally changes user-facing fetch behavior (reject vs. 500) and the author flags that as an open decision for a maintainer, and the diff is large enough in a memory-sensitive module that a human owner should confirm the semantic choice.
Stacked on #45241.
Decision needed. An error from the body stream now rejects
fetch("s3://", { body: stream }), as for anhttp:URL. Today it resolves a 500 response. Notes has the other option. The rest is the piece of #32131 that #36087 did not pick up.Problem
fetch("s3://", { body: stream })resolves a 500 when S3 answers 403. Bun then printsS3Error: Access Denied, which no handler can catch, and the process exits with code 1.upload_stream(src/runtime/webcore/s3/client.rs) returns a promise and takes a callback. fetch drops the promise (fetch.rs:1810). Each failure rejects it, and the check for a used stream settles nothing else.Fix
upload_streamcreates no promise. Every outcome goes to the callback, once.s3.test.ts,fetch-args.test.ts,s3-upload-abort.test.ts,s3-stream-error-gc.test.tsfail on main. Self-reviewed: 13 concerns raised, 13 addressed.Background
upload_streamfeeds a stream to a multipart upload. Its other callers (S3Client.write,Bun.write) return its promise.Downsides
res.okmust now catch a body stream that fails..text-1,536 B.Notes
Severity and reach
Fixesline. streams: leave a stream alone when a consumer fails to lock it #45004 names the hang as reported separately.awaitprints the error, continues, and exits with code 1 at the end. 1.4.3-canary.1 prints the error, continues, and exits with code 1 in both cases. Checked with the script below.S3Client.write,s3file.write,Bun.write) settle once on the same failures and are a workaround. A string or buffer body takes another path and is not affected.The rule that is implemented
An error object that the body stream produced rejects the fetch with that object. Every other outcome is unchanged here.
fetch("s3://", { method: "PUT", body })AccessDenied+ orphanAccessDeniedERR_S3_MISSING_CREDENTIALS+ orphanERR_S3_MISSING_CREDENTIALScontroller.error(),throw undefined)UnknownError+ orphanUnknownErrorcontroller.error(e), directclose(e),pull()throws)UnknownError+ orphaneUnknownError+ orphanENOENTUnknownError+ orphanECONNRESETUnknownError+ orphanAbortErrors3getter lockedUnknownError+ orphanERR_INVALID_STATEError: ReadableStream is already disturbedERR_S3_MISSING_CREDENTIALS"Orphan" is the rejection of the dropped promise. The last row is "500
UnknownError+ orphan" with #45241 alone.Questions for a maintainer
UnknownError?fetch("http://...")andS3Client.writewith the same error. A handler that reads onlyres.okhas to catch.The change
S3UploadStreamWrapperhadend_promise,callbackandcallback_context. It has one field now,done: the promise, or the callback, orDelivered. Both delivery sites (resolve,handle_reject_stream) take it out before they use it.upload_streamhas one exit before the upload exists: the stream was used after the caller checked it. That takes an option getter. The exit goes through the caller's channel.Invalidsource and for aFilesource with a stored error are deleted. Nothing givesupload_streamanInvalidsource (from_tagnever makes one). AFileReaderputs an error inpendingonly while a read is pending and delivers it at once (Release the poll and surface the error when a Bun.file() stream read fails #41420). AFilestream that fails is a started upload that rejects.Not fixed here
signalthat aborts during the upload does not stop it.s3file.writer().end()resolves0after an upload that failed synchronously (s3: report a writer() upload failure that no pending promise could carry #43140).Error.fetch("http://...")usesERR_STREAM_CANNOT_PIPEfor the same stream.Measurements (main 3d6c7bc and this PR, which includes #45241)
fetch("s3://", stream)upload (heapStats, n = 8, 16, 32)client.writeuploadJSPromisecreate / resolve /isDisturbedcalls (gdb, debug builds)client.writeuploads: the sameupload_stream/resolve/ fetch's callback (gdbnexti, release)client.writeupload:upload_stream/resolveunhandledRejection/ Promise cells kept per call.text(size -A)upload_stream/resolve_thunk/s3_stream_wrapper_resolve/handle_reject_stream(nm -S)perfandvalgrindare not installed here. The instruction counts step over calls, so the saved promise work is in the call counts.Tests
s3.test.ts: one child runs the started-upload rows of the table, with nounhandledRejectionlistener. A rejection that nothing handles is in stderr and in the exit code. On main 10 rows differ, stderr has the orphans, and the exit code is 1.s3-upload-abort.test.ts: the stream fails after one part was uploaded. The fetch rejects with the stream's error, and the upload is aborted (create 1, part 1, abort 1).fetch-args.test.ts: 10 rows for the used-stream refusal (6 entry points, POST, 3 getters, read and cancel), all pending on main. 4 control rows fors3file.writeandBun.write, which take the promise of the same exit and reject on main too. One row checks that the rejected promise is the one thatfetch()returned.s3-stream-error-gc.test.ts:fetch("s3://")with a stream that stored its failure, and with the S3 file as the body.s3.test.ts(308 pass),fetch-args.test.ts(106 pass),s3-upload-stream-gc,s3-networksink-leak,s3-write-to-file-sync-close,s3-connection-close.s3-upload-memoryand one test ofs3-stream-cancel-leaktime out at 5 s on the debug build of main on this machine too.