feat(http2): multiplex over a bounded undici pool (undici 7.26)#793
Conversation
Configure the opt-in http2 transport's undici Agent with connections=4 and pipelining=64 so concurrent requests multiplex over a few TLS sessions instead of opening one connection per request, and bump undici ^6.21 -> ^7.26. undici only multiplexes H2 streams when pipelining > 1 (default 1), and the multiplexed-H2 path assert-crashes on undici 6.x (fixed upstream in 7.23.0, nodejs/undici#4845). Verified on the pinned 7.26.0: 0 crashes and 4 sessions for 2000 concurrent requests. Raises the supported Node floor to >= 20.18.1. - src/lib/undici-fetch.ts: bounded multiplexing Agent + @internal test hooks - package.json: undici ^7.26.0, engines.node >= 20.18.1 - README: Node requirement + HTTP/2 transport section - tests/lib/undici-fetch.test.ts: hermetic normalizeBody / multiplexing / abort it-test - verify-http2.mjs: concurrent-multiplexing assertion (Pass D) - h2-transport-bench.mjs: committed perf/repro harness Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yarn classic hard-errors on the ROOT package's own engines during yarn install even without engine-strict, so engines.node>=20.18.1 failed the node-18 lint job. Keep the undici 7 bump + the README Node-floor docs; undici's transitive engine constraint still warns node-18 installers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The only node-18 job in CI was lint; undici 7's engine constraint hard-fails yarn install there. All other jobs already run on Node >= 20. Bump lint to Node 20 to match the build job and the SDK's documented Node floor. Co-Authored-By: Claude Opus 4.8 <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 · |
|
|
||
| beforeAll(async () => { | ||
| diagnostics_channel.subscribe('undici:client:connected', onConnected); | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'h2cert-')); |
There was a problem hiding this comment.
Suggestion: The temporary certificate directory created with mkdtempSync is never removed, so repeated test runs leave files behind in the OS temp folder. Add cleanup in afterAll (or a finally) to delete the generated cert directory once the server is closed. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Jest HTTP/2 multiplexing test leaves temp cert dirs behind.
- ⚠️ Repeated test runs slowly clutter the OS temp folder.Steps of Reproduction ✅
1. Run the Jest suite including `tests/lib/undici-fetch.test.ts` (e.g. `npm test --
tests/lib/undici-fetch.test.ts`).
2. During setup, the `beforeAll` in `describe('HTTP/2 multiplexing (shipped pool config)'`
at `tests/lib/undici-fetch.test.ts:59-106` executes `const dir =
fs.mkdtempSync(path.join(os.tmpdir(), 'h2cert-'));` (line 72) and writes `k`/`c` cert
files into that directory.
3. The corresponding `afterAll` block at `tests/lib/undici-fetch.test.ts:108-112`
unsubscribes the diagnostics channel, closes the undici `Agent`, and closes the HTTP/2
server but never deletes `dir` or its contents.
4. After the test run completes, inspect the OS temp directory (e.g. `/tmp` or
`os.tmpdir()`) and observe that `h2cert-*` directories created by this test remain on
disk, accumulating across repeated runs.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/lib/undici-fetch.test.ts
**Line:** 72:72
**Comment:**
*Resource Leak: The temporary certificate directory created with `mkdtempSync` is never removed, so repeated test runs leave files behind in the OS temp folder. Add cleanup in `afterAll` (or a `finally`) to delete the generated cert directory once the server is closed.
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| const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'h2cert-')); | ||
| const keyPath = path.join(dir, 'key.pem'), certPath = path.join(dir, 'cert.pem'); | ||
| execFileSync('openssl', ['req', '-x509', '-newkey', 'rsa:2048', '-nodes', '-keyout', keyPath, '-out', certPath, | ||
| '-days', '1', '-subj', '/CN=localhost', '-addext', 'subjectAltName=DNS:localhost,IP:127.0.0.1'], { stdio: 'ignore' }); | ||
| return { key: fs.readFileSync(keyPath), cert: fs.readFileSync(certPath), dir }; |
There was a problem hiding this comment.
Suggestion: makeCert returns the temp directory path but never deletes it, so each benchmark run leaks cert/key files under the temp directory. Remove the directory during teardown after server.close() to avoid accumulating leftover files. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Benchmark runs leak self-signed cert directories in temp.
- ⚠️ Repeated micro-benchmarks accumulate unused files under os.tmpdir.Steps of Reproduction ✅
1. From the repo root, run the benchmark script: `node
tests/smoketests/scripts/h2-transport-bench.mjs`.
2. In `main()` at `tests/smoketests/scripts/h2-transport-bench.mjs:307-369`, the call to
`startServer()` (lines 255-270) executes `makeCert()` at lines 246-252.
3. `makeCert()` creates a unique temp directory via `const dir =
fs.mkdtempSync(path.join(os.tmpdir(), 'h2cert-'));` (line 247), writes `key.pem` and
`cert.pem` inside it (lines 248-250), and returns `{ key, cert, dir }` (line 251).
4. `startServer()` destructures only `{ key, cert }` from the return value at line 256 and
never uses or deletes `dir`, and the rest of the script (lines 255-369) closes the HTTP/2
server but never removes the created directory, so each benchmark run leaves an `h2cert-*`
directory under `os.tmpdir()` on disk.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/h2-transport-bench.mjs
**Line:** 247:251
**Comment:**
*Resource Leak: `makeCert` returns the temp directory path but never deletes it, so each benchmark run leaks cert/key files under the temp directory. Remove the directory during teardown after `server.close()` to avoid accumulating leftover files.
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. |
The behavioral change is 2 lines (connections + pipelining). Drop the 369-line manual bench harness (its numbers live in the PR description) and the hermetic TLS-multiplexing/abort cases — the multiplexing case exercised a config-equivalent undici Agent (testing undici, not our adapter), and multiplexing stays covered by verify-http2.mjs Pass D against the real API. Keep the normalizeBody unit tests; un-export the now-unused test-only symbols from the adapter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Makes the opt-in
http2: truetransport actually multiplex, by configuring undici'sAgentwithconnections: 4+pipelining: 64and bumpingundici ^6.21 → ^7.26.Stacked on
feat/http2-undici(#791). This is a parallel alternative to #792 (the hand-rollednode:http2pool) — put up so we can compare the two head-to-head. It is the "simplest correct version of #791's own approach": stay on undici, configure it right, upgrade past the bug.Why
Two findings from benchmarking #791 + tracing undici:
pipelining > 1(default is 1; the dispatch loop gates onkRunning >= getPipelining()). With the default config feat: opt-in HTTP/2 multiplexing transport via undici 7 (Node >= 20.18.1) #791 opens one TLS connection per concurrent request — a connection storm, not multiplexing.Agent({allowH2, connections, pipelining>1})multiplexes correctly but crashes withassert(!this.completed)(undici/lib/core/request.js←client-h2.js) under concurrent multiplexed H2. Fixed upstream in undici v7.23.0 (nodejs/undici#4845, "ignore late data frames after request completion").So the fix is two
Agentoptions + a dependency bump — and undici owns the connection pool / stream lifecycle / 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 undici 7.26.0 (the pinned version):pipelining=1, unbounded)connections=4 pipelining=64)This PR and #792 are within noise of each other; #791 storms connections and starts failing requests at scale. (Reproduced with a local self-signed-h2 benchmark harness — kept out of the repo; available on request.)
Changes
src/lib/undici-fetch.ts— single boundedAgent({ allowH2: true, connections: 4, pipelining: 64, keepAlive… }); ~2 lines of real change over feat: opt-in HTTP/2 multiplexing transport via undici 7 (Node >= 20.18.1) #791.normalizeBodyand theundici.fetchcall are unchanged. Adds@internaltest hooks (not in the publicexportsmap).package.json—undici ^7.26.0. (Noenginesfield: yarn classic hard-errors on the root package's ownenginesduringyarn installeven withoutengine-strict, which broke the Node-18 lint job. The Node floor is documented in the README and enforced transitively by undici — whose install fails below 20.18.1 — so a hardenginesdeclaration is redundant.).github/workflows/ci.yml— lint job bumped Node 18 → 20 (undici 7 can'tyarn installon Node 18; every other job was already ≥ 20).README.md— Node requirement raised to ≥20.18.1; new HTTP/2 transport section.tests/lib/undici-fetch.test.ts(new) — unit tests fornormalizeBody(the adapter's only non-trivial logic). The adapter is already exercised end-to-end over both transports by the existing http1/http2 smoke matrix.verify-http2.mjs— adds a "Pass D" multiplexing assertion against the real API (smoke), alongside the existing ALPN-h2 check.undici 7 requires Node ≥20.18.1, and the SDK loads undici eagerly via the node shim, so the supported Node floor rises from 18 → 20.18.1 for all Node users (other runtimes — browser/Deno/Bun/Workers — use platform fetch and are unaffected). This is enforced at install (undici's own engine constraint hard-fails
yarn installbelow 20.18.1) and documented in the README; the CI lint job moved off Node 18 accordingly.Risk / known limitation
pipelining: 64also enables HTTP/1.1 request pipelining on the fallback path, sohttp2: true(opt-in) is intended for h2-capable origins like the Runloop API. If strict h1 safety is ever needed, the hardening is ALPN-aware routing to apipelining: 1h1 dispatcher (what #792 does) — not included here to keep it simple.Validation
tests/libsuite: 52/52 pass (incl. the new it-test).require+importload on undici 7) and lint (eslint + tsc) clean.🤖 Generated with Claude Code