webstreams: make Blob/File/body streams byte streams so BYOB readers attach - #33927
webstreams: make Blob/File/body streams byte streams so BYOB readers attach#33927robobun wants to merge 11 commits into
Conversation
The File API requires Blob.prototype.stream() to return a readable byte
stream, and Node.js and browsers do. Bun's lazily-materialized native
sources (Blob, Bun.file, fetch/Request/Response bodies) always installed
a ReadableStreamDefaultController, so getReader({ mode: "byob" }) threw
'ReadableStreamBYOBReader needs a ReadableByteStreamController'.
materializeNativeSource now installs a ReadableByteStreamController (the
native pull already fills a caller-provided buffer, so when a pull-into
descriptor is pending the BYOB view is passed straight through and
answered with respond(n); with no pull-into the adapter keeps its own
scratch buffer and enqueue()s the filled prefix). SourceKind::Native
moves to the byte-controller pull/cancel dispatch, getReader() and
new ReadableStreamBYOBReader() materialize a NativePending stream before
locking, and ReadableStream__empty now returns a closed byte stream so
an empty Blob's stream also accepts a BYOB reader.
|
Updated 9:01 PM PT - Jul 16th, 2026
❌ @Jarred-Sumner, your commit 2c0400f has 8 failures in
🧪 To try this PR locally: bunx bun-pr 33927That installs a local version of the PR into your bun-33927 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesNative sources now use byte-stream controllers throughout pull, BYOB response, enqueue, close, cancellation, and error paths. Native-pending streams materialize before BYOB reader setup, empty streams become byte streams, reader-release cleanup moves to byte-controller handling, and Blob/file stream tests cover reader and memory behavior. Native byte-stream integration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/JSReadableByteStreamController.cpp (2)
115-168: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment:
Nativemissing from documented reachable-kind set.The comment above
performByteControllerPullAlgorithmstill says the reachableSourceKindset on a byte controller is{JavaScript, Nothing, ByteTeeBranch}, but the switch below it now also handlesSourceKind::Native.JSReadableByteStreamController.h's analogous comment (lines 81-83) was correctly updated to addNative— this file's copy of the same invariant was not, creating a "one source of truth" mismatch for the same documented contract.📝 Proposed fix
-// The [[pullAlgorithm]] dispatch. The reachable kind set on a byte controller is exactly -// {JavaScript, Nothing, ByteTeeBranch}; the switch is total over SourceKind. +// The [[pullAlgorithm]] dispatch. The reachable kind set on a byte controller is exactly +// {JavaScript, Nothing, ByteTeeBranch, Native}; the switch is total over SourceKind.🤖 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/JSReadableByteStreamController.cpp` around lines 115 - 168, Update the doc comment above performByteControllerPullAlgorithm to include Native in the documented reachable SourceKind set, matching the switch implementation and the analogous comment in JSReadableByteStreamController.h.Source: Coding guidelines
170-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSame stale-set comment issue for the cancel dispatch.
"Same reachable kind set as the pull dispatch." at line 170 inherits the same staleness once the pull-dispatch comment above is corrected to include
Native.🤖 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/JSReadableByteStreamController.cpp` around lines 170 - 202, Update the comment above performByteControllerCancelAlgorithm to accurately describe the currently reachable algorithm kinds, including Native, instead of referencing the pull dispatch’s kind set. Keep the comment aligned with the switch cases and remove the stale cross-reference.
🤖 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/web/fetch/blob.test.ts`:
- Around line 603-618: Make the reader-handoff test non-vacuous by enlarging the
payload beyond one default read chunk, asserting that the first read is partial,
and verifying the BYOB read returns the expected next bytes from the payload
after r1.releaseLock(). Update the assertions in the test “releaseLock after a
default reader then attach a BYOB reader” so immediate EOF is not accepted when
bytes should remain.
---
Outside diff comments:
In `@src/jsc/bindings/webcore/streams/JSReadableByteStreamController.cpp`:
- Around line 115-168: Update the doc comment above
performByteControllerPullAlgorithm to include Native in the documented reachable
SourceKind set, matching the switch implementation and the analogous comment in
JSReadableByteStreamController.h.
- Around line 170-202: Update the comment above
performByteControllerCancelAlgorithm to accurately describe the currently
reachable algorithm kinds, including Native, instead of referencing the pull
dispatch’s kind set. Keep the comment aligned with the switch cases and remove
the stale cross-reference.
🪄 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: 9bf958d7-e28d-4f21-b8dc-a6c772f2e7fb
📒 Files selected for processing (11)
src/jsc/bindings/webcore/streams/BunStreamSource.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.hsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.cppsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.hsrc/jsc/bindings/webcore/streams/JSReadableStream.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamBYOBReader.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamDefaultController.cppsrc/jsc/bindings/webcore/streams/StreamsForward.hsrc/jsc/bindings/webcore/streams/WebStreamsExports.cppsrc/jsc/bindings/webcore/streams/WebStreamsInternals.htest/js/web/fetch/blob.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/webcore/streams/JSReadableStreamDefaultController.cpp
…ent; strengthen handoff test - nativeDecodePullResult: only Respond(N) when a pull-into descriptor is still present (cancel/release can clear it before an async pull settles), and guard on a non-null view before reading its length. - nativeSourceSever: clear m_pendingIsBYOB alongside the other per-pull state so a late fulfilment does not take the BYOB branch after teardown. - Update the reachable-kind comment above performByteControllerPullAlgorithm to include Native, matching the header and the switch. - Reworked the default-then-BYOB test: use a 40 KB payload so the first default read is partial, and assert the BYOB read returns the next 32 bytes of the payload rather than accepting immediate EOF.
|
Re: #29167 as a potential duplicate: that PR modified The outside-diff comment in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/webcore/streams/BunStreamSource.cpp:530-536— On the default-reader path,nativeDecodePullResultcomputes atailsubarray sharing the same backing ArrayBuffer astoEnqueue, butnativeByteControllerEnqueue→readableByteStreamControllerEnqueueunconditionally callstransferArrayBufferImplwhich detaches that shared buffer — so the returnedtailis always detached,nativeGetInternalBufferallocates a fresh 256 KiB–2 MiB buffer on the next pull, and the sliding-tail reuse (which worked before this PR becausereadableStreamDefaultControllerEnqueuenever transferred) is now dead. This is a per-pull allocation regression on the hot default-reader path (Bun.file().stream(), fetch/Response bodies, subprocess pipes) whenever a native pull returns fewer bytes than the scratch buffer; the comment onnativeByteControllerEnqueue("nothing else aliases") is false for this call site. One fix is to passautoAllocateChunkSizeso default-reader reads route throughrespond(); alternatively, copy the filled prefix into a fresh buffer before enqueue and keep the original scratch buffer as the pending view.Extended reasoning...
What the bug is
The PR migrates native streams from a
ReadableStreamDefaultControllerto aReadableByteStreamController. On the default-reader (non-BYOB) path withautoAllocateChunkSizeunset,nativeDecodePullResulthandles a partial write (written < view.length) by splitting the scratch buffer into two subarrays over the same backingArrayBuffer:toEnqueue = view[0..count)is enqueued andtail = view[count..)is returned as the nextm_pendingView. The new enqueue path (nativeByteControllerEnqueue→readableByteStreamControllerEnqueue) unconditionally callstransferArrayBufferImpl(*buffer), which detaches the sourceArrayBuffer(buffer.transferTo(vm, contents)inWebStreamsMisc.cpp). Becausetailshares that buffer, it is detached the momenttoEnqueueis enqueued.Code path
materializeNativeSourcecallssetUpReadableByteStreamController(..., std::nullopt)— noautoAllocateChunkSize.- A default reader's
read()reachesJSReadableByteStreamController::pullSteps; withm_autoAllocateChunkSize == 0it appends no pull-into descriptor, sonativePendingPullIntoViewreturnsnullptrandhasBYOBRequest = false. nativeGetInternalBufferallocates (or reuses) a 256 KiBUint8Array; the nativepull(view, closer)writesNbytes and returnsN.nativeDecodePullResult(result.isNumber(),hasBYOBRequest=false,N < view.length) createstoEnqueue = uint8Subarray(view, 0, N)andtail = uint8Subarray(view, N, len-N)— both overview.buffer.nativeByteControllerEnqueue(controller, toEnqueue)→readableByteStreamControllerEnqueuedoestransferArrayBufferImpl(globalObject, *buffer)before any reader-type branch, detachingview.buffer.tailis returned and stored inadapter->m_pendingView.- On the next pull,
nativeGetInternalBufferchecks!view->isDetached()→ false → allocates a freshchunkSizeUint8Array.
Why this didn't happen before
Pre-PR, this path called
readableStreamDefaultControllerEnqueue, which passes the JSValue through to the read request / queue with no transfer or detach. The tail subarray stayed valid,nativeGetInternalBuffersaw a live view over a ≥ chunkSize buffer, and one 256 KiB allocation was reused for the whole stream via sliding subarrays. The comment onnativeByteControllerEnqueue— "The native side hands over a fresh buffer that nothing else aliases, so the byte-controller transfer/detach is harmless" — is true for theonDrainandchunk-return call sites, but false for this partial-write site:tailaliases the same buffer.Impact
Every default-reader pull on a native stream (
Bun.file().stream(), fetch/Response bodies, subprocess pipe streams) that returns fewer bytes than the scratch buffer — the common case for file reads, sockets, and pipes — now allocates a fresh 256 KiB–2 MiB buffer instead of reusing the previous one. This affectsfor await (const chunk of blob.stream()),response.body.getReader(), and the internalreadStreamIntoSink/readManypumps that acquire a default reader. Correctness is preserved (the delivered bytes are right), but the tail-computation atBunStreamSource.cpp:527-529is effectively dead code and the buffer-reuse optimization that motivated it no longer fires.Step-by-step example
Streaming a 1 MiB
Bun.filewith a default reader, where each kernelreadreturns 64 KiB into the 256 KiB scratch buffer:- Before: pull #1 allocates one 256 KiB buffer, enqueues
[0,64K), keeps[64K,256K)as tail; pull #2 reuses that tail (backing buffer is 256 KiB ≥ chunkSize), and so on — ~1 allocation total. - After: pull #1 allocates 256 KiB, enqueues
[0,64K)→ detaches the buffer, stores detached tail; pull #2'snativeGetInternalBufferseesisDetached()and allocates a fresh 256 KiB; repeats every pull — ~16 × 256 KiB allocated for a 1 MiB file.
How to fix
Two straightforward options:
- Pass
autoAllocateChunkSize = adapter->m_chunkSizetosetUpReadableByteStreamControllerso a default reader creates a pull-into descriptor and the code takes thehasBYOBRequest→respond(N)path (which already handles partial fills correctly and doesn't need the tail hack). Then delete the now-dead tail-subarray branch. - Or, on the partial-write default-reader path, copy the
Nfilled bytes into a freshUint8Array(N)and enqueue that, returning the original (still-attached)viewas the pending view for reuse.
…roller Two regressions from moving native sources to a byte controller: - readableByteStreamControllerEnqueue detaches the chunk's backing buffer, so subarraying the scratch buffer into (head, tail) left the tail (and the scratch buffer) detached and forced a fresh 256KB allocation on every pull of the default-reader path. Copy the filled prefix into a fresh Uint8Array and keep the scratch buffer as the pending view so it is reused from offset 0. The now-unused uint8Subarray helper is deleted. - readableStreamReaderGenericRelease's updateRef(false) gate lived in the ControllerKind::Default arm only, so releasing a native stream's reader no longer dropped the event-loop keep-alive. The gate now lives in the ControllerKind::Byte arm (SourceKind::Native can only appear there).
|
Addressed the scratch-buffer-reuse regression in 90adb97: The other findings (cancel-vs-late-BYOB-respond, stale reachable-kind comment, vacuous handoff test) were already fixed in 7f8af8b before this review round ran. |
The native byte-controller path now copies each pull's bytes into a fresh Uint8Array (so the scratch buffer stays attached for reuse and is not detached by readableByteStreamControllerEnqueue). Each delivered chunk therefore owns its own small backing buffer instead of being a subarray of one shared 256KB buffer. The test's distinctBuffers.size < 8 assertion was a proxy for the real contract (total backing memory, which the original bug blew up to ~16MB); keep that contract and drop the sharing-count assertion.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/webcore/streams/BunStreamSource.cpp:533-539— On the default-reader path,nativeByteControllerEnqueue(toEnqueue)now detaches the adapter's scratch buffer (viareadableByteStreamControllerEnqueue→transferArrayBufferImpl), but the returnedtailis a subarray over that same buffer — som_pendingViewis stored detached,nativeGetInternalBuffer's!view->isDetached()check fails on the next pull, and a fresh 256KB–2MB buffer is allocated per pull instead of reusing the tail. Each default read also hands the user a small view whose.bufferis the full transferred chunkSize buffer. Pre-PRreadableStreamDefaultControllerEnqueuedid not transfer, so this is a per-chunk memory/allocation regression onBun.file()/fetch-body/subprocess-pipe streams read with a default reader. Fix: copy thecount-byte prefix into a fresh small buffer before enqueue on the non-BYOB path so the scratch buffer stays adapter-owned.Extended reasoning...
What the bug is
In
nativeDecodePullResult's non-BYOB numeric-result branch (hasBYOBRequest == false,result.isNumber(),written < view->length()):toEnqueue = uint8Subarray(globalObject, view, 0, count); auto* tail = uint8Subarray(globalObject, view, count, view->length() - count); newView = tail; ... nativeByteControllerEnqueue(globalObject, controller, toEnqueue); ... return newView; // stored as adapter->m_pendingView
toEnqueueandtailare subarrays over the sameArrayBuffer— the adapter-owned scratch buffer thatnativeGetInternalBufferallocated.nativeByteControllerEnqueueforwards toreadableByteStreamControllerEnqueue, whose first substantive step istransferArrayBufferImpl(globalObject, *buffer)(JSReadableByteStreamController.cpp), which callsbuffer.transferTo(...)and detaches the source buffer (WebStreamsMisc.cpp:79-104). Before this PR the call wasreadableStreamDefaultControllerEnqueue, which stores the JSValue in the queue with no transfer/detach.Consequences on the default-reader path
materializeNativeSourcepassesstd::nulloptforautoAllocateChunkSize, som_autoAllocateChunkSize == 0andpullStepscreates no pull-into descriptor for a default reader.nativePendingPullIntoViewreturnsnullptr,hasBYOBRequestisfalse, and every default-reader pull goes through this scratch-buffer branch.-
Tail reuse is dead. After the enqueue detaches the buffer,
tail(stored asm_pendingView) is a view over a detached buffer. On the next pull,nativeGetInternalBuffer's!view->isDetached()check fails and it allocates a freshm_chunkSize(256 KiB default, up to 2 MiB after resize) buffer. The whole "reuse the unfilled tail across pulls" optimisation thatm_pendingViewexists for is now dead code on the default-reader path. -
Each chunk pins a full-size backing buffer.
readableByteStreamControllerEnqueuedeliversconstructViewOfType(TypeUint8, transferredBuffer, byteOffset, byteLength)— acount-byteUint8Arraywhose.bufferis the full transferredm_chunkSizebuffer. A fetch body delivering 4 KB per pull now gives the user a 4 KB view over its own 256 KBArrayBuffer, one per read.
The comment above
nativeByteControllerEnqueue— "The native side hands over a fresh buffer that nothing else aliases, so the byte-controller transfer/detach is harmless" — is false for this call site: the tail aliases it, and the detach is not harmless.Step-by-step proof
Bun.file(bigPath).stream().getReader()→materializeNativeSourceinstalls a byte controller withautoAllocateChunkSize = std::nullopt,m_chunkSize = 256 KiB.await reader.read()→pullSteps:m_autoAllocateChunkSize == 0so no pull-into is appended;readableStreamAddReadRequest;readableByteStreamControllerCallPullIfNeeded→nativeSourcePull→nativeSourcePullImpl.nativePendingPullIntoViewseesm_pendingPullIntos.isEmpty()→ returnsnullptr;hasBYOBRequest = false.nativeGetInternalBufferallocates a fresh 256 KiBUint8Arrayand stores it asm_pendingView.handle.pull(view, closer)returns (or its promise fulfils with) e.g.4096.nativeDecodePullResult:written = 4096,hasBYOBRequest = false,view->length() = 262144, socount = 4096 < 262144.toEnqueue = view[0..4096),tail = view[4096..262144), both over the same 256 KiBArrayBuffer.nativeByteControllerEnqueue(controller, toEnqueue)→readableByteStreamControllerEnqueue:buffer = chunk->possiblySharedBuffer()(the 256 KiB scratch buffer);transferredBuffer = transferArrayBufferImpl(*buffer)→ the scratch buffer is now detached. A pending default read exists, so the user receivesconstructViewOfType(TypeUint8, transferredBuffer, 0, 4096)— a 4 KB view over a 256 KiB buffer.nativeDecodePullResultreturnstail;nativeStorePendingViewstores it asm_pendingView.tail->isDetached()is nowtrue.- Next
await reader.read()→ step 3 again.nativeGetInternalBuffer:m_pendingViewis set butview->isDetached()→ the reuse guard fails → another fresh 256 KiB allocation. Repeat for every pull.
Pre-PR, step 5 called
readableStreamDefaultControllerEnqueue, which enqueuedtoEnqueueas a plain JSValue with no transfer.tailstayed valid and step 7 reused it, so N partial pulls shared one scratch buffer instead of allocating N of them.Why the new tests don't catch it
The new tests either use BYOB readers (which take the
respond()path, not this branch) or in-memorynew Blob([...])payloads that hit thechunkSize == 0fully-buffered fast path (SourceKind::Nothing, one enqueue, no adapter). None exercise a default reader on a chunked native source across multiple pulls. Delivered bytes are correct, so no content assertion fails.Fix
On the non-BYOB partial-fill path, copy the
count-byte prefix into a freshArrayBufferbefore enqueue (e.g.JSC::ArrayBuffer::tryCreate+memcpy, or reusecloneAsUint8Array) so the scratch buffer is never handed toreadableByteStreamControllerEnqueueand remains adapter-owned. That keeps tail reuse working and gives the user a right-sized.buffer. Alternatively, callreadableByteStreamControllerEnqueueChunkToQueue/ a clone-then-enqueue helper directly for this branch. ThenativeByteControllerEnqueuecomment should also be corrected — it is only true for theonDrain/start-drain andchunk-result callers, not for the scratch-buffer subarray. -
Matches the sibling non-BYOB arm and nativeAdjustChunkSize above so a non-positive double is never cast to size_t.
|
The scratch-buffer-detach finding above is against commit 19633c6 and was already fixed in 90adb97 (the default-reader partial-fill path now copies the filled prefix into a fresh |
… the controller The prior default-controller path used highWaterMark=1, so after a satisfied read() the controller immediately pulled again and ByteStream/FileReader held a pending-pull promise through which close/error is delivered (those sources have no onClose push path). The byte controller was set up with highWaterMark=0, so after the first read emptied the queue desiredSize was 0, no proactive pull fired, and an abort that arrived with no read in flight stored the error on the source without ever reaching the controller. reader.closed stayed pending forever. Restoring highWaterMark=1 matches the old behaviour. Also drop the unused JSReadableStreamBYOBRequest.h include.
|
CI status: the stream-related failures ( |
…t as uncaught readableByteStreamControllerClose throws a TypeError when the head pull-into's bytesFilled is not a multiple of its element size (a Uint32Array BYOB view over a 3-byte source, etc). That throw is spec behaviour: close() has already errored the stream and rejected the pending read-into request. nativeSourceCallClose now swallows the exception when the stream is already Errored rather than routing it through Bun__reportError, so the user only sees the rejected read() promise and not an additional uncaughtException.
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/web/fetch/blob.test.ts`:
- Line 641: Replace the fixed await Bun.sleep(10) in the regression test with a
deterministic completion signal from the reader or stream close path, awaiting
that signal before asserting so late errors are captured reliably.
🪄 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: 2929c2ab-d76f-4307-a428-7926f0174a82
📒 Files selected for processing (2)
src/jsc/bindings/webcore/streams/BunStreamSource.cpptest/js/web/fetch/blob.test.ts
Both network-body faces route through materializeNativeSource and are fixed by this PR's byte-controller change, but were not directly covered by a test. Added to body-stream.test.ts so they run over both HTTP/1.1 and HTTP/3.
|
Added |
There was a problem hiding this comment.
No issues found in this pass, but this switches every native-backed ReadableStream (Blob/File/fetch/subprocess) from a default controller to a byte controller — a broad change to memory-sensitive JSC stream bindings that warrants a human look.
Checked: the BYOB respond()/enqueue() split in nativeDecodePullResult including the written > 0 guard; the scratch-buffer copy-before-enqueue so byte-controller detach doesn't defeat reuse; nativeSourceCallClose swallowing the spec partial-element close() throw only when the stream already transitioned to Errored; m_pendingIsBYOB cleared on sever/reject/fulfil so a late async settle can't mis-route; the updateRef(false) release gate moved from the Default arm to the Byte arm in readableStreamReaderGenericRelease; and the weakened streams-leak.test.ts assertion — the dropped distinctBuffers.size < 8 check is a genuine behaviour change (each chunk now owns a small copy), but the retained backing-bytes bound still catches the original 256KB-per-chunk regression.
Extended reasoning...
Overview
This PR changes materializeNativeSource (the lazy materialization path for Bun's $bunNativePtr streams — Blob, Bun.file, fetch/Request/Response bodies, subprocess pipes) to install a ReadableByteStreamController instead of a ReadableStreamDefaultController, so getReader({mode:'byob'}) works on those streams as the File API and Fetch specs require. It touches 11 files in src/jsc/bindings/webcore/streams/: the native-source adapter (BunStreamSource.{cpp,h}), both controller dispatch switches, both BYOB-reader entry points (getReader and the ReadableStreamBYOBReader constructor), the generic-release path in ReadableStreamOperations.cpp, ReadableStream__empty, and header/comment updates. Three test files gain ~210 lines of BYOB coverage; streams-leak.test.ts has one assertion removed and its comment rewritten.
Security risks
None identified. The change is internal to the streams subsystem and does not touch auth, crypto, permissions, or untrusted-input parsing. The buffer handling is between Bun's own native handles and JSC-managed ArrayBuffers; the detach semantics of readableByteStreamControllerEnqueue were traced against every Rust producer in an earlier review round and all hand over fresh/unaliased buffers.
Level of scrutiny
High. This is a behaviour-changing rewrite of a hot, GC-sensitive C++ path that every native stream in the runtime flows through. It interacts with JSC Weak references, WriteBarrier members, pull-into descriptor lifetimes, ArrayBuffer detachment, and the byte-controller's spec-mandated throwing paths. It has already been through five review rounds that surfaced and fixed real issues (scratch-buffer detach defeating reuse, missing written > 0 guard causing UB, spurious Bun__reportError on the partial-element close path, updateRef(false) left on an unreachable arm). The current diff looks correct, and the bug-hunting system found nothing this run, but the blast radius (every Blob/file/body stream) and the memory-safety surface make this exactly the kind of change a maintainer should sign off on.
Other factors
- Test-weakening:
streams-leak.test.tsdrops thedistinctBuffers.size < 8assertion. This is a real behaviour change — the default-reader path now copies each pull's filled prefix into a fresh small Uint8Array (becausereadableByteStreamControllerEnqueuewould otherwise detach the shared scratch buffer), so chunks no longer share a backing buffer. The remainingbackingBytes < 4 MBassertion still catches the original regression (pre-fix ≈16 MB), so the test's contract is preserved, but a human should confirm the per-chunk allocation is acceptable. - CI: an earlier round flagged
fetch-gzip.test.tsfailing on 4 platforms; robobun demonstrated it also fails on the base commit and is caused by unrelated PR #33613, not this change. - All 15 prior inline findings (from claude[bot], robobun, and CodeRabbit) are marked resolved with corresponding fix commits.
- Test coverage is thorough: BYOB fill, full-blob reassembly, constructor path,
Bun.file(), default-reader-still-works, release-then-BYOB handoff, empty blob, partial-element close, and fetch/serve request+response bodies over HTTP/1.1 and HTTP/3.
There was a problem hiding this comment.
No bugs found in this pass, but this switches the controller type for every native-backed ReadableStream (Blob/File/fetch bodies/subprocess pipes) from default to byte — a runtime-wide behavioural change in GC-sensitive JSC C++ that went through several non-trivial fix rounds. Worth a human look before merge.
What was reviewed:
nativeDecodePullResultBYOB vs enqueue paths: thewritten > 0guard, pull-into-gone-after-cancel guard, and scratch-buffer copy-before-enqueue all check out.nativeSourceCallClose: the partial-element close throw is now swallowed only when the stream already errored;respond(0)settles a pending BYOB read.readableStreamReaderGenericRelease: theupdateRef(false)gate correctly moved to the Byte arm so releasing a native-stream reader still drops the event-loop ref.streams-leak.test.ts: thedistinctBuffers.size < 8assertion was removed — the total-backing-bytes bound still guards the original regression, and the new per-chunk-copy behaviour is documented.
Extended reasoning...
Overview
This PR retypes Bun's lazily-materialized native ReadableStream sources (Blob, Bun.file(), fetch/Request/Response bodies, subprocess pipes) from ReadableStreamDefaultController to ReadableByteStreamController, so getReader({mode:'byob'}) and new ReadableStreamBYOBReader(stream) work on them per the File API spec. Roughly 350 lines of C++ across 11 files in src/jsc/bindings/webcore/streams/, plus new test coverage in blob.test.ts / body-stream.test.ts and an adjusted assertion in streams-leak.test.ts.
Security risks
None identified. No user-controlled parsing, no auth/crypto/permissions surface. The written value fed into the size cast comes from Bun's own native handles, and the added written > 0 guard removes the one UB conversion. Buffer bounds are clamped with std::min against the view length before respond().
Level of scrutiny
High. This is hot-path, GC-sensitive JSC binding code: it touches WriteBarrier members, a JSC::Weak back-edge, pull-into descriptor buffers that get transferred/detached, and the SourceKind dispatch invariant across both controller types. The change alters an observable runtime-wide invariant (default-reader chunks from native streams now come from a byte controller, so their backing buffers are detached on enqueue). It's exactly the class of change REVIEW.md flags for careful human review, and the PR history bears that out — earlier iterations had a scratch-buffer-detach regression, an updateRef(false) event-loop-ref leak, a UB float→size_t cast, and a spurious Bun__reportError on the spec-mandated partial-element close throw, all now fixed.
Other factors
- All prior inline findings from claude[bot]/robobun/coderabbit are resolved in the current diff.
- The one remaining CI failure (
fetch-gzip.test.tsredirect case) was traced to a pre-existing regression from #33613 that reproduces on the PR's base commit with no stream changes applied. - Test coverage is solid (BYOB read/loop/reassemble, constructor path,
Bun.file, default-reader-then-BYOB handoff, empty blob, partial-element close, and network bodies over HTTP/1.1 + HTTP/3), and the PR states WPT streams (1175 tests) and the 9090-test body-stream suite pass. streams-leak.test.tsdrops thedistinctBuffers.size < 8assertion because the byte-controller path now copies each pull's filled prefix into a fresh smallUint8Arrayrather than handing out subarrays of the shared scratch buffer. The remainingbackingBytes < 4 MBbound still catches the original 16 MB regression, and the change is documented in the test comment — but weakening a leak test is worth a maintainer's explicit sign-off.
Given the scope, the number of subtle fixes it took to converge, and the deliberate loosening of an existing leak assertion, I'm deferring rather than auto-approving.
2 participants
What
Blob.prototype.stream()(andBun.file().stream(), fetch/Request/Response bodies, and subprocess pipe streams) are now readable byte streams, sogetReader({ mode: "byob" })andnew ReadableStreamBYOBReader(stream)work on them. Previously they threw:The File API requires
Blob.prototype.stream()to return a byte stream, and Node.js and every browser already do, so zero-copy BYOB read loops that work on Node/browser Blobs failed only on a Bun Blob.Why
materializeNativeSource(the lazy materialization path for the$bunNativePtrstreams) always installed aReadableStreamDefaultController, andgetReader({ mode: "byob" })skipped materialization entirely, som_controllerKindwas neverByte.How
materializeNativeSourcenow installs aReadableByteStreamController(withautoAllocateChunkSizeunset). The nativepull(view, closer)already writes into a caller-provided buffer, so when a pull-into descriptor is pending the adapter passes the BYOB request's view through and answers withrespond(n); with no pull-into (default reader) it keeps its own scratch buffer andenqueue()s the filled prefix, preserving the existing default-reader behaviour.SourceKind::Nativemoves from the default controller's pull/cancel dispatch to the byte controller's;JSNativeStreamSourceAdapter::m_controlleris retyped accordingly and gains anm_pendingIsBYOBbit so the async-pull fulfilment path knows whether torespond()orenqueue().getReader({ mode: "byob" })andnew ReadableStreamBYOBReader(stream)materialize aNativePendingstream before locking it (aDirectPendingstream is left alone so it still rejects without running user code).ReadableStream__emptynow returns a closed byte stream, so an empty Blob's stream also accepts a BYOB reader and observesdone: true.Tests
Added a
Blob.prototype.stream() is a byte streamsuite totest/js/web/fetch/blob.test.tscovering: a single BYOB read fills the caller's view, a BYOB loop reassembles the full blob,new ReadableStreamBYOBReader(blob.stream()),Bun.file().stream()with a BYOB reader, default reader still works, release-then-BYOB on the same stream, and an empty Blob's stream.All six BYOB cases
TypeErroron main; all pass with this change. The existingtest/js/web/streams/streams.test.js,body.test.ts,body-stream.test.ts(9086 tests),native-source-onclose-leak.test.ts, nodestreamsuite, and WPT streams (1175 tests) all pass.Fixes #6643
Fixes #12908
Fixes #16402
no test proof · iteration 9 · 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