Repository navigation
test: track connections so HTTP/2 tests terminate in node 24.20.0 - #6285
Conversation
@bufbuild/protobuf 2.14.0 -> 2.14.1 and @connectrpc/connect, connect-node, connect-web 2.1.2 -> 2.2.0, moving together in @arcjet/guard, protocol, transport and arcjet (dev). No peer ranges changed, so no consumer-visible constraint moves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`test/wire.test.ts` hung indefinitely on "HTTP fallback", taking the whole file down with `Promise resolution is still pending but the event loop has already resolved`. The request itself was fine — it rejected with the expected `[unavailable] HTTP 503` in ~5ms — but the `close(server)` in `withWireServer`'s `finally` never settled. `trackHttp2Sessions` tracked server-side `Http2Session` objects and `close` called `session.destroy()` on each. By the time it runs, the session has already reached `closed: true` while still reporting `destroyed: false`, and in that state `destroy()` is a no-op and `session.socket` no longer refers to anything whose teardown closes the connection. The TCP socket stayed open, so `server.close()` never invoked its callback. Track the sockets from the server's own `connection` event instead and destroy those. That works for both `createServer` and `createSecureServer`, since dropping the TCP layer takes the TLS session with it. Renamed to `trackHttp2Connections` to match what it now holds. Confirmed the hang is specific to this code, not the connect-es version in the prior commit: reverting these three files while keeping @connectrpc/connect-node at 2.2.0 reproduces the identical hang; the fix also reproduces the same hang (and the same fix) against 2.1.2. Transport suite goes from an indefinite hang to 54 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![arcjet-review[bot]](https://avatars.githubusercontent.com/u/24397786?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arcjet Review — 🟡 Medium Risk
Decision: Approved
Rationale: This PR fires the dependency-changes escalation trigger because it updates package.json dependencies across several packages. The dependency updates are patch/minor bumps to existing, pinned dependencies rather than new packages, and the code change is limited to test infrastructure for HTTP/2 server teardown. The HTTP/2 change is consistent with the stated issue: tracking accepted sockets and destroying them during close is a reasonable way to release idle HTTP/2 connections that may otherwise keep the test runner alive. No hardcoded secrets, auth changes, injection risks, or other security issues were identified. I am approving despite Medium risk because the changed behavior is test-only and the dependency bumps are narrow and exact-version pinned.
Summary of Changes
Updates @bufbuild/protobuf from 2.14.0 to 2.14.1 and @connectrpc/connect, connect-node, and connect-web from 2.1.2 to 2.2.0 across affected packages. Renames the HTTP/2 test helper from trackHttp2Sessions to trackHttp2Connections and changes teardown to track and destroy accepted sockets instead of HTTP/2 sessions so tests do not hang on idle HTTP/2 connections.
Escalation Triggers
- Dependency Changes: Multiple package.json files update @bufbuild/protobuf and @connectrpc packages.
Review Focus Areas
- Confirm the package manager lockfile, if this repository uses one, was updated consistently with these dependency version changes.
Dependency bumps without a corresponding lockfile update can cause CI, local installs, or published package builds to resolve different versions than intended. - Verify the Connect 2.2.0 HTTP/2 behavior that motivated this change is covered by the existing transport tests on the Node versions supported by the project.
The teardown helper now destroys accepted sockets rather than sessions; this should be validated against the runtime matrix to ensure it fixes the hang without masking other HTTP/2 cleanup failures.
Notes
Path filtering: 1 file excluded by ignore paths. 7 of 8 files included in review.
Review: e1c1cece | Model: openai/gpt-5.5 | Powered by Arcjet Review
arcjet-rei
left a comment
arcjet-rei
left a comment
There was a problem hiding this comment.
I couldn't understand why a dependency bump would lead to changes in the functions used, so I had Claude take a look at the change. It spent a loooooooong time looking at everything, and the tl;dr is that this is actually a patch for an upstream regression in Node 24 (less tl;dr version appended). This fixes that regression, and the dependency bump is worth taking, but the PR title and description should be updated to reflect what's actually going on here. Everything else is fine!
What this patch does
This patch changes the test helper only. It changes no source file. Before, the
helper tracked the HTTP/2 sessions of the test server. Now it tracks the TCP
sockets that the server accepts, andclose()destroys those sockets.The cause is Node.js, not the dependency update
I put the lockfile back to
@connectrpc/connect2.1.2 and kept the old test
helper. The test run did not complete, in the same way. The new version of
connect-es is therefore not the cause.I then ran
transport/test/wire.test.tson different versions of Node.js:
- Node 22.23.2 — 21 tests pass
- Node 24.19.0 — 21 tests pass
- Node 24.20.0 — 21 tests pass
- Node 24.21.0 — the test run does not complete
- Node 26.8.2 — 21 tests pass
The release date of Node.js 24.21.0 is 2026-09-07. With this patch, all 54
transport tests pass on Node 24.21.0.What Node.js 24.21.0 changed
lib/internal/http2/core.jsis identical in 24.20.0 and 24.21.0. The change is
insrc/node_http2.cc, from nodejs/node#65093, "http2: adapt receive deferral
for Node.js 24".Http2Session::Close()now defers its work when JavaScript
calls it from insidenghttp2_session_mem_recv(). The same commit also stops
SendPendingData()while the session receives data. Node.js 26 does not have
that second condition, and Node.js 26 passes the tests.What you see in the test
On Node 24.20.0 and on Node 24.21.0,
session.destroy()leaves the socket open.
Its state is{ destroyed: false, readable: true, writable: false }. On 24.20.0,
each socket closes 1 to 5 milliseconds later. On 24.21.0, the socket of the
"HTTP fallback" test never closes.server.close()waits for that socket, and
its callback never runs. In that test, the client resets the stream after the
503 response, which is the path that nodejs/node#65093 changed.CI
The last test run on
mainused Node 24.20.0 from the runner tool cache. The
tests onmainpass for that reason. When the runner image gets 24.21.0, the
transport tests will not complete, on all three operating systems. This patch
prevents a failure that is not yet visible.One correction to the new comment
The comment in
proxy.tssays that a session in theclosedstate ignores
destroy(). That is not correct.destroy()runs, and it setsdestroyedto
true. It disconnects the session from the socket, but it does not close the
socket.session.socketis a JavaScriptProxy, and afterdestroy()every
property of thatProxythrowsERR_HTTP2_SOCKET_UNBOUND. A reference to
Node.js 24.21.0 and nodejs/node#65093 in that comment would help the next reader.
qw-in
changed the title
2 participants
Update
@connectrpc/connectto2.2.0and fix a change where the http2 session would hang in the tests:@arcjet-rei I'd appreciate you having a look at the http2 handling change!