Review requested:

• @nodejs/gyp

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (9f0ce45) to head (83a8457).
⚠️ Report is 12 commits behind head on main.

@@           Coverage Diff            @@
##             main   #65273    +/-   ##
========================================
  Coverage   90.12%   90.13%            
========================================
  Files         752      752            
  Lines      252297   252414   +117     
  Branches    47432    47452    +20     
========================================
+ Hits       227393   227523   +130     
+ Misses      16217    16199    -18     
- Partials     8687     8692     +5     

... and 31 files with indirect coverage changes

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Defensively marking this semver-major. If you can show that the optimization does not change observable behavior, that can be dropped, but the change in microtask timing from one pull to the next is likely observable.

@jasnell I believe semver-major is no longer needed.

Benchmark GHA (webstreams): https://github.com/nodejs/node/actions/runs/31808780686

@jasnell can you rereview please

Since I'm buried in a few other things and it looks like this is mostly being updated by AI anyway, I had my agent draft up a review. I skimmed it over and can't disagree with any part of it:

0. Reconstructed history (since the squash hid it)

I fetched all four force-pushed heads. The PR was never a single commit until the last push:

The rebase matters more than the squash: a large fraction of this PR already landed on main as #65138
(stream: cut promise churn in webstreams hot paths, mcollina, 2026-08-15) plus
stream: consolidate non-op algorithm callbacks. thenAlgorithmResult, kResolvedPromise,
nonOpCallback, parkOnReady, the queueMicrotask → PromisePrototypeThen(kResolvedPromise, …)
swaps in pipeTo/tee, and the raw-callback contract are all in main already. What's left in #65273
is a different, smaller, and riskier set of changes than the PR body describes.

Three things happened in the squash that reviewers can't see:

1. readableStreamDefaultControllerFillSync was deleted. This is the change jasnell (r3793793568)
   and Mattias (r3798610537) called semver-major. Good — that objection is resolved, but nobody said so.
2. The .github/workflows/benchmark.yml fix was lost. 1cf7319 changed make -j4 V=1 →
   make build-ci -j4 V=1 specifically so benchmark CI would relink after node.gyp gained
   src/node_webstreams.cc. Current head is back to make -j4 V=1. The one benchmark GHA run on the
   PR (31808780686) failed. So there is currently no CI-verifiable benchmark for this PR at all.
3. Same for the Makefile ninja-regeneration hunk from 27c4cf3 (dropped one push earlier).

Please push the review responses as fixup commits and let the commit-queue squash them. The
commit-queue-squash label is already on the PR; squashing by hand only destroys reviewability.

1. The PR description no longer describes the PR

• "pipeTo fills a default readable queue from sync pulls" — that code is gone.
• "Cheaper empty construction … skip validateObject on the shared empty sentinels" — massively
  understates it. new ReadableStream() now defers controller creation entirely (~40 new lines +
  4 new call-site guards).
• Not mentioned at all: promiseFromAlgorithmResult / delayedAlgorithmResult, resolvedRecord()
  now returning a process-shared promise, the new writable drain loop,
  nonOpCancel/nonOpFlush changing from async to sync.
• "Public constructors, methods, and WHATWG Streams behavior … are unchanged." — not accurate; see §2.
• The benchmark table was measured against a base predating stream: cut promise churn in webstreams hot paths #65138, whose own commit message claims
  "pipe-to improves by 8–14%". The 1.94x geomean is not this PR's remaining delta. It needs a full
  re-run against current main, with all 32–35 configs (the table shows 9 hand-picked rows), and with
  compare.js confidence output rather than bare ratios. --runs 10 is thin for these benchmarks.

The commit message says "Behavior-preserving" while the PR carries semver-major. Per
doc/contributing/pull-requests.md §5, a semver-major commit must explain the breaking change in the
message. Also §6: the only Signed-off-by is Cursor Agent <cursoragent@cursor.com>, which doesn't
match the commit author — at least one sign-off must match author metadata.

2. Breaking changes

2.1 resolvedRecord() leaks one shared promise to userland

// lib/internal/webstreams/util.js:433
function resolvedRecord() {
  return { promise: kResolvedPromise, /* was: PromiseResolve() */ ... };
}

