Skip to content

node:stream: wake a pipeline() pump that waits on an idle web stream - #44769

Open
robobun wants to merge 10 commits into
mainfrom
robobun/673ad347/pipeline-web-member-teardown
Open

robobun wants to merge 10 commits into
mainfrom
robobun/673ad347/pipeline-web-member-teardown

Conversation

@robobun

@robobun robobun commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • When the client leaves, pipeline(upstream.body, res, cb) never calls back while the upstream is idle. An abort hangs the same way, as on Node v26.3.0.
  • The pump waits in for await (pipeline.ts:129, :161 on main). return() on that iterator queues behind the pending next().

Fix

  • The pump holds the reader of a web source. A dead destination stops it: the source is cancelled in the next turn, and the pump fails on main's own path.
  • After an abort, the report of the destroyed destination stops the pump. Until then it stays on the source, as on main.
  • Self-reviewed: 25 concerns raised, 25 addressed. Verified: test/js/node/stream/node-stream.test.js (104 new tests, 62 fail on main), 271 vendored node tests.

Background

Downsides

  • A destination that its owner ends or destroys before the web source has ended fails the pipeline at once, as with a node source. On main, { end: false } resolved if the source ended.
  • After a signal, a later error of the source is not reported. A WritableStream destination is aborted, also with { end: false }. A cancel() that never settles blocks the callback, as with pipeTo().
  • Web source: +4 to +10 cells per call, -1 to +1 per chunk. require("node:stream"): +9 cells. Binary: +4,556 bytes. No perf here: instructions not measured.
Notes

