s3: resolve streamed write/download with the byte count, not 0 - #35671
s3: resolve streamed write/download with the byte count, not 0#35671robobun wants to merge 8 commits into
Conversation
Every S3 write entry point is documented as resolving with the number of
bytes written, and the buffered path (Uint8Array/Blob/string) already did
that. The streamed paths all resolved a hardcoded 0:
- upload_stream (Response with a ReadableStream body, Bun.file,
Bun.write(s3file, Bun.file)) via S3UploadStreamWrapper::resolve
- writable_stream (S3File.writer().end()) via wrapper_callback
- Bun.write(path, s3file) via on_file_stream_resolve_request_stream
MultiPartUpload never tracked a cumulative byte count, so there was
nothing to resolve with. Track it as uploaded, bumped on each part and on
the single-PUT response. NetworkSink mirrors that into a wrote field via
on_writable, because the JS sink wrapper can be collected while the upload
is in flight (nothing references it once end() hands back a Promise),
which detaches the task pointer before the completion callback runs.
end_from_js is also reordered so the end promise exists before the upload
is triggered.
The download path already had FileSink.written available; read it instead
of resolving 0 in on_file_stream_resolve_request_stream and the
synchronous-completion branches of pipe_readable_stream_to_blob.
WalkthroughChangesS3 multipart and single-request uploads now track transferred bytes and return those counts from stream and client promises. Blob file-stream completions also return written bytes. New tests cover buffered, streamed, file, multipart, and filesystem write paths. Transferred byte counts
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits: I traced the end_from_js reordering in streams.rs — the removed if !self.ended { ... } block was already dead (the old top-of-function self.end(None) set ended = true before it), and creating end_promise before end() is required so a synchronously-firing wrapper_callback has a promise to resolve. Also confirmed the new (*file_sink).written / (*this.sink).written reads in Blob.rs happen while the sink's +1 ref is still held (released by the immediately-following deref / FileStreamWrapper::Drop).
Extended reasoning...
Native S3/stream code with unsafe pointer derefs and control-flow reordering — deferring to human review. The two inline nits (flush() sync-path 0 vs cumulative, redundant server.stop alongside using) and the three comment-cop flags on the new multi-line comments are already on the thread; this note only records what was checked and ruled out so it isn't re-traced.
… drop redundant stop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/s3/s3-write-return-bytes.test.ts`:
- Around line 70-90: Extend the S3 write tests to directly call the client-file
write API (the S3 client’s write method with a key and Bun.file source) and
assert it returns and receives PAYLOAD bytes. Update the writer contract test
around writer() to call flush() and assert its returned byte count
independently, while preserving the existing end() total-byte assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28ea2155-c8e1-4d87-83c3-2a0955576c61
📒 Files selected for processing (5)
src/runtime/webcore/Blob.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-write-return-bytes.test.ts
There was a problem hiding this comment.
Both earlier nits (redundant server.stop(true) alongside using, and the two sync flush_from_js return sites still resolving 0) are addressed in 3b3bab0, and this pass found nothing new. Deferring to a human because the change reorders NetworkSink::end_from_js and adds unsafe (*file_sink).written reads across four native files — worth a maintainer's eyes on the lifecycle.
What was reviewed:
MultiPartUpload.uploadedis bumped only on server ack (per-part etag and single-PUT success), never on enqueue, so the resolved count matches bytes the origin confirmed.end_from_jsreorder: traced thatend()never clearsself.taskand the oldif !self.endedblock was dead, so creating the promise beforeend()is behavior-preserving.S3UploadStreamWrapper::resolvereadstask_mut().uploadedwhile the wrapper still holds its +1 task ref (released inDrop); Blob.rs readswrittenbefore the matchingFileSink::deref.- All
NetworkSinkflush/end resolution sites now agree on cumulativewrote; no remaining hardcoded-0 sibling was found.
Extended reasoning...
Overview
Fixes #23407: streamed S3 write/download paths resolved with a hardcoded 0 instead of the byte count. The change adds an uploaded: u64 accumulator to MultiPartUpload (bumped on part-ack and single-PUT success), mirrors it into a new NetworkSink.wrote via on_writable, and threads that value through every promise-resolution site: wrapper_callback, S3UploadStreamWrapper::resolve, both sync branches of flush_from_js, and the detached-task fallthrough of end_from_js. Separately, the three ReadableStream → FileSink resolution sites in Blob.rs now read FileSink.written (a pre-existing Cell<usize>) before releasing the sink ref. end_from_js is refactored to create end_promise before calling end() and to delegate the EOF write to end() in all three branches, deleting a dead if !self.ended block. A new test file exercises six entry points against an in-process mock S3 origin.
Security risks
None identified. No parsing of untrusted input changes, no auth/crypto/permissions surface. The mock server in the test is local (port: 0).
Level of scrutiny
Medium-high. This is a focused bugfix, but it spans four native files with intrusive-refcount and raw-pointer lifecycle invariants (MultiPartUpload, NetworkSink, FileSink, S3UploadStreamWrapper), and the end_from_js reorder changes when the upload's EOF write fires relative to promise creation. I traced each new unsafe deref back to the +1 ref that keeps the pointee live and confirmed the reorder is behavior-preserving (the removed inline EOF block was already dead because the top-level end() had set ended = true), but a maintainer familiar with the S3 sink lifecycle should confirm the wrapper_callback path can never observe a stale sink.wrote (it is last written in on_writable, which fires before completion in every path I traced).
Other factors
All prior review feedback on this PR has been addressed and marked resolved: the comment-cop multi-line-comment flags were condensed, the redundant server.stop(true) was dropped, and both sync flush_from_js return sites now use self.wrote. The bug-hunting system found nothing on the current revision. Test coverage is good — six cases including a real 2-part multipart upload — and the PR description states 5/6 fail on the released build. Given the unsafe-adjacent surface and the lifecycle reorder, I'm deferring rather than approving.
…iter tests under ASAN The writer() tests trip LeakSanitizer on the release-asan lane via a pre-existing NetworkSink leak (fix open in #34999); skip them under ASAN until that lands.
There was a problem hiding this comment.
No bugs found, but this touches unsafe pointer lifecycles across four native files and reorders NetworkSink::end_from_js control flow, so it's worth a human look.
Reviewed: the new uploaded/wrote counters are bumped before on_writable/wrapper_callback read them on both single-PUT and multipart paths; the (*file_sink).written reads in Blob.rs happen before the matching deref; the end_from_js refactor drops a dead if !self.ended block (end() already set it) and creates end_promise before end() can trigger completion; all four flush_from_js resolution sites now agree on cumulative bytes. The ASAN skip for the two writer() tests is scoped and cites #34999.
Extended reasoning...
Overview
The PR replaces hardcoded js_number(0.0) promise resolutions with actual byte counts across five S3/Blob streaming paths. It adds a uploaded: u64 counter to MultiPartUpload (bumped on part-ack and single-PUT success), mirrors it into a new NetworkSink.wrote field via on_writable, and reads FileSink.written before dereferencing the sink in three Blob.rs completion sites. NetworkSink::end_from_js is reordered so end_promise exists before end() runs. A new test file exercises seven entry points against a mock origin.
Security risks
None identified. No untrusted-input parsing, credential handling, or bounds arithmetic changed — the counters are internal accounting derived from data the runtime already sent.
Level of scrutiny
Medium-high. The individual edits are small and the counter additions are mechanical, but this is production S3 upload code with intrusive refcounting, raw-pointer callback contexts, and unsafe derefs whose ordering matters. The end_from_js change is a real control-flow refactor: the old code called end() first (which set self.ended and sent EOF), making the subsequent if !self.ended block dead; the new code consolidates to three end() calls placed after promise creation. That's correct as far as I can trace, but it's the kind of ordering change a maintainer familiar with the S3 state machine should confirm — particularly whether any path can fire wrapper_callback before end_promise is installed (I found none; HTTP completion is always async through the event loop).
Other factors
All prior inline feedback (mine, comment-cop, CodeRabbit) is resolved in 3b3bab0 and 2d452c5. Two new tests are ASAN-skipped due to a pre-existing NetworkSink leak tracked in #34999, which is reasonable but means the writer() paths aren't leak-checked here. The last CI status comment in the timeline is for the pre-fix commit; I don't have a green confirmation for 2d452c5.
…ve bytes Resolving flush() with the cumulative count broke the flush loops in s3.test.ts, which rely on flush() returning the bytes flushed by that call and 0 when nothing is pending. flush() resolution now matches main exactly; end() still resolves with the total uploaded.
There was a problem hiding this comment.
No issues found after the latest revisions — my earlier inline comments have been addressed and the flush() semantics were correctly reverted to the per-call delta after CI showed the cumulative change broke s3.test.ts. Deferring to a human because this touches the S3 multipart state machine, NetworkSink, and Blob→FileSink streaming across four files, and includes a control-flow reorder in NetworkSink::end_from_js (promise now created before end() runs) that goes beyond the headline byte-count fix.
What was reviewed:
end_from_jsreorder: verified the deduplicatedend()call is behavior-equivalent to the old inline EOF logic, and creatingend_promisebeforeend()is safe (value captured before any synchronous resolve).sink.wrotetiming: traced thaton_writable(which mirrorstask.uploadedintosink.wrote) fires beforewrapper_callbackon both the single-PUT and multipart-commit paths, soend_promiseresolves with the full count.Blob.rs:FileSink.writtenis aCell<usize>and each read happens while the +1 ref is still held, beforederef.flush_from_jssync-return sites are back to0.0, matching main and the delta contract the MinIO/R2 flush loops ins3.test.tsdepend on.
Extended reasoning...
Overview
The PR fixes five entry points (S3Client.write with a stream/file source, Bun.write(s3file, Bun.file), S3File.writer().end(), multipart writer().end(), and Bun.write(path, s3file)) that were resolving with a hardcoded 0 instead of the documented byte count. It adds an uploaded: u64 counter to MultiPartUpload (bumped on each part ack and on the single-PUT success), mirrors it into NetworkSink.wrote via on_writable, and reads FileSink.written for the download-to-file path. NetworkSink::end_from_js is also reordered so the end_promise exists before end() triggers the upload, and its inline EOF logic is folded into a call to end(). A new test file exercises all six entry points plus a 2-part multipart upload against an in-process mock origin; the two writer() tests are ASAN-skipped (pre-existing NetworkSink leak, tracked in #34999).
Security risks
None identified. The change is purely about what number a promise resolves with; no new input parsing, no auth/crypto/permission surface. The mock origin is an in-process Bun.serve({port: 0}).
Level of scrutiny
Moderate-to-high. S3 upload/download is a user-facing production path, and the change spans four files with a cross-file invariant (sink.wrote must be set by on_writable before wrapper_callback reads it) plus a control-flow reorder in end_from_js. The PR already went through one break-and-revert cycle on flush() semantics (my earlier suggestion to make flush() cumulative broke the flush loops in s3.test.ts on CI; dd50270 reverted it). That history and the multi-file coupling make this worth a maintainer's eyes rather than a bot approval.
Other factors
All prior review threads (mine, comment-cop, CodeRabbit) are resolved. The bug-hunting system found nothing on the current head. Tests look hermetic (local mock origin, tempDir, using for cleanup, no external network). The ASAN skip is narrowly scoped to the two writer() tests and documents the tracker; the direct-write and download tests remain enabled under ASAN. I traced the on_writable → wrapper_callback ordering on both the single-file (single_send_upload_response calls on_writable before done()) and multipart (drain_enqueued_parts calls on_writable before done() when the queue empties) paths and it holds, so sink.wrote is populated before the end promise resolves.
There was a problem hiding this comment.
No issues found, but I'd like a human to confirm the end_from_js reorder — moving self.end(None) to after end_promise creation changes when the completion callback (which runs finalize() → detach_writable()) can fire relative to promise setup, in refcounted S3 lifecycle code.
What was reviewed:
MultiPartUpload.uploadedis bumped at both ack sites (part etag + single-PUT) beforeon_writablemirrors it intosink.wrote, sowrapper_callbackreads the right total.FileSink.writtenreads happen while the +1 ref is still held (beforederef).flush()was reverted to its per-call delta contract in dd50270; the s3.test.ts flush loops are unaffected.- Prior nits (redundant
server.stop(true), comment length) are addressed; no new findings this run.
Extended reasoning...
Overview
The PR replaces three hardcoded js_number(0.0) promise resolutions with actual byte counts across S3 streamed upload (S3UploadStreamWrapper::resolve, writable_stream's wrapper_callback), NetworkSink::end_from_js, and Blob.rs FileSink pipe completion. It adds a u64 uploaded counter on MultiPartUpload (bumped on part-etag ack and single-PUT success) and a u64 wrote mirror on NetworkSink (populated in on_writable). end_from_js is reordered so end_promise exists before end() runs, and dead post-end() if !self.ended code is removed. A new test file covers seven entry points against a local mock origin.
Security risks
None identified. No user input parsing, no auth/crypto changes, no new external calls. The mock origin is a local Bun.serve({port: 0}); proxy env is cleared so the S3 client actually hits it.
Level of scrutiny
Moderate-to-high. The source changes are small and mostly additive (new counter + reads), but they sit in memory-unsafe Rust with intrusive refcounts and raw callback contexts. The end_from_js reorder is the one real control-flow change: previously end() ran first, so a synchronous completion path (e.g. process_multi_part → done() when buffered is empty and queue is drained) would fire wrapper_callback before end_promise existed and then finalize() would detach task, dropping into the 0.0 fallthrough. The new order creates the promise first, captures its value, then calls end(). I traced this and believe it's correct — end() doesn't touch self.task so the branch selection is unchanged, and the captured value survives wrapper_callback resolving/clearing the Strong — but it's exactly the kind of lifecycle reordering in refcounted networking code that benefits from a maintainer's eye.
Other factors
The PR has been through several review rounds. My earlier suggestion to make flush() cumulative was tried and reverted (dd50270) because it broke the flush loops in s3.test.ts on CI — the current diff leaves flush()'s per-call-delta contract exactly as on main and only changes end(). The two writer() tests are skipped under ASAN due to a pre-existing NetworkSink leak tracked in #34999, so that path isn't sanitizer-validated by this PR's coverage. All comment-cop and prior claude[bot] nits are resolved.
2 participants
Problem
S3Client.write/Bun.write(s3file, ...)/S3File.writer().end()/Bun.write(path, s3file)are all typed and documented as resolving with the number of bytes written. The buffered-source path (Uint8Array,Blob,Responsewith a string body) already returns the true count, but every streamed path resolves0:Any
n === expectedverification, progress accounting, or copy audit that trusts the documented return sees a spurious zero-byte transfer precisely on the large/streaming path where it matters.Cause
Three hardcoded
JSValue::js_number(0.0)resolutions:S3UploadStreamWrapper::resolve(src/runtime/webcore/s3/client.rs), which backss3.writewith aReadableStream/Bun.file/S3-to-S3 source.wrapper_callbackinsidewritable_stream(client.rs), which backsS3File.writer().end().on_file_stream_resolve_request_streamand the synchronous-completion branches ofpipe_readable_stream_to_blob(src/runtime/webcore/Blob.rs), which backBun.write(path, s3file)and any otherReadableStream -> FileSinkpipe.MultiPartUploadhad no cumulative byte counter to resolve with; the per-partflushedpassed toon_writablewas never a running total.Fix
MultiPartUploadgains anuploaded: u64, bumped on each part acknowledgement and on the single-PUT success response.S3UploadStreamWrapper::resolvereads it from the owned task pointer.NetworkSinkgains awrote: u64, mirrored fromtask.uploadedon everyon_writablecallback, andwrapper_callbackresolvesend()with that. Reading throughsink.taskinstead is unreliable: onceawait writer.end()suspends, the JS sink wrapper is eligible for collection (nothing references it past the returned promise), and its finalizer runsdetach_writable(), clearingsink.taskbefore the HTTP completion callback fires.end_from_jsis also reordered soend_promiseexists beforeend()triggers the upload.writer().flush()is untouched: it keeps its existing per-call delta contract (bytes flushed by that call,0when nothing is pending), which the flush loops ins3.test.tsdepend on.FileSink.writtenalready tracks bytes landed on disk; read it when resolvingon_file_stream_resolve_request_streamand theFulfilled/fallthrough branches ofpipe_readable_stream_to_blob.Verification
test/js/bun/s3/s3-write-return-bytes.test.tscovers all six entry points (including directS3Client.write(key, Bun.file(...)), the #23407 repro) plus a 2-part multipart upload against an in-process mock origin. The streamed cases fail on the released build and all pass with this change; relateds3-*.test.tsandbun-write.test.jscases are unchanged. The twowriter()tests are skipped under ASAN:writer()leaks itsNetworkSinkon main (pre-existing, fix open in #34999) and the new coverage trips LeakSanitizer on the release-asan lane.Fixes #23407
[review] gate passed · iteration 5 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 5
evidence per changed file