There are five resolvedRecord() call sites. Three of them are reached from public getters:

¹ kIsClosedPromise is SymbolFor('nodejs.webstream.isClosedPromise') — a registered symbol, so
stream[Symbol.for('nodejs.webstream.isClosedPromise')].promise reaches it from userland too. Not
public API, but not sealed either.

There are two directly reachable spec violations:

// 1. cross-stream identity leak — no closing or backpressure needed
const a = new WritableStream().getWriter();
const b = new WritableStream().getWriter();
a.ready === b.ready               // true after this PR; false on main

// 2. two distinct internal slots collapse to one object
const w = /* writer on a closed WritableStream */;
w.closed === w.ready              // true after this PR; false on main

Per spec [[closedPromise]] and [[readyPromise]] are separate slots, each initialised to "a new
promise", so (2) is unambiguously wrong. A third case exists but is narrower than I first wrote: a
writer.ready read before a backpressure cycle is === to one read after it only if the user
never observes ready during the backpressure window (otherwise the pending
PromiseWithResolvers() record is cached and reused). Still a deviation from
WritableStreamDefaultWriterEnsureReadyPromiseInitialized, just not unconditional.

WPT's aborting.any.js:43 / :1128 compare resolved-vs-pending and resolved-vs-rejected promises,
so neither catches this.

One more thing worth confirming (I have not executed it): kResolvedPromise is also the object the
implementation schedules on — PromisePrototypeThen(kResolvedPromise, pump) and friends. Handing
userland a reference to it means an own constructor property with a poisoned Symbol.species can be
installed on that specific object, and Promise.prototype.then does SpeciesConstructor(this, …)
before creating its result promise. This is not a new class of exposure — Node is already
susceptible to Promise.prototype.constructor poisoning for every internal promise — but it turns a
global-mutation attack into a targeted one that needs no global writes.

Suggested fix. The constraint is narrow: writerClosedPromise() and writerReadyPromise() back
two distinct spec slots, so they must hand out distinct promises. That doesn't require reverting the
whole helper. Two reasonable options:

• revert resolvedRecord() to promise: PromiseResolve() — one line, restores main's behaviour; or
• keep the shared instance but scope it to the two [kIsClosedPromise] sites, which aren't
  spec-observable slots, and mint a fresh promise in the writer getters.

I'd expect either to be unmeasurable. Every consumer is cold: the two public getters, [kInspect]
(writablestream.js:504-505), and one watchErrored() per pipeTo (readablestream.js:1819).
Notably writerReadyPromise() is not consulted per chunk any more — pipeTo parks via
parkOnReady() — so there's no per-chunk allocation being saved here. If there is a benchmark
showing otherwise I'd want to see it, but absent one this looks like it can go without argument.

Directly exposing the shared promise is not only breaking, it is potentially exploitable.

2.2 BLOCKER — two mutually inconsistent timing-compensation helpers

function promiseFromAlgorithmResult(result) {           // 0 extra ticks
  if (isNonThenable(result)) return kResolvedPromise;
  return PromiseResolve(result);
}
function delayedAlgorithmResult(result) {               // +1 tick vs. old
  if (isNonThenable(result)) return kResolvedPromise;
  return PromisePrototypeThen(kResolvedPromise, () => result);
}

createPromiseCallback{NoParams,1Param,2Params} stopped being async. For a non-thenable user
return both helpers reproduce the old 1-tick settlement. For a thenable return they do not:

• old async () => R: R is adopted via NewPromiseResolveThenableJob → consumer reaction at tick 3.
• promiseFromAlgorithmResult(R) = PromiseResolve(R) = identity → consumer at tick 1
  (2 ticks earlier).
• delayedAlgorithmResult(R) = kResolvedPromise.then(() => R) → still adopts R, plus one →
  consumer at tick 4 (1 tick later).

Reachable, user-observable, on public API:

The comment on delayedAlgorithmResult gives the game away:

// Cancel/flush/abort only: insert an extra microtask so "upon fulfillment" of an already-settled
// user promise runs after start-settlement reactions queued during construction. Pull/write must not use this.

