stream: fix addAbortSignal() for web streams

[ikeyan]

This fixes stream.addAbortSignal() for web streams.

addAbortSignal() used kControllerErrorFunction, which mixed stream erroring
with abort handling.

For readable byte streams, ReadableByteStreamController left the default no-op
hook in place, so addAbortSignal() could not abort the stream.

For writable streams, WritableStreamDefaultController wired the hook to
controller.error(), so the stream became errored but
controller.signal.aborted stayed false.

This replaces kControllerErrorFunction with kControllerAbortFunction and
wires each controller to the internal operation that matches its abort path:

• ReadableStreamDefaultController uses
  readableStreamDefaultControllerError()
• ReadableByteStreamController uses
  readableByteStreamControllerError()
• WritableStreamDefaultController uses writableStreamAbort()

The updated test covers addAbortSignal() on readable byte streams, aborts
that race with pending pulls, writable abort signal state, and readable and
writable controllers with overridden public methods.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[MattiasBuelens]

I believe this is a breaking change for WritableStreams.

I think this PR is a good change: using controller.error from a consumer API is fundamentally wrong, and we got this wrong from the start in #46273. But that does mean this PR is a semver-major change.

While we're at it, I think we should fix the behavior for ReadableStreams too to call readableStreamCancel instead.

@nodejs/streams Thoughts?

[MattiasBuelens]

Looks pretty good. Maybe this might not be as breaking of a change after all?

We'll need to update the docs though:

• Explain that for web streams, this will now call cancel (for ReadableStream) and abort (for WritableStream).
• Add this PR to the changes frontmatter for stream.addAbortSignal.

[ikeyan]

@MattiasBuelens
I analyzed the differences precisely.

Compared to controller.error(), readableStreamCancel(..., 'errored')
changes ReadableStream behavior in two ways:

• the stream becomes disturbed (Readable.isDisturbed() would start returning
  true)
• the stream's cancel steps run (controller[kState].cancelAlgorithm, i.e.
  source.cancel())

The second part is the intentional change, since it lets the source clean up.
It also fixes another issue for tee'd streams: if one branch is aborted and the
other branch is later canceled, the original source is not canceled today,
because controller.error() never runs the branch cancelAlgorithm. With
readableStreamCancel(..., 'errored'), the aborted branch still participates in
the tee cancellation bookkeeping, so the upstream cancel() can still happen.

I also checked the readRequest[kError] / readIntoRequest[kError] paths:
readableStreamError() would run earlier than it does in
readableStreamDefaultControllerError() /
readableByteStreamControllerError(), but I do not currently see an externally
observable difference from that reordering.

For WritableStream, the intentional changes are:

• controller.signal gets aborted.
• the stream's abort steps run (controller[kState].abortAlgorithm, i.e.
  sink.abort())

That also comes with a few side effects compared to controller.error():

• writableStreamDefaultControllerClearAlgorithms() is deferred until the
  stream finishes erroring, instead of running immediately in
  controller.error(), so the underlying algorithms can stay alive longer
• because there is now a pending abort request, closeRequest,
  stream[kIsClosedPromise], and writer[kState].close are not rejected until
  sink.abort() settles
• a later stream.abort() / writer.abort() reuses the existing pending abort
  request instead of replacing its reason

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (6d8baea) to head (113374f).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webstreams/readablestream.js	95.23%	1 Missing ⚠️
lib/internal/webstreams/writablestream.js	87.50%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #62450 +/- ##
==========================================
+ Coverage 90.15% 90.16% +0.01% 
==========================================
Files 746 746 Lines 242648 242660 +12 Branches 45720 45724 +4 ==========================================
+ Hits 218758 218802 +44 + Misses 15397 15364 -33 - Partials 8493 8494 +1 

Files with missing lines	Coverage Δ
lib/internal/streams/add-abort-signal.js	100.00% <100.00%> (ø)
lib/internal/streams/utils.js	97.79% <100.00%> (ø)
lib/internal/webstreams/readablestream.js	98.16% <95.23%> (-0.03%)	⬇️
lib/internal/webstreams/writablestream.js	99.42% <87.50%> (-0.08%)	⬇️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ 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.

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

[ikeyan]

The tree is byte-identical to 79df0f747d merged with main, but the history changed:

• 6 commits → 3.
  • main has since landed an equivalent move of the abort/closed hooks onto the prototypes, so e65f320db7 and ec8157377a became redundant.
  • The remaining commits are regrouped so each one passes tests on its own.
• Added Signed-off-by, which was missing and failing lint-commit-message.

Could you review once again? @MattiasBuelens

[MattiasBuelens]

LGTM. However, I'd like someone else from @nodejs/streams to also have a look and help decide whether this new behavior is okay.