feat!: enable h2 by default - #4828
Merged
Merged
Conversation
(cherry picked from commit 09862a2)
7 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #4828 +/- ##
=======================================
Coverage 93.26% 93.26%
=======================================
Files 107 107
Lines 34031 34031
=======================================
Hits 31738 31738
Misses 2293 2293 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
@metcoder95 can you look into the failure? It seems relevant. |
mcollina
added
the
semver-major
Closed
christopher-buss
added a commit
to christopher-buss/bedrock
that referenced
this pull request
Jul 31, 2026
## Summary A consumer's deploy lost all three of its places at once, two of them in the same millisecond. The codes were new: `ERR_HTTP2_STREAM_ERROR` and `UND_ERR_INFO`, where this failure has always surfaced as a socket error before. Node 26 bundles undici 8, which [enables HTTP/2 by default](nodejs/undici#4828) (taken into Node in [nodejs/node#62384](nodejs/node#62384)). That breaks the mitigation from #531 in two ways. `Connection` is a connection-specific header, forbidden in HTTP/2, so an h2 transport drops the `connection: close` uploads set — the directive was doing nothing. And h2 multiplexes, so every upload shares one session where HTTP/1.1 gave each its own: one session death now fails every place in flight, instead of one. Measured against a local server offering ALPN `["h2", "http/1.1"]`: | Client | ALPN | TCP connections for 2 uploads | `connection` on the wire | | --- | --- | --- | --- | | Node 26.5.1 `fetch` | h2 | 1 | absent (stripped) | | Node 24.18 `fetch` | http/1.1 | 2 | `close` | | Bun 1.3.14 `fetch` | http/1.1 | 2 | `close` | Bun implements `node:http2`, but its `fetch` does not offer h2 in ALPN, so Bun is unaffected. The exposure is Node 26 and later. The fix has two parts: 1. **Uploads pin HTTP/1.1.** There is no standard `fetch` option for this, and the documented route — `setGlobalDispatcher(new Agent({ allowH2: false }))` — means depending on undici, which ADR-008 rules out, and mutating a process-wide default, which a library should not do. Instead the transport reconstructs the class of the runtime's own global dispatcher with `allowH2: false`. No dependency, and it re-arms the directive #531 relies on. The symbol is undici's internal contract and moved from `.1` to `.2` in undici 8, so an absent symbol, a non-constructible value, or a throwing constructor each fall back to the runtime's default transport rather than failing the deploy. 2. **The h2 codes join `TRANSIENT_TRANSPORT_CODES`.** `ERR_HTTP2_STREAM_ERROR`, `ERR_HTTP2_SESSION_ERROR`, and `UND_ERR_INFO` are the h2 spellings of socket deaths already in that set. This is the safety net for any runtime where part 1's probe finds nothing. Part 2 corrects a claim #531 made. The transient set was described as failures that never reached Open Cloud. That is not true of the h2 codes — `UND_ERR_INFO` covers both a `GOAWAY` declaring a stream was never started and a stream that was fully sent — nor of `UND_ERR_SOCKET`, which can fire once a response is already streaming. For idempotent methods this changes nothing. For uploads, retry safety rests where #531 actually measured it: Roblox dedupes identical place content, so a retry that races a publish which did land returns that same version. The doc comments now say so. ## Verification A local ALPN server driving the real transport, asserting the symptom rather than the header: two uploads must reach it over two connections with `connection: close` on the wire. ``` before node 26.5.1 FAIL alpn=h2 tcpConnections=1 connectionHeader=[null,null] after node 26.5.1 PASS alpn=http/1.1 tcpConnections=2 connectionHeader=["close","close"] after node 24.18 PASS alpn=http/1.1 tcpConnections=2 connectionHeader=["close","close"] after bun 1.3.14 PASS alpn=http/1.1 connectionHeader=["close"] ``` The CI error was also reproduced end to end: a server that sends `RST_STREAM` mid-upload surfaces `ERR_HTTP2_STREAM_ERROR` through `fetch`, byte-identical to the deploy log. Full suite green, 100% mutation score on every touched file. ## Notes for reviewers - **`http1-dispatcher.spec.ts` has one test that makes a real network call** (`http://127.0.0.1:1/`, refused in ~20ms). It is deliberate. Every other test injects a fake scope, so nothing else would notice the day undici's symbol stops being reachable — which is the day this outage silently returns with a green suite. - **A consumer who calls `setGlobalDispatcher(new MockAgent())` in their own tests would see uploads bypass their interceptors**, because the fresh agent carries none of them. Uploads only; every other request still uses the global. I did not guard it: there is no reliable signal distinguishing a mock from a real agent, and `OpenCloudClientOptions.httpClient` is the documented test seam. - **`TRANSIENT_TRANSPORT_CODES` is publicly exported and its meaning widened** from "proved unprocessed" to "transport-level failure", under a `patch`. No signature changes. ## Follow-ups, deliberately not in this PR - **No consumer-reachable override.** If undici's symbol moves again, a consumer on that runtime loses the mitigation and has no supported workaround short of an SDK release. A `dispatcher` or `fetchFunc` option on `OpenCloudClientOptions` would fix that, but it is a public API change needing its own tests and changeset. - **A gateway `RST_STREAM(CANCEL)` hangs `fetch` forever.** undici emits no error event and dequeues the request without rejecting it, and uploads carry no timeout by design, so the hang is unbounded and retries cannot reach it — nothing ever fails. Needs its own decision plus an upstream fix; recorded in the ADR amendment. ## References Follows #531, which fixed the HTTP/1.1 half of this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Overall Summary The PR aligns with Bedrock’s architecture: HTTP dispatcher discovery is isolated to an adapter, while fetch-client orchestration remains separate. Transport failures are handled with typed error codes, and the retry-safety documentation is appropriately narrowed. Tests cover dispatcher construction, fallback behavior, upload integration, and retry decisions, including malformed and unavailable runtime dispatcher shapes. No blocking architecture, security, or implementation issues were identified. ## Recommendations - Confirm CI enforces 100% coverage for the new dispatcher branches. - Verify all new tests use the required `it("should ...")` naming convention. - Retain Node 24, Node 26, and Bun verification because dispatcher internals are runtime-dependent. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This relates to...
This enables HTTP/2 support by default in Undici.
Rationale
Currently, users must explicitly opt-in to H2 support via
allowH2: true. This change adjusts Undici's defaults to align with current web standards and improve out-of-the-box performance.Changes
This PR changes the default value of the
allowH2option fromfalsetotruein the connection builder (lib/core/connect.js). When connecting to HTTPS servers, Undici will now use HTTP/2 if the server advertises it through ALPN negotiation, falling back to HTTP/1.1 if not supported.Features
allowH2: falsein client optionsBug Fixes
N/A
Breaking Changes and Deprecations
Breaking Change: The default value of
allowH2changes fromfalsetotrue.Impact:
allowH2: falsein their client configurationStatus