That's a compensation reverse-engineered from a failing test, not derived from the spec. Two helpers
that differ only by a microtask, applied per-call-site by hand, is exactly the kind of thing that
rots. Either (a) keep the async wrapper for the cold algorithms (cancel/close/abort/flush/transform
— these run once per stream, not per chunk, so there is no measurable win to give up) and only use
the raw-callback contract for pull/write, or (b) show the tick accounting for each of the six
sites above in a test. This is Mattias's r3798423135 point in a nutshell.

2.3 The write-side batching is the same deviation fillSync was removed for

writableStreamDefaultControllerDrainWriteQueue() (writablestream.js:1201) processes consecutive
sink writes in one microtask whenever the in-flight request is pipeTo's shared tracker:

if (stream[kState].inFlightWriteRequest.promise === null) {
  writableStreamDefaultControllerCompleteWrite(controller);
  continue;                       // <-- no microtask between writes
}

Spec WritableStreamDefaultControllerProcessWrite ends with "Upon fulfillment of sinkWritePromise …
Perform WritableStreamDefaultControllerAdvanceQueueIfNeeded" — one microtask per write,
unconditionally. jasnell marked the readable-side equivalent semver-major and it was removed; the
writable-side equivalent survived and was never discussed. It is observable via the interleaving of
any concurrently-scheduled microtask, the settle position of pipeTo()'s promise, and fairness
between two concurrent pipes.

It's also a layering violation: writablestream.js now branches on promise === null, a sentinel
defined by the writeTracker object literal inside readableStreamPipeTo in
readablestream.js:1545. Nothing links the two but a comment.

2.4 cloneAsUint8Array changes the detached-buffer error

Old path threw V8's TypeError: Cannot perform ArrayBuffer.prototype.slice on a detached ArrayBuffer.
New path throws ERR_INVALID_STATE, which is mapped to Error, not TypeError
(src/node_errors.h:106). This is reachable: readableByteStreamTee's forwardChunk
(readablestream.js:2012) catches it and errors both branches with it, so it lands in user code as
the stream's stored error. If a byte tee is racing a buffer transfer, the observable error class
changes. Not covered by WPT or by the new test.

2.5 Lazy AbortController — looks correct, but check [kControllerErrorFunction]

(this[kState].abortController ??= new AbortController()) in both the getter (writablestream.js:547)
and writableStreamAbort (:724) is fine, and the abort-before-signal test covers the ordering. Note
WritableStream[kControllerErrorFunction] (writablestream.js:224) still does
this[kState].controller.error(error) unguarded — that's fine today because the writable always
materialises a controller, but it's now the odd one out versus the readable's new undefined guard.

3. Complexity vs. gain

3.1 The benchmarks measure the special cases this PR adds

benchmark/webstreams/creation.js times new ReadableStream() / new WritableStream() with no
arguments — a stream with no underlying source, which cannot produce data and has no real-world use.
The reported "creation: ReadableStream 1.49x, WritableStream 2.05x" is measuring the microbenchmark,
not a workload. The cost of that number is ensureEmptyDefaultController() plus guards in cancel(),
setupReadableStreamDefaultReader() and [kControllerErrorFunction], and a permanent obligation on
every future stream[kState].controller access to consider undefined.

Worse, it likely pessimizes the real path: createReadableStreamState() (readablestream.js:1439)
does not declare a controller field, so controller is added transitionally. Before this PR every
ReadableStream state object reached the same map before escaping the constructor. Now there are two
maps, and the hot read() fast path (readablestream.js:957 and :608) reads
stream[kState].controller — so those ICs go polymorphic as soon as a process mixes empty and
non-empty streams. If the empty-construction optimization is kept, createReadableStreamState()
should at minimum initialise controller: undefined.

benchmark/webstreams/pipe-to.js uses write(chunk, controller) {} — a fully synchronous sink. That
is precisely the shape §2.3's drain loop targets. Real sinks (fs, net, zlib, fetch) return promises and
take the thenable branch, getting zero benefit from the drain loop while paying its complexity
forever.

3.2 writableStreamDefaultControllerDrainWriteQueue duplicates two spec algorithms

It is a copy of writableStreamDefaultControllerAdvanceQueueIfNeeded (:1372) plus an inlined
writableStreamDefaultControllerProcessWrite (:1249), in a for(;;) with four returns and one
continue. Future spec fixes must now be applied in two places. Concrete problems in it:

• It reads controllerState.writeFulfilled / writeRejected but never initialises them. It happens to
  be safe because the only caller is writeFulfilled itself — an undocumented invariant with no
  assert. If anyone ever calls it from the start-completion path (which is what
  advanceQueueIfNeeded does), PromisePrototypeThen(kResolvedPromise, undefined, undefined) silently
  swallows the write completion and the stream hangs. Please add
  assert(controllerState.writeFulfilled !== undefined).
• Line 1235 passes writeRejected as the rejection handler of kResolvedPromise, which can never
  reject. Dead argument, and inconsistent with thenAlgorithmResult, which deliberately omits it.
• Line 1241 calls thenAlgorithmResult(result, …) after isNonThenable(result) has already returned
  false — a redundant second check on the hot path.
• The re-entrancy is subtle and undocumented: completeWrite() → writableStreamUpdateBackpressure()
  → writer[kState].ready.resolve() — and for pipeTo that resolve is pump, called
  synchronously from inside the loop. pump then re-enters writableStreamDefaultControllerWrite →
  advanceQueueIfNeeded → processWrite, and the loop's next iteration bails because a request is
  back in flight. I believe this is bounded and correct, but it needs a comment explaining why, and it
  means the continue fires far less often than the design implies — which makes me want the
  per-change benchmark attribution below even more.

3.3 The native binding is not justified

src/node_webstreams.cc exists for two functions.

isNonThenable replaces this inline JS:

result === null || (typeof result !== 'object' && typeof result !== 'function')

TurboFan compiles that to a couple of map/instance-type checks. A Fast API call cannot beat it, and in
unoptimized code (Ignition/Sparkplug — i.e. startup, and any stream that never gets hot) it degrades to
a full C++ call, which is strictly slower than the JS it replaced. jasnell asked this directly
(r3780428766: "I don't understand why this needs to be a C++ function"); the answer given —

Kept the Fast API so the JIT can call the predicate without a slow C++/JS call on every chunk

— assumes the alternative is a C++ call. It isn't; the alternative is two inlined typeofs. Please
post an isolated microbenchmark of the predicate alone (JS inline vs. Fast API) before keeping this.
My expectation is it's a regression.

Also, the implementation is dead-code-y and not quite equivalent to the JS:

return value->IsNullOrUndefined() || (!value->IsObject() && !value->IsFunction());

v8::Value::IsObject() is IsJSReceiver(), which is already true for functions — !value->IsObject()
suffices. And for an undetectable object (typeof x === 'undefined' but IsObject() true) the JS
predicate says "non-thenable" while the C++ says "maybe-thenable". Not reachable from Node userland
today, but it shows the two are not the same function.

cloneAsUint8Array is a more plausible win (one binding call instead of
ArrayBuffer.prototype.slice + new Uint8Array), but slice is already a fast V8 builtin doing a
memcpy, so the saving is one JS-level call and one intermediate object — on the byte-tee path only.
Given §2.4's error-behaviour change, I'd want a number for this specifically too.

Either way, adding a whole new internalBinding + node.gyp entry + external-reference registration +
typings for two predicates is a permanent cost. If isNonThenable really must be native, it belongs in
src/node_types.cc alongside the other fast type predicates rather than in a new webstreams binding.

3.4 Ask: per-change benchmark attribution

This PR bundles at least six independent optimizations behind one aggregate ratio. Given that #65138
already captured the shared parts, please split and measure each separately against current main:

1. raw callbacks + promiseFromAlgorithmResult/delayedAlgorithmResult for cancel/close/abort/flush
2. writableStreamDefaultControllerDrainWriteQueue
3. deferred ReadableStream controller
4. lazy writable AbortController
5. validateObject skips (readable/writable/transform)
6. native isNonThenable
7. native cloneAsUint8Array