Merged with main at 8d5672c (#45631). pipeline.ts is typed there, so pumpToNode and pumpToWeb keep the typed signatures of main and take the destroys list. The build of main checks every $newCppFunction binding and every $name of src/js against the table of built-ins. The three reader functions and functionSetImmediate are in src/codegen/builtins/host/unseen-code.builtins.ts now, and values (read as .$values) is in src/codegen/builtin-names.ts. The code spells ReadableStream and ReadableStreamDefaultReader without $: the bundler turns both into the private globals.

No issue exists for this. A fuzz run found it. Node v26.3.0 has the same two pumps (lib/internal/streams/pipeline.js) and hangs in the same cells. Its documentation says that stream.pipeline() calls stream.destroy(err) on every stream that has not ended, and calls the callback. cloudflare/workerd#7338 gives workerd's pipeline() the same pump-level teardown. One call avoids the hang on every runtime: Readable.fromWeb(body), body.pipeTo(Writable.toWeb(file), { signal }), or the same signal on fetch().

Repro

const { pipeline } = require("node:stream/promises");
const { Writable } = require("node:stream");

const ac = new AbortController();
const source = new ReadableStream({
  cancel() {
    console.log("source cancelled");
  },
});
const sink = new Writable({ write: (chunk, encoding, callback) => callback() });

pipeline(source, sink, { signal: ac.signal }).then(
  () => console.log("resolved"),
  err => console.log("rejected:", err.name),
);
setTimeout(() => ac.abort(), 10);
setTimeout(() => console.log("1 s later"), 1000);

Node v26.3.0 and Bun main print 1 s later. This branch prints source cancelled, rejected: AbortError, 1 s later.

How it works

  • HeldReader is the async iterable that the pump loops over in place of the stream. It keeps a ReadableStreamDefaultReader, so the pipeline can cancel a read that is pending. Both for await loops keep their text.
  • Three things stop a pump at once: an error that end-of-stream reports for the node destination, the 'close' of that destination before the source has ended (the rule that pipe() has for node sources), and an error of a web sink (the stream's closed promise rejects).
  • The teardown of the pipeline (one entry in destroys per pump) stops the pump too. With end, the teardown has destroyed the node destination, and the pump then stays on the source until that destination reports through end-of-stream, as on main. If the source ends or fails first, the pump goes main's way. The report stops the pump. So the callback comes after the 'close' of the destination, the destination's own error wins over the AbortError (stream: preserve error over AbortError in pipeline nodejs/node#62113), and a destination that never reports its destroy leaves the pipeline as it is on main.
  • A stopped pump reads no more chunks. The source is cancelled in the next turn of the event loop, in the async context of the pipeline() call. An owner of the source that reacts to the same signal within that turn closes, errors or feeds its stream first, as it does today.
  • The callback waits for the cancel() of the source where the pump had started to read, as the loop waits for return() today. With end: false the pipeline does not destroy a node destination and does not wait for it.
  • The pump takes nothing from the program. read(), cancel(), releaseLock() and setImmediate are $newCppFunction bindings to the host functions. The cancel goes through a reader, so a cancel() that a subclass of ReadableStream defines is never called, as on main. A program that replaces those properties (a spy, fake timers), before node:stream loads or later, does not reach the pump. WPT streams/readable-streams/patched-global.any.js requires the same of the stream's own iterator, which main's pump used.
  • A stream is held when its [Symbol.asyncIterator] is the built-in function. ReadableStream.prototype keeps that function under a private name ($values) for this comparison. A subclass with an iterator of its own, a stream with an own iterator, and a program that replaced the iterator on the prototype are read through that iterator, as on main.
  • A pump that the teardown stopped does not wait for room in a WritableStream before it aborts it. A write in flight that ends only on the sink's controller.signal then ends. Node and main abort the sink at once in that cell for a node Readable.
  • A web stream that a function stage returned is not held. Node documents that the stage must handle signal.
  • The class and its four bindings are created at the first pipeline() with a web source. Nothing is read at load.

What changes for a pipeline that settles today

One process per cell, on Node v26.3.0, on a release build of main (5900354) and on one of this branch. 3,298 cells: 58 kinds of web source (byte streams, type: "direct", fetch and Response bodies, Blob, Bun.file, tee branches, subclasses), node and web destinations (files, sockets, http.ServerResponse, zlib, compose(), legacy emitters, sinks that throw, reject or never report), destinations that are gone before the call, an abort or a dead destination, and what the owner of the source does in the next 0 to 5 turns. Node equals main in all but a few races.

  • 2,301 cells: the same events in the same order.
  • 379 cells hang on main and settle.
  • 464 cells: the same callback and other events. The pump cancels the source, or a WritableStream sink is aborted when the signal stops the pump (main wrote until the source ended, then closed the sink, or left it open and locked with { end: false }).
  • 133 cells: another callback. The classes are below.
  • 16 cells: main has an unhandled rejection for a WritableStream that is locked, and the branch reports that error through the callback.
  • 4 cells: a program that feeds the source through a TransformStream and does not handle the promises of its write() calls. Those promises reject, with undefined, when the pump cancels the readable side. On main that happens when a chunk meets the dead destination.
  • 1 cell settled on main and not on the branch in the stored run: a fetch body that the server resets, into a WritableStream with highWaterMark: 0. It is a race. Main does not call back either in 3 of 3 reruns.
  • 9,000 seeded random scripts (members, options, and a schedule of chunks, closes, errors, aborts and dead sinks): 7,927 have the same outcome, 498 settle where main hangs, 575 have another callback. None hangs where main settles. None has a new uncaught error.

The callbacks that change:

  • A destination that is gone before the web source has ended: its owner ended or destroyed it during the call, or it was destroyed before the call. The pipeline fails at once, with the destination's error or with ERR_STREAM_PREMATURE_CLOSE, as with a node source, and as on main when a chunk came next. On main the pump stayed on the source, so the result came from the source's next step: success with { end: false } if it ended, its own error if it failed, alone or in an AggregateError.
  • A signal, and then the source fails, or a write fails, one or more turns later. Main reported that later error, alone or in an AggregateError with the AbortError. The branch has cancelled the source and stopped the pump by then, and reports the AbortError.
  • A WritableStream whose controller errors with a falsy reason (controller.error() with no argument). The callback gets no error, as on main when a chunk meets that sink. On main a later error of the source was reported.
  • Races of one microtask. The pump has one microtask turn less per chunk, so the last chunk can reach a destination before its owner destroys it in the same tick.

More that a program can see:

  • A producer that has no cancel() and goes on calling controller.enqueue() from a timer: its first call after the cancel throws, and nothing catches it. Into a node destination that is so on main too, once a chunk meets the dead destination. Into a WritableStream it is new, because main ignored the abort there.
  • A source whose cancel() returns a promise that never settles blocks the callback. A tee branch, a Response body after clone() and ReadableStream.from() over a generator that waits are not in this class: their cancel settles when the source ends.
  • A signal aborts a WritableStream destination, with { end: false } too: the stopped pump fails, and a pump that fails aborts its sink. Node and main do this for a node Readable with { end: false }. The sink stays locked in every case, on Node too.
  • compose() with a web stream in front of a node stream now closes on destroy(). It emits the 'error' (AbortError) that a compose() of node streams emits.
  • A type: "direct" source that flushes every 10 KiB from a microtask loop arrives as 100 chunks, not 51, for the same 1,024,000 bytes. Bun.file().stream(), a fetch body and Blob.stream() keep their chunk counts and their write() counts.

Not changed, as on main and Node (#44770): a destination that finished and closed before the call, a WritableStream that was closed before the call, a destination that never reports its destroy, and a WritableStream whose write in flight never settles.

Measurements

Release builds of main (5900354) and of this branch (7c13a68), Linux x64. Cells: the sum of heapStats().objectTypeCounts before and after a batch of pipelines with no collection in between, one fresh process per case. The branch has merged main 11c3671 since then. That merge adds types to pipeline.ts and changes nothing that is measured here.

  • Cells per call: web source into Writable 137 to 141, into WritableStream 64 to 70, with a TransformStream between 181 to 191, Readable into WritableStream 131 to 132. Readable.fromWeb(web) into Writable is 237 on both.
  • Cells per chunk, by how the source produces. Chunks queued ahead: 3 to 3 (Writable), 6 to 6 (WritableStream), 17 to 16 (through a TransformStream). A synchronous pull(): 3 to 4, 6 to 6, 17 to 16. An async pull(): 11 to 10, 12 to 13, 23 to 22. A chunk that arrives while the pump waits: 5 to 4, 8 to 7.
  • Microtask turns per chunk: web source into Writable 2 to 1, into WritableStream 3 to 2.
  • No web member (Readable into Writable): cells per call and per chunk are equal.
  • require("node:stream"): 3,629 to 3,638 live cells after a full collection.
  • Kept alive by an idle pipeline: 92 to 94 cells (web into node), 49 to 54 (web into web). After a finished { end: false } pipeline, a WritableStream that the caller keeps holds 13 cells, not 8. It keeps no AsyncLocalStorage store of the call.
  • size of the two binaries: text +4,556 bytes (106,753,900 to 106,758,456), data and bss equal. The bundled pipeline.js is 9,950 and 14,246 bytes. Host functions: none added. Four $newCppFunction bindings to host functions that exist, and one private property on ReadableStream.prototype.
  • bun build --compile --trim: no feature more is kept. For a program that only imports node:stream, bun check --aot --explain=trim on this branch says that the web streams are kept for the native binding of internal/streams/native-readable. Main has that binding too.
  • Wall clock per chunk, 30 interleaved runs: no usable number. The Readable into Writable path, which the diff does not touch, moves by 14% between the builds in the same runs. perf, valgrind, bloaty and strace are not installed in the container that ran this, so there are no instruction counts.

Tests

  • 104 tests in one block of test/js/node/stream/node-stream.test.js. On a release build of main 62 fail and 42 pass. The 42 are cells that node settles, pinned so that they stay as they are.
  • Single-clause changes to the new code were tried against the block at each stage of this PR: 93 in all, of which 91 fail it. One that passes cancels a source that ended by itself, which costs allocations and changes nothing else. The other schedules a second immediate that does nothing.
  • 271 vendored node tests (test-stream*, test-webstreams*, test-whatwg-webstreams*, and every file that calls pipeline(): 266 pass and the same 5 fail with main's pipeline.ts. wpt-streams.test.ts (1,175 tests), test/js/web/streams/streams.test.js (688 tests) and web-stream-state.test.ts pass.

Review history

  • An earlier version of this change gave the pipeline error to cancel(), aborted the sink from the teardown, and reported a dead sink by itself. Self-review found pipelines that settle today and changed: an error lost behind an AbortError, an early exit through a TextDecoderStream that became an error, an { end: false } node destination that was destroyed. The version that opened this PR is the rework: the teardown only stops the pump. Not taken from that review: a tick-drain hook from child_process: deliver each stdio event after the nextTicks and promise jobs of the one before #43790 for the order of the cancel (the next turn of the event loop needs no hook), and the same stop for pumps with no web source (node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770).
  • After the first push, three forms broke that settle on main: a subclass whose cancel() returns no promise (the pump called it and then then on the result), end: false with a destination that never reports its destroy (the stopped pump waited for that report), and a TransformStream whose writable is not a WritableStream (the watch on the sink threw outside the pump's try). The fourth commit fixes the three and adds one test for each.
  • The sixth commit took the reader's methods when the class was created. A spy that was in place at that moment stayed in the pump after the program restored it. The same held for the iterator that hold() kept, and for a setImmediate that a program replaced before node:stream loaded: two pipelines that settle on main then hung. The seventh commit keeps nothing from the program.
  • A second self-review, of the seventh commit, found four more defects. After an abort the pump cancelled the source and then waited without bound for the destroyed destination: a destination that never reports (a destroy() that does not call back, compose() with a web tail) hung a pipeline that settles on main when the source fails later. A stopped pump waited for room in a WritableStream before it aborted it. The watch on a WritableStream kept the AsyncLocalStorage store of the call. hold() skipped an iterator that a program put on ReadableStream.prototype. The eighth commit fixes the four.
  • Kept, against a review finding: a signal aborts a WritableStream destination with { end: false }. Node v26.3.0 and main do the same when the source is a node Readable. Left out, and in node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770: a destination that finished and closed before the call. Node hangs there too, and a check at the start of the pump would fail a pipeline that resolves today when the source ends with no chunk.

Commits

  1. The reader, the teardown entry, the destination error and the web sink.
  2. The 'close' rule of pipe() for a web source. It carries the change for a destination that is gone early, and the Http2ServerResponse case.
  3. A locked web sink reports through the callback. It can go to its own PR.
  4. The three forms above, and the review fixes.
  5. Two style fixes from review: a default for the #stream field, and Promise.withResolvers() for the report wait.
  6. The reader's read, cancel and releaseLock taken when the class is created. Replaced by 7.
  7. Native functions for read, cancel, releaseLock and setImmediate.
  8. The report of the destroyed destination stops the pump. The private $values on the prototype, the sink that is aborted with no wait for room, the watch that keeps no store, and the tests of the second self-review.
  9. The merge of main 11c3671.
  10. Entries in the table of built-ins that main now checks: the four bindings run no code that the compiler has not seen, and values is a private name. ReadableStream and ReadableStreamDefaultReader are written without $: the bundler turns both into the private globals.

no test proof · iteration 0 · 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

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: e534085c-e251-41ec-9c31-67c18441f3f7
📥 Commits

Reviewing files that changed from the base of the PR and between 620b50f and 2d49e9c.

📒 Files selected for processing (3)
  • src/js/builtins.d.ts
  • src/js/internal/streams/pipeline.ts
  • test/js/node/stream/node-stream.test.js

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

Pipeline pumps now pass teardown handlers to eligible web-readable sources. Held readers stop and cancel those sources when teardown or destination closure occurs. Tests cover cancellation, destination errors, callback settlement, and socket-backed pipelines.

Changes

Web-stream pipeline teardown

Layer / File(s) Summary
Held-reader source control
src/js/builtins.d.ts, src/js/internal/streams/pipeline.ts
Adds typed globals for readable-stream constructors and a held-reader wrapper that reads eligible web streams, handles cancellation, and releases reader locks.
Pump teardown and pipeline coverage
src/js/internal/streams/pipeline.ts, test/js/node/stream/node-stream.test.js
Node and web pumps stop held sources on teardown or destination closure and report completion outcomes. Pipeline stages pass teardown handlers. Tests cover cancellation, closure, error precedence, async context, and socket-backed cases.

Suggested reviewers: cirospaciari

Priority: Normal

Merge Risk: Minimal · up to 2d49e

Pipelines reading from idle web streams now stop and cancel the source when the pipeline is aborted or the destination closes, so these pipelines no longer hang. Two intentional behavior changes are documented. A { end: false } pipeline now rejects with a premature-close error if the destination ends before the web source. After an abort, the callback can be delayed until an unsettled source cancel finishes. No concrete merge-blocking risk was identified.

Pre-merge checks: Passed 4
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 and concisely describes the main change: waking a pipeline pump that waits on an idle web stream.
Description check Passed The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background subsections explain the…
Full details: Description check

Explanation

The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background subsections explain the hang, the fix, and why the change is scoped as it is. The Downsides section lists behavior changes that reviewers need to see. The Notes section records the merge with main, the repro script, and the mechanics of HeldReader and the teardown. The verification section gives concrete numbers: 104 new tests with 62 failing on main, 271 vendored Node tests, and a cell-level comparison against Node v26.3.0. The description is detailed, but it has no formatting issues that block review.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@robobun

robobun commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Head e4b7ea8, which merges main 11c3671. No review thread is open.

  • Reproduced with the script in the Notes of the description. On Node v26.3.0 and on Bun main, pipeline(idleWebStream, writable, { signal }) is still pending 1 s after the abort, and the source is not cancelled. On this branch it rejects with AbortError and the source is cancelled.
  • Tests on the merged head, debug build: bun bd test test/js/node/stream/node-stream.test.js passes (222 pass, 0 fail). The new block has 104 tests. wpt-streams.test.ts (1,175 tests) and test/js/web/streams/streams.test.js (727 tests) pass.
  • CI on this head (Buildkite #134355, finished): every red test is in test/aot/. The diff does not touch that code, and the same tests are red on other pull requests on this base (Buildkite #134349, #134347). The other red jobs are not test results: agents that could not be created (CapacityLimitExceeded), and two test-bun jobs on one macOS x64 agent where the test runner itself crashed (uv_os_get_passwd returned ENOENT). test/js/node/stream/node-stream.test.js is red on no lane.
  • Compared with main in 3,298 cells and 9,000 random scripts, one process each, before the merge: no pipeline that settles on main hangs on the branch. The description lists the callbacks that change.
  • What this PR leaves out is in node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770.

@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.

Beyond the inline findings, two further concerns in pumpToWeb were examined and ruled out: aborting a WritableStream sink under { end: false } after the signal fires (the PR's own grid lists this as an intended change, matching pipeTo()), and cancelling a held web source before reporting a locked-sink getWriter() error (the teardown would cancel it anyway via finishImpl, so the ordering only moves the cancel earlier).

Extended reasoning...

The change reworks pumpToNode/pumpToWeb in src/js/internal/streams/pipeline.ts to iterate a web source through a cancellable HeldReader, adds teardown callbacks, premature-close reporting, and a load-time-captured setImmediate; it touches no auth, crypto, or input-parsing surface. Seven confirmed findings are posted inline, including hang paths that survive the fix and user-overridable cancel()/writable lookups, so a human review is already signalled; this note only records the two candidates investigated and ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread test/js/node/stream/node-stream.test.js Resolved
Comment thread test/js/node/stream/node-stream.test.js Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
@robobun
robobun force-pushed the robobun/673ad347/pipeline-web-member-teardown branch from a0b0c47 to 2d49e9c Compare October 8, 2026 17:38

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

The four new commits address what I raised on the previous version: cancel now goes through a $ReadableStreamDefaultReader rather than a subclass's cancel(), stopWhenGone is gated on isWritableStream, the onreported wait is bounded to end pipelines, a report on a torn pump stops the source, the compose() test asserts the AbortError, and the per-test timeouts are gone.

Beyond the inline nits, I also checked that throw error || torn at src/js/internal/streams/pipeline.ts:249 cannot throw undefined (every path that stops the source first sets error or torn), and that the locked-sink path in pumpToWeb awaits a cancel() that cannot reject (.catch(nop) plus the resolved fallback), so finish(err) is always reached. Only the inline nits remain from this pass.

Extended reasoning...

The change reworks the web-source pumps in src/js/internal/streams/pipeline.ts (about 190 lines of teardown logic: a lazily built HeldReader, destroys registration, close/eos wiring, and a closed-promise reaction for web sinks) plus two $-global type declarations and roughly 1,000 lines of new tests. It touches no auth, crypto or injection surface; the sensitive aspect is tamper-resistance of a built-in module, which the revised code handles by capturing intrinsics at load and cancelling through a reader. The prior round's seven inline findings are all addressed in the new commits except the deliberate load-time setImmediate capture, which now carries an explanatory comment. The remaining findings are two convention nits, and the async ordering of teardown versus destination reports is intricate enough that a maintainer familiar with node:stream should still look before merging.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
@robobun

robobun commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:56 AM PT - Oct 28th, 2026

❌ @robobun, your commit e4b7ea8 has 71 failures in Build #134355 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 44769

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

bun-44769 --bun

pipeline() with a ReadableStream or TransformStream member did not call
back after an abort, or after the destination failed, while the web
source had nothing to deliver. The pump waits in `for await` over the
stream. return() on the stream's own iterator queues behind the pending
next(), so nothing could wake that read, and the source was not
cancelled.

The pump now reads a member web source through a reader that the
pipeline can cancel. The teardown of the pipeline, an error of the node
destination and the end of a web sink stop the pump. They decide
nothing else. The stopped pump fails on the path it takes when the next
chunk meets a dead destination, so a pipeline that settled before
settles with the same error.

The cancel of a source that the pump is reading runs in the next turn
of the event loop. An owner of the source that reacts to the same
signal closes or feeds its stream first, as before. A pump that waits
for 'drain' is left to the report of the destination, as before.

A web stream that a function stage returned is not held. Node documents
that the stage has to handle `signal`.
pipe() fails the pipeline with ERR_STREAM_PREMATURE_CLOSE when the
destination closes before the source has ended. A pump over a web
source did not. End-of-stream reports no error for a destination that
its owner ended, for an Http2ServerResponse whose client left, or for a
socket that reported its end before it closed. The pump stayed on the
idle source, or on a 'drain' that could not come. The pump now applies
the rule of pipe() on the 'close' event of the destination.

With `{ end: false }`, a pipeline whose destination was ended by its
owner resolved if the web source then ended with no other chunk. It now
rejects, as it does with a node source.
getWriter() threw inside the pump before its first await. The rejection
was unhandled, the callback never ran, and no member was torn down. The
pump now reports the error through the callback, after pipelineImpl has
registered every member, so that finishImpl tears them all down.
A source of a ReadableStream subclass: the pump called the cancel() of
the subclass when it had not locked the stream yet, and then `then` on
what that returned. The cancel now goes through a reader in every path,
as the stream's own iterator cancels. A stream that someone else has
locked is left alone.

`end: false` and a destination that its owner destroyed and that never
reports: the stopped pump waited for that report. It waits only for a
destination that the teardown of the pipeline destroys.

A TransformStream whose `writable` is not a WritableStream: the watch
for the end of the sink threw outside the pump's try. Only a native
WritableStream is watched.

A report of the destination with no error that comes after the teardown
now stops the pump too. A getter of `Symbol.asyncIterator` that throws
for the pump is reported by the loop, as before.

Tests: no per-test timeouts, the compose() test asserts the error, and
one test for each of the above.
The `#stream` field of the reader gets a default in the class body, as
its other fields have. The wait for the report of the destination uses
Promise.withResolvers() in place of a `new Promise` executor.

No change of behaviour.
@robobun
robobun force-pushed the robobun/673ad347/pipeline-web-member-teardown branch from 2d49e9c to a68b520 Compare October 18, 2026 18:47

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

The new push addresses the points I raised on the earlier revisions (cancel now goes through a $ReadableStreamDefaultReader rather than stream.cancel(), onfinished stops the source on any report once torn, the sink is brand-checked before stopWhenGone, Promise.withResolvers and the #stream default are in place, the compose() test asserts the AbortError). Beyond the inline nit, I also checked the { end: false } WritableStream sink paths: stopWhenGone only retains the HeldReader (whose stream and reader are nulled in release()) for the sink's lifetime, and the sink abort on teardown goes through the pre-existing catch block and matches the downside the description documents, so neither is a defect.

Extended reasoning...

The change reworks node:stream pipeline() in src/js/internal/streams/pipeline.ts so a pump over a plain WHATWG ReadableStream source can be cancelled on abort or destination death, adds two builtin declarations, and adds roughly 1060 lines of tests. It touches no auth, crypto or injection surface. The remaining inline finding is a prototype-tamper nit; the two candidate issues on the end:false WritableStream path were ruled out as bounded retention and documented behavior. The change is large and semantically subtle enough that a maintainer look is still worthwhile, so this is informational rather than an approval.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

Comment thread src/js/internal/streams/pipeline.ts Resolved
…he prototype

The pump that holds a web source called read(), cancel() and releaseLock()
through ReadableStreamDefaultReader.prototype. A program that patched those
methods ran its patch inside pipeline(). Before this branch the pump read
through the stream's own iterator, which calls the internal operations.

HeldReader now takes the three methods when the class is created and calls
them with $call.

@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.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
Comment thread src/js/internal/streams/pipeline.ts Resolved
The pump of a held web source took read(), cancel() and releaseLock() from
ReadableStreamDefaultReader.prototype when the class was created, the built-in
iterator of ReadableStream.prototype at the first call, and setImmediate from
the global at load. A spy or a fake timer that was in place at that moment
stayed in the pump after the program restored it.

HeldReader now calls the three host functions of the reader and the native
setImmediate through $newCppFunction. hold() compares the iterator of a stream
with the current one of ReadableStream.prototype and keeps nothing.
After a teardown with `end`, the pump cancelled its source in the next turn
and then waited for the destination to report. A destination that never
reports (a destroy() that does not call back, compose() with a web tail) then
hung a pipeline that settles on main when the source fails or ends later.

The pump now stays on the source until the destination reports, as on main,
and the report stops it. The callback still comes after the close of the
destination, and the destination's own error still wins.

Also:
- hold() compares with the built-in iterator through a private name on
  ReadableStream.prototype. A replacement of the prototype's iterator is
  called, as on main.
- A pump that the teardown stopped does not wait for room in a
  WritableStream before it aborts it.
- A WritableStream that closed before the call is not watched.
- The watch on a WritableStream keeps no AsyncLocalStorage store.
- stop() schedules one immediate.

@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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js/internal/streams/pipeline.ts Resolved
src/js/internal/streams/pipeline.ts: main typed the file. `pumpToNode`
and `pumpToWeb` keep main's typed signatures and take the optional
`destroys` list of this branch. Four lines of the new code get the
types that the type check asks for: `#cancelled` holds a
`Promise<unknown>`, `destroys!.push` where a held source implies the
list, and `finish(err as Error)`.
… of built-ins

The build checks every `$newCppFunction` binding and every `$name` of
src/js against the table of built-ins (src/codegen/builtins/host).

- unseen-code.builtins.ts: the three reader functions and
  `functionSetImmediate` run no code that the compiler has not seen.
- builtin-names.ts: `values` is a private name of JavaScriptCore. The
  pipeline reads it as `.$values`.
- pipeline.ts: `ReadableStream` and `ReadableStreamDefaultReader`
  without `$`. The bundler turns both into the private globals, as in
  the other modules, so the two declarations in builtins.d.ts go.
@robobun

robobun commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

A note for the next merge of main into this branch. Nothing is merged or changed here.

Main added a build step in 8d5672c (#45631) that stops when src/js makes a native function with $newCppFunction and nothing classifies it. This PR makes four that main does not make:

  • streams/JSReadableStreamDefaultReader.cpp: jsReadableStreamDefaultReaderPrototypeFunction_read, _cancel and _releaseLock
  • node/NodeTimers.cpp: functionSetImmediate

I did not run the step on this branch, because the branch conflicts with main now. #46906 hit it with two other functions of NodeTimers.cpp. The step that generates the table of built-ins printed this for each:

error: src/js: native:NewCppFunction:node/NodeTimers.cpp:functionSetTimeout runs native code nobody has classified: declare it, or list its holder or file in host/unseen-code.builtins.ts
  • The entries go in src/codegen/builtins/host/unseen-code.builtins.ts (seenCodeEntries or unseenCodeEntries). The other way is a declaration of the function in host/native-functions.builtins.ts.
  • node:child_process: keep the timeout kill timer ref'd and arm one per call #46906 lists functionClearTimeout and functionSetTimeout in seenCodeEntries, directly after the Worker.cpp entry. An entry for functionSetImmediate belongs at the same place, so the two PRs will conflict in that list. The one that merges second keeps all three lines.
  • functionSetImmediate refuses a callback that is not a function (NodeTimers.cpp:141), as functionSetTimeout does. I did not read the three reader functions for this question. read() and cancel() can run the callbacks of the source of the stream.
  • host/features-web.builtins.ts lists ReadableStreamDefaultReader and the binding of streams/BunStreamConsumers.cpp under the features of web streams. The three reader bindings may need the same. I did not check that. Only the release build of CI links with support for --trim.

@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.

I reviewed the latest push (the merge of main and the built-ins table entries) and found no new bugs; given the size of the change to a hot-path node:stream builtin plus the C++ stream bindings, a human look is still worthwhile before landing.

What was reviewed:

  • The hand-resolved merge in src/js/internal/streams/pipeline.ts: the only non-type changes are dropping $ from ReadableStreamDefaultReader and ReadableStream — src/codegen/replacements.ts lists both in globalsToPrefix, so the bundler still emits the private @ globals and the tamper-resistance is unchanged.
  • The HeldReader teardown paths in pumpToNode/pumpToWeb (stop/cancel/release idempotence, torn propagation into finish, the $newCppFunction bindings and their new unseen-code entries) — every exit I traced settles the pending wait and calls finish.
  • The $values private name in JSReadableStream.cpp and its registration in builtin-names.ts/builtins.d.ts — non-writable, non-deletable, same function object as [Symbol.asyncIterator].
Extended reasoning...

The change reworks how node:stream's pipeline() pumps a web ReadableStream into node or web destinations (about 200 lines in src/js/internal/streams/pipeline.ts), exposes three ReadableStreamDefaultReader host functions and a $values private name from the C++ stream bindings, updates the builtin name and unseen-code tables, and adds roughly 1,300 lines of tests. It touches no auth, crypto, injection or data-exposure surface. Approval is withheld because the logic is intricate teardown/cancellation state across two async pumps in a hot-path builtin, src/js/builtins.d.ts is covered by the *.d.ts CODEOWNERS entry, and the earlier inline threads from prior runs were resolved by the PR author only. Since the last review the only commits are a merge of main (verified behavior-preserving in pipeline.ts) and the table entries, and this run's hunt ran dry with no findings.

This branch has not been deployed

No deployments
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