feat: opt-in HTTP/2 multiplexing transport via undici 7 (Node >= 20.18.1)#791
Conversation
Adds a `http2: true` client option that routes Node requests through an
undici adapter (`Agent({ allowH2: true })`), negotiating HTTP/2 over ALPN
with automatic fallback to HTTP/1.1. undici powers Node's global fetch, is
require-able from this CommonJS package, and returns a standard WHATWG
Response, so the rest of core.ts is unchanged -- no dynamic-import hack and
no second HTTP stack.
- src/lib/undici-fetch.ts: the adapter. Strips the node-fetch-style `agent`
that core.ts injects, reuses a module-scoped h2 dispatcher, converts
multipart Node Readable bodies via Readable.toWeb + duplex:'half', and
returns the undici Response directly.
- src/index.ts: the `http2` ClientOptions flag selects the adapter (opt-in,
default false; ignored when a custom `fetch` is supplied).
- src/_shims/*: expose an `http2Fetch` hook on the Node/web/deno runtimes
(web and deno reuse the platform fetch, which already speaks HTTP/2).
- tests/smoketests: run the suite over both transports (http1/http2 matrix)
and add a plain-node harness (verify-http2.mjs) that proves ALPN h2
negotiation, success-body parsing, and clean rejection on a 401.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
| const distPath = new URL('../../../dist/index.js', import.meta.url).pathname; | ||
| const { Runloop } = require(distPath); |
There was a problem hiding this comment.
Suggestion: Converting the module URL with .pathname can produce an invalid filesystem path (/C:/... on Windows and %20-encoded segments for spaces), which makes require fail with module-not-found in valid environments. Convert the URL with fileURLToPath(...) (or require the URL directly) before requiring the built file. [logic error]
Severity Level: Major ⚠️
- ❌ HTTP2 verification harness fails on paths with spaces.
- ⚠️ Windows developers cannot run HTTP2 verifier without path tweaks.Steps of Reproduction ✅
1. Build the SDK so `dist/index.js` exists, as used by the harness at
`tests/smoketests/scripts/verify-http2.mjs:21`.
2. On a machine where the repo path contains spaces or other characters that are
percent-encoded in URLs (for example `C:\Users\John Doe\api-client-ts` on Windows), run
`RUNLOOP_API_KEY=... node tests/smoketests/scripts/verify-http2.mjs` as described in the
usage comment at `tests/smoketests/scripts/verify-http2.mjs:14-15`.
3. The script computes `distPath` using `new URL('../../../dist/index.js',
import.meta.url).pathname` at `tests/smoketests/scripts/verify-http2.mjs:21`, which yields
a pathname with a leading slash and percent-encoded segments (e.g.
`/C:/Users/John%20Doe/api-client-ts/dist/index.js`).
4. `createRequire(import.meta.url)` at `tests/smoketests/scripts/verify-http2.mjs:20` then
calls `require(distPath)` at line 22 with this non-normalized, encoded path, causing
Node's module loader to fail to resolve the file (MODULE_NOT_FOUND / ENOENT) and terminate
the HTTP/2 verification harness before any checks run.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/smoketests/scripts/verify-http2.mjs
**Line:** 21:22
**Comment:**
*Logic Error: Converting the module URL with `.pathname` can produce an invalid filesystem path (`/C:/...` on Windows and `%20`-encoded segments for spaces), which makes `require` fail with module-not-found in valid environments. Convert the URL with `fileURLToPath(...)` (or require the URL directly) before requiring the built file.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // core.ts injects a node-fetch-style `agent` in RequestInit; undici uses a | ||
| // `dispatcher` instead, so drop `agent`. Pull `signal` and `body` out to | ||
| // normalize them; pass everything else (method, headers, redirect, …) through. | ||
| const { agent: _ignoredAgent, body: rawBody, signal, ...rest } = (init ?? {}) as any; |
There was a problem hiding this comment.
Suggestion: This drops the incoming agent unconditionally, so when a client is configured with httpAgent (proxy agent, custom CA, mTLS, corporate egress controls, etc.) and http2: true, those connection settings are silently ignored. That breaks the existing client contract and can cause requests to fail or bypass required network policy. Preserve equivalent transport configuration for undici (or explicitly reject incompatible agent usage with a clear error instead of silently discarding it). [api mismatch]
Severity Level: Critical 🚨
- ❌ HTTP/2 requests ignore configured proxy or custom TLS agents.
- ⚠️ Corporate egress controls relying on httpAgent are bypassed.
- ⚠️ Users enabling http2 see unexpected connection failures behind proxies.Steps of Reproduction ✅
1. Configure a client instance with both a custom agent and HTTP/2:
`new Runloop({ httpAgent: myAgent, http2: true })` using the `ClientOptions` type
defined in `src/index.ts:254-331` (`httpAgent?: Agent` at `src/index.ts:277-283`,
`http2?: boolean` at `src/index.ts:293-306`). The `Runloop` constructor passes these
into `Core.APIClient` at `src/index.ts:361-385`, setting `httpAgent: options.httpAgent`
and `fetch: options.fetch ?? (options.http2 ? http2Fetch : undefined)`.
2. Invoke any API operation, for example `runloop.devboxes.create()` implemented in
`src/resources/devboxes/devboxes.ts:9-31`. That method calls
`this._client.post('/v1/devboxes', { body, ...options })`
(`src/resources/devboxes/devboxes.ts:27-30`), which resolves to `APIClient.post()` in
`src/core.ts:279-281`, then `APIClient.request()` (`src/core.ts:295-311`), and finally
`APIClient.makeRequest()` / `buildRequest()` (`src/core.ts:478-493`,
`src/core.ts:339-388`).
3. In `APIClient.buildRequest()` (`src/core.ts:339-388`), the client computes `const
httpAgent = options.httpAgent ?? this.httpAgent ?? getDefaultAgent(url);` at
`src/core.ts:357`, so the `httpAgent` passed to `Runloop` is stored on the instance and
selected here. The resulting `RequestInit` includes `...(httpAgent && { agent: httpAgent
})` at `src/core.ts:377-382`, so the Node `http.Agent` is attached as `req.agent`.
`makeRequest()` then calls `fetchWithTimeout(url, req, timeout, controller)` at
`src/core.ts:490-503`.
4. In `fetchWithTimeout()` (`src/core.ts:573-600`), the method constructs `fetchOptions`
by spreading `init` minus the original `signal` (`const { signal, ...options } = init ||
{};` at `src/core.ts:579`) and then calling `this.fetch.call(undefined, url,
fetchOptions)` at `src/core.ts:594-597`. Because the `Runloop` constructor chose `fetch:
options.http2 ? http2Fetch : undefined` (`src/index.ts:378-385`), `this.fetch` is the
HTTP/2-capable `http2Fetch` shim. The shims registry wires `http2Fetch` to the Node
runtime's `http2Fetch` (`src/_shims/registry.ts:34-38`, set from `setShims()` at
`src/_shims/registry.ts:50-75`), and the Node runtime provides `http2Fetch: undiciFetch`
in `getRuntime()` at `src/_shims/node-runtime.ts:61-72`.
5. The HTTP/2 adapter `undiciFetch` in `src/lib/undici-fetch.ts:56-79` receives
`fetchOptions` with the `agent` field set, but immediately strips it with `const { agent:
_ignoredAgent, body: rawBody, signal, ...rest } = (init ?? {}) as any;` at
`src/lib/undici-fetch.ts:60`. It then constructs an undici-specific init object with a
hard-coded dispatcher `dispatcher: h2Dispatcher` at `src/lib/undici-fetch.ts:64-71`, where
`h2Dispatcher` is a module-scoped `new Agent({ allowH2: true, ... })` created at
`src/lib/undici-fetch.ts:25-32`. The original Node `http.Agent` (proxy agent, custom CA,
mTLS, etc.) from `ClientOptions.httpAgent` is never consulted in this HTTP/2 code path. As
a result, any connection-level configuration expressed via `httpAgent` is ignored when
`http2: true` is set, even though `httpAgent` is still accepted and stored by `APIClient`.
(The comment at `src/index.ts:293-303` notes that `httpAgent` "is not used" on the HTTP/2
path, but there is no runtime guard; the agent is simply dropped.)Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/undici-fetch.ts
**Line:** 60:60
**Comment:**
*Api Mismatch: This drops the incoming `agent` unconditionally, so when a client is configured with `httpAgent` (proxy agent, custom CA, mTLS, corporate egress controls, etc.) and `http2: true`, those connection settings are silently ignored. That breaks the existing client contract and can cause requests to fail or bypass required network policy. Preserve equivalent transport configuration for undici (or explicitly reject incompatible agent usage with a clear error instead of silently discarding it).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
CodeAnt AI finished reviewing your PR. |
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
- src/index.ts: add `http2` to constructor JSDoc; expand ClientOptions JSDoc to flag that H1 fallback enables request pipelining and is unsafe against non-h2 intermediaries. - README.md: drop "(experimental)" framing now that the JSDoc and README agree on stability/caveats. - tests/index.test.ts: add `custom fetch wins over http2` test to lock in the precedence in `options.fetch ?? (options.http2 ? http2Fetch : undefined)`. - tests/smoketests/scripts/: drop devbox-startup and health-endpoint benchmark harnesses (verify-http2.mjs is kept as the regression guard). package.json: drop the matching test:e2e:* scripts.
Make `http2` accept `boolean | undici.Dispatcher`: `true` keeps the default
bounded pool, and passing a configured undici Dispatcher uses it verbatim --
the SDK manages nothing, mirroring the `httpAgent` escape hatch. This lets
callers tune the connection pool (connections x pipelining) for large fan-outs
without the SDK owning new knobs.
Also warn once when `httpAgent` and `http2` are combined: undici has no Node
`http.Agent` concept, so `httpAgent` is inapplicable on the h2 path -- make the
override loud instead of silent (skipped when a custom `fetch` supersedes http2).
- src/lib/undici-fetch.ts: add createUndiciFetch(dispatcher?) factory over the
shared default pool; drop the standalone undiciFetch export.
- src/_shims/*: replace the internal http2Fetch export with a
makeHttp2Fetch(dispatcher?) factory across node/web/deno/registry/index.d.ts.
- src/index.ts: http2?: boolean | import('undici').Dispatcher; resolve the
dispatcher in the constructor; one-time httpAgent+http2 warning.
- README + JSDoc: document the passthrough; httpAgent does not apply to h2.
- tests/index.test.ts: MockAgent passthrough test (proves the dispatcher is
honored end-to-end) + a warn-once test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
Addresses review feedback on PR #791. - undici's bodyTimeout/headersTimeout both default to 300s; the default h2Dispatcher set neither, so on the HTTP/2 path a long-lived stream idle for >300s (e.g. an SSE/exec stream behind withStreamAutoReconnect) would hit an undici BodyTimeoutError. The reconnect predicate (isIdleTimeoutReconnectError) only recognizes 408 / TimeoutError, so it would throw instead of reconnect — a divergence from node-fetch, which has no client-side body timeout. Set bodyTimeout: 0, headersTimeout: 0 so the SDK's own AbortController (the `timeout` option) is the single source of truth on both transports. A caller passing their own dispatcher owns this policy. - Reword a stale comment that referenced a "got adapter" (a dropped iteration of this branch; never existed in the repo). - README: clarify undici >= 7.23.0 is the crash-fix floor and the package pins ^7.26.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Adds an opt-in
http2: trueclient option that sends Node requests over HTTP/2 with real stream multiplexing, backed by undici (Agent({ allowH2: true, connections: 4, pipelining: 64 })). Many concurrent requests share a small bounded pool of TLS sessions instead of opening one connection per request.undici is the same engine behind Node's built-in
fetch: it'srequire-able from this"type": "commonjs"package and returns a standard WHATWGResponse, socore.tsis unchanged. No dynamic-import()hack and no second HTTP stack to keep in sync with node-fetch.HTTP/2 stays opt-in (
@default false); the default transport is still node-fetch, and a user-suppliedfetchalways wins. On the web and Deno the platformfetchalready speaks h2, so the flag is a no-op there.http2acceptsboolean | undici.Dispatcher:trueuses the SDK's default bounded pool, or pass your own configured undiciDispatcherto control the pool yourself — the SDK uses it verbatim and manages nothing, the same contract as a customhttpAgent. This is how you tune the pool (e.g. raiseconnections/pipelining) for high-concurrency fan-outs without the SDK owning new knobs.httpAgent(a Nodehttp.Agent) does not apply to the HTTP/2 path — undici has nohttp.Agentconcept. The SDK emits a one-time warning if both are set, instead of silently ignoringhttpAgent.The SDK now depends on undici 7 (
^7.26.0), whose ownenginesrequires Node >= 20.18.1, and the Node shim loads undici eagerly — so the supported Node floor rises from 18 to 20.18.1 for all Node users, not only those who opt intohttp2. This is enforced at install (undici's engine constraint failsyarn installbelow 20.18.1) and documented in the README; the CI lint job moved off Node 18 accordingly. Other runtimes (browser/Deno/Bun/Workers) use platform fetch and are unaffected.This ships as a minor (not a major): Node 18 reached EOL on 2025-04-30 and the README already scopes support to non-EOL Node, so the floor bump is consistent with stated policy. Node 18 users should pin the prior SDK minor or upgrade Node.
Why multiplexing (and why undici 7)
Two findings from benchmarking the initial single-
Agentapproach and tracing undici:Agent({ allowH2: true })doesn't multiplex. undici only multiplexes H2 streams whenpipelining > 1(default is 1; the dispatch loop gates onkRunning >= getPipelining()). With the default config it opens one TLS connection per concurrent request — a connection storm, not multiplexing.Agent({ allowH2, connections, pipelining > 1 })multiplexes correctly but crashes withassert(!this.completed)under concurrent multiplexed H2. Fixed upstream in undici v7.23.0 (nodejs/undici#4845). Hence the^7.26.0floor.So the fix is two
Agentoptions + a dependency bump — undici owns the pool, stream lifecycle, and abort handling, so there's no hand-rolled concurrency to maintain.Evidence
Local self-signed h2 server, 2000 concurrent requests, 20ms server delay, server
maxConcurrentStreams=100, on the pinned undici 7.26.0:Agent({allowH2})(pipelining=1, unbounded)connections=4,pipelining=64)4 × 64 = 256in-flight requests/origin before undici queues the rest (bounded, no storm).What changed
src/lib/undici-fetch.ts(new) — the adapter, exposed as acreateUndiciFetch(dispatcher?)factory: with no argument it binds the shared default boundedAgent(thehttp2: truecase); pass aDispatcherto bind it verbatim (the passthrough case). Normalizes the body shapescore.tsproduces (string /Buffer/ typed array /ArrayBuffer/ multipart NodeReadable→ web stream viaReadable.toWeb()+duplex: 'half'); strips the node-fetch-styleagent; returns the undiciResponsedirectly.src/index.ts—http2?: boolean | import('undici').Dispatcher. The constructor resolves the dispatcher (makeHttp2Fetch(typeof http2 === 'object' ? http2 : undefined)) and emits a one-timeconsole.warnwhenhttp2+httpAgentare combined (guarded by!fetch). JSDoc documents the passthrough, the h1-fallback pipelining caveat, and thehttpAgentnote.src/_shims/*— replaces the internalhttp2Fetchvalue with amakeHttp2Fetch(dispatcher?)factory across Node/web/Deno/registry (web and Deno reuse the platformfetchand ignore the dispatcher).package.json/yarn.lock—undici ^7.26.0. (Noenginesfield: yarn classic hard-errors on the root package's ownenginesduring install, which broke the Node-18 lint job; the floor is enforced transitively by undici and documented in the README.).github/workflows/ci.yml— lint job Node 18 → 20 (undici 7 can'tyarn installon Node 18; every other job was already ≥ 20)..github/workflows/smoke-tests.yml— smoke suite runs over both transports ([http1, http2]matrix) viaSMOKE_HTTP2, plus averify-http2.mjsstep on the http2 leg.README.md— Node requirement raised to ≥20.18.1; new HTTP/2 transport section.tests/lib/undici-fetch.test.ts(units fornormalizeBody, the adapter's only non-trivial logic);tests/index.test.tslocks the precedence ("custom fetch wins over http2"), the passthrough (aMockAgent— a real undiciDispatcher— with net-connect disabled serves the request, proving the dispatcher is threaded end-to-end), and the warn-once behavior;verify-http2.mjsadds a "Pass D" assertion that 25 concurrent requests multiplex over ≤ 4 connections against the real API.Risk / known limitation
pipelining: 64also enables HTTP/1.1 request pipelining on the fallback path (undici gates both protocols ongetPipelining), sohttp2: true(opt-in) is intended for h2-capable origins like the Runloop API. The JSDoc and README warn against enabling it for traffic routed through non-h2 intermediaries. With the passthrough, strict-h1-safety or any other policy is now a one-liner — passhttp2: new Agent({ allowH2: true, pipelining: 1 })(or an ALPN-routed dispatcher) — so no hardening is baked into the default.On the HTTP/2 path the
httpAgentoption is not used — undici manages connections through its own dispatcher rather than a Nodehttp.Agent. The SDK warns once if both are set; to tune connections on the h2 path, pass aDispatcherashttp2.Verification
verify-http2.mjsagainstapi.runloop.pro: ALPN negotiatesh2(not a silent h1 fallback), success body parses, a 401 rejects cleanly without crashing the process, the h1 control path works, and 25 concurrent requests multiplex over ≤ 4 connections.yarn buildcompiles CJS + ESM (bothrequire()andimport()of the package load on undici 7);yarn lint(eslint +tsc --noEmit) clean. The publicdist/index.d.tscarriesimport('undici').Dispatcher(autocomplete) while the_shims.d.tsstayany, so web/deno declarations don't reference undici.yarn test: theMockAgentpassthrough test and the warn-once test pass alongside the existing suite.🤖 Generated with Claude Code