Skip to content

node:stream: make isReadable/isWritable/isErrored/isDisturbed work with WHATWG web streams - #34624

Open
robobun wants to merge 7 commits into
mainfrom
farm/40f3081b/stream-utils-web-stream-state
Open

node:stream: make isReadable/isWritable/isErrored/isDisturbed work with WHATWG web streams#34624
robobun wants to merge 7 commits into
mainfrom
farm/40f3081b/stream-utils-web-stream-state

Conversation

@robobun

@robobun robobun commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Node documents WHATWG web streams as valid operands for stream.isReadable/isWritable/isErrored/isDisturbed (since v17/18) and brands its own web stream classes with Symbol.for("nodejs.stream.*") prototype getters so the symbol-first checks in internal/streams/utils.js report real state.

Bun's ReadableStream/WritableStream are native JSC cells that never carry those symbols, so all four helpers fell through to the node-stream shape probe (typeof stream.readable !== "boolean"null, or the ?? chain → false) regardless of the stream's actual state. Guard code ported from node silently takes the wrong branch: an errored stream is reported healthy, a readable stream is reported not readable.

Repro

import { isReadable, isWritable, isErrored, isDisturbed } from "node:stream";

const fresh = new ReadableStream({ start(c) { c.enqueue("x"); } });
console.log(isReadable(fresh));   // node: true    bun (before): null
const er = new ReadableStream({ start(c) { c.error(new Error("we")); } });
await er.getReader().read().catch(() => {});
console.log(isErrored(er));       // node: true    bun (before): false
console.log(isDisturbed(er));     // node: true    bun (before): false
const wsF = new WritableStream({ write() {} });
console.log(isWritable(wsF));     // node: true    bun (before): null

Fix

Add a private $webStreamState() intrinsic (same pattern as the existing $webStreamClosedPromise/$webStreamControllerError) that reads the native [[state]] slot, and duck-type ReadableStream/WritableStream operands in the four helpers right after the symbol check, so any user-installed Symbol.for("nodejs.stream.*") override still takes precedence. isDisturbed reuses the existing $disturbed private accessor on ReadableStream.

Matches Node v26 semantics:

  • isReadable(ReadableStream)state === 'readable'
  • isWritable(WritableStream)state === 'writable'
  • isErrored(RS|WS)state === 'errored'
  • isDisturbed(ReadableStream)[[disturbed]]

TransformStream falls through unchanged (Node doesn't brand it either).

How did you verify your code works?

  • 11 new cases in test/js/node/stream/node-stream.test.js (8 fail with the unpatched build, all pass with the fix).
  • Full node-stream.test.js suite passes.
  • Behavior probe script produces identical output under node v26.3.0 and the patched build.

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/stream/node-stream.test.js

…th WHATWG web streams

Node brands its web stream classes with Symbol.for("nodejs.stream.*")
getters, and stream.isReadable/isWritable/isErrored/isDisturbed check
those symbols first. Bun's ReadableStream/WritableStream are native JSC
cells with no such getters, so all four helpers fell through to the
node-stream shape probe and returned null/false for every web-stream
state.

Add a private $webStreamState() intrinsic that reads the native
[[state]] slot directly, and duck-type ReadableStream/WritableStream
operands in the four helpers (after the symbol check, so user brands
still take precedence). isDisturbed reuses the existing $disturbed
private accessor on ReadableStream.
@robobun
robobun requested a review from alii as a code owner July 18, 2026 10:29
@robobun

robobun commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:47 AM PT - Jul 22nd, 2026

@robobun, your commit a42ed8a has 2 failures in Build #77698 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34624

That installs a local version of the PR into your bun-34624 executable, so you can run:

bun-34624 --bun

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Adds a private intrinsic exposing WHATWG stream state, wires it into Bun’s builtin identifiers and globals, updates Node stream predicates for web streams, and adds lifecycle and compatibility tests.

Web stream predicate support

Layer / File(s) Summary
Web stream state intrinsic
src/js/builtins.d.ts, src/js/builtins/BunBuiltinNames.h, src/jsc/bindings/ZigGlobalObject.cpp
Declares, registers, and implements $webStreamState for readable and writable WHATWG streams.
WHATWG stream predicate integration
src/js/internal/streams/utils.ts
Uses web-stream state and disturbance fields in isReadable, isWritable, isErrored, and isDisturbed.
Web stream predicate lifecycle tests
test/js/node/stream/node-stream.test.js
Tests web-stream lifecycle states, branding, property overrides, and unchanged Node stream behavior.

Suggested reviewers: alii, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: stream helpers now work with WHATWG web streams.
Description check ✅ Passed The description matches the required template and includes both the PR purpose and verification details.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/node/stream/node-stream.test.js`:
- Around line 1709-1717: Extend the “Symbol.for('nodejs.stream.*') overrides
still take precedence” test to cover writable, errored, and disturbed predicates
in addition to readable. Define each corresponding symbol override on a stream
and assert the respective predicate returns the override value, preserving ??
precedence across all changed entry points.
🪄 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: 14ba2304-a386-4c5f-8024-d263feb2748d

📥 Commits

Reviewing files that changed from the base of the PR and between 511caaa and 96e06a6.

📒 Files selected for processing (5)
  • src/js/builtins.d.ts
  • src/js/builtins/BunBuiltinNames.h
  • src/js/internal/streams/utils.ts
  • src/jsc/bindings/ZigGlobalObject.cpp
  • test/js/node/stream/node-stream.test.js

Comment thread test/js/node/stream/node-stream.test.js
Comment thread src/jsc/bindings/ZigGlobalObject.cpp Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier 4-line-comment nit has been addressed, and the remaining inline note is a cosmetic forward-declaration consistency nit that doesn't block.

Extended reasoning...

Overview

Adds a $webStreamState() private intrinsic (C++ host function in ZigGlobalObject.cpp, registered in BunBuiltinNames.h and the GlobalPropertyInfo array, declared in builtins.d.ts) and wires it into isReadable/isWritable/isErrored/isDisturbed in src/js/internal/streams/utils.ts so those predicates report correct state for native WHATWG ReadableStream/WritableStream operands. 11 new test cases in node-stream.test.js cover every state (readable/closed/errored/disturbed × RS/WS), TransformStream fall-through, symbol-override precedence, and that node-stream operands are unaffected.

Correctness checks

  • The C++ helper is a byte-for-byte structural clone of the adjacent jsWebStreamClosedPromise: dynamicDowncast on the argument, read a plain member, throw for anything else. No GC roots, no throwing calls before the throw scope, no lifetime concerns.
  • Verified the enum ordinals against StreamsForward.h: ReadableStreamState::{Readable=0, Closed=1, Errored=2} and WritableStreamState::{Writable=0, Erroring=1, Errored=2, Closed=3} — the === 0 and === 2 checks in utils.ts match, and the .d.ts doc comment agrees.
  • isDisturbed uses the pre-existing $disturbed private accessor (defined at JSReadableStream.cpp:433), not new surface.
  • The web-stream branches sit after the stream[kIs*] symbol checks, so user-installed Symbol.for('nodejs.stream.*') overrides still win — covered by the extended precedence test in f65ff0f.
  • isErrored's ternary returns a boolean (not undefined) for web streams, so a non-errored web stream short-circuits the ?? chain with false rather than falling through to node-stream shape probes — which is the intended semantics.

Security risks

None. Reads an internal enum slot on a brand-checked native cell; no user-controlled input reaches allocation, syscalls, or coercion paths.

Level of scrutiny

Medium — touches the C++ bindings layer and a hot-ish node-compat helper module, but the change is small, mechanical, and mirrors an established sibling pattern exactly. No new abstractions or state.

Other factors

  • CodeRabbit's variant-matrix comment was addressed in f65ff0f and confirmed resolved.
  • My prior nit (4-line comment) has been trimmed to 3 lines in the current diff.
  • The one remaining inline nit (missing JSC_DECLARE_HOST_FUNCTION(jsWebStreamState) in the forward-declaration block at lines 1706-1713) is pure local-convention consistency; it compiles fine as-is because the definition precedes the only use.

Comment thread src/jsc/bindings/ZigGlobalObject.cpp

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — both earlier nits are addressed.

What was reviewed:

  • Verified ReadableStreamState/WritableStreamState enum ordinals in StreamsForward.h match the === 0 / === 2 checks and the builtins.d.ts doc.
  • Confirmed $inheritsReadableStream/$inheritsWritableStream are codegen'd $inherits(id, ...) brand checks, so dynamicDowncast<JSReadableStream|JSWritableStream> in jsWebStreamState cannot miss.
  • Confirmed stream.$disturbed is backed by the existing disturbedPrivateName custom getter on JSReadableStream.prototype.
  • WritableStream "erroring" (state 1) reporting isErrored === false was checked against Node's [kIsErrored] getter — same semantics.
Extended reasoning...

Overview

Adds a private $webStreamState intrinsic (C++ host function in ZigGlobalObject.cpp, name in BunBuiltinNames.h, decl in builtins.d.ts) that exposes the native [[state]] slot of JSReadableStream/JSWritableStream as an integer, then wires isReadable/isWritable/isErrored/isDisturbed in src/js/internal/streams/utils.ts to consult it for WHATWG stream operands right after the Symbol.for('nodejs.stream.*') check. isDisturbed reuses the existing $disturbed private getter. 11 new tests in node-stream.test.js cover the RS/WS state matrix, TransformStream fall-through, symbol-override precedence, and node-stream regression.

Security risks

None. The intrinsic is a private-name global (unreachable from user code), does no allocation, and only reads a uint8_t field after a dynamicDowncast. No untrusted input parsing, no auth/crypto/permissions surface.

Level of scrutiny

Low–moderate. This is a Node compat gap fix that copies the exact pattern of the two adjacent intrinsics (jsWebStreamClosedPromise, jsWebStreamControllerError) — same forward-declaration block, same dynamicDowncast shape, same registration site in addBuiltinGlobals, same fallback throwTypeError. The JS-side changes are four one-line insertions gated on the pre-existing isReadableStream/isWritableStream brand checks, placed after the symbol check so user overrides keep precedence.

Other factors

I cross-checked the enum ordinals in StreamsForward.h (ReadableStreamState { Readable=0, Closed=1, Errored=2 }, WritableStreamState { Writable=0, Erroring=1, Errored=2, Closed=3 }) against the === 0 / === 2 comparisons and the JSDoc — they line up. $inherits*Stream compiles to the codegen'd $inherits(id, …) brand check (src/codegen/replacements.ts:38), so any value that passes it will succeed the C++ dynamicDowncast; the throw path is defensive only. The isErrored ternary returns a boolean for web streams so the ?? chain correctly short-circuits (matching Node, whose [kIsErrored] prototype getter also returns a boolean). The transient WritableStream "erroring" state was examined and matches Node (reports false until fully "errored"). All three prior review threads (symbol-override test coverage, 3-line comment cap, forward-declaration) are resolved in the current diff.

@robobun

robobun commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator Author

CI summary for this PR: test/js/node/stream/node-stream.test.js (the only test this PR adds to) passed on every lane across builds #77592 and #77698.

Only remaining red is test/js/node/test/parallel/test-tls-connect-memleak.js on linux x64 (ubuntu 25.04 / debian 13), a FinalizationRegistry GC-timing assertion from #31155. It failed identically on both build #77592 and the empty-commit retrigger #77698, so it is unrelated to this diff. Reported to main-break triage.

Everything else is [flaky] and passed on retry.

Ready for merge from my side.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun fix conflicts

@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Conflicts resolved in f23dd2f (merged main at 1ebff2f). Only conflict was both sides appending to test/js/node/stream/node-stream.test.js; kept both. Full test file passes locally (109 pass / 0 fail).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the merge-conflict resolution was a clean test-file append and everything else is unchanged since the green CI run.

What was reviewed:

  • jsWebStreamState mirrors the jsWebStreamClosedPromise sibling exactly (same dynamicDowncast gate as the JS-side $inheritsReadableStream/$inheritsWritableStream check, so operand types agree).
  • Enum ordinals in StreamsForward.h (Readable=0/Closed=1/Errored=2; Writable=0/Erroring=1/Errored=2/Closed=3) match the integer comparisons in utils.ts and the builtins.d.ts doc comment.
  • isErrored's ?? chain short-circuits to a boolean for web streams and falls through via undefined otherwise; isDisturbed reuses the existing $disturbed private accessor on JSReadableStream.prototype.
  • WritableStream 'erroring' state → isErrored === false was checked against Node's own kIsErrored getter and matches.
Extended reasoning...

Overview

Adds a $webStreamState private intrinsic that reads JSReadableStream::m_state / JSWritableStream::m_state as an integer, registers it in BunBuiltinNames.h and addBuiltinGlobals, and uses it in src/js/internal/streams/utils.ts to make isReadable/isWritable/isErrored/isDisturbed report correct state for WHATWG streams. 11 new test cases in node-stream.test.js covering every state and symbol-override precedence.

Security risks

None. The intrinsic is a read-only accessor on native stream state, guarded by dynamicDowncast (throws TypeError on mismatch), and only reachable via a private name from bundled builtins. No user-controlled input reaches native code without a type gate.

Level of scrutiny

Medium — this is Node compat glue that follows an established local pattern byte-for-byte. The C++ function is a near-copy of the adjacent jsWebStreamClosedPromise (same argument gating, same throw-scope shape, same registration attributes). The JS-side changes are additive branches placed after the existing Symbol.for override check, so precedence is preserved and node-stream operands are untouched. I verified the enum ordinals in StreamsForward.h match the hardcoded === 0 / === 2 comparisons, that the JS-side $inheritsReadableStream gate uses the same class-info check as the C++ dynamicDowncast, and that $disturbed is an existing private accessor on the ReadableStream prototype.

Other factors

  • Earlier CI (build #75374) was green on all lanes for the touched test file; the only change since then is a trivial merge with main that appended unrelated tests to the same file.
  • All three prior review comments (CodeRabbit's symbol-override coverage, my comment-length nit, my forward-declaration nit) were addressed in follow-up commits and are resolved.
  • The one candidate issue raised this run (WritableStream 'erroring' state) was verified to match Node's own [kIsErrored] getter semantics (#state === 'errored'), so it is not a divergence.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the merge from main only touched the test file append, and both earlier nits are addressed.

What was reviewed

  • jsWebStreamState mirrors the sibling jsWebStreamClosedPromise/jsWebStreamControllerError pattern exactly (dynamicDowncast → read public m_state, throw otherwise); enum ordinals in StreamsForward.h match the documented 0/1/2/3 mapping.
  • isErrored on a WritableStream in the erroring state returns false — checked against Node's [kIsErrored] getter (state === 'errored'), so this matches.
  • ?? precedence: symbol overrides still win; non-web-stream operands fall through unchanged (ternary yields undefined).
  • stream.$disturbed resolves to the existing disturbedPrivateName custom accessor on JSReadableStream.
Extended reasoning...

Overview

Adds a private $webStreamState() host function in ZigGlobalObject.cpp (registered via BunBuiltinNames.h + builtins.d.ts) that returns the integer value of JSReadableStream::m_state / JSWritableStream::m_state. Four predicates in src/js/internal/streams/utils.ts (isReadable/isWritable/isErrored/isDisturbed) gain a web-stream branch right after the existing Symbol.for('nodejs.stream.*') check. Eleven new test cases in node-stream.test.js cover the RS/WS state matrix, symbol-override precedence, TransformStream fall-through, and node-stream operands remaining unaffected.

Security risks

None. The intrinsic is a private-name global (unreachable from user JS), reads a plain enum field with no allocation or user-code re-entry, and throws on non-stream input. No auth/crypto/permissions/untrusted-input paths are touched.

Level of scrutiny

Moderate — it adds C++ to ZigGlobalObject.cpp, but the addition is a byte-for-byte copy of the two adjacent jsWebStream* siblings (same downcast, same throw-scope-on-fallthrough shape, same registration attributes). The JS-side changes are small conditional insertions that preserve the existing ?? chain ordering. I verified the enum ordinals in StreamsForward.h (ReadableStreamState: Readable=0/Closed=1/Errored=2; WritableStreamState: Writable=0/Erroring=1/Errored=2/Closed=3) match what utils.ts compares against, and that $disturbed is the existing private accessor on JSReadableStream.prototype.

Other factors

  • Both of my earlier nits (4-line comment, missing JSC_DECLARE_HOST_FUNCTION) and CodeRabbit's variant-matrix request were addressed in follow-up commits and are visible in the current diff.
  • CI on the pre-merge build passed node-stream.test.js on every lane; remaining red was unrelated (TLS memleak GC-timing, known flakies).
  • The one candidate issue raised this run — isErrored(WritableStream) during the transient 'erroring' state — was checked against Node's own [kIsErrored] getter (state === 'errored') and matches.
  • The most recent push was a conflict-only merge of main into the test file plus a CI retrigger; no logic changed since the last green build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants