streams: share directStreamControllerClearSource, set m_closed on direct cancel - #36703
Conversation
…ialize Follow-up to #36666. Once materializeNativeSource has installed a SourceKind::Native default controller, the JSNativeStreamSourceAdapter owns the JS{Blob,File,Bytes}InternalReadableStreamSource handle; the stream's m_nativePtr slot is a second edge to the same cell that only the pre-materialize fast paths ever needed (they are all gated on !m_disturbed). materializeNativeSource: clear stream->m_nativePtr once the adapter is wired up. ReadableStreamTag__tagged: fall through to controller->algorithmContext ->handle() when the stream's own slot is empty. FetchTasklet re-derives *mut ByteStream on every body chunk via this function, so without the fallback a materialized fetch body stops receiving data and the tasklet's Strong on the stream is never released. readableStreamReaderGenericRelease: drop the redundant m_nativePtr gate; kind == Native and adapter->handle() already cover it. readableStreamClearSourceBarriers: also clear m_nativePtr (cell only) so a NativePending stream cancelled before materialize drops its handle. readableStreamCancel Direct arm: set m_closed and reuse the now-public directStreamControllerClearSource helper instead of three inline .clear()s, so a late onDirectPullRejected short-circuits in handleError. The barrier-release test now covers all eight cases in one subprocess with a single GC storm, and adds a FileInternalReadableStreamSource heap-count case for the m_nativePtr hand-off.
WalkthroughThe change centralizes direct-stream source cleanup, updates native controller release handling, and consolidates terminal-state leak coverage into one spawned-process test. ChangesReadable stream lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Updated 12:14 AM PT - Aug 2nd, 2026
❌ @robobun, your commit eab4848 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36703That installs a local version of the PR into your bun-36703 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this restructures native handle ownership across the C++ streams subsystem, the ReadableStreamTag__tagged FFI surface, and JS-side $bunNativePtr consumers — worth a human look.
What was reviewed:
ReadableStreamTag__taggedfallback:ClearAlgorithmssetskind = Nothingbefore nullingalgorithmContext, so theuncheckedDowncast<JSNativeStreamSourceAdapter>is guarded;nativeSourceSeverclearsadapter->handle()on every terminal path so the tag correctly falls to 0.readableStreamClearSourceBarriersnow clearingm_nativePtr: verifiedcancelPendingNativeSourcereads the slot before the tailclearSourceBarriersruns, and theisCell()guard preserves thejsNumber(-1)detached sentinel.- JS-side
$bunNativePtrreaders (native-readable.ts,webstreams_adapters.ts,ProcessObjectInternals.ts,tty.ts): the ones on the WHATWG stream seeundefinedpost-materialize and fall back to the reader path; the Node-Readable ones read from the copy taken pre-transfer.
Extended reasoning...
Overview
This PR is a follow-up to #36666 that transfers ownership of the native $bunNativePtr handle from JSReadableStream::m_nativePtr to the JSNativeStreamSourceAdapter at materialize time, and adjusts every downstream reader of that slot accordingly. It touches five C++ files in the Web Streams subsystem (BunStreamSource.cpp, JSDirectStreamController.cpp, JSReadableStream.h, ReadableStreamOperations.cpp, WebStreamsExports.cpp), the frozen-ABI header WebStreamsInternals.h, and reworks a GC-leak regression test into a single subprocess.
Security risks
None identified. The change is internal handle-ownership plumbing; no user input parsing, auth, or trust boundaries are involved.
Level of scrutiny
High. This is memory-lifecycle code in the JSC bindings: WriteBarrier clearing order, GC-visible handle ownership, and an FFI tag function (ReadableStreamTag__tagged) that Rust's FetchTasklet re-enters on every body chunk. A wrong assumption about when m_nativePtr vs adapter->handle() is populated could stall a fetch body or leak a Strong root. The uncheckedDowncast on algorithmContext in the tag fallback is only sound because ClearAlgorithms resets kind to Nothing in the same step that nulls algorithmContext — that pairing is load-bearing and non-local.
Other factors
- The PR description's claim that "every JS-side $bunNativePtr fast path is gated on !m_disturbed" isn't literally true for
tryTransferToNativeReadable(it only checks!ptr || ptr === -1), but the resulting behavior — falling back toReadableFromWebon a materialized stream — appears to be a correctness improvement, not a regression. Still, this is the kind of cross-cutting invariant a maintainer familiar with the Node-Readable transfer path should confirm. - The
readableStreamCancelDirect arm now setsm_closed = truebefore clearing source barriers, which the description says addresses a review note on #36666 aboutonDirectPullRejectedreaching a nullm_underlyingSource.handleErrordoes early-return onm_closed(viawasClosed), so this looks correct. - Test coverage: the reworked test now covers the new
FileInternalReadableStreamSourcerelease, and the description reportsnative-source-onclose-leak,wpt-streams, andfetch/bodystill passing. - No prior human reviews on this PR.
…st consolidation Clearing m_nativePtr (at materialize or at ClearAlgorithms) breaks PendingValue::size_hint(): Bun.inspect(response) re-derives *mut ByteStream via ReadableStreamTag__tagged to read bytes.size_hint, and FetchTasklet's readable_stream_ref path only updates the ByteStream's size_hint (not the PendingValue fallback field). With the slot cleared, tag returns JavaScript and inspect falls back to a stale first-chunk value (test/js/web/fetch/fetch.stream.test.ts 'response inspected size'). m_nativePtr keeps the handle alive past terminal state so inspect can report the final byte count; that is intentional, so leave it alone. What remains: readableStreamCancel Direct arm: set m_closed and call the shared directStreamControllerClearSource helper instead of three inline .clear()s, so a late onDirectPullRejected short-circuits in handleError. directStreamControllerClearSource: declared in WebStreamsInternals.h and defined in Bun::WebStreams so ReadableStreamOperations.cpp can call it. readableStreamReaderGenericRelease: drop the redundant m_nativePtr gate; kind == Native already implies a live adapter with a handle. readable-stream-terminal-barrier-release.test.ts: one subprocess, one GC storm, no per-test timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/streams/readable-stream-terminal-barrier-release.test.ts`:
- Around line 62-77: Extend the stream terminal-barrier scenarios with a
native-source reader-release case that materializes a native source, acquires a
reader, and calls releaseLock() for both an active readable stream and an
already-terminal stream. Use the existing retention and registry tracking
patterns so the native file-source objects remain retained and the changed
readableStreamReaderGenericRelease native-controller branch is exercised.
🪄 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: f35757e5-2032-4295-841b-c7ac6d871153
📒 Files selected for processing (4)
src/jsc/bindings/webcore/streams/JSDirectStreamController.cppsrc/jsc/bindings/webcore/streams/ReadableStreamOperations.cppsrc/jsc/bindings/webcore/streams/WebStreamsInternals.htest/js/web/streams/readable-stream-terminal-barrier-release.test.ts
There was a problem hiding this comment.
No issues found, but this touches JSC streams lifecycle (a real behavior change, not just a refactor) and the only CI status on the thread is for the reverted commit b36d4ce — worth a human look once 3c00ce8 is green.
What was reviewed:
kind == SourceKind::Nativeis only set alongside a non-nullalgorithmContext(BunStreamSource.cpp:580-581) andClearAlgorithmsresetskindtoNothing, so dropping them_nativePtrgate cannot reach a null adapter;adapter->handle()is still null-checked.- Traced every
m_closedreader after direct-cancel:handleErrorfrom a lateonDirectPullRejectednow short-circuits (previously it rancloseDirectSinkForErrorthen hit a nullm_underlyingSourceearly-return — no crash, but redundant);boundDirectWrite/etc. now correctly throw "controller is closed" post-cancel. directStreamControllerClearSourcealso callsreadableStreamClearSourceBarriers, whichreadableStreamCancelalready calls right after the switch — redundant but idempotent.- The comment-cop inline on WebStreamsInternals.h:544 looks like a false positive — the added 2-line doc comment matches every other declaration in that header.
Extended reasoning...
Overview
Follow-up to #36666 touching four files: promotes the file-static directStreamControllerClearSource helper into Bun::WebStreams (declared in WebStreamsInternals.h) so ReadableStreamOperations.cpp can share it; makes readableStreamCancel's Direct arm set controller->m_closed = true and call the shared helper instead of three inline .clear()s; drops a redundant stream->m_nativePtr guard in readableStreamReaderGenericRelease's Native arm; and consolidates seven per-case subprocess GC tests into one subprocess with a shared GC storm (N=200→20, 20→8 GC rounds, no 60s per-test timeout).
Security risks
None. No untrusted input parsing, no auth/crypto, no new user-JS entry points. All changes are internal C++ stream-lifecycle bookkeeping and a test refactor.
Level of scrutiny
Medium-high. This is JSC bindings code with WriteBarrier lifecycle and reentrancy concerns — the exact category the repo's review guide flags as most-blocked. The m_closed = true addition is a real behavior change (not a pure refactor): it alters what a late onDirectPullRejected / onDirectPullFulfilled / boundDirect* sees after cancel. I traced every read of m_closed and the change looks correct (previously handleError would run closeDirectSinkForError on an already-torn-down sink and then early-return on the null m_underlyingSource; now it cleanly short-circuits), but the state machine here is subtle enough that a maintainer familiar with #36666's review notes should confirm.
The m_nativePtr gate removal is safe: SourceKind::Native is set exactly once (BunStreamSource.cpp:580) with algorithmContext set to the adapter on the next line, ClearAlgorithms resets kind to Nothing when it clears the context, and releaseSteps() on a default controller is a no-op — so kind == Native implies a live adapter, and the body already null-checks adapter->handle().
Other factors
- CI: The robobun status is for b36d4ce (the m_nativePtr hand-off, since backed out in 3c00ce8) and shows fetch.stream.test.ts failing everywhere — expected and explained in the PR description. No CI status is posted yet for the current head 3c00ce8, so I can't confirm the backed-out revision is green.
- Test consolidation: Threshold went from
< N*0.2to>= floor(N*0.2)on the leak filter, which with N=20 means each case must have ≤3 survivors. The PR notes the pre-fix baseline was N survivors per case, so the gap is wide; but N=20 with 8 GC rounds is tighter than N=200 with 20 rounds, and one flaky case now fails the whole test instead of onedescribe.concurrententry. Reasonable trade-off for the debug+ASAN timeout problem it solves, but worth a maintainer glance. - comment-cop bot flagged WebStreamsInternals.h:544, but the added comment is a standard 2-line declaration doc matching the file's convention — looks like a false positive.
…ancel) The barrier-release scenarios test behavior that #36666 already shipped, so the gate's without-src build passes them. Add a case for the one new observable change in this PR: readableStreamCancel's Direct arm now sets m_closed, so a captured controller's write() throws 'closed' instead of silently writing into the cleared sink (main returns the byte count, 12).
There was a problem hiding this comment.
I didn't find any bugs, but this touches JSC streams lifecycle (readableStreamCancel / readableStreamReaderGenericRelease) and adds an observable behavior change, so it's worth a human look — especially given the earlier revision here had to be backed out after breaking fetch.stream.test.ts.
What was reviewed:
directStreamControllerClearSourcemove: body is byte-identical; existing WebCore callers still resolve viausing namespace Bun::WebStreamsat the top of the file.m_closed = trueon Direct cancel: verifiedonCloseearly-returns (stream already Closed) som_closedwas left false; the extrareadableStreamClearSourceBarriersvia the helper is redundant with the post-switch call but idempotent.m_nativePtrconjunct removal: confirmedkind = SourceKind::NativeandalgorithmContext = adapterare set together (BunStreamSource.cpp:580-581) and cleared together inClearAlgorithms;releaseSteps()is a no-op, andadapter->handle()is null-checked.
Extended reasoning...
Overview
Follow-up to #36666. Four changes: (1) promote directStreamControllerClearSource from file-static to Bun::WebStreams and declare it in WebStreamsInternals.h; (2) in readableStreamCancel's ControllerKind::Direct arm, set controller->m_closed = true and call the shared helper instead of three inline .clear()s; (3) drop the stream->m_nativePtr && conjunct from the Native gate in readableStreamReaderGenericRelease; (4) collapse seven per-scenario subprocess tests into one shared-GC subprocess and add a new test proving ctrl.write() throws after reader.cancel().
Security risks
None. No untrusted-input parsing, no auth/crypto, no allocation-size arithmetic. The change is state-flag ordering and WriteBarrier clearing on a controller whose stream is already Closed.
Level of scrutiny
High. This is JSC C++ streams lifecycle — the REVIEW.md "most-blocked category" — and the PR's own history shows fragility: revision b36d4ce (the m_nativePtr hand-off) broke fetch.stream.test.ts on every platform and was backed out in 3c00ce8. The current diff is much more conservative, but it still (a) changes observable behavior (controller.write() after cancel now throws TypeError instead of returning a byte count), and (b) removes a guard in a path that reaches uncheckedDowncast.
Correctness details I checked
- Helper move: the function body is unchanged. The existing call sites in
handleErrorandonClose(WebCore namespace) still resolve because JSDirectStreamController.cpp opensWebCorewithusing namespace Bun::WebStreams;. The new call site in ReadableStreamOperations.cpp is already insideBun::WebStreams. m_closed = true:readableStreamCloseruns before the switch, soonCloseseesstate != Readableand returns without settingm_closed. Without the fix, a captured controller's boundwriteskips them_closedguard and reacheswriteToDirectSinkon a livem_arrayBufferSink— the PR's evidence shows it returned12on main. The addedreadableStreamClearSourceBarriers(stream)inside the helper duplicates the post-switch call, but that function only.clear()s two WriteBarriers and is documented idempotent.m_nativePtrconjunct:materializeNativeSourcesetskind = NativeandalgorithmContext = adapterin adjacent lines;readableStreamDefaultControllerClearAlgorithmsresetskindtoNothingand clearsalgorithmContexttogether;releaseSteps()is empty. Sokind == Native⇒algorithmContextis a live adapter, andadapter->handle()is already null-checked before use. The removed conjunct also wouldn't have caught the detached case anyway (m_nativePtrbecomesjsNumber(-1), which is still truthy forWriteBarrier<Unknown>::operator bool).- Test consolidation: N drops 200→20 and GC rounds 20→8, but the leak signal is all-or-nothing (before the fix every probe survives), so the 20% threshold at N=20 (≥4) still has wide margin. Each scenario is asserted individually via the
leakedfilter, so a single regressing case still fails with its name in the diff.
Other factors
The robobun CI comment in the timeline is for the backed-out commit b36d4ce, not the current HEAD (717cab8); the PR body's evidence block shows the current HEAD passing under both debug+ASAN and release for the touched test file, but I don't see a full-suite CI result for it. All CodeRabbit / comment-cop threads are resolved. Given the area and the back-out in this PR's own history, deferring for a maintainer familiar with the streams subsystem.
|
Build #87583 (current head eab4848): every stream/fetch test file this PR touches is green on every lane. Remaining red is |
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 (2)
test/js/web/streams/readable-stream-terminal-barrier-release.test.ts (2)
94-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the complete scenario matrix.
Object.entries(results)only checks keys that were populated. If a scenario is deleted or its key is misspelled, the consolidated test can pass with incomplete coverage. Assert the expected seven keys before applying the leak threshold. (raw.githubusercontent.com)As per coding guidelines: tests must cover the complete variant matrix and assert setup preconditions.
Proposed coverage guard
+ const expectedScenarios = [ + "byte-cancel", + "byte-error", + "default-cancel", + "default-close", + "default-error", + "direct-end", + "direct-pending-cancel", + ].sort(); + expect(Object.keys(results).sort()).toEqual(expectedScenarios);🤖 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 `@test/js/web/streams/readable-stream-terminal-barrier-release.test.ts` around lines 94 - 96, Update the consolidated assertion in the readable-stream terminal barrier test to first verify that results contains exactly the expected seven scenario keys, including setup preconditions, before filtering entries by the leak threshold. Preserve the existing leaked, results, threshold, and exitCode assertions after this complete-matrix guard.Source: Coding guidelines
90-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve child-process failure diagnostics.
The child output is parsed before
exitCodeis asserted, and the final expectation does not includeexitCode. If the child exits non-zero without valid JSON,JSON.parse()masks the process failure. Guard the parse withstdout,stderr, andexitCodein the diagnostic, then assert the parsed result,stderr, andexitCodetogether. (raw.githubusercontent.com)As per coding guidelines: subprocess tests must drain stdout, stderr, and process exit concurrently and assert a combined result.
Proposed assertion shape
- expect({ leaked, results, threshold }).toEqual({ leaked: [], results, threshold }); - expect(exitCode).toBe(0); + expect({ leaked, results, threshold, stderr, exitCode }).toEqual({ + leaked: [], + results, + threshold, + stderr: "", + exitCode: 0, + });🤖 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 `@test/js/web/streams/readable-stream-terminal-barrier-release.test.ts` around lines 90 - 97, Update the child-process result handling around the JSON.parse call so stdout, stderr, and exitCode are included in failure diagnostics before parsing, preventing invalid child output from masking process failures. Assert the parsed results, stderr, and exitCode together, while preserving the existing leak-threshold validation and ensuring stdout, stderr, and process exit continue to be drained concurrently.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 `@test/js/web/streams/readable-stream-terminal-barrier-release.test.ts`:
- Around line 94-96: Update the consolidated assertion in the readable-stream
terminal barrier test to first verify that results contains exactly the expected
seven scenario keys, including setup preconditions, before filtering entries by
the leak threshold. Preserve the existing leaked, results, threshold, and
exitCode assertions after this complete-matrix guard.
- Around line 90-97: Update the child-process result handling around the
JSON.parse call so stdout, stderr, and exitCode are included in failure
diagnostics before parsing, preventing invalid child output from masking process
failures. Assert the parsed results, stderr, and exitCode together, while
preserving the existing leak-threshold validation and ensuring stdout, stderr,
and process exit continue to be drained concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c152a777-e465-4e18-9630-01e25beaeb4a
📒 Files selected for processing (1)
test/js/web/streams/readable-stream-terminal-barrier-release.test.ts
There was a problem hiding this comment.
Beyond the inline nit, I checked whether dropping the stream->m_nativePtr conjunct in readableStreamReaderGenericRelease could reach uncheckedDowncast on a null algorithmContext: releaseSteps() is a no-op and ClearAlgorithms sets kind = SourceKind::Nothing in the same statement it clears algorithmContext, so kind == Native still implies a live adapter. Deferring to a human on the rest — this is direct-controller lifecycle / WriteBarrier-release code where the first revision already surfaced a non-local ReadableStreamTag__tagged interaction.
Extended reasoning...
Verified the one safety-relevant edge of the m_nativePtr gate removal: JSReadableStreamDefaultController::releaseSteps() is empty, and every path that clears m_algorithms.algorithmContext (readableStreamDefaultControllerClearAlgorithms) also sets kind = SourceKind::Nothing; SourceKind::Native is only assigned at BunStreamSource.cpp:580 immediately alongside algorithmContext.set(..., adapter). So the new bare kind == Native check cannot dereference a null adapter. The remaining changes (setting m_closed on the Direct cancel arm, promoting directStreamControllerClearSource to Bun::WebStreams, and the leak-test consolidation with N=200→20 / GC rounds 20→8) are in the memory-safety / GC-lifecycle category REVIEW.md flags as most-blocked, and the PR body itself documents a backed-out first revision that broke fetch.stream.test.ts via a non-obvious size_hint interaction — worth a maintainer's eyes.
There was a problem hiding this comment.
No issues found, but this touches core ReadableStream C++ lifecycle (readableStreamCancel, readableStreamReaderGenericRelease), so leaving it for a maintainer to sign off.
What was reviewed:
m_closed = truein the Direct cancel arm: tracedhandleError'swasClosedgate — with this set, a lateonDirectPullRejectednow no-ops instead of re-runningcloseDirectSinkForError/callUnderlyingSourceClose; the newwrite() after canceltest proves the user-visible half.directStreamControllerClearSourcemove: body is byte-identical; the extrareadableStreamClearSourceBarriers(stream)it adds relative to the old inline clears is redundant (already called atreadableStreamCancel's tail) but idempotent.- Dropping the
m_nativePtrconjunct: verifiedreleaseSteps()is a no-op for the default controller andClearAlgorithmssetskind = Nothingatomically with clearingalgorithmContext, sokind == Native⇒ the adapter downcast is safe. - Test consolidation keeps all seven scenarios; N=20 with a 20% threshold is still well below the pre-fix N/N leak.
Extended reasoning...
Overview
Follow-up to #36666. Four files touched: (1) directStreamControllerClearSource moved from file-static in WebCore:: to Bun::WebStreams and declared in WebStreamsInternals.h; (2) readableStreamCancel's ControllerKind::Direct arm now sets m_closed = true and calls the shared helper instead of three inline .clear()s; (3) readableStreamReaderGenericRelease drops the stream->m_nativePtr && conjunct from the Native-source updateRef(false) gate; (4) the terminal-barrier leak test is collapsed from seven concurrent subprocesses (N=200 each, 20 GC rounds, 60s timeout) into one subprocess covering all seven cases (N=20, 8 GC rounds, default timeout), plus a new in-process test asserting ctrl.write() throws /closed/ after reader.cancel().
Security risks
None. No untrusted-input parsing, no auth/crypto, no FFI/ABI surface change. The helper move stays within the same TU's linkage and only gains a header declaration.
Level of scrutiny
High — this is JSC-bindings streams lifecycle C++, which historically hides GC/reentrancy subtleties. That said, the actual delta is very small (~15 C++ lines) and each piece is independently verifiable:
- The helper move is mechanical; body unchanged.
- The Direct-cancel change nets to exactly
m_closed = true(the helper's extrareadableStreamClearSourceBarriersis already called unconditionally at the function tail, so it's a harmless idempotent duplicate). I tracedhandleError: withm_closedalready true it skipscloseDirectSinkForErrorandcallUnderlyingSourceClose(both of which are null-safe anyway on a cleared source), calls the idempotent clear, and finds the stream already Closed — so a lateonDirectPullRejectedbecomes a no-op, matching the PR description. The new test's evidence block shows it fails on main (write() returned 12) and passes on the PR. - The
m_nativePtrgate: I confirmedJSReadableStreamDefaultController::releaseSteps()is empty (spec: "Return.") so it doesn't touchm_algorithms, andreadableStreamDefaultControllerClearAlgorithmssetskind = SourceKind::Nothingin the same statement block that clearsalgorithmContext. Sokind == Nativeguarantees a liveJSNativeStreamSourceAdapterinalgorithmContext, making theuncheckedDowncastsafe and the removed conjunct redundant.adapter->handle()is separately null-checked. Existing coverage innative-source-onclose-leak.test.tsandstreams-leak.test.ts(per the resolved coderabbit thread) exercises this branch.
Other factors
My earlier inline nit (drop the redundant WebCore:: qualifier in the header) was addressed in eab4848. All coderabbit and comment-cop threads are resolved. CI on the previous build was green for every streams/fetch test the PR touches; unrelated flakes were noted by the author. The test consolidation reduces per-case N and GC rounds, but the threshold stays proportional (20%) and the pre-fix behavior leaked N/N, so sensitivity is preserved while cutting debug+ASAN wall-clock.
I'm not approving because streams C++ lifecycle is exactly the area where a maintainer's sign-off is worth having even on small, well-reasoned changes.
) #36703 set `m_closed = true` in `readableStreamCancel`'s `ControllerKind::Direct` arm, and the bound `write`/`end`/`close`/`flush`/`error` handlers throw `TypeError: ReadableStreamDirectController is now closed` whenever `m_closed` is set. That is a user-visible change from v1.3.x for a producer whose in-flight `pull()` keeps calling the controller after the consumer has cancelled (or after its own `end()`/`error()`). ```js const rs = new ReadableStream({ type: "direct", async pull(c) { ctrl = c; c.write("first"); await new Promise(() => {}); }, }); const r = rs.getReader(); r.read().catch(() => {}); // ...after pull has started... await r.cancel(); ctrl.write("after-cancel"); // v1.3.x: byte count // after #36703: TypeError: ReadableStreamDirectController is now closed ``` ## Fix Keep `m_closed = true` on cancel (the state is accurate) and change the bound handlers to no-op instead of throw once `m_closed` is set: `write()` returns `0`, `end()`/`close()`/`flush()`/`error()` return `undefined`. `ReadableStreamOperations.cpp` is unchanged from main; the now-unused `directControllerClosedMessage` constant is removed. ## Verification ``` # without src/ change TypeError: ReadableStreamDirectController is now closed (fail) direct controller methods no-op once closed # with src/ change (incl. BUN_DESTRUCT_VM_ON_EXIT=1 + detect_leaks=1) (pass) ReadableStream releases source-only WriteBarriers once terminal (pass) direct controller methods no-op once closed ``` The test covers both the `reader.cancel()` path and the `controller.end()` path. Open PR #34854 also throws on `m_closed` after cancel and would need the same treatment if it lands. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 2 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/streams/readable-stream-terminal-barrier-release.test.ts bun test v1.4.0 (c8be033) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [2645.80ms] 113 | }); 114 | const reader = rs.getReader(); 115 | reader.read().catch(() => {}); 116 | await pullStarted.promise; 117 | await reader.cancel(); 118 | expect(ctrl.write("after-cancel")).toBe(0); ^ TypeError: ReadableStreamDirectController is now closed at <anonymous> (/workspace/bun/test/js/web/streams/readable-stream-terminal-barrier-release.test.ts:118:15) (fail) direct controller methods no-op once closed [47.15ms] 1 pass 1 fail 3 expect() calls Ran 2 tests across 1 file. [4.73s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: 1 FAILED bun test v1.4.0-canary.1 (a6fb7a6) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [28.31ms] 113 | }); 114 | const reader = rs.getReader(); 115 | reader.read().catch(() => {}); 116 | await pullStarted.promise; 117 | await reader.cancel(); 118 | expect(ctrl.write("after-cancel")).toBe(0); ^ error: expect(received).toBe(expected) Expected: 0 Received: 12 at <anonymous> (/workspace/bun/test/js/web/streams/readable-stream-terminal-barrier-release.test.ts:118:38) (fail) direct controller methods no-op once closed [1.54ms] 1 pass 1 fail 4 expect() calls Ran 2 tests across 1 file. [175.00ms] __F:1:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/streams/readable-stream-terminal-barrier-release.test.ts bun test v1.4.0 (c8be033) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [2656.43ms] (pass) direct controller methods no-op once closed [51.20ms] 2 pass 0 fail 11 expect() calls Ran 2 tests across 1 file. [4.74s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 677ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/41] gen cpp.rs (cppbind) [2/41] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited [3/41] gen JS modules (bundle-modules) Preprocess modules (8808ms) Bundle modules (32ms) Postprocesss modules (250ms) Bundle Functions (698ms) Generate Code (29ms) [9.83s] Bundled "src/js" for production 2559 kb 193 internal modules 13 native modules 90 internal functions across 19 files [3/30] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../webcore/streams/JSDirectStreamController.cpp | 17 ++++-------- ...eadable-stream-terminal-barrier-release.test.ts | 30 ++++++++++++++++++---- 2 files changed, 30 insertions(+), 17 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests …c/bindings/webcore/streams/JSDirectStreamController.cpp 10 2 0 …treams/readable-stream-terminal-barrier-release.test.ts 3 5 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
Follow-up to #36666 addressing the review notes there, plus a test consolidation.
Changes
readableStreamCancelDirect arm: setcontroller->m_closed = trueand call the shareddirectStreamControllerClearSourcehelper instead of three inline.clear()s.onCloseearly-returns in this arm because the stream is already Closed, som_closedwas left false; a lateonDirectPullRejectedwould then re-enterhandleErrorand reachcallUnderlyingSourceCloseon the now-nullm_underlyingSource. Settingm_closedmakes the stated invariant hold and short-circuits that path.directStreamControllerClearSource: moved intoBun::WebStreamsand declared inWebStreamsInternals.hsoReadableStreamOperations.cppcan reuse it.readableStreamReaderGenericRelease: drop the redundantstream->m_nativePtrgate;kind == Nativealready implies a live adapter whosehandle()the body reads.readable-stream-terminal-barrier-release.test.ts: collapsed into a single subprocess with one shared GC storm and no per-test timeout override (the previous per-case subprocess shape brushed the default timeout under debug+ASAN).Why
m_nativePtris left aloneThe first revision of this PR cleared
m_nativePtronce the adapter owned the handle. That broketest/js/web/fetch/fetch.stream.test.ts"response inspected size should reflect stream state":Bun.inspect(response)reports the body byte count viaPendingValue::size_hint(), which re-derives*mut ByteStreamthroughReadableStreamTag__taggedon every call;FetchTasklet::on_body_receivedonly updatesbytes.size_hinton the ByteStream (not thePendingValuefallback field), so once the slot is cleared inspect falls back to a stale first-chunk value. Adding an adapter fallback toReadableStreamTag__taggedfixed the fetch-body-delivery path but not this one, because the slot is also cleared at ClearAlgorithms (when the adapter has already severed). Handing the handle off cleanly needs the Rust side to cache the final size somewhere tag-independent; left for a separate change.[review] gate passed · iteration 8 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 8
evidence per changed file