fix(request): don't fail requests on HTTP/2 GOAWAY or refused streams

[petersutter]

How to categorize this PR?

/area robustness
/kind bug

What this PR does / why we need it:

Occasionally, one request in a burst of concurrent requests to the Kubernetes API server fails with ERR_STREAM_PREMATURE_CLOSE or a truncated body, and the user gets a 500. The client can't tell why, because it ignores the stream's rstCode and GOAWAY frames.

Improvements:

• Record why a stream ended. Transport errors are wrapped in a StreamError that keeps the original code, message and cause, and carries err.termination, which says whether the server processed the request. Each failure logs one Request <method> <path> [<x-request-id>] failed: ...; termination=... line. Messages shown to users are unchanged.
• Keep draining sessions alive. After a graceful GOAWAY, the pool destroyed the closing session on the next request, cutting off responses the server was still sending. Draining sessions now finish their streams, and new requests go to a new session.
• Retry streams the server never processed. request() retries a request of any HTTP method when the server refused its stream or cut it with a GOAWAY, up to maxRetries times (default 2). Go's HTTP/2 client does the same, and kube-apiserver's --goaway-chance relies on it. Connection failures are not retried. stream() and fetch() never retry, so watches keep the reflector's own retry loop.

Not fixed here: a Node.js bug. With this PR deployed on a development landscape, the remaining ERR_STREAM_PREMATURE_CLOSE failures turned out not to come from the API server. They were logged as processed (neverProcessed: false, responseReceived: true, no GOAWAY), so they are correctly not retried. They are caused by a regression in Node.js v24.20.0 and later: when an HTTP/2 stream closes normally before the client starts reading its fully buffered body, async iteration reports a premature close (nodejs/node#65677). The fix, nodejs/node#65689, is not released yet, and the dashboard has run Node 24.21.0 since #3210. This PR does not work around the regression. The improvements above address failure modes that are independent of it.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Easiest to review commit by commit.

Related PR: #3301

Release note:

The dashboard backend now retries Kubernetes API requests that the API server refused or cut off with an HTTP/2 GOAWAY, and no longer truncates responses on connections that are shutting down gracefully. Both could cause sporadic `500` errors.

Summary by CodeRabbit

• New Features
  • Requests can automatically retry eligible failures when the server has not processed them, up to a configurable limit.
  • Added a request timeout option that applies across all retry attempts.
• Bug Fixes
  • Requests in progress can complete while an HTTP/2 connection gracefully shuts down, while new requests use an available connection.
  • Caller cancellations and timeouts remain effective during retries, and failures that may have been processed are not retried.

[gardener-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign petersutter for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 3581c87b-5b39-43dc-a92d-dfcd35c3a73b

📥 Commits

Reviewing files that changed from the base of the PR and between ca7405b and c9bded6.

📒 Files selected for processing (7)

• packages/request/__tests__/acceptance.spec.js
• packages/request/__tests__/client.spec.js
• packages/request/__tests__/errors.spec.js
• packages/request/__tests__/session-pool.spec.js
• packages/request/lib/Client.js
• packages/request/lib/SessionPool.js
• packages/request/lib/errors.js

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The request client now classifies HTTP/2 stream termination, retries eligible requests within configured limits, and shares a timeout budget across attempts. The session pool records GOAWAY frames and retains draining sessions while their accepted streams finish.

Changes

HTTP/2 request retries and session draining

Layer / File(s)	Summary
Stream termination metadata and errors packages/request/lib/errors.js, packages/request/lib/SessionPool.js, packages/request/__tests__/errors.spec.js, packages/request/__tests__/session-pool.spec.js	StreamError supports causes and additional properties. New helpers classify termination metadata and map transport errors. SessionPool records per-stream termination details and adjusts error logging based on whether a stream may have been processed.
GOAWAY session draining packages/request/lib/SessionPool.js, packages/request/__tests__/session-pool.spec.js, packages/request/__tests__/acceptance.spec.js	SessionPool records GOAWAY details, excludes closed sessions from new request selection, and retains draining sessions until they close. Tests cover new requests using another session while existing streams finish.
Request retry and timeout handling packages/request/lib/Client.js, packages/request/__tests__/client.spec.js, packages/request/__tests__/acceptance.spec.js	Client.request validates retry limits and shares one timeout budget across attempts. Client.fetch maps stream termination errors, logs failures, and retries eligible streams. Tests cover retry limits, timeout and abort behavior, refused streams, GOAWAY, and connection failures.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant SessionPool
  participant HTTP2Peer
  Client->>SessionPool: Request a stream
  SessionPool->>HTTP2Peer: Send request
  HTTP2Peer-->>SessionPool: Refuse stream or send GOAWAY
  SessionPool-->>Client: Return StreamError with termination metadata
  Client->>Client: Check retry eligibility and timeout budget
  Client->>SessionPool: Request another stream when eligible

Loading

Merge Risk: ⚪ Minimal · up to c9bde

The identified GOAWAY race cannot occur on this path, and retries do not consume a one-shot request body. The change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to c9bde

Automatic retries now include write requests, while active connections can remain open during draining. Retry limits and checks that the server did not process a stream reduce the risk, but the behavior warrants design review.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The behavioral change reaches operations made through this request client, including write operations. The supplied links to files outside the package do not establish additional production callers or a broader service boundary.

Trust Boundaries and Controls

• observed — The client relies on the upstream HTTP/2 refusal or GOAWAY boundary to identify unprocessed streams. It excludes streams with responses or no assigned ID from retry and shares a deadline across attempts.

Resilience and Maintainability Implications

• observed — Draining sessions keep their accepted streams, but are excluded from new stream selection and removed when the session closes. The retry loop checks the caller abort signal before continuing.

Hardening Proposals

• proposed — For deployments where an intermediary cannot reliably guarantee never-processed signals for writes, consider an idempotency mechanism or a caller-selectable retry policy; this is a precaution, not an observed duplicate mutation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

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: handling HTTP/2 GOAWAY and refused streams without failing requests.
Description check	✅ Passed	The description is complete and directly related to the changes. It includes categorization, motivation, implementation details, reviewer notes, and a release note. The issue reference remains as `Fix…

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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