ByteStream: hand off owned buffers from on_pull/on_data and skip the adapter pull view - #36736
ByteStream: hand off owned buffers from on_pull/on_data and skip the adapter pull view#36736robobun wants to merge 15 commits into
Conversation
…adapter pull view ByteStream is a push source: body chunks arrive via on_data and are held in a native Vec until on_pull takes them. on_pull used to copy those bytes into the adapter-supplied Uint8Array view and return IntoArray; the adapter allocated a 256-512 KiB scratch view per stream to receive that copy. on_pull now takes the Vec and returns Owned (the same allocation becomes the Uint8Array backing), and on_data's pending path resolves with Owned directly instead of copying into the parked view. Since the view is never written, on_start now returns Start::ReadyOwned, which start_from_js encodes as -1. materializeNativeSource treats a negative start result as m_sourceOwnsChunks: nativeGetInternalBuffer then skips the view allocation and passes jsUndefined() to pull (pull_from_js passes an empty slice when the view is missing). nativeDecodePullResult no longer grows m_chunkSize when the source returns its own ArrayBufferView (the view was not the bottleneck). native-readable.ts mirrors the negative start result by keeping its pull buffer at MIN_BUFFER_SIZE. Measured on the Hono upload-stream route (oha -c100 -n2000, 2 MiB bodies, release build on Linux x64) this lowers server VmHWM by roughly 10 percent and post-full-GC RSS by roughly 19 percent; the remaining peak is the per-request native buffer Vec (REQUEST_BODY_HIGH_WATER_MARK) plus in-flight body chunks.
WalkthroughChangesNative readable streams now support source-owned chunks. Source-owned chunk streaming
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 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 dead-field nit, I also checked: the nativeAdjustChunkSize removal for JSArrayBufferView results is unconditional (not gated on m_sourceOwnsChunks) but ByteBlobLoader only returns IntoArray* (number path, unaffected) and FileReader's own-buffer returns genuinely didn't need the view grown; and the new _ => unreachable!() in on_data covers the same variants (Pending/IntoArray*) as the pre-existing one in append(), which producers never pass.
Extended reasoning...
This PR is a cross-language (Rust/C++/JS) protocol change to the hot-path request/response body streaming code, introducing a new Start::ReadyOwned → -1 sentinel and reworking ByteStream::on_pull/on_data buffer ownership. It ships without a new automated test (justified in the description, but REVIEW.md treats that as a hard requirement). Combined with the dead-field cleanup nit, this warrants human review rather than auto-approval. The two additional concerns I traced and ruled out are recorded in the message so a later pass doesn't re-derive them.
…eStream fields The test sends a 2 MiB body in small writes after the handler's first pull has parked and asserts every chunk the reader sees has backing === byteLength and byteOffset === 0. On main each chunk is a subarray over the adapter's ~516 KiB scratch view. Dead-field cleanup flagged by review: offset, pending_buffer, and high_water_mark are no longer written non-trivially or read after the Owned handoff, so remove them along with append's offset parameter, the empty_pending_buffer helper, and the two Body.rs writers.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ources The kHighWaterMark assignment was dead: _read(n) is always passed Readable's own state.highWaterMark, so getRemainingChunk's nullish fallback never fires. Stash the negative start() result on kSourceOwnsChunks instead and pass undefined straight to ptr.pull (pull_from_js already handles a missing view). Also wire handler-side errors in the new test to the awaited promise so a regression surfaces the stream error instead of timing out.
pending_value now only ever held jsUndefined() and was read solely by on_cancel's outstanding-pull check, which pending.state == Pending already answers. Remove the field, set_value()/value(), the clear in on_data, and the two deinit() sites; on_cancel and the on_pull debug_assert now gate on pending.state directly. Test: give pullParkedP a reject arm shared with handlerP so a handler/socket failure before onParked surfaces instead of hanging, and move the body writes and handler await into a try/finally that always sock.destroy()s.
There was a problem hiding this comment.
Beyond the two inline nits: also checked whether pull_from_js dropping its early-return on a non-ArrayBuffer view affects FileReader/ByteBlobLoader — it doesn't, since neither returns Start::ReadyOwned, so both adapters (BunStreamSource.cpp and native-readable.ts) still pass a real Uint8Array view for those sources and the new on_pull(&mut [], view) branch is only reached for ByteStream.
Extended reasoning...
This run surfaced two nits (posted inline). One additional candidate was raised by finders and refuted by verifiers: the pull_from_js change in ReadableStream.rs removes the else { return Ok(UNDEFINED) } guard when view.as_array_buffer() is None, now falling through to on_pull(&mut [], view). For ByteStream this is the intended path (it ignores the buffer). For FileReader/ByteBlobLoader, on_start never returns ReadyOwned, so m_sourceOwnsChunks/kSourceOwnsChunks stay false and both callers keep passing a real view — the empty-slice branch is unreachable for them.
The client now awaits onChunk() after each write, so the count assertion is deterministic rather than relying on 20 ms of wall time outrunning kernel coalescing.
on_pull subsumes drain for a ReadyOwned source and also writes closer[0] on OwnedAndDone, so the last-chunk close lands in the same pull instead of the next one. pendingView is provably null under m_sourceOwnsChunks, so the gate reverts to the original if (pendingView()).
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/webcore/streams/BunStreamSource.cpp (2)
483-483: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winResize the reusable buffer for non-owned view results.
When
!adapter->m_sourceOwnsChunks, callnativeAdjustChunkSize(adapter, chunk->byteLength())before enqueueing the view. Otherwise, later pulls keep the previous buffer size when a larger view is returned.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp` at line 483, In the non-owned chunk path around the byteLength check, call nativeAdjustChunkSize(adapter, chunk->byteLength()) before enqueueing the view. Keep this adjustment limited to !adapter->m_sourceOwnsChunks so owned chunks retain their existing behavior.
559-565: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the non-negative startup size before the
size_tcast.
Start::ReadyOwneduses the documented< 0contract, so do not change either adapter to accept only-1. InBunStreamSource.cpp, reject non-finite, fractional, and out-of-range non-negative values beforestatic_cast<size_t>(chunkSize); otherwise the cast can cause undefined behavior or an invalid allocation. Keepnative-readable.ts’s negative check aligned with the< 0contract.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp` around lines 559 - 565, In BunStreamSource.cpp, update the startup-size handling around Start::ReadyOwned to validate non-negative chunkSize values for finiteness, integral form, and size_t range before casting or allocating, while retaining the documented < 0 ownership path. In src/js/internal/streams/native-readable.ts at lines 149-151, preserve the negative check as < 0 rather than restricting it to -1.Source: Coding guidelines
🤖 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/http/serve-request-body-pipeline-memory.test.ts`:
- Around line 23-38: The write acknowledgement logic around onChunk must track
complete per-write byte markers rather than assuming one stream chunk per
sock.write. Introduce ordered unique markers for each write, parse incoming
handler data incrementally across split or combined chunks, and resolve each ack
only when its corresponding full marker has been received; advance parsing
through all complete markers in a chunk to avoid deadlocks. Preserve fail’s
rejection behavior for every pending acknowledgement.
---
Outside diff comments:
In `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp`:
- Line 483: In the non-owned chunk path around the byteLength check, call
nativeAdjustChunkSize(adapter, chunk->byteLength()) before enqueueing the view.
Keep this adjustment limited to !adapter->m_sourceOwnsChunks so owned chunks
retain their existing behavior.
- Around line 559-565: In BunStreamSource.cpp, update the startup-size handling
around Start::ReadyOwned to validate non-negative chunkSize values for
finiteness, integral form, and size_t range before casting or allocating, while
retaining the documented < 0 ownership path. In
src/js/internal/streams/native-readable.ts at lines 149-151, preserve the
negative check as < 0 rather than restricting it to -1.
🪄 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: c3a0b0f0-8a63-4c6c-80f3-feec8e7cee54
📒 Files selected for processing (3)
src/js/internal/streams/native-readable.tssrc/jsc/bindings/webcore/streams/BunStreamSource.cpptest/js/bun/http/serve-request-body-pipeline-memory.test.ts
There was a problem hiding this comment.
No further findings — all five earlier rounds of nits are addressed as of 8537aa0. This is a cross-language (Rust/C++/JS-builtin) rework of the ByteStream ownership handoff on the request-body hot path, so it's worth a human pass.
Checked: pull_from_js's new empty-slice branch is only reachable for ByteStream (FileReader/ByteBlobLoader never return ReadyOwned, so both callers always pass a real view); nativeAdjustChunkSize removal from the JSArrayBufferView arm doesn't affect File/Blob (they return numbers, not views); the removed pending_value Strong root is no longer load-bearing since on_pull no longer parks a JS view; the two Body.rs high_water_mark writers and the RequestContext offset read were the only external users of the deleted fields.
Extended reasoning...
Overview
This PR reworks ByteStream (the Bun.serve request-body / fetch response-body push source) so that on_pull and the pending-pull arm of on_data hand the buffered Vec<u8> directly to JS as an Owned/OwnedAndDone StreamResult instead of copying into the adapter's ~256-512 KiB scratch Uint8Array and returning IntoArray. To support that, a new Start::ReadyOwned variant encodes as -1 from start_from_js, and both consumers of the start protocol — materializeNativeSource in BunStreamSource.cpp and constructNativeReadable in native-readable.ts — now recognise a negative start result, set an m_sourceOwnsChunks/kSourceOwnsChunks flag, and skip allocating the pull view. pull_from_js in ReadableStream.rs was widened to call on_pull(&mut [], view) when no view is passed instead of early-returning UNDEFINED. With the copy-into-view path gone, pending_buffer, pending_value (a StrongOptional GC root), offset, and high_water_mark on ByteStream become dead and are removed along with their two writers in Body.rs and one reader in RequestContext.rs.
Security risks
None identified. This is an internal buffer-ownership refactor with no user-controlled input parsing, no auth/crypto, and no new external surface. The removed pending_value Strong root previously kept the JS pull view alive across a parked pull; that root is safe to drop because on_pull no longer stores a raw pointer into a JS-owned buffer — the parked-pull path now carries no borrowed JS memory at all.
Level of scrutiny
High. This is a hot-path memory-ownership change spanning three languages: a Rust Vec<u8> allocation is transferred to a JS Uint8Array backing store (via the existing Owned → to_js → MarkedArrayBuffer_deallocator path), the C++ adapter's pull-view allocation is skipped based on a new sentinel, and a JS builtin mirrors the same protocol. It also deletes a GC root and rewrites the on_data pending-pull state machine. REVIEW.md flags exactly this class (native memory safety, GC rooting, hot-path perf claims) as the most-blocked category. The +46% req/s / -24% RSS claim and the JS-visible behaviour change (each chunk now has backing === byteLength instead of a shared ~516 KiB backing) are the kind of thing a maintainer should sign off on.
Other factors
The PR has been through five review iterations with all nits addressed (dead pending_value removed, kSourceOwnsChunks initialized in the constructor, test sleep replaced with per-chunk acks, unhandled-rejection noise suppressed, and the redundant drain-path disjunct dropped). The new test file demonstrably fails on main (backing = 528384) and passes on the PR under ASAN. The generic pull_from_js change applies to all three SourceContext impls, but the empty-buffer branch is only reachable when the adapter sets m_sourceOwnsChunks, which only ByteStream::on_start triggers — FileReader and ByteBlobLoader still always receive a real view. Given the scope (9 files, ~220 net-line delta in memory-safety-critical code), deferring to a human reviewer rather than auto-approving.
There was a problem hiding this comment.
No issues found in this pass — all four rounds of prior feedback (test flakiness/acks, dead pending_value, redundant drain-path disjunct, unhandled-rejection noise) have been addressed. Deferring to a human for the final call: this rewrites the buffer-ownership handoff on the Bun.serve request-body / fetch response-body hot path and removes a GC root (pending_value: Strong), which warrants a maintainer sign-off.
What was reviewed:
on_data/on_pullOwned handoff: everystreams::Resultvariant is covered,Temporaryis copied to a right-sizedVec, anddone/signal_drainedordering matches the old path.pull_from_jsnow callson_pull(&mut [], view)instead of early-returningUNDEFINEDwhen no view — onlyByteStreamreaches this (it alone returnsReadyOwned), and itson_pullignores the buffer.nativeDecodePullResultdroppingnativeAdjustChunkSizeon theJSArrayBufferViewarm: checked thatFileReader/ByteBlobLoaderreturn numeric results (still adjusted); only Owned-returning sources hit this arm.high_water_mark/offset/pending_bufferremovals: grepped for remaining readers;RequestContext.rsand bothBody.rswriters were the only ones.
Extended reasoning...
Overview
This PR changes ByteStream (the native source behind Bun.serve request bodies and fetch response bodies) from a copy-into-scratch-view model to a direct owned-buffer handoff. on_pull now takes self.buffer and returns Owned/OwnedAndDone; on_data's parked-pull path resolves the pending promise with an Owned result (forwarding an owned input or copying a Temporary into a right-sized Vec) instead of copying into a JS-owned ~512 KiB scratch Uint8Array and spilling the tail. A new Start::ReadyOwned variant (encoded as -1 from start_from_js) signals the C++ adapter (m_sourceOwnsChunks) and native-readable.ts (kSourceOwnsChunks) to skip allocating the per-stream scratch view entirely. The now-dead pending_buffer (raw *mut [u8]), pending_value (a Strong GC root), offset, and high_water_mark fields are removed along with their writers in Body.rs and RequestContext.rs.
The change spans 8 source files across Rust (ByteStream.rs, ReadableStream.rs, streams.rs, Body.rs, RequestContext.rs), C++ (BunStreamSource.cpp/.h), and built-in JS (native-readable.ts), plus a new two-test file that fails on main and passes here (verified in the PR's evidence block).
Security risks
None identified. The change is internal to the stream-source adapter and does not touch parsing, validation, auth, or network-boundary code. The removed pending_buffer was a raw pointer into a JS-owned buffer that had to be re-derived to guard against detach; deleting it removes a raw-pointer hazard rather than adding one.
Level of scrutiny
High. This is a hot-path rewrite of buffer ownership on every streamed Bun.serve request body and every streamed fetch response body, with a claimed +46% req/s / -24% peak RSS on the target workload. It removes a Strong GC root and a raw *mut [u8], changes the StreamResult variant returned to process_result (which drives JS-visible chunk shape), and adds a cross-layer sentinel (-1) that three consumers (C++, built-in JS, start_from_js) must agree on. Per the repo's REVIEW.md this is squarely in "performance / touching hot paths" territory where a maintainer should confirm the ownership model and the removed-field audit.
Other factors
- The PR has been through 7+ automated iterations; every inline comment I raised (sleep-based pacing → per-chunk ack, chunk-count assertion, unhandled-rejection noise on the error path, dead
pending_value, redundantm_sourceOwnsChunksdrain disjunct) was addressed in follow-up commits, and the CodeRabbit finding was withdrawn. - The bug hunting system found nothing this run.
- I checked that
pull_from_js's new&mut []branch is only reachable fromByteStream(the soleReadyOwnedsource):FileReader/ByteBlobLoaderstill receive a real view from bothmaterializeNativeSourceandnative-readable.ts, and the internal source objects are not user-reachable. - I checked that removing
nativeAdjustChunkSizefrom theJSArrayBufferViewarm ofnativeDecodePullResultdoes not regress the pull-source resize heuristic:FileReader/ByteBlobLoaderreturn numeric byte counts (the number arm still adjusts), andm_hasResized = trueis set at construction for owned sources so the heuristic is a no-op there anyway. - Test coverage: two tests exercise
for await (req.body)andReadable.fromWeb(req.body); both assertbacking === len && off === 0per chunk, wire every failure event to reject, use per-chunk acks instead of sleeps, and assert the multi-chunk precondition. The PR description reportsserve.test.ts -t body(50 tests incl. backpressure) andstreams.test.jsalso pass.
Given the scope (hot path, GC-root removal, cross-language protocol change), I'm deferring rather than approving.
|
Updated 11:05 PM PT - Aug 1st, 2026
❌ @robobun, your commit e179877 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36736That installs a local version of the PR into your bun-36736 --bun |
…r the C++ adapter Readable.fromWeb's native-readable.ts calls _read ahead of downstream consumption (node:stream Readable's maybeReadMore fires while state.length < hwm regardless of src.pause()), so handing it the whole ByteStream.buffer and signal_drained()ing on every pull let the fetch socket re-resume one recv ahead of the writer. fetch-backpressure.test.ts download-proxy window went from ~170 MB back to ~250-280 MB. on_pull/on_data now dispatch on whether the caller passed a pull view: - buffer empty (C++ native-source adapter, m_sourceOwnsChunks): Owned handoff, right-sized chunks, signal_drained when self.buffer is taken. - view present (native-readable.ts): main's copy-into-view + offset + IntoArray path, which meters chunks to the view's size and only signals once self.buffer is fully drained. native-readable.ts keeps passing a view sized to Readable's hwm; the -1 from start() just sets kHasResized so the view stays at that size. Readable.fromWeb test dropped accordingly; streams-leak.test.ts's pull-buffer-reuse assertion updated to the right-sized invariant it was really guarding.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/runtime/webcore/ByteStream.rs (4)
647-666: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
value()emptiness is overloaded as the "no parked pull" signal, which the owned handoff breaks.value()both consumes the strong JS view handle and reports its presence. The code then treats an empty result as "no pull is parked". The new owned handoff parks a pull without ever setting a view, so that proxy is no longer valid. Track pending-pull state throughpending.stateand keepvalue()for consumption only.
src/runtime/webcore/ByteStream.rs#L647-L666: replace the!view.is_empty()gate with a check onself.pending.get().state == streams::PendingState::Pendingso a parked owned pull is settled on cancel.src/runtime/webcore/ByteStream.rs#L135-L143: add a non-consuminghas_pending_value()predicate and use it for thedebug_assert!calls at lines 564 and 586, so presence checks never mutate the strong handle.🤖 Prompt for 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. In `@src/runtime/webcore/ByteStream.rs` around lines 647 - 666, Update src/runtime/webcore/ByteStream.rs lines 647-666 in on_cancel to use pending.state == streams::PendingState::Pending instead of the consuming value() emptiness check, ensuring parked owned pulls are settled. Add a non-consuming has_pending_value() predicate at lines 135-143 and use it in the debug_assert! calls at lines 564 and 586; keep value() exclusively for consuming the strong view handle.
496-518: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
appendnever returnsErr, so theResultforces dead panic handling at every call site.Every arm ends in
Ok(()).Vec::with_capacityandextend_from_sliceuse the global allocator and abort on allocation failure rather than returningAllocError. The three call sites at lines 252, 466, and 491 therefore carry an unreachableunwrap_or_else(|_| panic!("Out of memory while copying request body")).Change the signature to return
()and drop theunwrap_or_elseat the call sites.Note: I am not suggesting
bun_core::handle_oomhere, because this path uses infallible global-allocator construction.Based on learnings:
bun_core::handle_oomis intended only for fallible allocation APIs that returnResult<_, bun_alloc::AllocError>; do not wrap infallible construction such asVec::with_capacity.🤖 Prompt for 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. In `@src/runtime/webcore/ByteStream.rs` around lines 496 - 518, Change ByteStream::append to return unit instead of Result<(), bun_alloc::AllocError>, preserving its existing arm behavior without wrapping results in Ok. Update all three append call sites to remove the unreachable unwrap_or_else panic handling and invoke append directly; do not add bun_core::handle_oom around the infallible vector allocation path.Source: Learnings
135-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid calling the side-effecting
value()fromdebug_assert!.
value()clears the strong handle throughclear_without_deallocation(). Lines 564 and 586 call it insidedebug_assert!.debug_assert!compiles out in release builds, so the clear runs only in debug builds. Today the assert passes only when the handle is already empty, so the observable state matches. The pattern is still fragile: any future change that makes the assert non-trivially true would silently drop a rooted view in debug builds only.Use a non-consuming predicate for the assertions and keep
value()for the consuming call sites.♻️ Proposed non-consuming predicate
fn value(&self) -> JSValue { self.pending_value.with_mut(|pv| { let Some(result) = pv.get() else { return JSValue::ZERO; }; pv.clear_without_deallocation(); result }) } + + #[inline] + fn has_pending_value(&self) -> bool { + self.pending_value.get().get().is_some() + }Then at lines 564 and 586:
- debug_assert!(self.value().is_empty()); + debug_assert!(!self.has_pending_value());🤖 Prompt for 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. In `@src/runtime/webcore/ByteStream.rs` around lines 135 - 143, Replace the side-effecting value() calls used by the debug_assert! checks with a non-consuming predicate that inspects whether pending_value is empty, while preserving the existing assertion behavior at both call sites. Keep value() unchanged and retain it only where consuming the stored handle is intended.
433-476: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winUse
!has_remainingto guard terminal completion.
to_copy_len <= pending_buffer_lenis always true becauseto_copy_lenis the minimum of both values. If a final chunk exceeds the pull view, the code returnsIntoArrayAndDoneand appends the tail tobuffer.ReadableStreamthen marks the source closed, so the tail is not requested or delivered.Use
self.has_received_last_chunk.get() && !has_remaining. This predicate is also present on the base revision, so this is an existing data-loss bug, not a regression from this change.🤖 Prompt for 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. In `@src/runtime/webcore/ByteStream.rs` around lines 433 - 476, Update the terminal-completion guard in the ByteStream pull handling to use self.has_received_last_chunk.get() && !has_remaining instead of comparing to_copy_len with pending_buffer_len. Ensure oversized final chunks remain IntoArray while their tail is appended, and only mark the stream done when no bytes remain.src/js/internal/streams/native-readable.ts (1)
39-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeclare
kRemainingChunkasBuffer | undefined.handleNumberResultreturnsundefinedwhen a pull consumes the complete buffer, and the field is unset during construction.getRemainingChunkreplenishes it beforeptr.pull, soptr.pullstill receives aBuffer.🤖 Prompt for 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. In `@src/js/internal/streams/native-readable.ts` at line 39, Update the kRemainingChunk field declaration to use the nullable type Buffer | undefined, reflecting that it is unset during construction and may be cleared by handleNumberResult when the buffer is fully consumed. Preserve getRemainingChunk’s replenishment before ptr.pull so the pull operation continues receiving a Buffer.
🤖 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 `@src/runtime/webcore/ByteStream.rs`:
- Around line 562-583: Extract the duplicated terminal-result handling into a
shared helper near the surrounding ByteStream methods, covering the
has_received_last_chunk check, pending Result::Err extraction, and fallback to
Result::Done. Replace both the terminal branch in the empty-buffer path and the
matching logic around the later branch near lines 627-636 with calls to that
helper, preserving all existing non-terminal behavior.
In `@test/js/bun/http/serve-request-body-pipeline-memory.test.ts`:
- Around line 109-114: Add a regression test for the Readable.fromWeb(req.body)
path described by the surrounding invariant comment. Consume the request body
through Readable.fromWeb and assert the copy-into-view behavior by verifying
chunk backing buffers exceed their chunk lengths or are shared across chunks,
inverting the checkRightSized assertion used for the C++ adapter path. Keep the
existing adapter test unchanged and ensure the new assertion detects a future
owned-buffer handoff regression.
---
Outside diff comments:
In `@src/js/internal/streams/native-readable.ts`:
- Line 39: Update the kRemainingChunk field declaration to use the nullable type
Buffer | undefined, reflecting that it is unset during construction and may be
cleared by handleNumberResult when the buffer is fully consumed. Preserve
getRemainingChunk’s replenishment before ptr.pull so the pull operation
continues receiving a Buffer.
In `@src/runtime/webcore/ByteStream.rs`:
- Around line 647-666: Update src/runtime/webcore/ByteStream.rs lines 647-666 in
on_cancel to use pending.state == streams::PendingState::Pending instead of the
consuming value() emptiness check, ensuring parked owned pulls are settled. Add
a non-consuming has_pending_value() predicate at lines 135-143 and use it in the
debug_assert! calls at lines 564 and 586; keep value() exclusively for consuming
the strong view handle.
- Around line 496-518: Change ByteStream::append to return unit instead of
Result<(), bun_alloc::AllocError>, preserving its existing arm behavior without
wrapping results in Ok. Update all three append call sites to remove the
unreachable unwrap_or_else panic handling and invoke append directly; do not add
bun_core::handle_oom around the infallible vector allocation path.
- Around line 135-143: Replace the side-effecting value() calls used by the
debug_assert! checks with a non-consuming predicate that inspects whether
pending_value is empty, while preserving the existing assertion behavior at both
call sites. Keep value() unchanged and retain it only where consuming the stored
handle is intended.
- Around line 433-476: Update the terminal-completion guard in the ByteStream
pull handling to use self.has_received_last_chunk.get() && !has_remaining
instead of comparing to_copy_len with pending_buffer_len. Ensure oversized final
chunks remain IntoArray while their tail is appended, and only mark the stream
done when no bytes remain.
🪄 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: 71cc2ed7-60b3-4f67-a542-5a2483e8d4a5
📒 Files selected for processing (4)
src/js/internal/streams/native-readable.tssrc/runtime/webcore/ByteStream.rstest/js/bun/http/serve-request-body-pipeline-memory.test.tstest/js/web/streams/streams-leak.test.ts
The Owned-handoff Pending path parks without setting pending_value, so the previous !view.is_empty() check left a parked pull unsettled on cancel, pinning the ReadableStream via the protected promise. Gate on pending.state == Pending instead, which is the invariant both park paths establish. Also make the on_pull Owned-path debug_assert non-consuming.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/ByteStream.rs (1)
392-393: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute this allocation through Bun’s OOM handling.
temp.slice().to_vec()uses an infallible allocation API for user-reachable stream data. An allocation failure bypassesbun_core::handle_oomand the runtime’s controlled error path. Use the repository’s fallible allocation or OOM-handling helper before storing the result inPending.As per coding guidelines, user-reachable allocation failures must not use generic panic or allocation-failure paths; route OOM through
bun_core::handle_oom.🤖 Prompt for 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. In `@src/runtime/webcore/ByteStream.rs` around lines 392 - 393, Update the Temporary and TemporaryAndDone handling in the stream result match to replace temp.slice().to_vec() with the repository’s fallible allocation or Bun OOM helper, routing failures through bun_core::handle_oom before storing the owned data in Pending.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/ByteStream.rs`:
- Around line 392-393: Update the Temporary and TemporaryAndDone handling in the
stream result match to replace temp.slice().to_vec() with the repository’s
fallible allocation or Bun OOM helper, routing failures through
bun_core::handle_oom before storing the owned data in Pending.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d41a59c2-0037-48d4-a82d-676515c303c2
📒 Files selected for processing (1)
src/runtime/webcore/ByteStream.rs
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/ByteStream.rs:587-590—on_cancelstill gatespending.run()on!self.value().is_empty()(line 668), but the new owned-handoffon_pullbranch returnsResult::Pendingat line 591 without callingset_value(view)— so cancellingreq.bodywhile a pull is parked via the C++ adapter never runspending.run(), and theprotect()ed pull promise (streams.rs:789) is never unprotected.finalizewon't rescue it either sinceon_cancelsetdone = truefirst. c306505 had already fixed this gate toself.pending.get().state == streams::PendingState::Pending; fe6bc93 reverted it when re-addingpending_value— restoring that gate (or callingp.run()unconditionally, since it self-gates) fixes the leak.Extended reasoning...
What the bug is
fe6bc93 restored the
native-readable.tsview-copy path and with iton_cancel's original gate:fn on_cancel(&self) { let view = self.value(); // reads pending_value; None → JSValue::ZERO ... self.done.set(true); self.pending_value.with_mut(|pv| pv.deinit()); if !view.is_empty() { // ZERO.is_empty() → true → block SKIPPED self.pending_buffer.set(Self::empty_pending_buffer()); self.pending.with_mut(|p| { p.result.release(); p.result = streams::Result::Done; }); self.pending.with_mut(|p| p.run()); // ← never reached on the owned path } ... }
But the new owned-handoff
on_pullbranch (entered when the C++ adapter passes no view —buffer.is_empty()at ByteStream.rs:568) returnsstreams::Result::Pending(self.pending.as_ptr())at line 591 without callingself.set_value(view). That is by design:on_datadispatches onpending_value.get().get().is_none()(line 373) to pick theOwnedresolve path instead of copying into a view. So when a pull is parked via the C++ adapter,pending.state == Pendingandpending_value == None— a stateon_cancel's gate does not recognise.Code path that triggers it
for await (const chunk of req.body)(orreader.read()) →nativeSourcePullImpl→nativeGetInternalBufferreturnsnullptr(m_sourceOwnsChunks) →pull_from_jscallson_pull(&mut [], undefined).on_pull:buffer.is_empty()→self.bufferempty,!has_received_last_chunk→ returnsResult::Pending(self.pending.as_ptr()).set_valueis not called;pending_valuestaysNone.process_result→Result::Pending::to_js(streams.rs:784–790) →Pending::promise()setsstate = Pending,future = PendingFuture::Promise{ raw *mut JSPromise }, thenpromise_js.protect()— the promise is now a GC root.- Client aborts / handler
breaks out offor await/reader.cancel()→nativeSourceCancel→invokeNativeHandleCancel→cancel_from_js→NewSource::cancel()→ByteStream::on_cancel. on_cancel:self.value()readspending_value→None→ returnsJSValue::ZERO.self.done.set(true).!view.is_empty()is false → the block containingpending.run()is skipped.- Later, when the wrapper is GC'd,
ByteStream::finalizeruns — but itspending.run()/run_on_next_tick()is gated onif !self.done.get(), anddonewas set totruein step 5. So this rescue path is also skipped.
Why existing code doesn't prevent it
- On
mainthis couldn't happen: the onlyon_pullpath always calledself.set_value(view)before returningPending(there was adebug_assert!(!buffer.is_empty())), soon_cancel's!view.is_empty()gate was always satisfied when a pull was parked. - c306505 (earlier in this PR) correctly changed the gate to
self.pending.get().state == streams::PendingState::Pendingwhen it removedpending_valueentirely. fe6bc93 re-addedpending_value(needed for thenative-readable.tscopy path) and revertedon_cancelto the!view.is_empty()gate — but the ownedon_pullbranch that leavespending_valueempty is still there. Pending::run()self-gates onstate == Pending(streams.rs:650), so calling it when no pull is parked is a no-op — the!view.is_empty()gate is not protecting against anything the state check doesn't already handle.
Impact
Every cancel-while-pull-parked on the C++ adapter path — the primary path this PR optimizes:
for await (req.body),reader.read(),pipeline(req.body, …)viapumpToNode— leaves the pending promise unsettled. The promise wasprotect()ed into_jsand is onlyunprotect()ed viafulfill_promiseinsiderun(), andPendingFuture::Promiseholds a raw*mut JSPromisewith noDropimpl. So this is a permanent GC root per aborted upload / early-break, leaking the JSPromise plus its reaction chain (the adapter cell attached viaperformPromiseThenWithContext). This is a regression onmainand lands squarely in REVIEW.md → Error handling: "Every error/abort/timeout path actively completes the operation. Settle every pending promise slot (an unsettled promise pins objects and hangs callers forever)."The consumer does not hang —
nativeSourceCancelsevers the adapter and the ReadableStream's cancel promise resolves independently — so this is a leak, not a deadlock. But cancel-while-parked is the common case for aborted uploads (client disconnects mid-body while the server is waiting for the next chunk).Step-by-step proof
Server handler:
for await (const c of req.body) { break; }on aBun.serverequest with a body larger than one chunk.- First iteration parks:
on_pull([], undefined)→Pending;pending_value = None,pending.state = Pending, promiseprotect()ed. on_datafires with the first chunk → owned branch (pending_value.get().get().is_none()) →pending.run()resolves withOwned(chunk)→ promise unprotected. ✅- Second iteration parks: same as step 1 — new promise
protect()ed,pending_value = None. - Handler
breaks → async iterator'sreturn()→reader.cancel()→ … →ByteStream::on_cancel. self.value()=ZERO(pending_value is None);done.set(true);!ZERO.is_empty()= false →pending.run()skipped. Promise from step 3 staysprotect()ed.finalize(whenever the wrapper is collected):!self.done.get()= false → skipped.- The step-3 JSPromise is a permanent GC root. Repeat per request.
Fix
Restore c306505's gate in
on_cancel:if self.pending.get().state == streams::PendingState::Pending { self.pending_buffer.set(Self::empty_pending_buffer()); self.pending.with_mut(|p| { p.result.release(); p.result = streams::Result::Done; }); self.pending.with_mut(|p| p.run()); }
(or drop the gate entirely —
Pending::run()already self-gates onstate == Pendingat streams.rs:650). Thepending_bufferclear andresultreset are harmless whenpending_valuewas never set.
|
CI build 87535 at e179877: the three cancel-leak failures from 87531 are resolved. Remaining failures are unrelated to this diff:
Diff is green on the paths it touches. Ready for review. |
There was a problem hiding this comment.
I didn't find any bugs on this iteration, but this is a hot-path memory-ownership change to the request/response body streaming path (ByteStream on_pull/on_data/on_cancel, the C++ native-source adapter, and the Start::ReadyOwned protocol) with a deliberate design split between the C++ adapter and Readable.fromWeb for backpressure — worth a human sign-off.
What was reviewed:
- The new
buffer.is_empty()branch inon_pulland thepending_value.is_none()branch inon_data: both correctly mirror the existing terminal-error / Done / Owned handling and callsignal_drained()beforepending.run(). on_cancel's gate change from!view.is_empty()→pending.state == Pending: equivalent for the view path and correct for the new no-view path (previouslyviewwas.zerothere, sopending.run()would have been skipped).m_sourceOwnsChunksin the C++ adapter:nativeGetInternalBufferreturns null →pull(jsUndefined())→pull_from_jspasses&mut [];nativeDecodePullResult's JSArrayBufferView arm returnsview ? view : jsUndefined()so PendingView stays cleared.high_water_markremoval traced to its only remaining reader (on_start), and thenative-readable.tsresult < 0branch just setskHasResized(no owned-handoff there, preserving the metered-view backpressure covered byfetch-backpressure.test.ts).
Extended reasoning...
Overview
This PR changes how ByteStream (the push source behind Bun.serve request bodies and fetch response bodies) hands data to the C++ native-source adapter. Instead of copying each incoming chunk into a persistent ~256-512 KiB scratch Uint8Array and returning IntoArray subarrays, the adapter now signals Start::ReadyOwned (encoded as start() → -1), skips allocating a pull view, and on_pull/on_data hand the buffered Vec<u8> straight to JS as Owned/OwnedAndDone. The Readable.fromWeb path (native-readable.ts) is deliberately kept on the old copy-into-view path because Node's Readable calls _read ahead of downstream consumption and relies on the view size for producer-side metering. Files touched: ByteStream.rs (on_start/on_pull/on_data/on_cancel, drop high_water_mark), ReadableStream.rs (pull_from_js empty-view path, start_from_js -1 encoding), streams.rs (new Start::ReadyOwned), BunStreamSource.{cpp,h} (m_sourceOwnsChunks, skip pull-view allocation, drop chunk-size growth on ArrayBufferView results), Body.rs (drop dead high_water_mark writes), native-readable.ts (handle start()<0 by pinning kHasResized), plus a new test and an updated streams-leak.test.ts assertion.
Security risks
None identified. This is an internal buffer-ownership optimization on an existing data path; no new user-controlled input parsing, no auth/crypto/permissions surface. The Vec → JS handoff goes through the existing streams::Result::Owned → to_js machinery (same allocator/deallocator pairing as before). The unreachable!() in on_data's new match covers only IntoArray*/Pending variants, which no producer feeds into on_data.
Level of scrutiny
High. This is production-critical hot-path code (every streamed Bun.serve request body and fetch response body), it changes memory ownership across the Rust↔C++↔JS boundary, and it encodes a non-obvious architectural split (C++ adapter gets Owned handoff; native-readable.ts stays on metered copy) whose backpressure justification a maintainer should confirm. The PR went through ~8 review iterations already (dead-field cleanup, test flakiness, redundant drain path, comment-cop), which is itself a signal that the surface is subtle.
Other factors
The bug-hunting system found nothing on the final revision; every prior inline finding (mine and CodeRabbit's) is resolved. Test coverage is reasonable: the new serve-request-body-pipeline-memory.test.ts asserts the right-sized-backing invariant deterministically via per-chunk acks, streams-leak.test.ts is updated to the same invariant, and the Readable.fromWeb backpressure regression the earlier iteration hit is guarded by the existing fetch-backpressure.test.ts download-proxy test. The on_cancel gate change and the removal of nativeAdjustChunkSize from the ArrayBufferView arm both look correct but are the kind of one-line semantic shifts a human familiar with the streaming layer should eyeball. Not approving.
2 participants
What
ByteStream(theBun.serverequest-body / fetch response-body source) is a push source: chunks arrive viaon_dataand are held in a nativeVec<u8>untilon_pulltakes them. Onmain,on_pullcopied those bytes into the native-source adapter's ~256-512 KiB scratchUint8Arrayand returnedIntoArray, andon_data's pending path did the same; each chunk surfaced to JS as a subarray over that whole backing.This PR adds an owned-handoff path for the C++ native-source adapter (the
for await/reader.read()path thatpipeline(req.body, …)drives viapumpToNode): when the adapter passes no pull view,on_pull/on_datahand theVecstraight to JS as anOwnedStreamResultand the adapter skips allocating its scratch view.The
Readable.fromWebpath (native-readable.ts) is left onmain's copy-into-view + tail-subarray metering.node:stream.Readablecalls_readwhenever its own buffer is underhighWaterMark, independently of downstream backpressure, so handing it whole-buffer chunks and resuming the producer on every pull let the upstream socket run one recv ahead of the writer; the view-size metering is what keeps the producer paused until the Readable's buffer is drained.Mechanically:
ByteStream::on_startreturns a newStart::ReadyOwnedvariant;start_from_jsencodes it as-1.materializeNativeSourcetreats a negative start result asm_sourceOwnsChunks:nativeGetInternalBufferskips the view allocation and the pull call passesjsUndefined()for the view.pull_from_jspasses an empty slice when the view is missing.ByteStream::on_pulldispatches onbuffer.is_empty(): empty (C++ adapter) → takeself.bufferasOwned/OwnedAndDone; non-empty (native-readable.ts) →main's copy-into-view +offset+IntoArray.ByteStream::on_datapending dispatches onpending_valuebeing set: not set → resolve withOwned/OwnedAndDone; set →main's copy + spill path.nativeDecodePullResultno longer growsm_chunkSizewhen the source returns its ownJSArrayBufferView.native-readable.tstreats-1fromstart()by settingkHasResizedso the pull buffer stays at Readable's ownhighWaterMark.high_water_markonByteStreamis now dead and removed.Measurement
Route under test (matches the thread's Hono repro):
Load:
oha -c 100 -n 2000 -m POST -D <2 MiB file>, release build, Linux x64, 3-5 reps median.mainf91d5c9VmHWMData integrity (100 concurrent × 2 MiB uploads,
md5sumof each written file) is identical before and after.JS-visible effect
On
main,for await (const chunk of req.body)on a 2 MiB body yields subarrays over a single ~516 KiB backing:[{"len":8192,"backing":528384,"off":0}, {"len":8192,"backing":528384,"off":8192}, {"len":109568,"backing":528384,"off":16384}, ...]With this PR each chunk is its own allocation:
[{"len":8192,"backing":8192,"off":0}, {"len":8192,"backing":8192,"off":0}, {"len":109568,"backing":109568,"off":0}, ...]Readable.fromWeb(req.body)is unchanged (chunks are still subarrays over the native-readable pull buffer, sized to Readable'shighWaterMark).Verification
bun bd test test/js/bun/http/serve-request-body-pipeline-memory.test.tsThe test asserts every yielded
for await (req.body)chunk hasbacking === byteLengthandbyteOffset === 0; onmainthey observebacking = 528384.test/js/web/streams/streams-leak.test.ts's native-ReadableStream assertion is updated to the same right-sized invariant (that test also reads viafor await).Also passing:
bun bd test test/js/bun/http/serve.test.ts -t body(50 tests includingrequest body backpressure),test/js/web/fetch/fetch-backpressure.test.ts(44 tests including theReadable.fromWebdownload-proxy window), andtest/js/web/streams/streams.test.js(the two pre-existing debug-build timeouts there reproduce onmainas well).Related
Supersedes the narrower #31128 (zero-copy
on_pullonly); overlaps with #31126'sByteStream::appendadopt-owned path.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/streams/streams-leak.test.ts