Repository navigation
Conversation
#34432 made the socket getters of ServerHttp2Session and ClientHttp2Session return undefined once the session has detached from its socket, also when the session had already handed out its cached proxy. No test covers that behavior. The test reads session.socket on both sides while the socket is attached, destroys the client, and waits for both sessions to close. Then session.socket is undefined on both sides, and a reference taken before destroy() still throws ERR_HTTP2_SOCKET_UNBOUND, as in Node.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe change adds an HTTP/2 regression test for client and server session socket behavior after destruction. It verifies that current socket access becomes ChangesHTTP/2 socket lifecycle
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The added regression coverage does not introduce a current merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. This is a test-only change with no diff under How to check it:
CI: in build 114097 the new test passes on every lane (Linux x64 and aarch64, the ASAN lane, Windows x64 and aarch64, macOS x64 and aarch64). The only red job is The test replaces #33791, which is closed because #34432 landed the same getter change. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only addition, well-scoped and follows the file's conventions.
What was reviewed:
- Test hygiene:
port: 0on127.0.0.1,Promise.withResolvers()for event sync (no sleeps), error events wired to reject,try/finallycleanup registered before assertions. - Assertion strength: checks both client and server sessions, asserts the exact
ERR_HTTP2_SOCKET_UNBOUNDcode on the pre-destroy proxy reference, andtoBeUndefined()on the post-destroy getter. - Confirmed
netis already imported at the top of the file; no CODEOWNERS coverage for this path.
Extended reasoning...
Overview
This PR adds a single 44-line test case to test/js/node/http2/node-http2.test.js covering the Node-documented behavior that Http2Session#socket returns undefined after the session is destroyed. It exercises both client and server sessions: it captures the socket Proxy before destroy, destroys the client, awaits close on both sides, then asserts the getter now returns undefined, instanceof net.Socket is false, and the previously-captured Proxy still throws ERR_HTTP2_SOCKET_UNBOUND on instanceof. No production code is touched — the fix landed in #34432 and this backfills coverage.
Security risks
None. The test spins up an in-process plaintext HTTP/2 server on 127.0.0.1:0 and connects to it locally. There is no external network access, no credential handling, no untrusted-input parsing, and no changes to runtime code.
Level of scrutiny
Low. This is a small, self-contained test addition to an existing compatibility test file, following the exact patterns already present in that file (http2.createServer(), Promise.withResolvers(), try/finally with server.close()). I checked it against the test-authoring rules in CLAUDE.md and REVIEW.md: it uses port 0 on 127.0.0.1, awaits events rather than time, wires error handlers to reject the awaited promises, registers cleanup before assertions, and asserts the specific error code rather than a bare toThrow(). The net import it relies on already exists at the top of the file.
Other factors
The PR description states the test fails when the getters are reverted to their pre-#34432 form and passes 25/25 on the debug build, and that Node v26.3.0 produces identical results — consistent with the Node-compat testing bar. The changed path is not covered by CODEOWNERS. There are no prior reviews or outstanding objections on the timeline, and the bug hunt exited on dry_streak with no findings.
|
Updated 8:14 PM PT - Sep 10th, 2026
❌ @robobun, your commit d94abfe has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42255That installs a local version of the PR into your bun-42255 --bun |
The test now uses node:test and node:assert, like node-http2-client-close.test.ts, so the same file runs under `bun test` and under `node --test`. It compares one probe object for the client session and one for the server session. In its own file the test no longer shares a run with the timing sensitive tests of node-http2.test.js.
|
@cirospaciari about the test error: it did not come from the new test.
d94abfe moves the new test to its own file, |
There was a problem hiding this comment.
LGTM — test-only change, and the move to a standalone node:test file matches the sibling node-http2-continuation.test.ts / node-http2-upgrade.test.mts pattern.
What was reviewed:
- Event sequencing:
once()rejects on'error'for listen/connect; server'session'is awaited before destroy; both'close'events are awaited — no sleeps or timing races. - The server session's no-op
'error'handler is correct here (clientdestroy()is expected to error the peer); it can't mask a real failure because the deepStrictEqual runs after both closes. - Cleanup is in
try/finallyregistered before assertions;port: 0on127.0.0.1; single deep-equal on a combined object with the exactERR_HTTP2_SOCKET_UNBOUNDcode.
Extended reasoning...
Overview
This PR adds a single 75-line test file, test/js/node/http2/node-http2-session-socket.test.ts, covering Http2Session#socket behavior after destroy(). It creates an h2 server on 127.0.0.1:0, connects a client, captures session.socket on both sides while attached (forcing the Proxy cache), destroys the client, awaits 'close' on both sessions, then asserts via one deepStrictEqual that session.socket is now undefined on both sides while the pre-captured Proxy still throws ERR_HTTP2_SOCKET_UNBOUND on instanceof. There are no changes under src/. Since the prior review, two commits landed; the final one moved the test into its own file so it can run identically under node --test, matching two existing siblings in the same directory that already use node:test.
Security risks
None. Test-only, no network egress beyond loopback, no credentials, no changes to runtime code paths.
Level of scrutiny
Low-to-moderate: a small, self-contained test addition. The main things to check for a test PR here are flakiness, hermeticity, and whether the assertion can actually fail for the right reason. All events are awaited (once from node:events rejects on 'error', so listen/connect failures surface); the server 'session' promise is awaited before destroy so the accept order can't race; both 'close' events are awaited before probing. The server session's swallowed 'error' is deliberate — destroying the client is expected to error the peer — and cannot hide a hang because the subsequent 'close' await and the final deepStrictEqual still gate the test. Cleanup is in try/finally registered before the assertion, port: 0, IPv4 loopback, no per-test timeout.
Other factors
The "add to the existing file" default is satisfied by directory precedent: node-http2-continuation.test.ts and node-http2-upgrade.test.mts already use node:test in standalone files here, and the file header documents the dual-runner intent. The assertion checks the exact ERR_* code and uses a single combined deep-equal, per repo convention. The only third-party review (cirospaciari) is DISMISSED, so there are no outstanding objections. Exit reason was dry_streak.
Problem
session.socketmust beundefinedafter anHttp2Sessionis destroyed (Node docs). http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 (a07354a) fixed both getters, atsrc/js/node/http2.ts:4554and:5553. No test covers that behavior.destroy(),session.socket instanceof net.SocketthrewERR_HTTP2_SOCKET_UNBOUND.Fix
test/js/node/http2/node-http2-session-socket.test.tswith one test. There is no change undersrc/.session.socketon the client session and on the server session while the socket is attached. It destroys the client and waits for'close'on both sessions. Thensession.socketisundefinedon both sides. A reference taken beforedestroy()still throwsERR_HTTP2_SOCKET_UNBOUND.node:testandnode:assert, likenode-http2-client-close.test.ts. The same file passes undernode --test(v26.3.0) andbun bd test.bun bd test test/js/node/http2/node-http2-session-socket.test.tspasses in 25 of 25 runs. With both getters put back to their form before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432, the test fails for both sessions (after: 'object',afterIsSocket: 'ERR_HTTP2_SOCKET_UNBOUND').Background
Http2Sessionowns anet.Socketor atls.TLSSocket.session.socketreturns aProxyaround the session, not the socket. TheProxyblocks methods such aswriteanddestroy, because a raw write corrupts the HTTP/2 framing.Proxyin#socket_proxy.destroy()clears the real socket (this[bunHTTP2Socket]). From then on every trap of the cachedProxythrowsERR_HTTP2_SOCKET_UNBOUND. That includesgetPrototypeOf, whichinstanceofcalls.if (session.socket && !session.socket.destroyed). That guard needsundefined, not the staleProxy.Notes
The test comes from node:http2: return undefined from session.socket after destroy() #33791. That PR proposed the same getter change and is now closed, because http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 landed it first. The test there covered the client session only. This version also covers the server session, because http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 changed both getters.
The first version of this PR added the test to
node-http2.test.js. It now has its own file. A run of that whole file with a debug build has timing sensitive tests: on a machine under load, thehttp2 DATA payload survives its ArrayBuffer being detached/resized by transport JS mid-sendblock reaches its 5000 ms timeouts (9 of 9 with 3 CPUs), andhttp2 server with minimal maxSessionMemory handles multiple requeststimed out once on macOS aarch64 in build 114070 before it passed on the retry. The new test passed in each of those runs.The test waits for the server
'session'event before it destroys the client. So the server always has a session to close, and the test does not depend on the accept order.Output with the getters of main:
Output with the getters put back to their form before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432:
test/js/node/test/parallel/test-http2-unbound-socket-proxy.jsalready covers the captured reference on the client. It does not readsession.socketagain after the close.[auto-merge] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file