fetch: support getReader({ mode: 'byob' }) on response bodies#29167
fetch: support getReader({ mode: 'byob' }) on response bodies#29167
Conversation
|
Updated 7:05 PM PT - Apr 12th, 2026
❌ @robobun, your commit c4384da has some failures in 🧪 To try this PR locally: bunx bun-pr 29167That installs a local version of the PR into your bun-29167 --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:
WalkthroughImplemented BYOB support and handling across ReadableStream: getReader now invokes the stream’s lazy start with a BYOB flag; stream close/release drains and fulfills pending BYOB read-into requests; native/lazy byte-source internals and signatures adjusted. Added comprehensive BYOB regression tests. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/ReadableStreamInternals.ts`:
- Around line 1688-1695: The commit currently creates an always-zero-length view
when fulfilling BYOB read requests after close; update the fulfillment to size
the returned view using the descriptor's bytesFilled and elementSize (compute
viewLength = Math.floor(descriptor.bytesFilled / descriptor.elementSize)) and
construct the view with new descriptor.ctor(descriptor.buffer,
descriptor.byteOffset, viewLength) so buffered bytes are returned; modify the
branch handling pendingPullIntos in the loop over readIntoRequests (use
descriptor.bytesFilled / descriptor.elementSize when descriptor is present) and
keep done: true.
In `@test/regression/issue/29162.test.ts`:
- Around line 101-102: Replace the dynamic import of the test harness with a
static module-scope import: remove the await import("harness") usage and instead
import tempDirWithFiles at top-level (e.g., `import { tempDirWithFiles } from
"harness"`), then use tempDirWithFiles("issue-29162", {...}) in the test; update
any local references to tempDirWithFiles accordingly so the test follows the
repository pattern and does not hide the dependency behind a dynamic import.
🪄 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: a295cfb2-396c-4310-b962-fab4c95a397f
📒 Files selected for processing (3)
src/js/builtins/ReadableStream.tssrc/js/builtins/ReadableStreamInternals.tstest/regression/issue/29162.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — all review issues addressed (callClose resource-leak fix in de8178a, bytesFilled view sizing and static import in 6bbac18).
Extended reasoning...
Overview
The PR enables on native-backed ReadableStreams (fetch response bodies, Bun.file streams, new Response(body).body) by making NativeReadableStreamSource declare type: "bytes" and moving autoAllocateChunkSize to a private #chunkSize field so the byte controller never auto-allocates pending pull descriptors. Four coordinated fixes cover: lazy start in the BYOB getReader branch, pending BYOB read resolution on close, native-source resume on reader release, and resource cleanup on the close-from-pull path.
Security risks
No security-sensitive changes. The PR touches ReadableStream internals (stream controller type, pending descriptor handling) but has no auth, crypto, or permission implications.
Level of scrutiny
This is non-trivial production infrastructure touching the critical I/O path for all native-backed streams. It required three separate commits to address issues found during review. However, the code is now verified correct:
callCloseusesunderlyingByteSource ?? underlyingSource(line 1969) — resource-leak fix confirmed presentreadableStreamCloseusesbytesFilled / elementSize(line 1698) — spec-correct view sizing confirmedreadableStreamReaderGenericReleaseuses the dual-slot lookup (line 1807) — confirmed- Test file uses static
import { tempDirWithFiles } from "harness"at module scope — confirmed
Other factors
The bug hunting system found no additional bugs. My prior inline comment (the callClose resource leak) and both CodeRabbit suggestions were resolved in subsequent commits. The test suite covers the reproduction case, EOF handling, reader.closed resolution, large body drain, Bun.file and Response sources, regression guards for non-bytes streams, and the default reader path. No outstanding reviewer comments remain.
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 (1)
src/js/builtins/ReadableStreamInternals.ts (1)
2238-2248:⚠️ Potential issue | 🟠 MajorThe first reader still permanently fixes the controller kind.
type = "bytes"is only set when the initial lazy start happens withbyob = true. Becausesrc/js/builtins/ReadableStream.tsconsumesstarton that first acquisition,const r = body.getReader(); r.releaseLock(); body.getReader({ mode: "byob" })on a native response/file/request body still leaves the stream backed by aReadableStreamDefaultControllerand BYOB reader construction keeps throwing.Also applies to: 2251-2260, 2263-2269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/builtins/ReadableStreamInternals.ts` around lines 2238 - 2248, The bug is that the source.type ("bytes") is assigned after the start/pull methods are created so the first lazy start fixes a default controller kind; fix this by ensuring the BYOB type is set on the source object before the start/pull are invoked—i.e., set source.type = "bytes" based on the byob flag at creation time (or create the source object with its type pre-initialized) so ReadableStreamInternals' source used in start/pull (the object in the snippet and the similar blocks around lines 2251-2260 and 2263-2269) will have the correct type when the first reader triggers the lazy start.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/ReadableStream.ts`:
- Around line 385-398: Validate the incoming mode argument before triggering the
lazy start hook: ensure the mode value is checked for allowed values (e.g., only
"byob" or undefined) prior to computing isByob and calling the private start
(start_ via $getByIdDirectPrivate) so that invalid getReader({ mode }) inputs
throw before any native-backed stream start runs; move or add the mode
validation step ahead of the lines using isByob and the
start_/$putByIdDirectPrivate start_ clearing to prevent marking the stream
disturbed on argument-validation failures.
---
Outside diff comments:
In `@src/js/builtins/ReadableStreamInternals.ts`:
- Around line 2238-2248: The bug is that the source.type ("bytes") is assigned
after the start/pull methods are created so the first lazy start fixes a default
controller kind; fix this by ensuring the BYOB type is set on the source object
before the start/pull are invoked—i.e., set source.type = "bytes" based on the
byob flag at creation time (or create the source object with its type
pre-initialized) so ReadableStreamInternals' source used in start/pull (the
object in the snippet and the similar blocks around lines 2251-2260 and
2263-2269) will have the correct type when the first reader triggers the lazy
start.
🪄 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: 78cf42cd-b2b4-4e4c-a3be-21bffa65572d
📒 Files selected for processing (2)
src/js/builtins/ReadableStream.tssrc/js/builtins/ReadableStreamInternals.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/builtins/ReadableStreamInternals.ts:1959-1973— When a BYOB reader uses a multi-byte typed array (e.g. Uint16Array, elementSize=2) on a native byte stream and the native pull delivers a final chunk whose byte count is not divisible by elementSize, the partial bytes are copied into the pending descriptor (bytesFilled=1, elementSize=2), leaving the descriptor in pendingPullIntos with queue empty. When callClose subsequently calls controller.close() → readableByteStreamControllerClose(), the spec mandates erroring the stream (bytesFilled>0, queue empty, closeRequested) — it calls readableByteStreamControllerError() AND throws a TypeError. callClose's catch block catches this throw and calls globalThis.reportError(e), firing a spurious unhandled-error event even though the stream was already properly errored and the BYOB read promise was already properly rejected. The fix is to guard globalThis.reportError with a check that the stream is not already in the errored state before reporting.Extended reasoning...
What the bug is and how it manifests
When a BYOB reader uses a multi-byte typed array (e.g. Uint16Array, elementSize=2) on a native stream and the native pull returns a final chunk of bytes not divisible by elementSize, the following sequence triggers a spurious globalThis.reportError call.
Consider a stream delivering exactly 1 byte total with isClosed=true, with an active reader.read(new Uint16Array(128)) pending:
- readableByteStreamControllerEnqueue is called with a 1-byte Uint8Array chunk
- fillDescriptorFromQueue runs: maxAlignedBytes = (0 + 1) - ((0 + 1) % 2) = 0, which is not > currentAlignedBytes(0), so ready=false. However, totalBytesToCopy=1 still copies the byte into the descriptor (bytesFilled=1). The descriptor stays in pendingPullIntos because it was not committed (not enough bytes for one complete element), and the queue is now empty.
- callClose fires (scheduled via enqueueJob) and calls controller.close() → readableByteStreamControllerClose()
- Inside readableByteStreamControllerClose (line ~142): queue.size === 0, pendingPullIntos.peek().bytesFilled === 1 > 0. Per the WHATWG spec, this is an error condition: a partial element at close-time with an empty queue must error the stream. The function calls readableByteStreamControllerError(controller, e) AND throws a TypeError.
- callClose's catch block (line 1979) receives the TypeError and calls globalThis.reportError(e) — a spurious unhandled-error event.
Why existing code doesn't prevent it
The stream erroring via readableByteStreamControllerError is spec-correct and already properly rejects the pending BYOB read promise. However, callClose has no awareness that the throw came from the byte controller's legitimate spec-mandated error path, so it unconditionally reports it as an unhandled error. Before this PR, native streams used ReadableStreamDefaultController and readableByteStreamControllerClose was completely unreachable from native streams, so this path was impossible to trigger.
Why the refutation is partially correct but incomplete
The refutation correctly traces a scenario where a 3-byte stream delivers 3 bytes with isClosed=true and the first read consumes 2 bytes (1 Uint16 element), leaving 1 byte in the queue. In that sub-scenario, callClose fires with queue.size=1 > 0 → sets closeRequested=true, returns normally without throwing, and the error surfaces only during the second read. In that specific case, globalThis.reportError is indeed not triggered.
However, the refutation does not cover the case where the native pull delivers a small final chunk (e.g., 1 byte) that goes directly into the pending descriptor (bytesFilled=1, queue empty afterward). This is the sub-scenario confirmed by all three affirmative verifiers: queue.size=0, pendingPullIntos has bytesFilled=1, readableByteStreamControllerClose throws, and callClose's catch fires globalThis.reportError spuriously.
Impact
Any BYOB read with a multi-byte typed array (Uint16Array, Int16Array, Float32Array, Uint32Array, etc.) against a native stream (fetch body, Bun.file, Response.body, Blob.stream) whose total byte count is not divisible by the typed array's elementSize will fire a spurious 'unhandledrejection'-equivalent globalThis.reportError. Application-level error handlers (window.onerror, process.on('uncaughtException')) may incorrectly receive this error, causing false alarms or unexpected behavior.
How to fix it
In callClose's catch block, guard globalThis.reportError by checking whether the stream is already in the errored state before firing the event:
This way, when readableByteStreamControllerClose legitimately errors the stream and throws, the error is not double-reported.
There was a problem hiding this comment.
LGTM — all prior feedback has been addressed across the review iterations.
Extended reasoning...
Overview
The PR adds BYOB reader support to native-backed ReadableStreams (fetch response bodies, Bun.file, new Response(body).body, Blob.stream, request.body). Three files are touched: ReadableStream.ts (getReader restructuring), ReadableStreamInternals.ts (readableStreamClose BYOB drain, readableStreamReaderGenericRelease dual-slot lookup, callClose cleanup, NativeReadableStreamSource refactoring), and a new regression test file.
Security risks
No authentication, crypto, or permission boundaries are involved. The change operates entirely within the Streams API layer.
Level of scrutiny
High — streaming internals are critical code and this PR was appropriately scrutinized. Several meaningful bugs were caught and fixed during review: the callClose underlyingByteSource slot miss, the WeakRef-before-clear ordering bug in #onClose, mode validation before start_, null vs undefined sentinel mismatch, missing Math.floor, the undefined-value else-branch, and the async pull race introduced by fixing #onClose. All are resolved in the final diff.
Other factors
The bug hunting system found no additional issues. The regression test suite covers the primary repro, multi-read drain, reader.closed, large-body BYOB, Bun.file, new Response, Blob.stream, request.body, default-reader regression guard, invalid-mode throw, multi-byte element-size rejection, and user-constructed bytes streams. Test coverage is comprehensive.
There was a problem hiding this comment.
LGTM — all critical issues raised in prior rounds have been addressed.
Extended reasoning...
Overview
This PR adds BYOB reader support to native-backed ReadableStreams (fetch response bodies, Bun.file, Response.body, Blob.stream, request.body) by conditionally marking NativeReadableStreamSource as a byte stream only when BYOB is explicitly requested, keeping the default-reader path on ReadableStreamDefaultController to preserve existing memory/GC semantics.
Security risks
No security-sensitive code touched. Changes are scoped to WebStream internals: controller type selection, pending-read fulfillment on close, and reader release lifecycle. No auth, crypto, or permission paths involved.
Level of scrutiny
This PR warranted and received high scrutiny — it touches critical streaming infrastructure with subtle async race conditions. Several rounds of review identified and fixed multiple real bugs: the WeakRef ordering in #onClose, the dual-slot underlyingByteSource ?? underlyingSource lookup missing from callClose, per-instance #closer array to prevent cross-stream EOF signal clobbering, mode validation before the one-shot lazy start, stream state guard on the BYOB rejection path, and the source?.$resume?.(false) double-optional-chain to avoid TypeError on plain source objects. All of these are present and correct in the final code.
Other factors
The test suite is comprehensive — covering construction, single read + EOF, reader.closed resolution, large-body drain, Bun.file / Blob.stream / Response.body / request.body paths, regression guards for default readers and invalid modes, and the odd-byte-count Uint16Array rejection-without-spurious-error case. The bug hunting system found no additional issues. The one remaining edge case (zombie closeRequested state when a BYOB buffer is smaller than a single native chunk at EOF) is not a hard hang — subsequent reads drain the byte controller queue — and is consistent with pre-existing byte stream behavior.
fetch().body.getReader({ mode: 'byob' }) threw
'ReadableStreamBYOBReader needs a ReadableByteStreamController' because
native-backed fetch response bodies (and Bun.file, Response body, etc.)
were created with a default ReadableStreamDefaultController instead of
ReadableByteStreamController.
- NativeReadableStreamSource now declares type: 'bytes', so
createReadableStreamController wires up a byte controller. The old
autoAllocateChunkSize public field is moved to a private #chunkSize
field so the byte controller does not pick it up and create pending
pull descriptors (the original reason byte streams were disabled
here in 1.1.44). The controller's queue path still works with BYOB
readers: enqueue pushes to the queue and fills any waiting read-into
descriptor from it.
- ReadableStream.prototype.getReader now calls the lazy start() in the
BYOB branch too. Previously the controller wasn't created until a
default reader was requested, so ReadableStreamBYOBReader saw null
and threw.
- readableStreamClose now resolves pending readIntoRequests for BYOB
readers, using the descriptor ctor/buffer to return a zero-length
typed array with done: true. Without this, a BYOB read on a
closing byte stream would hang forever.
- readableStreamReaderGenericRelease reads the source via either
'underlyingByteSource' or 'underlyingSource' so the native resume
path still works once native streams are byte streams.
- readableStreamClose: when fulfilling pending readIntoRequests after stream close, size the returned typed array by descriptor.bytesFilled / descriptor.elementSize per the Streams spec's ReadableByteStreamControllerConvertPullIntoDescriptor. The common EOF case still returns a zero-length view, but partial-fill cases no longer discard buffered bytes. (coderabbit) - test: hoist tempDirWithFiles to a module-scope import from 'harness' instead of a dynamic import inside the test body. (coderabbit) - test: add Blob.stream() BYOB and request.body BYOB (inside Bun.serve) coverage. These are the other native-backed paths covered by the same fix — also addresses #6643, #12908, #16402.
Native-backed streams are now byte streams, so they store their underlying source under the 'underlyingByteSource' private slot (via ReadableByteStreamController), not 'underlyingSource'. callClose was still reading only 'underlyingSource', so the finally block that releases the native handle (source.\$stream) and internal pull buffer (source.\$data) silently no-op'd for every native byte stream — leaking the handle until GC. This only affected the close-from-pull path (isClosed=true synchronously from handle.pull), since the #onClose callback has its own cleanup for the native-side close event. Use the same underlyingByteSource ?? underlyingSource fallback we already added to readableStreamReaderGenericRelease.
CI surfaced a 5GB memory leak in test/js/bun/http/serve-body-leak.test.ts
("should not leak memory when streaming the body incompletely") and a
memory-delta-over-100MB failure in bun-serve-file.test.ts. Both tests
exercise native-backed req.body with a plain default reader.
Root cause: the previous commits unconditionally switched native
ReadableStreams to a ReadableByteStreamController. For BYOB consumers
that fixes #29162, but for default-reader consumers — especially
server-side 'grab body, read once, drop on the floor' patterns — the
byte controller retained refs to the pulled buffers in a way the
default controller did not. The implicit cleanup that used to happen
when the stream was GC'd no longer fired, so every abandoned streaming
request kept its 256-512KB pull buffer alive until process exit.
Fix: thread a 'byob' flag from getReader through the lazy start to
lazyLoadStream, and only stamp 'type: "bytes"' on the native source
instance when BYOB was actually requested. Default readers fall back to
the exact same ReadableStreamDefaultController path we had before this
PR, preserving legacy cancellation / GC semantics. BYOB readers still
get the full ReadableByteStreamController + close/release fixes added
in earlier commits.
Verified locally:
- test/regression/issue/29162.test.ts: 11/11 pass (BYOB still works)
- test/js/bun/http/serve-body-leak.test.ts incomplete streaming: pass,
peak_memory 99MB (was 5.3GB)
- test/js/bun/http/bun-serve-file.test.ts memory usage stress: pass
CodeRabbit flagged that 'getReader({ mode: "bogus" })' would run the
lazy native start (consuming the one-shot start callback and marking
the stream disturbed) before throwing the 'invalid argument value'
error. Reorder so we validate mode first and only trigger the lazy
start on known-good inputs.
Also added a regression test asserting that after an invalid-mode
throw the stream is still not locked and a plain getReader() still
drains the body.
Pre-existing bug where 'this.#controller = undefined' was set before the 'this.#controller?.deref?.()' capture. The deref therefore always yielded undefined and the callClose enqueue below it never ran. Before BYOB was wired up through the byte controller, this was effectively harmless because BYOB reads could not reach the close path at all. With BYOB now supported, a pending readIntoRequest would hang forever if the native layer closed out-of-band via this callback (connection reset, transport EOF) instead of through a pull returning isClosed=true. Swap the order so the controller is captured first, then the internal WeakRef is cleared.
The else-branch (unreachable in practice — pendingPullIntos and
readIntoRequests are kept 1:1 by the byte stream controller) resolved
pending BYOB reads with { value: undefined, done: true }. The Streams
spec requires \`value\` to always be a typed array when \`done: true\`,
so return a zero-length Uint8Array as a defense-in-depth fallback.
ReadableByteStreamController.close() throws 'Close requested while there remain pending bytes' when a pending BYOB pull-into descriptor has bytesFilled > 0 with an empty queue. This fires whenever a BYOB reader uses a multi-byte typed array (Uint16Array, Uint32Array, etc.) over a native stream whose total byte count isn't a multiple of elementSize — the trailing odd byte lands in the descriptor via fillDescriptorFromQueue, the queue drains, callClose fires, close() throws. The throw already errored the stream via readableByteStreamControllerError and rejected the pending readIntoRequest with the same error, so callClose's catch surfacing it a second time as a globalThis.reportError produces a spurious uncaughtException. Guard the reportError call by checking that the stream is not already in the errored state. Added a regression test for a Uint16Array reader over a 1-byte response body.
Two spec/precision nits from claude review: 1. View length should be Math.floor(bytesFilled / elementSize) per ReadableByteStreamControllerConvertPullIntoDescriptor step 8. JavaScript's TypedArray constructor coerces non-integer lengths via ToIndex which behaves like Math.trunc, matching Math.floor for the non-negative values we see here — so this was harmless in practice, but the code comment cited the exact spec step and should match it. 2. The guard against a missing controller used '!== undefined', but 'readableStreamController' is explicitly initialized to null (not undefined) in initializeReadableStream so the two values are intentionally distinct. Use loose '!= null' to cover both cases. The null path is structurally unreachable (BYOB reader requires a live byte controller) but the wrong sentinel is misleading.
The class is used for both default and byte controller-backed streams (depending on the 'byob' flag threaded through lazyLoadStream), so the stored WeakRef needs the union type. callClose already takes 'ReadableStreamDefaultController | ReadableByteStreamController'; match that here. No runtime impact — only .enqueue/.close/.error are called on the deref'd value, which both controllers expose.
CLAUDE.md forbids arbitrary time-based waits in tests. The microtask queue has already drained by the time await resolves the read rejection, so any callClose → reportError would have fired synchronously within that microtask. No sleep needed.
The previous callClose slot fix made #onClose actually enqueue a callClose microtask. This surfaces a new race: if the native transport closes out-of-band while a handle.pull() is in flight, both the callClose microtask (from #onClose) and the pull success callback are queued. FIFO ordering runs callClose first, which closes the controller; then the pull success callback calls #onNativeReadableStreamResult → controller.enqueue() on the already-closed byte controller, which throws and trips JSC debug assertions. Guard the success callback with 'if (#closed) return' before touching the controller. The sync pull + drain paths are unaffected because they run to completion within the same turn; #onClose cannot interleave.
Empty commit to re-trigger the farm gate after it hit a transient WebKit prebuilt fetch failure on the release build step. The ASAN-with-fix side passed (13/13 regression tests) and the release build succeeds locally on the farm container once the WebKit tarball cache is populated.
The 'race fix' I added in response to claude's earlier review was speculative and, on closer inspection, harmful. When #onClose fires while a handle.pull is in flight, the right behavior is to still enqueue the pull's data: the data arrived over the socket before the close signal, and dropping it is what causes the fetch upgrade test to hang on aarch64 Linux (pull2 delivers frames + FIN, #onClose fires, my bailout drops the frames, client loop never sees the WebSocket close frame, 90s timeout). The original code pre-this-PR unconditionally called #onNativeReadableStreamResult in the success callback — any subsequent throw from enqueue on a closed controller would become an unhandled rejection, which is in theory unfortunate but is not something my PR newly exposed (the same code paths existed before) and is strictly better than silent data loss. If a real race is confirmed it should be fixed by checking the controller/stream state right before the enqueue call and skipping only when the stream is actually closed — not when #onClose fired but callClose has not yet run. Deferring that to a follow-up.
Pre-existing race condition that became more visible with this PR. The 'const closer = [false]' at the factory scope was shared across every NativeReadableStreamSource instance backed by the same prototype — i.e., every fetch() response body in the process. Concurrent fetches could clobber each other's EOF signal: stream A hits EOF and native writes closer[0]=true, stream B has more data and writes closer[0]=false before A's async pull callback runs, A misses the EOF and reader.closed hangs forever. Moved to a per-instance private #closer field. Each NativeReadableStream- Source gets its own array, eliminating cross-stream mutation. This bug predates the PR but surfaces more on BYOB paths because the callClose path is now reachable from BYOB reader close, and it's a plausible explanation for the fetch.upgrade aarch64 timeout under concurrent CI test load.
ReadableByteStreamController.error() throws ERR_INVALID_STATE_TypeError when the stream state is not $streamReadable. That path is reachable via reader.cancel() racing an in-flight pull: readableStreamCancel closes the stream first, then propagates the cancel to the native handle, whose pull Promise rejects on a now-closed stream; the rejection handler would then try to forward the error to the closed byte controller and throw from inside the .then callback. Default controllers have their own graceful early-return in readableStreamDefaultControllerError, so pre-this-PR the error was silently absorbed. Match that behavior on the BYOB path by checking the stream state before calling controller.error, with a try/catch for defense in depth.
Defense in depth: for small native streams where lazyLoadStream takes
the chunkSize===0 branch, 'source' is the plain { start, pull } object
with no $resume method. The stream.$bunNativePtr guard currently skips
this block for such streams (they zero out the native ptr on init),
but if that invariant ever changes, 'source?.$resume(false)' would
throw TypeError. Double optional chain makes it a silent no-op.
Previous CI run (#45211) failed at [78/651] fetch WebKit (prebuilt) on linux-aarch64 with 5 consecutive HTTP 502 errors from github.com/oven-sh/WebKit releases. Pure infra flake — retriggering.
Reverts the 'fix' from efb14ff. The enqueue created a FIFO race with in-flight async pulls: when #onClose fires while handle.pull is pending, callClose is queued as microtask M1, then the pull resolution queues its .then callback as M2. M1 runs first and closes the controller; M2 then calls controller.enqueue(...) on the closed controller and throws 'Controller is already closed' as an unhandled rejection. This was masked on x64 by timing luck (the pull usually resolved before #onClose fired). On aarch64 the ordering inverted consistently, causing the fetch.upgrade test to drop the final WebSocket close frame and hang 90 seconds waiting for it. Revert semantics: #onClose still sets #closed=true and clears the internal state, it just doesn't directly enqueue callClose. The close is delivered the next time #pull is called, via its existing if (!handle || this.#closed) { ...enqueueJob(callClose)... } top-level guard. BYOB consumers still work because they drain via #pull just like default readers. Regression tests: 13/13 for #29162 still pass, fetch.upgrade passes locally, fetch-stream-cancel-leak passes locally.
robobun
force-pushed
the
farm/db1f6f1c/fetch-body-byob-reader
branch
from
c4405b4 to
91a3129
Compare
Build #45341 failed only on test/js/bun/util/v8-heap-snapshot.test.ts SIGKILL on a single ubuntu 25.04 x64-baseline lane. Zero stream code references — the test calls Bun.generateHeapSnapshot (pure native) and node:stream Readable (not native Web ReadableStream). OOM kill on a small CI runner. Retriggering.
'tempDirWithFiles' returns a plain string with no Symbol.dispose, so the temp directory persists after the test. 'tempDir' wraps it in a DisposableString that fs.rmSync's on scope exit. Drop-in with the 'using' declaration.
…king
Pre-existing one-shot-start violation that my PR surfaces as a real
regression. Every other caller of a stream's private 'start' slot
(getReader, readableStreamTee) clears the slot via
$putByIdDirectPrivate(stream, 'start', undefined) before invoking.
acquireReadableStreamDefaultReader did not, leaving the callback
live for a second invocation.
Pre-this-PR, this was harmless: the only code path that could run
the lazy start a second time was getReader({mode:'byob'}), which
threw at the ReadableStreamBYOBReader constructor before ever
touching 'start'. This PR routes BYOB through the lazy start itself,
so a sequence like
await res.body.pipeTo(dest, { preventCancel: true }).catch(() => {});
res.body.getReader({ mode: 'byob' });
now re-invokes the lazy native start: lazyLoadStream runs a second
time, handle.start() is called twice on the same native handle, and
the second createReadableStreamController throws 'ReadableStream
already has a controller' instead of the spec-correct 'needs a
ReadableByteStreamController'.
Fix: clear the slot before calling it, matching every other call
site. Verified locally: the pipeTo+byob sequence now throws the
pre-PR error message, 13/13 regression tests pass,
pipeTo-signal-leak tests pass.
Build #45371 on 0682e53 failed only because 2 darwin aarch64 test lanes hit 'Expired' status — the buildkite agent timed out waiting for the job to start. Zero actual test failures. Pure infra capacity issue. Retriggering.
Mirrors the state guard already present on the rejection branch. reader.cancel() racing an in-flight native pull takes this path: readableStreamCancel closes the stream, then NativeReadableStreamSource #cancel clears the handle but does NOT set this.#closed. When the promise resolves, the success callback's existing this.#closed guard is bypassed (still false) and controller.enqueue() fires on the now- closed ReadableByteStreamController, which trips the state assertion in debug/ASAN builds and pushes to an orphaned queue in release. Check stream.state === $streamReadable before calling #onNativeReadableStreamResult, same shape as the rejection-branch guard. Found by claude review, 13/13 regression tests pass.
|
PR is code-complete at c4384da. 17 substantive commits addressing all review findings, 13/13 regression tests pass locally on both debug ASAN and release, ASAN gate validates the fix every run (9 baseline fails → 13 with-fix passes). Two bot reviewers (coderabbit, claude) have said LGTM. Blocked only on 2 darwin-aarch64 test-bun lanes hitting |
1 participant
Fixes #29162.
Reproduction
Node.js on the same snippet reads the body into a BYOB buffer and then
returns
{ done: true }on a second read. Bun threw fromgetReader.Cause
Native-backed
ReadableStreams — fetch response bodies,Bun.filestreams,
new Response(body).body, etc. — were created with adefault
ReadableStreamDefaultControllerinstead ofReadableByteStreamController. The BYOB reader constructor checksisReadableByteStreamController(...)and threw.They used to be byte streams until 1.1.44, but were downgraded because
autoAllocateChunkSizeon the underlying source caused the controller'sown pull path to create per-call pending pull descriptors that the
native pull never satisfied. See the class comment in
createLazyLoadedStreamPrototypefor the detail.Fix
NativeReadableStreamSourcenow declarestype: "bytes". The oldpublic
autoAllocateChunkSizefield is moved to a private#chunkSizefield so the byte controller never sees it, so it nevercreates auto-allocated pending pull-into descriptors. The controller's
queue path works fine with BYOB readers:
enqueuepushes the chunkinto the queue and, if a BYOB read is waiting, fills its pending
descriptor from the queue.
ReadableStream.getReadernow calls the lazystart()in the BYOBbranch too. Before this, the controller wasn't created until a
default reader was requested, so the BYOB constructor saw
null.readableStreamClosenow resolves pendingreadIntoRequestsfor BYOBreaders with a zero-length typed array +
done: true, constructedvia the matching pull-into descriptor's
ctor/buffer. Withoutthis, a BYOB read on a byte stream whose native pull finishes with
closed = trueand zero bytes would hang forever.readableStreamReaderGenericReleasenow reads the source via eitherunderlyingByteSourceorunderlyingSourceso.\$resume(false)still fires on release for native byte streams.
Verification
Added
test/regression/issue/29162.test.tscovering:done: trueon second read (the exactrepro from the bug report)
reader.closedresolves after full drainBun.filestreams +new Response(body).body(other native-backedpaths)
type: 'bytes'stream still works(regression guard)
Fixes #6643
Fixes #12908
Fixes #16402
Supersedes stale prior attempts #27221 and #27271 (both stuck on CI flakes, neither landed).