My prior is that (2) is the only one with a defensible number, (1) is the only one with a real
breaking-change cost, and (3)–(6) are benchmark-shaped. Items with no measurable win on current main
should be dropped — that alone would remove most of the complexity and most of the risk.

4. Test gaps

test/parallel/test-whatwg-webstreams-hotpath.js is decent on isNonThenable/Proxy and the subclass
cases, but nothing tests the risky behaviour:

• No test that writer.ready !== writer.closed, or that two streams' ready promises are distinct
  (would fail today — see §2.1).
• No tick-ordering test for any of the six call sites in §2.2. These are exactly the cases WPT
  under-covers.
• No test for the pipeTo sync-write drain interleaving with an unrelated queueMicrotask, nor for the
  re-entrant pump path in §3.2.
• cloneAsUint8Array: no detached-buffer, DataView, zero-length, resizable/length-tracking, or
  Buffer (offset ≠ 0) case. The offset case matters — the old code sliced
  [byteOffset, byteOffset+byteLength); the new code relies on CopyContents. A
  Buffer.from(pool).subarray(k) test would pin that.
• (async () => {…})().then(common.mustCall()) (×4) has no rejection handler; use
  .then(common.mustCall(), common.mustNotCall()) so a failure reports the actual error rather than a
  mustCall miss.
• The "each pull is separated by a microtask" test encodes an exact tick schedule via nested
  queueMicrotask. That's the right intent, but it will break on unrelated changes; a comment saying
  so would help the next person.
• No doc changes. jasnell asked in r3780357430 for the non-standard behaviour to be documented;
  doc/api/webstreams.md is untouched.

5. Nits

• node.gyp:177 — src/node_webstreams.cc inserted between node_wasm_web_api.cc and
  node_watchdog.cc, breaking the sort. Move after node_watchdog.cc.
• Import/export ordering: promiseFromAlgorithmResult between kResolvedPromise and kState
  (readablestream.js:112), isNonThenable between cloneAsUint8Array and copyArrayBuffer
  (util.js exports), delayedAlgorithmResult after nonOpFlush (transformstream.js:57), and the
  promiseFromAlgorithmResult/delayedAlgorithmResult pair dropped into the middle of
  module.exports (util.js:502-506) along with a removed blank line.
• transformstream.js:127-135 — inconsistent braces (if (transformer !== kEmptyObject) unbraced, the
  next two braced).
• transformstream.js:601-611 / 669-679 — pure brace reformatting unrelated to the change; drop it
  to keep the diff reviewable.
• THROW_ERR_INVALID_ARG_TYPE(env, "The \"view\" argument must be an ArrayBufferView") doesn't follow
  the usual ERR_INVALID_ARG_TYPE phrasing (must be an instance of X. Received …).
• src/node_webstreams.cc uses std::unique_ptr without including <memory>.
• ReadableStream.prototype.cancel() calls ensureEmptyDefaultController(this) unconditionally,
  including when the stream is already closed/errored — where readableStreamCancel returns before
  touching the controller. That allocates a controller purely to throw it away, which contradicts the
  stated goal.
• jasnell's r3799526914 (braces in [kControllerErrorFunction]) is still open, though the substance is
  already there.

6. Recommendation

Request changes. Concretely, before this can be re-reviewed:

1. Push review responses as fixup commits, not a squash.
2. Re-run the full benchmark suite against current main (post-stream: cut promise churn in webstreams hot paths #65138) and publish all configs with
   confidence intervals; restore the benchmark.yml build-ci fix so CI can verify it.
3. Rewrite the PR body and commit message to describe what the code actually does, and either justify
   or drop semver-major with the §2 list in hand.
4. Make writer.ready and writer.closed distinct promises again (§2.1) — reverting
   resolvedRecord() to a fresh PromiseResolve() is the one-line version; scoping the shared
   instance to the internal [kIsClosedPromise] sites also works.

@jasnell do you think that this is still semver major or can we remove the label?

Still semver-major

@nodejs/tsc since this is semver major, it requires your review.

@anonrig ... see the details in #65273 (comment) for a longer review. i'll try to do a line-by-line review later this week.

Note: Bots are not permitted to attest with Signed-off-by