Skip to content

s3: do not store an empty object when a failed stream is written twice #45241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

s3: do not store an empty object when a failed stream is written twice #45241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 2 additions & 27 deletions src/runtime/webcore/s3/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,33 +836,8 @@ pub(crate) fn upload_stream(
)
.to_js());
}
// The File/Bytes payload types
// differ (`*FileReader` vs `*ByteStream`), so the arms are
// unrolled here.
ReadableStreamPtr::Bytes(_) => {
// BACKREF: see `Source::bytes()` — payload live while the
// ReadableStream JS wrapper is rooted. R-2: `pending` is `JsCell`.
let stream = readable_stream.ptr.bytes().expect("matched Bytes");
if matches!(
stream.pending.get().result,
crate::webcore::streams::StreamResult::Err(_)
) {
// we got an error, fail early
let err = match stream.pending.with_mut(|p| {
core::mem::replace(&mut p.result, crate::webcore::streams::StreamResult::Done)
}) {
crate::webcore::streams::StreamResult::Err(err) => err,
_ => unreachable!(),
};
stream.pending.set(crate::webcore::streams::Pending {
result: crate::webcore::streams::StreamResult::Done,
..Default::default()
});
let js_err = err.to_js(cx.global());
js_err.ensure_still_alive();
return Ok(bun_jsc::JSPromise::rejected_promise(cx.global(), js_err).to_js());
}
}
// A `Bytes` source that stored an error is not refused here: the native attach
// below ends the stream with that error.
Comment on lines +839 to +840

@claude claude Bot Oct 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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."

ReadableStreamPtr::File(_) => {
// BACKREF: see `Source::file()` — payload live while the
// ReadableStream JS wrapper is rooted. R-2: `pending` is `JsCell`.
Expand Down
4 changes: 4 additions & 0 deletions test/js/bun/s3/s3-stream-error-gc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ test.concurrent("S3 stream that failed before anything read it rejects every con
"Bun.write(path, new Response(stream))": rejected,
"client.write(key, new Response(stream))": rejected,
"Bun.write(s3file, new Response(stream))": rejected,
// The first write ends the stream with its failure, so `new Response()` refuses it.
"client.write(key, new Response(stream)), twice": [rejected, "rejected TypeError"],
"client.file(key).write(new Response(stream)), twice": [rejected, "rejected TypeError"],
"Bun.write(s3file, new Response(stream)), twice": [rejected, "rejected TypeError"],
"requests the bucket got": [],
"fetch(url, { method: 'POST', body: stream })": rejected,
"complete requests the receiver got": 0,
Expand Down
13 changes: 13 additions & 0 deletions test/js/bun/s3/s3-stream-stored-error-fixture.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading