node:stream: sync the stream suite to Node v26.3.0 and fix the gaps it surfaces — stream/iter + zlib/iter behind --experimental-stream-iter, read()-one-buffer semantics, compose/pipeline/web-adapter/http/fs/net parity (247/251 upstream tests, +63 vendored)#31826
Merged
Conversation
Collaborator
|
Updated 3:27 PM PT - Jun 17th, 2026
❌ @cirospaciari, your commit eafe811 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 31826That installs a local version of the PR into your bun-31826 --bun |
Contributor
|
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:
WalkthroughAdds experimental stream/iter modules, web/classic adapters, pull/push/share/broadcast primitives, compression transforms, runtime gating for node:stream/iter, async_hooks–nextTick init calls, stream/http/net adjustments, error-code/binding updates, process exception hardening, and extensive test coverage. ChangesExperimental Streams Iteration Stack
Suggested reviewers:
Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/internal/streams/operators.ts:397-400— Nit: removingcomposefromstreamReturningOperatorsleavesfunction compose(stream, options)(line 21) and itsstaticComposerequire (line 6) unreferenced —Readable.prototype.composenow lives in readable.ts with its own lazy require. TheisWritable/isNodeStream/addAbortSignalNoValidateimports (lines 7-8) are also only used by that function, so all of these can be deleted.Extended reasoning...
What
This PR moves
Readable.prototype.composeoff the operators registry and onto the prototype directly inreadable.ts(so it returns the composed Duplex rather than wrapping it in a Readable). The diff tooperators.tsremovescomposefrom thestreamReturningOperatorsexport object — but it leaves thefunction compose(stream, options)definition at line 21 in place, along with thestaticComposerequire at line 6 that only this function uses.Step-by-step
- Before this PR,
operators.tsdefinedfunction composeand exported it viastreamReturningOperators.compose;stream.tsthen iterated that object to installReadable.prototype.composeas a registry-wrapped operator. - This PR adds
Readable.prototype.compose = function compose(stream, options) { … }directly insrc/js/internal/streams/readable.ts:1289, with its owncomposeImpl ??= require("internal/streams/compose")lazy load. - The diff hunk at
operators.ts:397-400removescompose,fromstreamReturningOperatorsso the registry no longer installs a competing prototype method. - After that removal, grepping
operators.tsshowscomposereferenced only at line 21 (the definition) and line 33 (staticCompose(this, stream)inside its own body). Nothing else in the file — and nothing outside it, since the function was never exported by name — references it. - The
staticComposeimport at line 6 is used solely at line 33 insidecompose. Likewise,addAbortSignalNoValidate(line 7) is used only at line 37, andisWritable/isNodeStream(line 8) only at line 29 — all inside the now-dead function body.
Why nothing prevents it
composeis a top-level function declaration with no other call sites, so it simply sits in the module as dead code. Bun's builtin bundler may or may not tree-shake the function and its CommonJS-stylerequire("internal/streams/compose")import; either way there is no observable runtime behavior change.Impact
None at runtime — this is purely a maintenance/cleanliness issue. The dead function is ~20 lines plus four unused import bindings across three
requirecalls. It's mildly confusing to leave acomposehere when the real implementation has moved toreadable.ts, but it cannot cause incorrect behavior.Fix
Delete
function compose(stream, options) { … }(lines 21-41) and the now-unused imports:const staticCompose = require("internal/streams/compose");(line 6),const { addAbortSignalNoValidate } = require("internal/streams/add-abort-signal");(line 7), andconst { isWritable, isNodeStream } = require("internal/streams/utils");(line 8). - Before this PR,
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/streams/iter/broadcast.ts`:
- Around line 146-147: Replace truthy checks that treat falsy cancellation
reasons as absent by explicitly checking for undefined: change occurrences of
"if (self.#error)" to "if (self.#error !== undefined)" (and similarly for the
abort-idempotency flag, e.g., replace "if (this.#aborted)" with "if
(this.#aborted !== undefined)"). Update all spots in broadcast.ts that currently
use truthy checks on private fields (notably the self.#error checks and the
abort/idempotency check) so cancel(0), cancel(""), or cancel(false) correctly
trigger rejection instead of returning kDone.
- Line 528: The end(options) method in broadcast.ts has an unused parameter
causing eslint no-unused-vars; rename the parameter to _options (e.g.,
end(_options)) or prefix it with an underscore to silence the linter, or remove
the parameter entirely if the method signature isn't required by an interface;
update any internal references to that parameter accordingly and keep the method
body unchanged.
In `@src/js/internal/streams/iter/classic.ts`:
- Around line 511-517: The sync stub methods writeSync and writevSync are
triggering eslint(no-unused-vars); rename their parameters to unused-prefixed
names (e.g., _chunk for writeSync and _chunks for writevSync) or otherwise
prefix them with an underscore so the linter recognizes them as intentionally
unused, leaving the bodies returning false unchanged; update the function
signatures for writeSync(chunk) and writevSync(chunks) to use the new names
(_chunk, _chunks) to resolve the lint errors.
In `@src/js/internal/streams/iter/duplex.ts`:
- Around line 112-123: The abort listener currently stays registered if both
endpoints close before abort; keep a reference to the listener (abortBoth) as
you already have, attach it with signal.addEventListener("abort", abortBoth, {
__proto__: null, once: true }), and ensure you call
signal.removeEventListener("abort", abortBoth) when both channels are closed
(e.g., in the completion/close handlers for the aWriter and bWriter endpoints)
so the listener and its captured channel state can be released; also call
removeEventListener inside abortBoth after failing both writers to guarantee
cleanup when abort actually fires.
In `@src/js/internal/streams/iter/from.ts`:
- Around line 289-310: The async-iterable branch can emit arbitrarily large
batches (via isUint8ArrayBatch and the accumulated `batch` from
normalizeAsyncValue), bypassing FROM_BATCH_SIZE; modify the logic in the
isAsyncIterable path so that pre-batched arrays from isUint8ArrayBatch are split
into chunks of at most FROM_BATCH_SIZE before yielding, and when collecting
normalized chunks from normalizeAsyncValue push into a temporary buffer and
yield slices of size FROM_BATCH_SIZE as they fill (yield any final partial slice
at the end). Keep the existing fast path for single Uint8Array (isUint8Array
yielding [value]) but ensure all emitted batches never exceed FROM_BATCH_SIZE.
Use the existing symbols isAsyncIterable, isUint8ArrayBatch, isUint8Array,
normalizeAsyncValue and the FROM_BATCH_SIZE constant to locate and implement the
changes.
In `@src/js/internal/streams/iter/pull.ts`:
- Around line 870-875: The code calls writer.writev(batch, opts).then(...)
assuming a Promise; change this to handle non-Promise returns by normalizing the
result (e.g., assign result = writer.writev(batch, opts) and use
Promise.resolve(result).then(...) or check for a thenable before calling .then)
so that if writev returns undefined or a non-Promise the byte-counting loop
still runs without throwing; update the same pattern if writer.write(...) is
used elsewhere in this pull flow to ensure consistent handling.
In `@src/js/internal/streams/iter/push.ts`:
- Line 609: The method end(options) declares an unused parameter causing lint
failures; either remove the parameter or rename it to _options (or /* options
*/) in the end function signature to mark it as intentionally unused. Locate the
end function declaration (symbol: end) and update its signature to drop the
options argument or underscore it, and run tests/lint to confirm the warning is
resolved.
In `@src/js/internal/streams/iter/transform.ts`:
- Around line 586-610: The sync transform loop currently only finalizes when
receiving a null batch and can finalize multiple times; add a boolean guard
(e.g., finalized) alongside the existing async pattern so finalization (calling
processSyncInput(..., finishFlag) and draining via drainBatch()) happens exactly
once when the source ends or when a null flush marker is seen; update the for-of
loop over source to check/trigger finalization on end-of-loop (before leaving
the try) and ensure the finally block also uses the finalized guard to avoid
duplicate finalize calls—modify references in this function to processSyncInput,
drainBatch, pending, pendingBytes, BATCH_HWM, finishFlag and processFlag
accordingly.
In `@src/js/internal/webstreams_adapters.ts`:
- Around line 441-445: Guard against null/undefined `options` before
dereferencing `options.type`: first check that `options` is an object (e.g. if
(options != null && options.type !== undefined) { validateOneOf(options.type,
"options.type", ["bytes", undefined]); } const isBYOB = options != null &&
options.type === "bytes";) Update the code paths that use `options.type`,
referencing the existing `validateOneOf` and `isBYOB` identifiers so property
access is always preceded by the null check.
In `@src/js/node/net.ts`:
- Around line 744-757: The adoption path for FIFO/character-device/file
descriptors can leave the stream readable forever if options.readable === true
but there is no backing reader (_handle), so update the fd-adoption branch
(where stats.isFIFO(), stats.isCharacterDevice(), stats.isFile(), and
stats.isSocket()) to detect absence of a reader (e.g. this._handle or
equivalent) and explicitly close the readable side in that case by calling
this.push(null) and this.read(0); ensure this logic runs regardless of
options.readable value when no _handle exists so the stream does not hang
(adjust the condition around kSyncWriteFd/_write/_writev assignment and the
push(null)/read(0) calls to handle the "readable but no _handle" case).
🪄 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: 9deac0d9-283a-4f76-a5f4-25d4408e8ad6
📒 Files selected for processing (171)
src/js/builtins/ProcessObjectInternals.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal-for-testing.tssrc/js/internal/async_context_frame.tssrc/js/internal/async_hooks.tssrc/js/internal/async_hooks_tick.tssrc/js/internal/fs/streams.tssrc/js/internal/http.tssrc/js/internal/streams/duplex.tssrc/js/internal/streams/duplexify.tssrc/js/internal/streams/from.tssrc/js/internal/streams/iter/broadcast.tssrc/js/internal/streams/iter/classic.tssrc/js/internal/streams/iter/consumers.tssrc/js/internal/streams/iter/duplex.tssrc/js/internal/streams/iter/from.tssrc/js/internal/streams/iter/pull.tssrc/js/internal/streams/iter/push.tssrc/js/internal/streams/iter/ringbuffer.tssrc/js/internal/streams/iter/share.tssrc/js/internal/streams/iter/transform.tssrc/js/internal/streams/iter/types.tssrc/js/internal/streams/iter/utils.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/writable.tssrc/js/internal/webstreams_adapters.tssrc/js/node/_http_client.tssrc/js/node/_http_incoming.tssrc/js/node/_http_server.tssrc/js/node/async_hooks.tssrc/js/node/net.tssrc/js/node/stream.iter.tssrc/js/node/zlib.iter.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/JSCommonJSModule.cppsrc/resolve_builtins/HardcodedModule.rssrc/resolve_builtins/lib.rssrc/runtime/jsc_hooks.rssrc/runtime/node/zlib/NativeBrotli.rstest/js/node/stream/node-stream-uint8array.test.tstest/js/node/test/common/index.jstest/js/node/test/parallel/test-crypto-cipheriv-decipheriv.jstest/js/node/test/parallel/test-stream-add-abort-signal.jstest/js/node/test/parallel/test-stream-big-push.jstest/js/node/test/parallel/test-stream-compose.jstest/js/node/test/parallel/test-stream-construct.jstest/js/node/test/parallel/test-stream-consumers.jstest/js/node/test/parallel/test-stream-destroy.jstest/js/node/test/parallel/test-stream-drop-take.jstest/js/node/test/parallel/test-stream-duplex-destroy.jstest/js/node/test/parallel/test-stream-duplex-from.jstest/js/node/test/parallel/test-stream-duplex-readable-writable.jstest/js/node/test/parallel/test-stream-duplex-writable-finished.jstest/js/node/test/parallel/test-stream-duplex.jstest/js/node/test/parallel/test-stream-duplexpair.jstest/js/node/test/parallel/test-stream-end-paused.jstest/js/node/test/parallel/test-stream-filter.jstest/js/node/test/parallel/test-stream-finished-async-local-storage.jstest/js/node/test/parallel/test-stream-finished-bindAsyncResource-path.jstest/js/node/test/parallel/test-stream-finished-default-path.jstest/js/node/test/parallel/test-stream-forEach.jstest/js/node/test/parallel/test-stream-iter-broadcast-backpressure.jstest/js/node/test/parallel/test-stream-iter-broadcast-basic.jstest/js/node/test/parallel/test-stream-iter-broadcast-coverage.jstest/js/node/test/parallel/test-stream-iter-broadcast-from.jstest/js/node/test/parallel/test-stream-iter-consumers-bytes.jstest/js/node/test/parallel/test-stream-iter-consumers-merge.jstest/js/node/test/parallel/test-stream-iter-consumers-tap.jstest/js/node/test/parallel/test-stream-iter-consumers-text.jstest/js/node/test/parallel/test-stream-iter-cross-realm.jstest/js/node/test/parallel/test-stream-iter-disabled.jstest/js/node/test/parallel/test-stream-iter-duplex.jstest/js/node/test/parallel/test-stream-iter-from-async.jstest/js/node/test/parallel/test-stream-iter-from-coverage.jstest/js/node/test/parallel/test-stream-iter-from-sync.jstest/js/node/test/parallel/test-stream-iter-from-writable-cache-options.jstest/js/node/test/parallel/test-stream-iter-namespace.jstest/js/node/test/parallel/test-stream-iter-pipeto-edge.jstest/js/node/test/parallel/test-stream-iter-pipeto-signal.jstest/js/node/test/parallel/test-stream-iter-pipeto-writev.jstest/js/node/test/parallel/test-stream-iter-pipeto.jstest/js/node/test/parallel/test-stream-iter-pull-async.jstest/js/node/test/parallel/test-stream-iter-pull-sync.jstest/js/node/test/parallel/test-stream-iter-push-backpressure.jstest/js/node/test/parallel/test-stream-iter-push-basic.jstest/js/node/test/parallel/test-stream-iter-push-writer.jstest/js/node/test/parallel/test-stream-iter-readable-interop-disabled.jstest/js/node/test/parallel/test-stream-iter-readable-interop.jstest/js/node/test/parallel/test-stream-iter-share-async.jstest/js/node/test/parallel/test-stream-iter-share-coverage.jstest/js/node/test/parallel/test-stream-iter-share-from.jstest/js/node/test/parallel/test-stream-iter-share-sync.jstest/js/node/test/parallel/test-stream-iter-sharedarraybuffer.jstest/js/node/test/parallel/test-stream-iter-to-readable.jstest/js/node/test/parallel/test-stream-iter-transform-compat.jstest/js/node/test/parallel/test-stream-iter-transform-coverage.jstest/js/node/test/parallel/test-stream-iter-transform-errors.jstest/js/node/test/parallel/test-stream-iter-transform-output.jstest/js/node/test/parallel/test-stream-iter-transform-roundtrip.jstest/js/node/test/parallel/test-stream-iter-transform-sync.jstest/js/node/test/parallel/test-stream-iter-validation.jstest/js/node/test/parallel/test-stream-iter-writable-from.jstest/js/node/test/parallel/test-stream-iter-writable-interop.jstest/js/node/test/parallel/test-stream-map.jstest/js/node/test/parallel/test-stream-pipe-await-drain.jstest/js/node/test/parallel/test-stream-pipe-flow.jstest/js/node/test/parallel/test-stream-pipe-objectmode-to-non-objectmode.jstest/js/node/test/parallel/test-stream-pipeline-http2.jstest/js/node/test/parallel/test-stream-pipeline-listeners.jstest/js/node/test/parallel/test-stream-pipeline.jstest/js/node/test/parallel/test-stream-push-strings.jstest/js/node/test/parallel/test-stream-readable-aborted.jstest/js/node/test/parallel/test-stream-readable-async-iterators.jstest/js/node/test/parallel/test-stream-readable-compose.jstest/js/node/test/parallel/test-stream-readable-didRead.jstest/js/node/test/parallel/test-stream-readable-dispose.jstest/js/node/test/parallel/test-stream-readable-emittedReadable.jstest/js/node/test/parallel/test-stream-readable-ended.jstest/js/node/test/parallel/test-stream-readable-event.jstest/js/node/test/parallel/test-stream-readable-hwm-0-no-flow-data.jstest/js/node/test/parallel/test-stream-readable-infinite-read.jstest/js/node/test/parallel/test-stream-readable-needReadable.jstest/js/node/test/parallel/test-stream-readable-readable-one.jstest/js/node/test/parallel/test-stream-readable-strategy-option.jstest/js/node/test/parallel/test-stream-readable-to-web-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination.jstest/js/node/test/parallel/test-stream-readable-to-web.mjstest/js/node/test/parallel/test-stream-reduce.jstest/js/node/test/parallel/test-stream-toArray.jstest/js/node/test/parallel/test-stream-transform-destroy.jstest/js/node/test/parallel/test-stream-transform-final.jstest/js/node/test/parallel/test-stream-transform-flush-data.jstest/js/node/test/parallel/test-stream-typedarray.jstest/js/node/test/parallel/test-stream-uint8array.jstest/js/node/test/parallel/test-stream-unpipe-event.jstest/js/node/test/parallel/test-stream-unshift-empty-chunk.jstest/js/node/test/parallel/test-stream-unshift-read-race.jstest/js/node/test/parallel/test-stream-wrap-drain.jstest/js/node/test/parallel/test-stream-wrap-encoding.jstest/js/node/test/parallel/test-stream-wrap.jstest/js/node/test/parallel/test-stream-writable-change-default-encoding.jstest/js/node/test/parallel/test-stream-writable-decoded-encoding.jstest/js/node/test/parallel/test-stream-writable-destroy.jstest/js/node/test/parallel/test-stream-writable-ended-state.jstest/js/node/test/parallel/test-stream-writable-finished-state.jstest/js/node/test/parallel/test-stream-writable-finished.jstest/js/node/test/parallel/test-stream-writable-null.jstest/js/node/test/parallel/test-stream-writable-samecb-singletick.jstest/js/node/test/parallel/test-stream-writable-write-error.jstest/js/node/test/parallel/test-stream-writev.jstest/js/node/test/parallel/test-stream2-base64-single-char-read-end.jstest/js/node/test/parallel/test-stream2-basic.jstest/js/node/test/parallel/test-stream2-compatibility.jstest/js/node/test/parallel/test-stream2-httpclient-response-end.jstest/js/node/test/parallel/test-stream2-objects.jstest/js/node/test/parallel/test-stream2-push.jstest/js/node/test/parallel/test-stream2-read-correct-num-bytes-in-utf8.jstest/js/node/test/parallel/test-stream2-readable-wrap.jstest/js/node/test/parallel/test-stream2-transform.jstest/js/node/test/parallel/test-stream2-writable.jstest/js/node/test/parallel/test-stream3-cork-end.jstest/js/node/test/parallel/test-stream3-cork-uncork.jstest/js/node/test/parallel/test-stream3-pause-then-read.jstest/js/node/test/parallel/test-stream3-pipeline-async-iterator.jstest/js/node/test/parallel/test-zlib-flush-write-sync-interleaved.jstest/js/node/test/sequential/test-stream2-stderr-sync.js
💤 Files with no reviewable changes (1)
- src/js/internal/streams/operators.ts
Member
Author
|
Review triage (claude[bot] + coderabbit rounds): Fixed:
Already fixed before the review snapshot (stale findings):
Faithful-port rebuttals — these match node v26.3.0 source line-for-line, so changing them would diverge from upstream (and from the vendored upstream tests):
🤖 PR feedback addressed by Claude Code. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/streams/readable.ts (1)
1269-1275:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse the CLI-parsed gate here instead of
process.execArgv.
process.execArgvis userland-mutable, so this feature gate can be flipped after startup: pushing--experimental-stream-iterenables the API without the CLI flag, and removing it makes this method throw even though resolver gating still uses the write-once flag fromsrc/resolve_builtins/HardcodedModule.rs. Please plumb the same immutable startup bit into this builtin so both resolution andReadable[Symbol.for("Stream.toAsyncStreamable")]()stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/streams/readable.ts` around lines 1269 - 1275, The code checks process.execArgv for "--experimental-stream-iter" which is user-mutable; replace that check in Readable.prototype[toAsyncStreamable] (the block referencing createBatchedAsyncIterator, normalizeBatch and kValidatedSource) with the immutable CLI-parsed startup bit used by module resolution; locate the existing startup flag exported into the JS runtime (the same flag the resolver/hardcoded gating logic reads) and use that boolean here to decide whether to require("internal/streams/iter/classic") and to throw $ERR_STREAM_ITER_MISSING_FLAG() when unset, so resolution and Readable[Symbol.for("Stream.toAsyncStreamable")] use the same write-once gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js/internal/streams/readable.ts`:
- Around line 1269-1275: The code checks process.execArgv for
"--experimental-stream-iter" which is user-mutable; replace that check in
Readable.prototype[toAsyncStreamable] (the block referencing
createBatchedAsyncIterator, normalizeBatch and kValidatedSource) with the
immutable CLI-parsed startup bit used by module resolution; locate the existing
startup flag exported into the JS runtime (the same flag the resolver/hardcoded
gating logic reads) and use that boolean here to decide whether to
require("internal/streams/iter/classic") and to throw
$ERR_STREAM_ITER_MISSING_FLAG() when unset, so resolution and
Readable[Symbol.for("Stream.toAsyncStreamable")] use the same write-once gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5448f35-9cc3-43d3-b389-8c70e887ee7b
📒 Files selected for processing (14)
src/js/internal/streams/iter/broadcast.tssrc/js/internal/streams/iter/classic.tssrc/js/internal/streams/iter/push.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/readable.tssrc/js/internal/webstreams_adapters.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.cppsrc/resolve_builtins/HardcodedModule.rssrc/resolve_builtins/lib.rssrc/runtime/cli/Arguments.rstest/expectations.txttest/js/bun/crypto/cipheriv-decipheriv.test.tstest/js/node/crypto/crypto.test.ts
💤 Files with no reviewable changes (1)
- src/js/internal/streams/operators.ts
Contributor
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 `@src/runtime/cli/Arguments.rs`:
- Around line 1280-1283: The flag handling for
args.flag(b"--experimental-stream-iter") is only in the runtime-command branch
so bun build cannot enable resolver behavior; move the
bun_resolve_builtins::set_stream_iter_enabled(true) and
Bun__streamIterEnabled.store(...) out of the runtime-only block and into the
common/BUILD_PARAMS path (expose the flag in the build table as well) so that
when BUILD_PARAMS is constructed the resolver gate is reachable for both build
and runtime flows; update any parameter parsing that currently writes only to
RUNTIME_PARAMS_ to also populate the build params so the resolver sees the flag.
🪄 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: 6e2c2bf0-ba6d-4442-80fa-49f5b67a715d
📒 Files selected for processing (5)
src/js/internal/streams/readable.tssrc/js/node/_http_incoming.tssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcess.hsrc/runtime/cli/Arguments.rs
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
e66c8ab to
660f2b4
Compare
Member
Author
|
Finding from the macOS CI failures ( The With the exception fix in this PR the throw finally surfaces, and the child reports ~70 MB RSS growth over 5000 watch cycles (~14 KB per Marked expected-fail on darwin in 🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/io/posix_event_loop.rs:1024-1031— Commit 5f0b651 ("test: expect fs.watch leak test failure on macOS") silently reverts merged PR #31821 insrc/io/posix_event_loop.rs— deletingderegistration_already_gone()and its three POSIX call sites, restoring the file byte-for-byte to its pre-#31821 state. This is unrelated to the node:stream sync, isn't mentioned in the commit message or PR description, and re-introduces the regression #31821 fixed (ENOENT/EBADF on deregistration early-returns before clearing registration flags, leaving the poll re-issuing doomed deletes). Please drop theposix_event_loop.rshunk from this PR.Extended reasoning...
What the bug is
Commit 5f0b651 in this PR is titled "test: expect fs.watch leak test failure on macOS" and the PR description discusses only the node:stream sync. But
git show 5f0b651a --statshows it touches two files:test/expectations.txt(the described change) andsrc/io/posix_event_loop.rs(+23/−39, never mentioned anywhere).The
posix_event_loop.rshunk is a byte-for-byte revert of merged PR #31821 ("Treat unregistering an already-removed poll registration as success", commit 29f2c7e, merged by Dylan Conway 2026-06-04). Verified viagit diff 29f2c7e6^..29f2c7e6 -- src/io/posix_event_loop.rsbeing the exact inverse of this PR's hunk for that file: the file blob goes from6a82cf50(post-#31821) back to630461f4(pre-#31821). PR #31821 was merged ~5h before this PR was opened, consistent with a rebase-conflict mis-resolution where the branch's older copy of the file was kept.Step-by-step proof
The diff for
src/io/posix_event_loop.rsin this PR:- Deletes the
deregistration_already_gone()helper (the function whose doc comment explained "the registration flags must still be cleared, which an error return would skip, leaving the poll claiming to be registered and re-issuing doomed deletes on later teardown calls"). - Removes its call site in the Linux epoll
CTL_DELbranch (lines 1024-1029): thematch sys::get_errno(ctl)that treatedderegistration_already_gone(e)as success is replaced with the oldif let Some(errno) = errno_sys(ctl, …) { return errno; }. - Removes its call site in the macOS kevent64
EV_DELETEbranch (lines 1124-1132): the loop that skippedEV_ERRORresults matchingderegistration_already_goneis replaced with the old unconditionalreturn kevent_change_error(...). - Removes its call site in the FreeBSD kevent
EV_DELETEbranch (lines 1171-1176): same pattern. - Restores the pre-#31821 doc comment on
kevent_change_errorat lines 58-61 ("the deinit path tolerates that race by discarding the returned error") — this is not new documentation; it's the old comment that #31821 deleted, reappearing because the file went back to its prior state.
This is the entirety of #31821, undone.
Why existing code doesn't prevent it
There is no test asserting the tolerance behavior (per #31821's own commit message, "there's no JS-observable surface" — callers discard the error). The file change is buried in a 180-file PR and the commit message describes only the test-expectation half. Per CLAUDE.md: "Before deleting odd-looking code … git-blame why it was written; it is usually load-bearing."
Functional impact
Per #31821's commit message: when an fd is closed while still registered (or a pty master close marks the slave's knotes
EV_EOF|EV_ONESHOTand the kernel auto-deletes them — "happens on every terminal window/tab close while a tty is polled"), the deregistration syscall returns ENOENT/EBADF. With the tolerance removed,unregister_with_fd_implnow early-returns at lines 1027-1029 / 1127-1132 / 1174-1176 before reaching the flag-clearing block at lines 1179-1184.The
deinitpath itself is fine (deinit_possibly_deferat line 408 doeslet _ = self.unregister(...)thenself.flags = FlagsSet::empty()at line 412, clearing flags regardless). But the non-deinit callers —PipeReader::pause()at PipeReader.rs:308 anddns.rs:4972— callunregister()directly and discard the result without a subsequent flag clear. So after a failed unregister the poll'sis_registered()keeps returning true, and a laterdeinit()re-issues another doomedEV_DELETE— exactly the double-delete #31821 instrumented ("observed via instrumentation: the same poll failing the delete twice during one pty teardown"). libuv ignores these same errnos for the same reason.The runtime impact is mild (callers discard the error; no crash/hang). But silently reverting another contributor's merged fix via an unrelated commit in a 180-file PR is exactly what review must catch — it would otherwise re-land as a regression with no audit trail.
How to fix
Drop the
src/io/posix_event_loop.rshunk from commit 5f0b651 (e.g.git checkout origin/main -- src/io/posix_event_loop.rsand amend). The test-expectation half of that commit is fine and should stay. - Deletes the
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/node/_http_server.ts (1)
1552-1559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFall back to
this.socketwhen the request already detached its alias.
ServerResponse.end()passesreq.sockethere. AfterIncomingMessage._destroy()sets that alias tonull, this guard skips clearing the live response socket’s_httpMessage/"close"listener and only nullsthis.socket, leaving a stale back-reference on the keep-alive socket.🐛 Proposed fix
ServerResponse.prototype.detachSocket = function (socket) { + socket ??= this.socket; // socket can be null when the stream destroyer detached the request's // socket (req.socket = null) before the response finished. if (socket && socket._httpMessage === this) { if (socket[kCloseCallback]) socket[kCloseCallback] = undefined; socket.removeListener("close", onServerResponseClose); socket._httpMessage = null; } this.socket = null; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/node/_http_server.ts` around lines 1552 - 1559, ServerResponse.prototype.detachSocket currently only operates on the passed-in socket, which can be null after IncomingMessage._destroy(), leaving a stale back-reference on the real keep-alive socket; change the logic in detachSocket to fall back to the live socket (use socket || this.socket) and operate on that alternativeSocket: check alternativeSocket._httpMessage === this, clear alternativeSocket[kCloseCallback], removeListener("close", onServerResponseClose) from alternativeSocket, set alternativeSocket._httpMessage = null, and ensure this.socket is also nulled so no stale references remain (refer to ServerResponse.prototype.detachSocket, kCloseCallback, onServerResponseClose, _httpMessage, and ServerResponse.end).src/js/internal/webstreams_adapters.ts (1)
576-597:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate mapped readable type before fast paths.
readableOptions.typeis only validated when the readable branch callsnewReadableStreamFromStreamReadable(). The destroyed/non-readable branches bypass that and pass the value directly intonew ReadableStream(...), which can produce inconsistent error behavior for invalid values.Suggested fix
function newReadableWritablePairFromDuplex(duplex, options = kEmptyObject) { @@ const readableOptions = { type: options.readableType }; if (options.readableType == null && options.type != null) { // 'options.type' is a deprecated alias for 'options.readableType'. emitDEP0201(); readableOptions.type = options.type; } + if (readableOptions.type !== undefined) { + const typeArgName = + options.readableType == null && options.type != null + ? "options.type" + : "options.readableType"; + validateOneOf(readableOptions.type, typeArgName, ["bytes", undefined]); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/webstreams_adapters.ts` around lines 576 - 597, The code constructs ReadableStream with readableOptions.type in the destroyed and non-readable fast paths without validating it, causing inconsistent errors; before any fast-path creation (inside isDestroyed and the non-readable branch where new ReadableStream is used), run the same validation/conversion logic used by newReadableStreamFromStreamReadable on readableOptions.type (extract or call the validator used there) and use the validated value when constructing new ReadableStream so invalid types throw the same error as in newReadableStreamFromStreamReadable; update references around readableOptions, isDestroyed(duplex), isReadable(duplex), and newReadableStreamFromStreamReadable accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/builtins/ProcessObjectInternals.ts`:
- Around line 380-387: Iterating tickInitHooks directly can skip callbacks if
hooks mutate the array during iteration; fix by taking a snapshot (e.g., const
hooks = tickInitHooks.slice()) after obtaining asyncId (from
asyncHooksTick.newAsyncId()) and then iterate over that snapshot, calling each
hook with (asyncId, "TickObject", 0, tock) so mutations to tickInitHooks inside
callbacks do not affect the current dispatch.
In `@src/js/builtins/ReadableStreamInternals.ts`:
- Around line 1609-1617: The BYOB read-into queue draining logic currently only
runs in cancel() causing BYOB readers to hang on EOF; move the readIntoRequests
drain into the shared readableStreamClose() path (use
$getByIdDirectPrivate/$isReadableStreamBYOBReader to detect a BYOB reader and
$getByIdDirectPrivate to read its "readIntoRequests", then $putByIdDirectPrivate
to replace it with a new FIFO and $fulfillPromise pending requests with {value:
undefined, done: true}) and update cancel() to call/reuse readableStreamClose()
so both close and cancel share the same drain behavior for readIntoRequests.
In `@src/js/internal/streams/iter/consumers.ts`:
- Around line 379-437: The ready.shift() draining is O(n^2); replace the queue
with a head-pointer ring: keep the existing ready array but add a numeric head
(e.g., readyHead = 0) and change pushes to ready.push(...) as-is, then drain by
reading items with const item = ready[readyHead++]; treat FIFO emptiness as
readyHead < ready.length (and update the outer loop condition to use readyHead
vs ready.length); when readyHead grows large (or readyHead === ready.length)
reset the buffer by doing ready = ready.slice(readyHead); readyHead = 0 to avoid
unbounded memory; update both the synchronous drain loop (where ready.shift() is
used) and any code paths that check ready.length (e.g., the outer while
condition and waitResolve logic) to use readyHead/ready.length semantics; keep
onSettled, iterator.next().then, and the error-push logic unchanged aside from
the new empty-checks.
In `@src/js/internal/streams/iter/push.ts`:
- Around line 213-237: The method `#createPendingWrite` currently pushes the entry
into `#pendingWrites` before validating/using the provided signal; change the flow
so the entry is only enqueued after any signal work succeeds: if signal is
falsy, push immediately; if signal is provided, prepare onAbort and the wrapped
resolve/reject, attempt to addEventListener inside a try/catch, and only push
the entry into `#pendingWrites` after addEventListener succeeds (on failure,
throw/reject without mutating `#pendingWrites`); ensure you still remove the
listener in the wrapped entry.resolve/entry.reject and remove the entry from the
queue in onAbort as before.
In `@src/js/node/_http_incoming.ts`:
- Around line 413-421: The connection getter must be kept in sync with the
socket getter: stop using the `??=` pattern on the same backing field
`this[fakeSocketSymbol]` (which resurrects a FakeSocket when the field is
intentionally set to null) and instead match the socket getter's logic (only
create a new FakeSocket when `this[fakeSocketSymbol] === undefined`, otherwise
return the current value including null); update the `connection` accessor (and
any code that sets `stream.socket`) to reference `fakeSocketSymbol`,
`FakeSocket`, and the same undefined-only lazy-init behavior so `socket` and
`connection` remain alias-consistent.
In `@src/js/node/async_hooks.ts`:
- Around line 382-403: The current disable() removes the shared function
reference stored in enabledInit which can remove another hook using the same
init callback; instead create and store a per-instance wrapper/token when
registering init (e.g. assign a unique wrapper value alongside enabledInit
inside the installation path where enabledInit = init and
tickInitHooks.push(init)), push that wrapper/token into
require("internal/async_hooks_tick").tickInitHooks, and in disable() remove that
specific wrapper/token (use the stored wrapper variable rather than enabledInit)
and clear the wrapper variable; adjust any references that read enabledInit to
use the instance wrapper token so other hooks with the same raw init function
aren’t affected.
In `@src/js/node/net.ts`:
- Around line 1368-1397: The synchronous fd writers fdSyncWrite and fdSyncWritev
perform writes but never update the socket's byte counters; update
this[kBytesWritten] and this._bytesDispatched with the number of bytes returned
from each fs.writeSync call so socket.bytesWritten and _bytesDispatched reflect
the writes. In both fdSyncWrite and fdSyncWritev, capture the return value from
fs.writeSync for each write chunk/iteration, add that value to
this[kBytesWritten] and this._bytesDispatched (accumulating across the inner
loop and across v-write entries), then proceed as before with offset and
callback handling.
- Around line 1157-1170: When releasing the adopted fd (this[kSyncWriteFd]) also
restore the instance's sync-write function overrides so future reconnects don't
keep using the stale path: after clearing this[kSyncWriteFd] in the Socket
shutdown/cleanup code, restore or remove the fdSyncWrite and fdSyncWritev
overrides (the properties set earlier around lines where fdSyncWrite /
fdSyncWritev were installed) back to their default/original implementations (or
delete them from the instance) so subsequent Socket.connect() calls use the
fresh handle's normal write path instead of the stale sync-fd handlers.
In `@test/js/node/stream/node-stream.test.js`:
- Around line 582-589: The test spawns a subprocess into variable proc
(Bun.spawn) and only drains proc.stdout, risking a deadlock because stderr is
piped but never read; update the Promise.all call that awaits proc.stdout.text()
and proc.exited to also include proc.stderr.text() so both stdout and stderr are
consumed concurrently (i.e., use Promise.all([proc.stdout.text(),
proc.stderr.text(), proc.exited])) and adjust the destructuring accordingly to
capture and ignore or assert stderr as needed.
In `@test/js/node/test/parallel/test-stream-iter-broadcast-basic.js`:
- Around line 221-230: The test testCancelWithFalsyReason is asserting that
bc.cancel(0) should reject (expects result 0) but upstream semantics treat falsy
cancel reasons as normal completion; update the test to expect successful
completion instead of rejection: call text(consumer) without the .catch(...) (or
change the promise handling) and assert that the consumer completes normally
(e.g., resolved value rather than 0). Adjust assertions in
testCancelWithFalsyReason to reflect completion behavior for broadcast().cancel
when given a falsy reason.
In `@test/js/node/test/parallel/test-stream-iter-from-sync.js`:
- Around line 195-207: The test claims zero-copy for DataView inputs but only
checks content; either enforce aliasing or drop the claim. In
testFromSyncDataView, if zero-copy is required, add assertions that the returned
Uint8Array (from fromSync(view), e.g. batches[0][0]) shares the same underlying
ArrayBuffer and offsets as the original DataView (compare .buffer === buf and
.byteOffset/.byteLength with view.byteOffset/view.byteLength); otherwise remove
or reword the comment about zero-copy and leave only the content assertion.
In `@test/js/node/test/parallel/test-stream-iter-share-from.js`:
- Around line 103-125: The testShareBlockBackpressure test currently starts both
consumers together so the slow consumer never lags; modify it to force the fast
consumer to run ahead before the slow one is allowed to pull. Specifically, in
testShareBlockBackpressure (the shared = share(...), fast = shared.pull(), slow
= shared.pull() block) advance the fast reader (call fast.next() repeatedly or
await text(fast) partially) to consume enough items to exceed highWaterMark
(e.g., consume 2–3 items) and only then start/await the slow reader (or gate
slow.next() with a deferred promise) so the source must stall under
backpressure:'block' and the test actually verifies blocking behavior. Ensure
final assertions still check both readers receive all items.
In `@test/js/node/test/parallel/test-stream-iter-writable-from.js`:
- Line 270: Replace the non-deterministic sleep with an event-based wait: in the
tests testDestroyDelegatesToFail, testDestroyWithoutError, testDestroyWithError,
and testDestroyWithoutFail, remove the await new Promise(resolve =>
setTimeout(resolve, 10)) and instead await the stream close event (for example
using once(writable, 'close') from the 'events' module or stream.finished) so
the test proceeds only after writable emits 'close' following destroy().
---
Outside diff comments:
In `@src/js/internal/webstreams_adapters.ts`:
- Around line 576-597: The code constructs ReadableStream with
readableOptions.type in the destroyed and non-readable fast paths without
validating it, causing inconsistent errors; before any fast-path creation
(inside isDestroyed and the non-readable branch where new ReadableStream is
used), run the same validation/conversion logic used by
newReadableStreamFromStreamReadable on readableOptions.type (extract or call the
validator used there) and use the validated value when constructing new
ReadableStream so invalid types throw the same error as in
newReadableStreamFromStreamReadable; update references around readableOptions,
isDestroyed(duplex), isReadable(duplex), and newReadableStreamFromStreamReadable
accordingly.
In `@src/js/node/_http_server.ts`:
- Around line 1552-1559: ServerResponse.prototype.detachSocket currently only
operates on the passed-in socket, which can be null after
IncomingMessage._destroy(), leaving a stale back-reference on the real
keep-alive socket; change the logic in detachSocket to fall back to the live
socket (use socket || this.socket) and operate on that alternativeSocket: check
alternativeSocket._httpMessage === this, clear
alternativeSocket[kCloseCallback], removeListener("close",
onServerResponseClose) from alternativeSocket, set
alternativeSocket._httpMessage = null, and ensure this.socket is also nulled so
no stale references remain (refer to ServerResponse.prototype.detachSocket,
kCloseCallback, onServerResponseClose, _httpMessage, and ServerResponse.end).
🪄 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: 12ad007f-2b22-408e-b158-d95e9ecc5945
📒 Files selected for processing (179)
src/js/builtins/ProcessObjectInternals.tssrc/js/builtins/ReadableStreamInternals.tssrc/js/internal-for-testing.tssrc/js/internal/async_context_frame.tssrc/js/internal/async_hooks.tssrc/js/internal/async_hooks_tick.tssrc/js/internal/fs/streams.tssrc/js/internal/http.tssrc/js/internal/streams/duplex.tssrc/js/internal/streams/duplexify.tssrc/js/internal/streams/from.tssrc/js/internal/streams/iter/broadcast.tssrc/js/internal/streams/iter/classic.tssrc/js/internal/streams/iter/consumers.tssrc/js/internal/streams/iter/duplex.tssrc/js/internal/streams/iter/from.tssrc/js/internal/streams/iter/pull.tssrc/js/internal/streams/iter/push.tssrc/js/internal/streams/iter/ringbuffer.tssrc/js/internal/streams/iter/share.tssrc/js/internal/streams/iter/transform.tssrc/js/internal/streams/iter/types.tssrc/js/internal/streams/iter/utils.tssrc/js/internal/streams/operators.tssrc/js/internal/streams/pipeline.tssrc/js/internal/streams/readable.tssrc/js/internal/streams/writable.tssrc/js/internal/webstreams_adapters.tssrc/js/node/_http_client.tssrc/js/node/_http_incoming.tssrc/js/node/_http_server.tssrc/js/node/async_hooks.tssrc/js/node/net.tssrc/js/node/stream.iter.tssrc/js/node/zlib.iter.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/ErrorCode.cppsrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/JSCommonJSModule.cppsrc/resolve_builtins/HardcodedModule.rssrc/resolve_builtins/lib.rssrc/runtime/cli/Arguments.rssrc/runtime/jsc_hooks.rssrc/runtime/node/zlib/NativeBrotli.rstest/expectations.txttest/js/bun/crypto/cipheriv-decipheriv.test.tstest/js/node/crypto/crypto.test.tstest/js/node/net/node-net.test.tstest/js/node/stream/node-stream-uint8array.test.tstest/js/node/stream/node-stream.test.jstest/js/node/test/common/index.jstest/js/node/test/parallel/test-crypto-cipheriv-decipheriv.jstest/js/node/test/parallel/test-stream-add-abort-signal.jstest/js/node/test/parallel/test-stream-big-push.jstest/js/node/test/parallel/test-stream-compose.jstest/js/node/test/parallel/test-stream-construct.jstest/js/node/test/parallel/test-stream-consumers.jstest/js/node/test/parallel/test-stream-destroy.jstest/js/node/test/parallel/test-stream-drop-take.jstest/js/node/test/parallel/test-stream-duplex-destroy.jstest/js/node/test/parallel/test-stream-duplex-from.jstest/js/node/test/parallel/test-stream-duplex-readable-writable.jstest/js/node/test/parallel/test-stream-duplex-writable-finished.jstest/js/node/test/parallel/test-stream-duplex.jstest/js/node/test/parallel/test-stream-duplexpair.jstest/js/node/test/parallel/test-stream-end-paused.jstest/js/node/test/parallel/test-stream-filter.jstest/js/node/test/parallel/test-stream-finished-async-local-storage.jstest/js/node/test/parallel/test-stream-finished-bindAsyncResource-path.jstest/js/node/test/parallel/test-stream-finished-default-path.jstest/js/node/test/parallel/test-stream-forEach.jstest/js/node/test/parallel/test-stream-iter-broadcast-backpressure.jstest/js/node/test/parallel/test-stream-iter-broadcast-basic.jstest/js/node/test/parallel/test-stream-iter-broadcast-coverage.jstest/js/node/test/parallel/test-stream-iter-broadcast-from.jstest/js/node/test/parallel/test-stream-iter-consumers-bytes.jstest/js/node/test/parallel/test-stream-iter-consumers-merge.jstest/js/node/test/parallel/test-stream-iter-consumers-tap.jstest/js/node/test/parallel/test-stream-iter-consumers-text.jstest/js/node/test/parallel/test-stream-iter-cross-realm.jstest/js/node/test/parallel/test-stream-iter-disabled.jstest/js/node/test/parallel/test-stream-iter-duplex.jstest/js/node/test/parallel/test-stream-iter-from-async.jstest/js/node/test/parallel/test-stream-iter-from-coverage.jstest/js/node/test/parallel/test-stream-iter-from-sync.jstest/js/node/test/parallel/test-stream-iter-from-writable-cache-options.jstest/js/node/test/parallel/test-stream-iter-namespace.jstest/js/node/test/parallel/test-stream-iter-pipeto-edge.jstest/js/node/test/parallel/test-stream-iter-pipeto-signal.jstest/js/node/test/parallel/test-stream-iter-pipeto-writev.jstest/js/node/test/parallel/test-stream-iter-pipeto.jstest/js/node/test/parallel/test-stream-iter-pull-async.jstest/js/node/test/parallel/test-stream-iter-pull-sync.jstest/js/node/test/parallel/test-stream-iter-push-backpressure.jstest/js/node/test/parallel/test-stream-iter-push-basic.jstest/js/node/test/parallel/test-stream-iter-push-writer.jstest/js/node/test/parallel/test-stream-iter-readable-interop-disabled.jstest/js/node/test/parallel/test-stream-iter-readable-interop.jstest/js/node/test/parallel/test-stream-iter-share-async.jstest/js/node/test/parallel/test-stream-iter-share-coverage.jstest/js/node/test/parallel/test-stream-iter-share-from.jstest/js/node/test/parallel/test-stream-iter-share-sync.jstest/js/node/test/parallel/test-stream-iter-sharedarraybuffer.jstest/js/node/test/parallel/test-stream-iter-to-readable.jstest/js/node/test/parallel/test-stream-iter-transform-compat.jstest/js/node/test/parallel/test-stream-iter-transform-coverage.jstest/js/node/test/parallel/test-stream-iter-transform-errors.jstest/js/node/test/parallel/test-stream-iter-transform-output.jstest/js/node/test/parallel/test-stream-iter-transform-roundtrip.jstest/js/node/test/parallel/test-stream-iter-transform-sync.jstest/js/node/test/parallel/test-stream-iter-validation.jstest/js/node/test/parallel/test-stream-iter-writable-from.jstest/js/node/test/parallel/test-stream-iter-writable-interop.jstest/js/node/test/parallel/test-stream-map.jstest/js/node/test/parallel/test-stream-pipe-await-drain.jstest/js/node/test/parallel/test-stream-pipe-flow.jstest/js/node/test/parallel/test-stream-pipe-objectmode-to-non-objectmode.jstest/js/node/test/parallel/test-stream-pipeline-http2.jstest/js/node/test/parallel/test-stream-pipeline-listeners.jstest/js/node/test/parallel/test-stream-pipeline.jstest/js/node/test/parallel/test-stream-push-strings.jstest/js/node/test/parallel/test-stream-readable-aborted.jstest/js/node/test/parallel/test-stream-readable-async-iterators.jstest/js/node/test/parallel/test-stream-readable-compose.jstest/js/node/test/parallel/test-stream-readable-didRead.jstest/js/node/test/parallel/test-stream-readable-dispose.jstest/js/node/test/parallel/test-stream-readable-emittedReadable.jstest/js/node/test/parallel/test-stream-readable-ended.jstest/js/node/test/parallel/test-stream-readable-event.jstest/js/node/test/parallel/test-stream-readable-hwm-0-no-flow-data.jstest/js/node/test/parallel/test-stream-readable-infinite-read.jstest/js/node/test/parallel/test-stream-readable-needReadable.jstest/js/node/test/parallel/test-stream-readable-readable-one.jstest/js/node/test/parallel/test-stream-readable-strategy-option.jstest/js/node/test/parallel/test-stream-readable-to-web-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination-byob.jstest/js/node/test/parallel/test-stream-readable-to-web-termination.jstest/js/node/test/parallel/test-stream-readable-to-web.mjstest/js/node/test/parallel/test-stream-reduce.jstest/js/node/test/parallel/test-stream-toArray.jstest/js/node/test/parallel/test-stream-transform-destroy.jstest/js/node/test/parallel/test-stream-transform-final.jstest/js/node/test/parallel/test-stream-transform-flush-data.jstest/js/node/test/parallel/test-stream-typedarray.jstest/js/node/test/parallel/test-stream-uint8array.jstest/js/node/test/parallel/test-stream-unpipe-event.jstest/js/node/test/parallel/test-stream-unshift-empty-chunk.jstest/js/node/test/parallel/test-stream-unshift-read-race.jstest/js/node/test/parallel/test-stream-wrap-drain.jstest/js/node/test/parallel/test-stream-wrap-encoding.jstest/js/node/test/parallel/test-stream-wrap.jstest/js/node/test/parallel/test-stream-writable-change-default-encoding.jstest/js/node/test/parallel/test-stream-writable-decoded-encoding.jstest/js/node/test/parallel/test-stream-writable-destroy.jstest/js/node/test/parallel/test-stream-writable-ended-state.jstest/js/node/test/parallel/test-stream-writable-finished-state.jstest/js/node/test/parallel/test-stream-writable-finished.jstest/js/node/test/parallel/test-stream-writable-null.jstest/js/node/test/parallel/test-stream-writable-samecb-singletick.jstest/js/node/test/parallel/test-stream-writable-write-error.jstest/js/node/test/parallel/test-stream-writev.jstest/js/node/test/parallel/test-stream2-base64-single-char-read-end.jstest/js/node/test/parallel/test-stream2-basic.jstest/js/node/test/parallel/test-stream2-compatibility.jstest/js/node/test/parallel/test-stream2-httpclient-response-end.jstest/js/node/test/parallel/test-stream2-objects.jstest/js/node/test/parallel/test-stream2-push.jstest/js/node/test/parallel/test-stream2-read-correct-num-bytes-in-utf8.jstest/js/node/test/parallel/test-stream2-readable-wrap.jstest/js/node/test/parallel/test-stream2-transform.jstest/js/node/test/parallel/test-stream2-writable.jstest/js/node/test/parallel/test-stream3-cork-end.jstest/js/node/test/parallel/test-stream3-cork-uncork.jstest/js/node/test/parallel/test-stream3-pause-then-read.jstest/js/node/test/parallel/test-stream3-pipeline-async-iterator.jstest/js/node/test/parallel/test-zlib-flush-write-sync-interleaved.jstest/js/node/test/sequential/test-stream2-stderr-sync.jstest/napi/node-napi-tests/test/node-api/test_uv_threadpool_size/do.test.ts
💤 Files with no reviewable changes (8)
- test/js/node/test/parallel/test-stream-pipe-flow.js
- test/js/node/test/parallel/test-stream-pipe-await-drain.js
- test/js/node/test/parallel/test-stream-pipe-objectmode-to-non-objectmode.js
- test/js/node/test/parallel/test-stream-map.js
- test/js/node/test/parallel/test-stream-pipeline-listeners.js
- src/js/internal/streams/operators.ts
- test/js/node/test/parallel/test-stream-pipeline-http2.js
- test/js/node/test/parallel/test-stream-iter-writable-interop.js
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
22307d5 to
2305b6e
Compare
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
f6fd167 to
22fd91f
Compare
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
699fc81 to
1c721e2
Compare
Contributor
There was a problem hiding this comment.
All prior review findings have been addressed and the bug-hunt found nothing new this round, but at 166 files spanning core stream semantics, http/net/fs behavior, native C++/Rust bindings, and a new ~7.5k-line experimental API, this needs a human sign-off.
Extended reasoning...
Overview
This PR syncs Bun's node:stream implementation to Node v26.3.0, vendoring all 251 upstream stream tests (247 passing). It spans 166 files:
- New experimental API (~7.5k lines):
node:stream/iterandnode:zlib/iter— push/pull/from/share/broadcast/transform/consumers/classic/duplex/ringbuffer modules, gated behind--experimental-stream-iterat the resolver level. - Core stream behavioral changes:
read()one-buffer-at-a-time semantics,compose()moved to the prototype, pipeline error precedence,Readable.fromdestroy chaining,Duplex.fromerror propagation, web-adapter BYOB support. - Adjacent subsystems: http client (
destroy(err)error-then-close ordering, sync EOF push), http server (IncomingMessage.socketnull sentinel,detachSockettolerance),fs.ReadStream(kIsPerformingIO/kIoDone),net.Socket({fd})sync-write adoption,async_hooksTickObjectinitevents. - Native code:
BunProcess.cpplazy-property exception scoping (6 builders),JSCommonJSModule.cppeval-entry exception fix (the 3-argJSC::evaluateoverload was silently discarding errors),ErrorCode.cppmessage formatting,NodeModuleModule.cppflag accessor. - Rust:
ErrorCode.rsfull renumbering (191 stale ordinals fixed), brotli error codes rewritten to match Node, resolver gating for the new modules, CLI argument registration.
Security risks
None identified. The new --expose-internals require interceptor is scoped to the test harness (test/js/node/test/common/index.js) and only serves a fixed map from bun:internal-for-testing, which is itself gated. The net.Socket({fd}) adoption path is gated on explicit writable: true, validates the fd via fstatSync, closes non-stdio fds on destroy, and was reworked twice during review to avoid leaking fds and stomping the connect path. No new auth/crypto/permission surfaces.
Level of scrutiny
High — human review required. This touches production-critical Node-compat hot paths (every process.nextTick, every Readable read(), http request/response lifecycle, fs stream destroy). The review history bears this out: across ~26 inline findings from me and ~20 from CodeRabbit over two days, several were real correctness bugs — an ErrorCode.rs ordinal collision that made native code throw the wrong error codes, a net.Socket({fd}) fd leak, a WHATWG Streams spec regression in BYOB close handling (introduced mid-review by a bad bot suggestion and caught/reverted), and a ClientRequest.destroy(err) resource-leak chain that took three iterations to close fully. All threads are now resolved, but the density of issues found is itself a signal that the surface area warrants human eyes on the design choices (the http destroy() synchronous-emit-with-try/catch approach, the partial async_hooks implementation, the fd-adoption gating).
Other factors
- Test coverage is strong: 247/251 vendored upstream tests pass; the 4 failures are documented native-subsystem gaps with expectations entries. Blast-radius A/B against baseline (488 http tests, zlib/crypto, console/process/fs/net/child_process/web-streams suites) was reported clean.
- No human reviewer has weighed in — only robobun (build status), CodeRabbit, github-actions (dup-detector), and me.
- All bot review threads resolved; nothing outstanding from the bug-hunt this run.
- Design decisions a human should weigh: whether
net.Socket({fd, writable:true})sync-write adoption is the right shape vs. waiting for the libuv-handle layer; whether theClientRequest.destroy(err)synchronous-emit-before-teardown approach (vs. Node's teardown-first) is acceptable; theprocess.nextTickhot-path cost of thetickInitHooks.lengthcheck + slice-on-dispatch; thetest/expectations.txtdarwinfs.watchexpected-fail entry (pre-existing leak unmasked, deferred to follow-up).
…experimental-stream-iter, read()-one-buffer semantics, compose/pipeline/web-adapter/http/fs/net parity [build images]
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
1c721e2 to
421998a
Compare
… images] The verbatim node v26.3.0 test asserts a FinalizationRegistry callback fires within one gc() + one setImmediate. JSC's FR callbacks are scheduled via DeferredWorkTimer with no spec-guaranteed ordering relative to the immediate queue; on alpine x64 (build 63145) the callback delivery slips past the single setImmediate after this PR's added module loads shift startup heap layout. The connect listener IS removed (listenerCount === 0 verified) and the object IS collected on every other platform (70/70 macOS arm64, glibc Linux, Windows). The robust shape is gcUntil(), but the file is a verbatim upstream port.
Resolves conflicts from #31587 (node:http client rewrite, merged as c4a937c): - _http_client.ts, _http_incoming.ts: take main. #31826's edits targeted the now-removed native-handle paths (kHandle/webRequestOrResponse-based _read, the manual destroy(err) error/close ordering); #31587's rewrite delivers the same semantics via the real net.Socket — destroy(err) routes through socketErrorListener so 'error' precedes 'close', and the FakeSocket lazy-getter is gone (real socket assigned). - _http_server.ts: keep #31826's detachSocket null-socket comment; the body (the if-socket guard) auto-merged identically. - ErrorCode.rs: regenerated from the merged ErrorCode.ts (mirrors generate-node-errors.ts's slice(3) extras counting). main = 318+3 extras = 321; merged = 320+4 = 324 (#31826 adds ERR_OPERATION_FAILED and ERR_STREAM_ITER_MISSING_FLAG mid-list, shifting all subsequent indices). Verified against the live codegen: NODE_ERROR_COUNT = 324. - node-http.test.ts: concatenated #31826's 3 destroy(err)-ordering tests with main's 30 new tests (no name overlap).
The stream destroyer (internal/streams/destroy.ts:312) does `req.socket = null; req.destroy(err)` for server requests so the connection outlives the request and the response can still reply. Node's _destroy (lib/_http_incoming.js:252) gates teardown on `this.socket`; the native-handle path here did not, so destroyer's intent was lost and the connection was reset (test-stream-destroy.js saw socketOnEnd's hang-up on the client). Restores the guard the streams-side merge resolution dropped.
…iming assertions The merge took #31587's _http_client/_http_incoming rewrite, leaving: - emitEOFIncomingMessage's `sync` parameter dead (all 4 callers pass one arg) — reverted to single-arg. - Two ClientRequest.destroy(err) tests asserting #31826's old synchronous-close-inside-destroy timing. #31587's node-port routes destroy(err) through socket.destroy(err) → async error then close. Verified vs node v26.3.0 (closed=false synchronously after destroy(); test 3 script outputs 'teardown-ran:false'); rewrote the assertions to match. Test 1 (error-before-close ordering) was already correct.
Root cause (block 10, L271-303): pipeline(rs, req) writes 11x'hello' raw after a never-ended GET's CRLFCRLF (no Transfer-Encoding: chunked is added). The server parses the GET as no-body and treats the trailing bytes as the next pipelined request. node's llhttp rejects lowercase 'h' as a method byte -> HPE_INVALID_METHOD -> clientError -> 400+close -> the client's req closes -> the pipeline callback fires. bun's uWS HttpParser buffers any incomplete run of valid tchars waiting for the request-line (verified: 'A_B' triggers clientError, 'HELLO' does not), so the connection stays open and the callback never fires. Pre-existing server-parser leniency in packages/bun-uws/src/HttpParser.h; out of scope for this PR.
cirospaciari
force-pushed
the
claude/streams-node26-sync
branch
from
ddfce53 to
eafe811
Compare
Comment on lines
+84
to
+87
| #[strum(serialize = "node:stream/iter")] | ||
| NodeStreamIter, | ||
| #[strum(serialize = "node:zlib/iter")] | ||
| NodeZlibIter, |
Contributor
There was a problem hiding this comment.
🟡 With --experimental-stream-iter, Module.isBuiltin('node:stream/iter') / 'node:zlib/iter' return false and module.builtinModules omits 'stream/iter'/'zlib/iter', where Node returns true / includes them — Bun::isBuiltinModule (isBuiltinModule.cpp) and builtinModuleNames[] (NodeModuleModule.cpp) are static tables this PR did not touch and have no flag awareness. Introspection-only Node-parity gap on an experimental API (the inverse of resolved #3365867904, which was about require.resolve.paths over-reporting without the flag); Bun__streamIterEnabled() is already exported and could gate dynamic entries in both, or this can go in Known limitations if making the static tables flag-aware is out of scope.
Extended reasoning...
What the bug is
With --experimental-stream-iter passed, Node v26's introspection APIs report the new modules as builtins:
Module.isBuiltin('node:stream/iter')→trueModule.isBuiltin('node:zlib/iter')→truemodule.builtinModules.includes('stream/iter')→true(and'zlib/iter')
Node implements this via BuiltinModule.canBeRequiredByUsers, which is conditionally populated from the flag in lib/internal/bootstrap/realm.js — both Module.isBuiltin and module.builtinModules derive from it, so they pick up flag-gated experimental modules automatically (same pattern as node:sqlite with --experimental-sqlite).
In Bun, these APIs are backed by static, flag-unaware tables that this PR did not touch:
Module.isBuiltin→jsFunctionIsBuiltinModule(NodeModuleModule.cpp:199-213) →Bun::isBuiltinModule(moduleStr)(isBuiltinModule.cpp), which switch-matches against a hardcoded string list. Grep confirms it contains"stream/consumers"but not"stream/iter"or"zlib/iter".module.builtinModules→builtinModuleNames[](NodeModuleModule.cpp:52), astatic constexpr ASCIILiteral[]. Grep confirms it contains"stream/consumers"(line 109) but not"stream/iter"or"zlib/iter".
Neither file is in the PR's 166-file changed list. So with the flag set, require('node:stream/iter') succeeds (the resolver/alias gate at HardcodedModule.rs:872 is wired up correctly), but the introspection APIs report it as not-a-builtin.
Step-by-step proof
bun --experimental-stream-iter -e "
const Module = require('node:module');
console.log(Module.isBuiltin('node:stream/iter')); // false (Node: true)
console.log(Module.isBuiltin('node:zlib/iter')); // false (Node: true)
console.log(module.builtinModules.includes('stream/iter')); // false (Node: true)
console.log(typeof require('node:stream/iter')); // 'object' — loads fine
"Module.isBuiltin('node:stream/iter')callsjsFunctionIsBuiltinModule(NodeModuleModule.cpp:199).- That calls
Bun::isBuiltinModule("node:stream/iter")(line 213), which strips thenode:prefix and switch-matches against the static table in isBuiltinModule.cpp. "stream/iter"is not in that table (only"stream/consumers","stream/promises","stream/web"are) → returnsfalse.- The function takes only a
const String&— there is no flag parameter to consult, andBun__streamIterEnabled()is not called anywhere in isBuiltinModule.cpp. - Meanwhile
require('node:stream/iter')goes throughAlias::get()(HardcodedModule.rs:872), which does checkstream_iter_alias_gated()→ flag is set → resolves and loads fine.
Relationship to resolved finding #3365867904
That finding was about require.resolve.paths('stream/iter') returning null (treating it as a builtin) without the flag, because ModuleLoader__isBuiltin linear-scanned the raw alias tables and bypassed the flag gate. It was fixed in f67acf3 by adding stream_iter_alias_gated() to bun_aliases_get. That finding's reasoning explicitly listed:
Module.isBuiltin('stream/iter')→ usesBun::isBuiltinModule(isBuiltinModule.cpp), a separate hardcoded list;stream/iterwas not added there → returnsfalse✓module.builtinModules→ …stream/iternot added → not listed ✓
Those ✓ marks were correct in the without-flag context: returning false / being absent is what Node does without the flag. This finding is the inverse: with the flag set, Node returns true / includes them, and Bun still returns false / omits them. The author's own analysis in #3365867904 thus already documented these two tables as separate from the alias-gate fix — they were just out of scope for that (over-reporting) bug, and this (under-reporting) gap remained.
Why existing code doesn't prevent it
This PR wired the flag gate into three layers: Alias::get() (resolver), bun_aliases_get (require.resolve.paths / Module._resolveLookupPaths), and the JS-side interop check (createStreamIterEnabledFlag). Bun::isBuiltinModule and builtinModuleNames[] are a fourth and fifth layer that were not touched. They are entirely separate code paths (C++ static tables, not the Rust alias machinery), so none of the PR's gating reaches them. The vendored test-stream-iter-namespace.js doesn't call Module.isBuiltin, so no test catches this.
Impact
Low — introspection-only, experimental, nit. require('node:stream/iter') works correctly; only feature-detection code that uses Module.isBuiltin('node:stream/iter') instead of try/catch around require would falsely conclude the module is unavailable. Feature-detecting a flag-gated experimental module via isBuiltin is rare. This is the same class of "static table not updated for new module" gap that any new Node builtin would hit; the only thing that makes it PR-specific is that the table needs to be flag-aware, not just appended to.
How to fix
Bun__streamIterEnabled() is already exported as extern "C" (HardcodedModule.rs / declared in NodeModuleModule.cpp), so both sites could call it:
- isBuiltinModule.cpp: after the static-table miss, add
if ((name == "stream/iter"_s || name == "zlib/iter"_s) && Bun__streamIterEnabled()) return true;(mirroring how thenode:prefix is already special-cased there). - NodeModuleModule.cpp (
builtinModuleNames[]consumer that builds the JS array): after the static loop, conditionallyarray->push(... "stream/iter")etc. whenBun__streamIterEnabled().
Alternatively, accept the gap and add it to the PR's Known limitations section — it's an experimental introspection edge and the fix touches two more tables that are otherwise out of this PR's scope.
This was referenced Jun 17, 2026
This was referenced Jun 18, 2026
cirospaciari
added a commit
that referenced
this pull request
Jun 18, 2026
- The 8 internal/streams/iter/* + stream.iter.ts + zlib.iter.ts files were add/add: both this PR and #31826 (now in main) ported them independently. Took main's versions — they are a strict superset (larger in every file, include push/duplex/broadcast/share/merge factories this PR's trimmed port omitted) and contain no fs-specific code (the FileHandle pull/writer integration lives in internal/fs/streams.ts and uses the public iter API). - BunProcess.cpp: took main — the duplicate shareableBuiltins declaration this PR added is already covered by main's TOP_EXCEPTION_SCOPE pattern at the function head. - HardcodedModule.rs: took main — adds the node:zlib/iter entries this PR's trimmed iter port omitted. - node_fs.rs / sys/lib.rs / sys_uv.rs: comment-only conflicts (both rewrote the same lchown comment); took main. - ErrorCode.ts: appended this PR's 3 fs codes (ERR_FS_CP_EEXIST, ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY, ERR_DIR_CONCURRENT_OPERATION) after main's existing entries to preserve main's indices. - ErrorCode.rs: regenerated from the merged .ts; COUNT=327, verified against generate-node-errors.ts.
cirospaciari
added a commit
that referenced
this pull request
Jun 18, 2026
Resolves conflicts in BunProcess.cpp, JSCommonJSModule.cpp, and test/expectations.txt. The eval-entry exception fix this branch carried landed on main via #31826, so JSCommonJSModule.cpp now only adds the noteCommonJSEvaluation call. Also folded in while resolving: - exit 6 when process._fatalException is deleted (was only handling the set-to-undefined case; Node treats both as a non-callable handler) - module.isBuiltin('bun:wrap') now agrees with builtinModules - adopt main's reportUncaughtExceptionAtEventLoop in the new lazy property builders (constructFinalization / constructVersions / constructAllowedNodeEnvironmentFlags) - drop now-unused Process_stubEmptySet and its JSSet.h include
cirospaciari
added a commit
that referenced
this pull request
Jun 18, 2026
- The 8 internal/streams/iter/* + stream.iter.ts + zlib.iter.ts files were add/add: both this PR and #31826 (now in main) ported them independently. Took main's versions — they are a strict superset (larger in every file, include push/duplex/broadcast/share/merge factories this PR's trimmed port omitted) and contain no fs-specific code (the FileHandle pull/writer integration lives in internal/fs/streams.ts and uses the public iter API). - BunProcess.cpp: took main — the duplicate shareableBuiltins declaration this PR added is already covered by main's TOP_EXCEPTION_SCOPE pattern at the function head. - HardcodedModule.rs: took main — adds the node:zlib/iter entries this PR's trimmed iter port omitted. - node_fs.rs / sys/lib.rs / sys_uv.rs: comment-only conflicts (both rewrote the same lchown comment); took main. - ErrorCode.ts: appended this PR's 3 fs codes (ERR_FS_CP_EEXIST, ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY, ERR_DIR_CONCURRENT_OPERATION) after main's existing entries to preserve main's indices. - ErrorCode.rs: regenerated from the merged .ts; COUNT=327, verified against generate-node-errors.ts.
cirospaciari
added a commit
that referenced
this pull request
Jun 18, 2026
… gate landed This branch's stream/iter port assumed the modules are always available; main's #31826 (now merged) gates them behind --experimental-stream-iter. The merge auto-merged three places that encoded the old assumption: - common/index.js intercepted the --experimental-stream-iter flag comment with "always enabled in Bun; no flag needed" and skipped the re-spawn — so tests with that flag ran without it and got "Cannot find module 'stream/iter'". Removing the interception lets the re-spawn path pass the flag (as on main). - NodeModuleModule.cpp listed stream/iter and zlib/iter unconditionally in builtinModules; main appends them only when Bun__streamIterEnabled(). With both, builtinModules listed modules that require() couldn't load, breaking test-require-builtins.ts and test-require-resolve.js. - isBuiltinModule.cpp / HardcodedModule.zig had the same unconditional entries; reverted to main. Verified: builtinModules excludes */iter without the flag; test-require-builtins/-resolve pass; stream-iter suite 42/0 via the re-spawn path; the FileHandle pull/writer/pullsync tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the node:stream suite to full parity with Node v26.3.0 by vendoring every upstream stream test, implementing the experimental
stream/iter/zlib/iterAPI behind--experimental-stream-iter, and fixing the runtime gaps the tests expose.All 251 upstream stream test files are now vendored (63 added, 64 resynced to v26.3.0 content); before this PR 124 were byte-identical to upstream and 188 existed at all.
New feature:
stream/iter+zlib/iter(experimental)Node v26's iterable-streams API, ported (~7.5k lines: push/pull/from/share/broadcast/transform/consumers/classic/duplex/ringbuffer + entry modules), gated like Node:
--experimental-stream-iter: without the flag,node:stream/iterreportsNo such built-in moduleand barestream/iterfalls through to filesystem resolution, byte-matching Node's errors.Readable.prototype[Symbol.for("Stream.toAsyncStreamable")]interop with Node's lazy flag check (ERR_STREAM_ITER_MISSING_FLAG).test-stream-iter-*files pass (the 2disabledtests also required the eval-entry exception fix below).Behavior changes
read()returns one buffered chunk at a time instead of concatenating the whole buffer (stream: readable read one buffer at a time nodejs/node#60441): thehowMuchToReadfast path for buffers.read(n)with a decoder no longer over-returns:fromListcomparednagainst the buffer-array length instead of the chunk length.resume()/pause()are no-ops on destroyed streams.Readable.prototype.composemoved off the operators registry onto the prototype returning the composed Duplex directly (Node v26 layout); the registry wrapper hid the writable side.Readable.fromdestroy chaining:_destroychains the instance's original_destroy(e.g. duplexify's abort) before closing the iterator, andduplexify'sfromAsyncGen.destroyunblocks a generator parked on its source — errors now propagate through nested.compose()chains instead of hanging.Duplex.from({ readable, writable }): a readable-side error destroys the writable side (was destroying the readable itself).write(string, "buffer")throwsERR_UNKNOWN_ENCODING.Duplex.toWeb/Readable.toWebsupport byte streams (readableType: 'bytes'+ the deprecatedtypealias with DEP0201);ReadableStream.cancel()settles pending BYOB reads with{done: true}; readable completion no longer waits for a half-open writable side, so the web stream closes when the readable side ends.'readable'for data+EOF, like Node's parser);ClientRequest.destroy(err)emits'error'on the request and destroys an incomplete response withECONNRESET(Node's socketCloseListener semantics).IncomingMessage.socketgetter no longer resurrects a FakeSocket after the stream destroyer detached it —req.socket = nullnow signals "connection must outlive the request" like Node, sores.end()afterdestroy(req)still delivers the body;detachSockettolerates the detached state.kIsPerformingIO/kIoDone, Node-verbatim), soclose()between reads yieldsERR_STREAM_PREMATURE_CLOSEinstead of a clean end, and_destroywaits for in-flight IO before closing the fd.net.Socket({ fd })adopts pipe/character-device/file/socketpair fds with synchronouswrite(2)-backed writes — Node's effective stdio-socket semantics (data survives an immediateprocess.exit()). Network-socket fd adoption still needs the handle layer.createHookinitevents now fire forTickObjectresources — one perprocess.nextTick()call, with a single array-length check on the disabled hot path. Init-only hooks no longer emit the "not implemented" warning. stdio writers are born constructed (no construct TickObjects; Node's stdio streams never have_construct).--expose-internals: vendored node tests can requireinternal/*modules, served frombun:internal-for-testingvia a require interceptor installed by the test harness; newinternal/async_context_frameandinternal/async_hooksshims back thefinished()ALS tests.JSC::evaluateoverload silently discards exceptions into an unused out-param, sobun -e 'require("./missing")'exited 0 with no output; it now prints the resolution error and exits nonzero (also un-hangs--printfor the same case).ERR__ERROR_FORMAT_PADDING_2style —"ERR_"+ enum suffix with its leading underscore);ERR_CRYPTO_UNKNOWN_CIPHERmessage isUnknown cipher;Unknown encoding:values format like Node's%s(objects inspect to{}); new codesERR_OPERATION_FAILED,ERR_STREAM_ITER_MISSING_FLAG,ERR_INVALID_ARG_VALUERangeError variant.src/jsc/ErrorCode.rshad drifted from the generated C++ table (191 stale ordinals — native code threw adjacent error codes, e.g.ERR_INVALID_HANDLE_TYPEforERR_INVALID_HTTP_TOKEN); it is renumbered fromErrorCode+List.h.Known limitations / follow-ups
test-stream-wrap{,-drain,-encoding}needinternal/js_stream_socket, i.e. anet.Socket({ handle })libuv-handle compatibility layer — the same subsystem TLS-over-arbitrary-duplex wants; left as the follow-up.test-stream-pipelineneeds per-socket-read http client body delivery: the native fetch path coalesces response-body segments between JS pulls (one merged buffer instead of Node's one'data'per TCP read). Pre-existing, proven on the unmodified baseline; a fix wants an opt-in per-segment mode on the native response stream to avoid regressing fetch throughput.AsyncContextFrame.current()returnsundefined(bun tracks async context natively and never materializes frame objects);enabledis honestlytrue. Tests couplingenabledto a non-nullcurrent()would not pass.initis emitted for TickObject only; promise/timer/native resources and before/after/destroy remain unimplemented (and still warn).test-stream-readable-default-encoding-error, held-back pair) run pre-v26 content because their upstream versions require internals not in the exposed map.Testing
Validated locally with the debug build:
test-streamsweep (CI-parity mode): 247/251 files pass; the 4 failures are the documented native gaps above, each verified pre-existing on an unmodified baseline build.test-httpsweep (488 files — the only deltas were the ErrorCode-ordinal regressions, fixed in this PR), vendored zlib/crypto tests, and the bun-nativeconsole/process/fs/net/child_process/async_hooks/ web-streams suites — identical or better everywhere.-ehappy path, missing-require error+exit-1, stdin scripts,--print.