Skip to content

fix(h2): ensure every request settles - #5603

Merged
mcollina merged 2 commits into
mainfrom
fix/h2-request-liveness
Jul 29, 2026
Merged

fix(h2): ensure every request settles#5603
mcollina merged 2 commits into
mainfrom
fix/h2-request-liveness

Conversation

@mcollina

Copy link
Copy Markdown
Member

Three ways an HTTP/2 request could stop making progress without the caller ever hearing back. All three are reachable with default options now that allowH2 defaults to true.

1. A request could be dropped silently

completeRequestStream() runs on the stream's 'close':

releaseRequestStream(this)
if (state.pendingEnd && !aborted && !completed) request.onResponseEnd(...)
finalizeRequest(state)   // frees the queue slot unconditionally

There was no branch for "the stream closed with no response and no error". In that case the request was spliced out of the queue and nothing ever called request.onResponseError(), so the caller's promise never settled — while the client's own stats looked perfectly healthy.

The window is reachable after a GOAWAY. detachRequestStreamForClose()releaseRequestStream() removes the request-facing listeners and installs once('error', noop), swallowing any later error, but leaves the 'close' listener and stream[kRequestStreamState] in place. The request is requeued and re-dispatched on a fresh session, and then the old stream closes and takes the new in-flight request's queue slot with it.

severRequestStream() now unbinds an abandoned stream for good, and completeRequestStream() errors a request whose stream closed before the response was complete.

2. A refused request could be retried without bound

A peer that refuses with GOAWAY(lastStreamID = 0) is saying "I processed nothing, retry elsewhere" (RFC 9113 §6.8) — what a draining proxy or a load balancer shedding load sends. onHttp2SessionGoAway() requeued the request and reconnected immediately, with no attempt counter and no backoff, so a peer that keeps refusing turned one request into an unbounded connect → HEADERS → GOAWAY → requeue → connect loop. Measured against a peer that always refuses: ~360 reconnects/s indefinitely, the request never settling, and unrelated origins in the same process losing ~75% of their throughput.

Refusals now count against MAX_REFUSED_ATTEMPTS, after which the request fails.

3. SETTINGS_MAX_CONCURRENT_STREAMS: 0 wedged the client permanently

A peer may advertise 0 to temporarily refuse new streams (RFC 9113 §6.5.2) and raise it again later — graceful drain, overload shedding, an LB draining a backend. busy() then degenerates to running >= 0, i.e. permanently busy, and nothing covers a queued request:

  • headersTimeout / bodyTimeout are armed on a stream that is never created
  • the h2 idle timeout bails while kSize !== 0
  • PING keeps succeeding, so the session is never torn down
  • _resume() returns at busy(request), so no new connection is made — which means the later SETTINGS frame that would lift the limit can never arrive either

resumeH2() also unrefs the session in this state, so the event loop can drain while a request is outstanding: the process exits with status 0 and the line after the await never runs.

headersTimeout now covers a request that cannot be sent at all — a request that never gets a stream has missed the same deadline as one whose headers never arrive — and the unusable session is dropped so the next request gets a fresh connection.

Tests

Three new files, 7 tests. Each was verified to fail on a pristine main checkout and pass with this change:

file covers
test/http2-request-never-settles.js 1 — seeded connection churn, asserts every request settles
test/http2-goaway-refusal-storm.js 2 — bounded reconnects, and no throughput collapse for unrelated origins
test/http2-max-concurrent-streams-zero.js 3 — queued request settles, wedged connections do not accumulate

test/http2-goaway-refusal-storm.js carries a small frame-level h2 peer because Node's http2 server clamps lastStreamID to the last stream it actually received, so it cannot send this GOAWAY.

Existing suites: test/+(http2|h2)*.js 97 pass, npm run test:unit 1447 pass, retry + redirect interceptors 87 pass.

Not covered here

v7.x ignores headersTimeout entirely in the h2 path (only bodyTimeout arms the stream timer) — followed up separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM

Three ways an HTTP/2 request could stop making progress without the caller
ever hearing back:

1. A stream that closed with no response and no error had its queue slot
   freed by completeRequestStream() without anyone calling
   request.onResponseError(), so the request simply vanished. This is
   reachable after a GOAWAY: detachRequestStreamForClose() releases the
   request-facing listeners and installs `once('error', noop)`, but leaves
   the 'close' listener and kRequestStreamState in place, so the abandoned
   stream later completes — and splices out — the request that has since
   been requeued onto another session. severRequestStream() now unbinds
   such a stream for good, and completeRequestStream() errors a request
   whose stream closed before the response was complete.

2. A peer that refuses a request with GOAWAY(lastStreamID = 0) had it
   requeued with no attempt budget, so a peer that keeps refusing turned
   one request into an unbounded connect/refuse/reconnect loop that never
   settled. Refusals now count against MAX_REFUSED_ATTEMPTS.

3. A peer may advertise SETTINGS_MAX_CONCURRENT_STREAMS = 0 to refuse new
   streams (RFC 9113 6.5.2). busy() then reports the client as permanently
   busy, and queued requests can neither open a stream — so no per-stream
   timeout covers them — nor trigger a reconnect, so the SETTINGS frame
   that would lift the limit can never arrive. headersTimeout now covers a
   request that cannot be sent at all, and the unusable session is dropped
   so the next request gets a fresh connection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@codecov-commenter

codecov-commenter commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.72131% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.50%. Comparing base (8453a77) to head (09ef22f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/dispatcher/client-h2.js 96.72% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5603      +/-   ##
==========================================
+ Coverage   93.49%   93.50%   +0.01%     
==========================================
  Files         110      110              
  Lines       38281    38424     +143     
==========================================
+ Hits        35789    35929     +140     
- Misses       2492     2495       +3     

☔ 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
mcollina requested review from metcoder95 and ronag July 28, 2026 20:50
RST_STREAM(CANCEL) received before the response is the one reset code Node
reports as a bare 'close' on the client stream -- no 'end' (unlike
NO_ERROR) and no 'error' (unlike PROTOCOL_ERROR / INTERNAL_ERROR /
REFUSED_STREAM) -- and destroying the stream unenrolls its timeout, so no
'timeout' follows either.

That is the deterministic path into the case completeRequestStream() did
not handle, and it is reachable against ordinary infrastructure: a proxy
sends this upstream whenever its own downstream client goes away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
mcollina merged commit bd43ddf into main Jul 29, 2026
38 checks passed
@mcollina
mcollina deleted the fix/h2-request-liveness branch July 29, 2026 07:18
mcollina added a commit that referenced this pull request Jul 29, 2026
Two follow-ups to the rebase onto main.

RFC 9113 section 8.7 says a client SHOULD NOT automatically retry a request
more than once. The REFUSED_STREAM path already followed that; the GOAWAY
replay budget introduced in #5603 allowed three. Lower it to one so both
refusal signals behave the same, and rename the budget after the signal it
belongs to now that REFUSED_STREAM retries live in this file too:

  MAX_REFUSED_ATTEMPTS -> MAX_GOAWAY_REPLAY_ATTEMPTS
  kRefusedAttempts     -> kGoAwayReplayAttempts

A peer that refuses every connection now opens two connections rather than
four before the request fails, so tighten the regression bound accordingly.

retryRefusedStream() also open-coded the stream detach that
detachRequestStreamForClose() already performs. Reuse the helper: it drops
the 'close' listener rather than relying only on the nulled state, and it
guards the stream-count decrement, so the abandoned attempt cannot touch
the retried request.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A49JamgF2TkZHu5h58ChUM
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants