fix: retry refused HTTP/2 streams

[mcollina]

This relates to...

HTTP/2 peers can reject an unprocessed stream with RST_STREAM(REFUSED_STREAM). Node.js surfaces this as the generic ERR_HTTP2_STREAM_ERROR, and Undici previously failed the request without considering the retry-safe HTTP/2 reset code.

Rationale

RFC 9113 section 8.7 permits clients to retry a request rejected with REFUSED_STREAM, including non-idempotent requests, because the peer asserts that no application processing occurred. It also says clients should not automatically retry the same request more than once.

Changes

Features

N/A

Bug Fixes

• Detect NGHTTP2_REFUSED_STREAM using stream.rstCode before response headers are exposed.
• Retry the request once on the existing HTTP/2 session.
• Restrict transparent retries to replayable bodies: no body, Buffer, or Blob.
• Preserve the numeric reset code as error.http2ErrorCode when an error is ultimately exposed.
• Add regression coverage for concurrent streams, buffered POST bodies, the one-retry limit, streaming bodies, and other reset codes.

Breaking Changes and Deprecations

None.

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

Validation:

• npm run test:h2:core — 96 passed
• npm run test:h2:fetch — 11 passed
• npx eslint lib/dispatcher/client-h2.js test/http2-refused-stream.js
• Commit hook npm run lint

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.50%. Comparing base (c76fe46) to head (5f9a959).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5598   +/-   ##
=======================================
  Coverage   93.49%   93.50%           
=======================================
  Files         110      110           
  Lines       38429    38476   +47     
=======================================
+ Hits        35931    35976   +45     
- Misses       2498     2500    +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

[mcollina]

