Repository navigation
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.
|
Reproduction (debug build of main 3d6c7bc, no S3 credentials in the environment, a local const client = new Bun.S3Client({ endpoint, bucket: "bucket", accessKeyId: "id", secretAccessKey: "secret" });
const stream = new Bun.S3Client({}).file("source").stream(); // the download fails: no credentials
await client.write("copy", new Response(stream)).catch(e => console.log(e.code)); // ERR_S3_MISSING_CREDENTIALS
console.log(await client.write("copy", new Response(stream))); // 0
// the bucket got: PUT /bucket/copy, 0 bytesWith this PR the second write rejects and the bucket gets no request. The same rows are in PR: #45241 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe S3 upload path now leaves pending byte-stream errors for native attachment. Tests cover repeated writes through three S3 write APIs, including the stored failure and the subsequent ChangesS3 stream error handling
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| 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. |
| Title check | |
The title clearly and concisely describes the main change: preventing an empty S3 object when a failed stream is written twice. |
| Description check | |
The description explains the problem, fix, behavior changes, verification steps, test results, and known downside. It does not use the exact template headings, but it provides the required information… |
✨ 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.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/webcore/s3/client.rs— Callers offetch("s3://…", { body })with a stream that already failed now get an uncatchable unhandled rejection and exit code 1 alongside the 500 response. With the early return gone, the stored error reaches src/runtime/webcore/s3/client.rs:994, which rejectsctx.end_promise; on the fetch path nobody holds that promise because fetch.rs:1810 discards upload_stream's return value. Fix: on a stored error with a nativecallbackset (the fetch path), fail the task without rejecting an orphanend_promise, or have fetch.rs keep the returned promise and route its rejection into the fetch promise. On the base this call hangs instead, so the merged state trades a hang for a process-level error the caller cannot handle. [also at: src/runtime/webcore/s3/client.rs:639 - Users callingfetch("s3://...", { body })with a stream that already failed now get an unhandled promise rejection where the base left the fetch pending.]Why this was flagged
A ReadableStream whose native ByteStream source stored an error before any read (for example
new Bun.S3Client({}).file("k").stream(), which fails for missing credentials) is passed as the body offetch("s3://bucket/key", { method: "PUT", body }). On the base branch the deleted Bytes arm returned a rejected promise before any task was built, so the discarded promise was already rejected and unobserved and the fetch promise never settled (a hang). After this change the stored error flows to src/runtime/webcore/s3/client.rs:990-995:take_pending_errorreturns it,ctx.handle_reject_stream(err_js)runs, and client.rs:638 rejectsself.end_promise, a promise created at client.rs:944 that no JS code holds on the fetch path. JSC reports it through the rejection tracker; in Bun's default mode (src/jsc/VirtualMachine.rs:4492-4493) that incrementsunhandled_error_counterand prints the error, so the script's exit code becomes 1 even thoughtask.failthen resolves the fetch promise with a 500 via the native callback.Verification: On the base branch the same trigger hits the deleted Bytes arm, which returned
JSPromise::rejected_promise(cx.global(), js_err).to_js(); fetch.rs:1810 discards that too. The change does not introduce the unhandled rejection; it replaces a hang with a 500 response. fetch.rs:1810 discards the promiseupload_streamreturns, which is the channel a stored stream error is reported on.
| // A `Bytes` source that stored an error is not refused here: the native attach | ||
| // below ends the stream with that error. |
There was a problem hiding this comment.
🟡 nit (optional): maintainers reading this match will take the new comment as describing the File arm it now sits above, and it documents code that no longer exists. src/CLAUDE.md says never to add comments to deleted code blocks and to avoid comments that are not necessary; src/runtime/webcore/s3/client.rs:839-840 narrates the removed Bytes arm ("is not refused here") instead of anything the code does. Fix: drop the two lines, or if a pointer is wanted, put a one-line note at the native attach site (client.rs:990, take_pending_error) saying it also covers a stored error, so the match reads as plain File handling.
Why this was flagged
The diff deletes the ReadableStreamPtr::Bytes(_) arm in upload_stream and replaces it with a two-line comment at src/runtime/webcore/s3/client.rs:839-840 that describes the absence of the deleted check. The comment is placed directly above ReadableStreamPtr::File(_) => at client.rs:841, which still contains the identical take-the-error-and-reject logic, so a reader sees a sentence about a Bytes source attached to the File arm. src/CLAUDE.md:23 says NEVER add comments to deleted code blocks and src/CLAUDE.md:25 says avoid unnecessary comments; the repository review guidance also says never narrate the change. On the base branch the arm existed and the comment did not. No runtime behaviour is affected; this is a convention violation that leaves a misleading comment next to the sibling arm.
Verification: nit. Triggering condition: anyone reading the match readable_stream.ptr in upload_stream. The ReadableStreamPtr::Bytes(_) arm is deleted and replaced by the two-line comment at src/runtime/webcore/s3/client.rs:839-840, which sits directly above ReadableStreamPtr::File(_) => at line 841. src/CLAUDE.md:23 states "NEVER add comments to deleted code blocks."
Problem
stream(), a fetch response body) can fail before anything reads it. A write of it to S3 rejects with the stream's error. A second write of the same stream resolves0and stores an empty object (S3Client.write,s3file.write,Bun.write).upload_stream(src/runtime/webcore/s3/client.rs:842). It takes the stored error out of the stream and returns. The stream then reads as complete and empty.Fix
test/js/bun/s3/s3-stream-error-gc.test.tsfail on main (resolved 0 bytes, three empty PUTs). Also:s3.test.ts, the S3 upload suites. Self-reviewed: 3 concerns raised, 3 addressed.Background
upload_streamfeeds a stream to an S3 multipart upload.Downsides
fetch("s3://", { body })with such a stream no longer stays pending. It resolves a 500UnknownErrorresponse with an unhandled rejection, as main does for a body that fails during the upload. s3: one result channel per streaming upload, so a failed fetch("s3://") upload is reported once #45242 makes both reject.Notes
Reach
fetch("s3://")hang in the same function. There is no user issue.Bun.serveas the bucket):Before and after (debug builds of main 3d6c7bc and of this PR)
client.write(key, new Response(stream))0, empty PUTTypeErrorclient.file(key).write(new Response(stream))0, empty PUTTypeErrorBun.write(s3file, new Response(stream))0, empty PUTTypeErrorThe
TypeErrorisnew Response()refusing a stream that is locked. The first write isrejected ERR_S3_MISSING_CREDENTIALSwith 0 requests on both builds.Why the sibling check stays
Filesource next to the removed one is not touched. It cannot fire: 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).Cost
JSC__JSPromise__rejectedPromise100 to 0,JSC__JSPromise__create0 to 100, requests at the bucket 0 to 0. All 100 reject on both.Self-review
fetch("s3://")in the same function. It is its own PR so that the write APIs do not wait for that one.fetch("s3://")and the siblingFilecheck are described above.