Rebased onto main (was conflicting since #5603 landed).

The conflict was not purely textual, so flagging the resolution for review:

#5603 turned canRetryRequestAfterGoAway() into a stateful function — it increments a per-request GOAWAY retry budget (kRefusedAttempts, capped at MAX_REFUSED_ATTEMPTS) as a side effect of being called. This PR renames that same function to canReplayRequest() and calls it from retryRefusedStream() as a pure "is this body replayable?" predicate.

Merged naively, asking whether a REFUSED_STREAM retry is possible would silently consume one of the request's GOAWAY reconnect attempts. So I split them:

function canReplayRequest (request) {        // pure predicate
  const { body } = request
  return body == null || util.isBuffer(body) || util.isBlobLike(body)
}

function registerGoAwayRefusal (request) {   // the budget, GOAWAY path only
  const attempts = (request[kRefusedAttempts] ?? 0) + 1
  request[kRefusedAttempts] = attempts
  return attempts <= MAX_REFUSED_ATTEMPTS
}

with the GOAWAY site now reading canReplayRequest(request) && registerGoAwayRefusal(request). Both limits are preserved and independent:

peer behaviour	connections opened	outcome
GOAWAY(lastStreamID=0) on every connection	4 (1 + 3 retries)	fails with the GOAWAY error
RST_STREAM(REFUSED_STREAM) on every request	1	retried once on the same session, then fails

One thing worth deciding separately: RFC 9113 §8.7 says clients SHOULD NOT automatically retry more than once, which this PR follows for REFUSED_STREAM (kRefusedStreamRetry), while the GOAWAY path from #5603 allows 3. Different signals, so not necessarily wrong, but the asymmetry is now visible in one file.

Verification on the rebased branch: test/http2-refused-stream.js 6/6, the four h2 suites from #5603 all pass, test/+(http2|h2)*.js 105 pass, full unit suite 1456 pass, 0 fail.

[mcollina]

Pushed the refactor and the spec alignment — but please read the second half of this comment first, it blocks the PR.

Changes in 5f9a9598

RFC 9113 §8.7 alignment. The REFUSED_STREAM path already retried at most once; the GOAWAY replay budget from #5603 allowed three. Lowered to one so both refusal signals agree, and renamed the budget now that two refusal mechanisms share this file: MAX_REFUSED_ATTEMPTS → MAX_GOAWAY_REPLAY_ATTEMPTS, kRefusedAttempts → kGoAwayReplayAttempts. A peer refusing every connection now opens 2 connections instead of 4, and test/http2-goaway-refusal-storm.js asserts <= 4 rather than the now-meaningless <= 20.

Detach refactor. retryRefusedStream() open-coded what detachRequestStreamForClose() already does. Reusing the helper also drops the 'close' listener instead of relying only on the nulled state, and guards the stream-count decrement.

test/http2-refused-stream.js 6/6, storm test 3/3, test/+(http2|h2)*.js 105 pass.

⚠️ This branch segfaults

While measuring the above I found that this PR crashes the process under HTTP/2 connection churn — SIGSEGV, exit 139, a native crash rather than a JS error.

Reproduce with the churn harness already in the tree (test/http2-request-never-settles.js, from #5603), run directly so the exit code is visible:

for i in $(seq 1 8); do
  node --expose-gc test/http2-request-never-settles.js >/dev/null 2>&1
  echo "exit=$?"
done

build	non-zero exits
main (c76fe46)	0/8
this branch at 34476c29 (rebase only, before today's commit)	3/8
this branch at 5f9a9598 (with the changes above)	1/12

So it is not the rebase resolution and not the two changes in this commit — the crash tracks the PR's own retry path. The refactor seems to reduce the rate rather than fix it, which is consistent with it removing one stale reference to the abandoned stream, but that is a guess.

The most likely area is retryRefusedStream() splicing the request back into the pending queue while the reset stream is still being torn down by nghttp2 — the request gets re-dispatched onto the same session while native state for the old stream is still in flight. Under borp this surfaces as a bare 'test failed' with one subtest missing and no assertion, which is easy to mistake for flakiness; running the file directly is what exposes the signal handler.

I did not chase it further — a native use-after-free needs a debug build and a gdb backtrace to pin down, and it may well be a Node bug that this access pattern triggers rather than something to fix here. Happy to dig in if useful.

[mcollina]

Correction to my earlier comment above — the part blaming this PR for the crash was wrong, twice over.

What I said: that the process crash "tracks the PR's own retry path", based on 3/8 crashes on this branch versus 0/8 on main.

What is actually going on: there were two unrelated crashes behind one signature, and neither is caused by this PR. Under borp both appear as a bare 'test failed' with a subtest silently missing and no assertion text, which is what let me conflate them.

1. Assertion failed: onread->IsFunction() (SIGABRT). Node main only. Bisected to 46de80de88c (http2: avoid uaf while receiving and sending rst_stream, #64166) — the deferred session close it introduces lets nghttp2 deliver DATA to streams whose JS wrappers are gone. Reproduces from a dependency-free node:http2 script with no undici involved, and reproduces on undici main as well as this branch. Filed as nodejs/node#64850.

2. V8 maglev SIGSEGV. This is what the CI failures on this PR actually are, on released v24.18.0:

#1 v8::internal::Heap::UnregisterStrongRoots(...)
#3 v8::internal::maglev::MaglevConcurrentDispatcher::FinalizeFinishedJobs()
   (V8Worker) #3 v8::internal::maglev::MaglevCompiler::Compile(...)

build	crash rate
this branch, v24.18.0	~24% (12/50)
main, v24.18.0, without this PR	~4% (2/50)
this branch, --no-maglev	0/45

It reproduces on main too, just less often. The PR's added code plausibly raises maglev compilation pressure; there is no semantic defect here. Only Node 24 / ubuntu was affected in CI — 22, 25, 26, macOS and Windows were green, matching local rates.

Net: no undici bug in this PR from either crash. The rebase and the RFC 9113 §8.7 changes still stand as described. CI on this branch will keep flaking on Node 24 until the V8 issue is fixed or test/http2-request-never-settles.js is made less maglev-intensive — worth deciding separately from this PR.

Apologies for the noise from the earlier comment.