Emit upgrade event on http.ClientRequest for 101 responses#28470
Emit upgrade event on http.ClientRequest for 101 responses#28470robobun wants to merge 15 commits into
Conversation
|
Updated 12:25 PM PT - Mar 25th, 2026
❌ @robobun, your commit 1cacba9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 28470That installs a local version of the PR into your bun-28470 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements raw-socket HTTP/1.1 Upgrade and CONNECT handling: parses a status line and headers from buffered bytes, opens Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 82-119: The parseRawHTTPResponse implementation uses public
string/object methods on untrusted data (split, match, trim, toLowerCase,
indexOf, Object.create/Object.keys) — capture the primordials/JSC intrinsics at
the top of the file (e.g., String.prototype.split, .match, .trim, .toLowerCase,
.indexOf, Object.keys, Object.create) and replace direct calls in
parseRawHTTPResponse (references: kHeaderSeparator, headerStr, statusLine,
lines, line, key, value, headers, rawHeaders, head) with the intrinsics invoked
via .$call or the captured functions to prevent prototype tampering; apply the
same pattern to the other mentioned parser locations (the blocks around the
other ranges noted) so all string and object operations use the captured
intrinsics.
- Around line 153-155: The socket created by connectModule.connect(...) is never
bound to the request instance or the synthesized IncomingMessage objects; assign
the created socket to this.socket (or self.socket) immediately after
connectModule.connect(...) and before calling maybeEmitSocket(), and ensure any
IncomingMessage constructed in functions that currently pass null (the creations
at IncomingMessage(..., null) around where req/res are created) are either
constructed with that socket or have their .socket property set to the same
socket afterward so req.socket, res.socket and the "socket" event reference the
actual net.Socket/tls.TLSSocket used for the connection (update locations using
connectModule, the local socket const, maybeEmitSocket, and the IncomingMessage
constructions to perform this binding).
- Around line 223-245: The current fallback for non-101 responses relies on
socket 'data'/'end' and ignores parsed.headers, causing hangs for keep-alive
responses; instead instantiate and use the HTTPParser from
process.binding("http_parser") (same approach as the normal response path) to
parse the body and framing according to Content-Length and Transfer-Encoding:
feed incoming chunks from socket.on("data") into parser.execute, push
parsed.head and parser.onBody data into res via res.push, and mark res.complete
and push null in parser.onMessageComplete (not on socket.on("end");
remove/replace the existing socket.on("end") logic). Ensure res._read remains
overridden but that res.complete is set by the parser, and reference symbols
res._read, parsed.headers, socket.on("data"), socket.on("end"),
HTTPParser/process.binding("http_parser"), parser.onBody,
parser.onMessageComplete in your changes.
- Around line 156-168: doUpgradeRequest is writing raw HTTP headers from
this.getHeaders() to the socket without ensuring an explicit Host header, which
breaks HTTP/1.1 upgrade handshakes; before building/writing the request string
(where requestPath, headers and socket.write are used), synthesize a Host header
if none exists by deriving host[:port] from the connection target (mirror the
logic used at the DNS-callback path around line 773) and insert it into headers
(or prepend to the serialized request) so the final request always includes a
valid Host header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b644374e-9a10-4886-98b6-6fc5718700fc
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/28450.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/js/node/_http_client.ts (1)
270-339:⚠️ Potential issue | 🟠 MajorThe manual raw-socket/non-101 state handling is still incomplete.
This fallback only knows
Content-Lengthand EOF. ATransfer-Encoding: chunkedrejection on a keep-alive connection never completes, a post-header socket error is swallowed becausereject(err)is inert afterresolve(), and an abort/timeout falls through the generic"socket hang up"path and gets re-emitted as a real error. Please route this branch through the normal HTTP parser or a single state machine that can handle chunked framing, premature closes, and abort suppression consistently.Also applies to: 597-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 270 - 339, The fallback raw-socket branch in onData (handling parsed.headers, contentLength, bytesReceived, socket listeners and resolve/reject) is incomplete: it only supports Content-Length/EOF, swallows post-header socket errors after resolve, and re-emits abort/timeout as real errors. Replace this ad-hoc logic with a single framing/state-machine or re-route the socket into the normal HTTP parser/path so chunked Transfer-Encoding is handled; ensure socket "error" after headers still rejects the Promise if response is not yet resolved, and ensure abort/timeout (kAbortController signal and timeouts cleared via kClearTimeout) suppress or convert socket errors into non-throwing abort completions instead of "socket hang up". Update handlers around onData/onAbortSocket/socket.on("error")/socket.on("close")/maybeEmitClose to use that unified state so chunked framing, premature closes, and abort suppression behave consistently (affecting the branch around parsed.headers handling and the analogous code at lines ~597-607).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 237-242: The code emits "upgrade"/"connect" events via
process.nextTick but doesn't handle the case where self.emit returns false,
causing leaked sockets; update the logic around
maybeEmitSocket()/process.nextTick so that when emitting (use the same eventName
determination and call self.emit(eventName, res, socket, parsed.head)) you check
the boolean return and if it is false immediately destroy/close the socket and,
if appropriate, destroy the response (res) to fully tear down the connection;
locate the emit call in the same block (reference symbols: maybeEmitSocket,
method, eventName, self.emit, process.nextTick, res, socket, parsed.head) and
add a branch that calls socket.destroy() (and res.destroy() if needed) when the
emit returns false.
- Around line 108-116: The code currently overwrites repeated response headers
(except set-cookie) in the header consolidation block (the lowerKey/headers
assignment in _http_client.ts), causing loss of valid repeated headers like
sec-websocket-extensions; update this branch to preserve duplicates by using the
existing buildHeaders() helper: when headers[lowerKey] already exists and
lowerKey !== "set-cookie", replace the direct assignment headers[lowerKey] =
value with headers[lowerKey] = buildHeaders(headers[lowerKey], value) (ensuring
buildHeaders handles string|string[] inputs), so duplicate values are joined
with ", " consistent with the rest of the codebase.
- Around line 148-152: The code rebuilds the outbound request target from new
URL(...) which breaks CONNECT (authority-form) and plain CONNECT requests never
reach the connect branch because go() only routes Connection: upgrade to
doUpgradeRequest(); fix by passing and using the original raw request target
(e.g., rawRequestTarget or request.rawUrl/request.urlRaw) through the call chain
instead of reconstructing from parsedUrl: keep using parsedUrl.hostname/port for
connectHost/connectPort but use the raw request target when method === "CONNECT"
or when routing to the connect branch so the outbound request line uses
authority-form ("host:port"); update go(), doUpgradeRequest(), and the connect
branch to accept and forward that raw target (also adjust the helper that
currently defines parsedUrl/requestPath — use raw target for CONNECT and
pathname+search for others), and apply the same change to the other affected
spots (lines referenced: 168-169, 223-239, 575-585).
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 270-339: The fallback raw-socket branch in onData (handling
parsed.headers, contentLength, bytesReceived, socket listeners and
resolve/reject) is incomplete: it only supports Content-Length/EOF, swallows
post-header socket errors after resolve, and re-emits abort/timeout as real
errors. Replace this ad-hoc logic with a single framing/state-machine or
re-route the socket into the normal HTTP parser/path so chunked
Transfer-Encoding is handled; ensure socket "error" after headers still rejects
the Promise if response is not yet resolved, and ensure abort/timeout
(kAbortController signal and timeouts cleared via kClearTimeout) suppress or
convert socket errors into non-throwing abort completions instead of "socket
hang up". Update handlers around
onData/onAbortSocket/socket.on("error")/socket.on("close")/maybeEmitClose to use
that unified state so chunked framing, premature closes, and abort suppression
behave consistently (affecting the branch around parsed.headers handling and the
analogous code at lines ~597-607).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f89e3328-e1a8-415d-a69f-a8a3e0e5559a
📒 Files selected for processing (1)
src/js/node/_http_client.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 295-326: The current socket "end" handler unconditionally treats
EOF as message completion; change it so completion follows HTTP framing: only
call res.push(null) and set res.complete in socket.on("end") when the response
is allowed to be framed by EOF (no contentLength, not Transfer-Encoding:
chunked, and the status/method indicates an implicit empty body like
1xx/204/304/HEAD); otherwise treat socket EOF as a truncation/error (emit an
error on res and do not silently mark res.complete). Update the logic around
contentLength, bytesReceived, res.complete, socket.on("data") and
socket.on("end") to consult flags (e.g., isChunked / expectsBody derived from
response headers/status) so fixed-length bodies only complete when bytesReceived
>= contentLength and chunked bodies complete only when the chunked parser
signals end.
- Around line 285-286: The merged Content-Length header from
parseRawHTTPResponse() can be a joined string like "5, 7" so using
parseInt(parsed.headers["content-length"], 10) silently accepts invalid values;
update the logic in _http_client.ts where contentLength is computed
(parsed.headers["content-length"], contentLength variable) to treat anything
other than a single numeric token as invalid: reject strings containing commas
or non-digit characters (e.g. validate with a strict /^\s*\d+\s*$/ check) and
handle it the same way the fetch path does (mark as parse error / set to NaN /
throw as appropriate) before starting streaming so duplicate or malformed
Content-Length values are not accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8db8f5db-eedd-48f9-87d9-c3a094f047c6
📒 Files selected for processing (1)
src/js/node/_http_client.ts
|
Verified by robobun (iteration 3, final pass): Lint JS passed on b224c85. Buildkite #41445 building; previous build #41438 on 73c37bd had only unrelated failures (worker_threads segfault aarch64, 24364.test.ts, bun-types.test.ts — none touched by this PR). Diff clean: no TODO/FIXME/HACK/XXX, only _http_client.ts and new 28450.test.ts touched. All symbols (kUpgradeOrConnect, kMaxHeaderSize, etc.) properly imported. Code verified: parseRawHTTPResponse with header overflow and malformed-status guards, doUpgradeRequest with socket binding (line 194), res.socket assignment (lines 235/264), Host header synthesis with IPv6 brackets (lines 160-166), abort wiring (lines 356-365), destroy no-op after upgrade (line 501), Content-Length framing for non-101 fallback (lines 282-323). Three regression tests all exercise the new code path and would fail on main. |
There was a problem hiding this comment.
All prior inline bugs have been addressed and the bug hunter found nothing new, but this is a large, complex addition (~400 lines) to production-critical HTTP client infrastructure — the raw socket upgrade path touches WebSocket/Playwright-core flows and required 10+ fix iterations to get right, so a human should review before merge.
Extended reasoning...
Overview
This PR adds parseRawHTTPResponse() and doUpgradeRequest() to src/js/node/_http_client.ts, routing Connection: upgrade requests through a raw net/tls socket instead of the existing fetch()-based path. Two files changed: the HTTP client implementation and a new regression test file (test/regression/issue/28450.test.ts). The new code is ~400 lines of net-new production JavaScript.
Security risks
No authentication or permission-sensitive code is touched. The new code parses raw HTTP response bytes from the network (trusted server responses), not user-controlled JS objects, so injection risks are minimal. The kMaxHeaderSize overflow guard is present. The one risk area is that a malformed or adversarial server response that triggers edge cases in parseRawHTTPResponse() could leave sockets in an unclean state, but this is bounded to the HTTP client context and is the same class of risk present in the existing llhttp parser path.
Level of scrutiny
High. _http_client.ts is used by every Node.js HTTP client call in Bun. The upgrade code path specifically affects real-world libraries (ws, playwright-core, socket.io) that large applications depend on. The PR went through 10+ fix iterations (Blob body TypeError, missing Host header, IPv6 bracketing, socket leaks, abort race conditions, destroy() killing live WebSockets, and more). While all of those are now addressed, the density of past bugs relative to the size of the change signals that this needs careful review from someone deeply familiar with the Bun HTTP client internals.
Other factors
All 33 inline review threads from prior claude[bot] and CodeRabbit rounds are resolved, and the bug hunting system raised no new issues. CI failures on the latest commits are unrelated (24364.test.ts, bun-types.test.ts). The three regression tests cover the main code paths. The history of rapid fix commits is not a red flag per se — it reflects thorough review catching real issues — but it does mean the code is less battle-tested than a simpler patch would be. A human reviewer familiar with _http_client.ts internals and the Node.js http.ClientRequest upgrade contract should give this a final pass.
|
Verified (iteration 6): Lint JS ✅, Format ✅ on b08d319. Buildkite #41505 compiling (previous #41415 passed on 8bd951a — only unrelated failures). Three regression tests exercise upgrade event (101 + echo), IncomingMessage fields, and non-101 fallback; tests 1-2 fail on main proving regression coverage. Diff clean: only _http_client.ts + new test file, no TODO/FIXME/HACK markers. kUpgradeOrConnect symbol properly defined and guarded. CodeRabbit chunked-encoding concern is a hardening nit, not blocking for the primary 101 upgrade path. |
When http.request() sends a request with Connection: Upgrade headers, route the request through a raw TCP socket instead of the fetch-based path. This allows proper handling of 101 Switching Protocols responses: - Emit 'upgrade' event with (res, socket, head) for 101 responses, providing a real duplex net.Socket for bidirectional communication - Emit 'response' event for non-101 responses on upgrade requests - Support TLS upgrade requests via tls.connect() This fixes playwright-core connectOverCDP and any library using the real ws package (not Bun's shim) for WebSocket connections via http.request() upgrade handshake. Closes #28450
…framing, Blob body, CONNECT event, Unix socket path, maxHeaderSize, fetching cleanup - Bind raw socket to self.socket and res.socket for proper socket event - Synthesize Host header when not present (HTTP/1.1 requirement) - Track Content-Length for non-101 response body framing - Resolve Blob body to ArrayBuffer before synchronous socket write - Emit 'connect' event for CONNECT method, 'upgrade' for 101 - Support Unix domain sockets via kSocketPath - Enforce maxHeaderSize limit on response header buffer - Add .finally() to reset fetching flag on upgrade path - Destroy socket on response close/end for non-101 responses
…ade sockets, forward socket errors to res
…ent-Length overflow - Check signal.aborted after async Blob resolution to prevent socket leak - Skip socket destruction in destroy() after upgrade handoff (kUpgradeOrConnect) - Suppress spurious error emit on intentionally-aborted upgrade requests - Remove abort listener after successful upgrade (socket owned by user) - Truncate response chunks to Content-Length boundary
… fix Blob body race - Remove error/close/abort listeners from socket after 101 success so user code owns the socket cleanly - Return false from parseRawHTTPResponse for malformed status lines (separator found but unparseable) and reject with HPE_INVALID_STATUS - Make doUpgradeRequest synchronous (not async) to avoid process.nextTick race with maybeEmitSocket when body is a Blob
…lesced header+body data
robobun
force-pushed
the
farm/710ad42a/fix-http-upgrade-event
branch
from
b08d319 to
1cacba9
Compare
| // Bind socket to request so req.socket and the "socket" event reference | ||
| // the actual net.Socket/tls.TLSSocket used for the connection. | ||
| self.socket = socket; | ||
|
|
||
| let responseBuffer = Buffer.alloc(0); |
There was a problem hiding this comment.
🔴 When options.lookup is provided for an HTTP upgrade request, the socket event fires with the FakeSocket placeholder instead of the real net.Socket/tls.TLSSocket. This means libraries that rely on req.on('socket', ...) to access the underlying TCP connection (e.g., happy-eyeballs, proxy/tunnel wrappers) will receive a non-functional stub when custom DNS resolution is used. To fix, reset the kEmitState.socket bit after self.socket = socket at line 202 so the maybeEmitSocket() call at line 249 can re-fire with the real socket.
Extended reasoning...
Root Cause: Event Loop Ordering Race in the options.lookup Async DNS Path
What the bug is: When http.request() is called with both an upgrade handler and options.lookup (custom DNS resolver), the socket event is emitted with FakeSocket (the placeholder inherited from OutgoingMessage) rather than the real net.Socket or tls.TLSSocket. The kEmitState.socket one-shot guard bit is set prematurely, so the later maybeEmitSocket() call in doUpgradeRequest becomes a no-op.
The specific code path:
- send() calls startFetch() which calls options.lookup(host, callback) — the DNS resolution is asynchronous; the I/O callback is queued but has not yet fired.
- send()'s finally block immediately schedules process.nextTick(maybeEmitFinish).
- The ClientRequest constructor had already scheduled process.nextTick(emitContinueAndSocketNT).
- Both nextTick callbacks fire before any I/O callbacks — this is a fundamental Node.js/Bun event loop guarantee.
- emitContinueAndSocketNT calls maybeEmitSocket(): at this point self.socket is still the FakeSocket. The socket event fires with the stub, and kEmitState.socket is permanently set.
- The DNS I/O callback finally fires, then go() calls doUpgradeRequest(socket), and self.socket = socket runs at line 202 (the real socket is now assigned).
- On receiving the 101 Switching Protocols response, maybeEmitSocket() is called at line 249 — but it is a no-op because the bit was already set in step 5.
Why existing code does not prevent it: The kEmitState.socket flag is designed as a one-shot guard to ensure the socket event fires exactly once. There is no mechanism to reset it when the socket is subsequently replaced by doUpgradeRequest. In the non-lookup (synchronous) path, go() runs immediately inside send() before the finally block queues the nextTick, so self.socket is already the real socket when maybeEmitSocket() eventually fires — the synchronous path is unaffected.
Impact: Any code that upgrades an HTTP connection (WebSocket, raw TCP tunnel, CONNECT proxy) while supplying a custom lookup function will receive a FakeSocket from the socket event. The real socket is inaccessible via req.socket or the socket event. Libraries implementing happy-eyeballs, proxy tunneling, or connection pooling with custom resolvers are silently broken: operations like socket.setTimeout(), socket.setNoDelay(), reading socket.remoteAddress, or piping the socket will either do nothing or throw on the stub.
How to fix it: After the self.socket = socket assignment at line 202 inside doUpgradeRequest, reset the kEmitState.socket bit so that the maybeEmitSocket() call at line 249 (in the 101 success branch) can re-emit with the real socket.
Step-by-step proof:
- Call http.request({ host: 'example.com', lookup: myLookup }, res => {}).on('upgrade', (res, socket, head) => { ... }).
- Inside myLookup, introduce an artificial async delay (setImmediate or setTimeout(cb, 0)).
- Add a listener: req.on('socket', s => console.log(s.constructor.name)).
- Without the fix: prints FakeSocket; s.remoteAddress is undefined; the socket event in the upgrade handler also sees the stub.
- With the fix (bit reset at line 202): prints Socket or TLSSocket; s.remoteAddress reflects the real peer address.
|
Superseded by #32204, which keeps the fetch-based transport (the native client has since gained first-class upgrade support in the Rust port: upgrade_state in src/http/lib.rs) instead of rerouting upgrade requests through a JS-parsed net.connect path. |
1 participant
Problem
http.ClientRequestnever emits theupgradeevent for 101 Switching Protocols responses. It always emitsresponseinstead. This breaks playwright-coreconnectOverCDPand any library using the realwsnpm package (not Bun's built-in shim) for WebSocket connections viahttp.request()upgrade handshake.Repro: (from #28450)
Root Cause
Bun's
ClientRequestis built on top offetch()internally. The fetch API returns a standardResponseobject with no access to the underlying TCP socket. When a 101 response arrives, the code unconditionally emitsresponseinstead ofupgrade, and cannot provide the raw duplex socket that libraries likewsneed.Fix
When
http.request()is called withConnection: Upgradeheaders, route the request through a raw TCP socket (net.connect()/tls.connect()) instead of the fetch-based path. This enables:upgradewith(res, socket, head), providing a real duplexnet.Socketfor bidirectional communicationresponsefor non-101 responses on upgrade requeststls.connect()Verification
Closes #28450
Verified by robobun (iteration 2, pass 2): Build #41437 on b0d089b in progress (Lint JS passed, Buildkite building). Previous Build #41415 on 8bd951a passed — only failures were unrelated (24364.test.ts, bun-types.test.ts). Changes since that build are targeted fixes: malformed-status-line rejection, listener cleanup after 101 upgrade, Blob body race removal. All Claude and CodeRabbit review comments addressed with code fixes and replies; CodeRabbit confirmed resolutions on both latest review rounds. Diff clean: no TODO/FIXME/HACK/XXX, only _http_client.ts and new test file touched. Three regression tests exercise upgrade event (bidirectional echo), IncomingMessage fields (crypto-verified Sec-WebSocket-Accept), and non-101 fallback (400 body). Tests 1-2 fail on main, proving regression coverage.
Verified by robobun (iteration 3): Lint JS passed. Buildkite build #41443 on 3827eb4 in progress (previous builds canceled by new pushes). Code review: all Claude-bot and CodeRabbit issues addressed — socket binding (line 194), res.socket assignment (lines 235/264), CONNECT/upgrade eventName (line 247), Blob body async handling (line 185), fetching reset via .finally() (lines 638-642), Host header with IPv6 (lines 160-166), socket destroy on unhandled upgrade (lines 249-251), cleanup via res close and socket end (lines 277-278/316-322), header overflow (lines 205-208), malformed status rejection (lines 213-217), abort wiring (lines 356-365). Remaining CodeRabbit nits (primordials, chunked TE in fallback) are style/edge-case consistent with rest of file. Diff clean — no TODO/FIXME. Three regression tests: (1) 101 upgrade with bidirectional echo, (2) IncomingMessage fields with crypto-verified Accept, (3) non-101 fallback emitting response with body.
Verified by robobun (iteration 3, final pass): Lint JS passed on b224c85. Buildkite #41445 building; previous build #41438 on 73c37bd had only unrelated failures (worker_threads segfault aarch64, 24364.test.ts, bun-types.test.ts — none touched by this PR). Diff clean: no TODO/FIXME/HACK/XXX, only _http_client.ts and new 28450.test.ts touched. All symbols properly imported. Code verified: parseRawHTTPResponse with header overflow and malformed-status guards, doUpgradeRequest with socket binding, Host header synthesis with IPv6 brackets, abort wiring, destroy no-op after upgrade, Content-Length framing for non-101 fallback. Three regression tests exercise the new code path and would fail on main.
Verified by robobun: Lint JS passed on f4532d2. Buildkite #41463 building (pipeline passed); previous builds had only unrelated failures (worker_threads segfault aarch64, 24364.test.ts, bun-types.test.ts — none touched by this PR). All 33 review threads (claude, coderabbitai) resolved. Diff clean — no TODO/FIXME/HACK/XXX, only _http_client.ts and new 28450.test.ts touched. All symbols properly imported. Three regression tests: (1) 101 upgrade with bidirectional echo, (2) IncomingMessage fields with crypto-verified Accept, (3) non-101 fallback emitting response with body — tests 1-2 would fail on main, proving coverage.
Verified by robobun (iteration 4): Lint JS passed on 568b566. Buildkite #41479 building (pipeline passed); previous Build #41463 on f4532d2 had only unrelated failures (24364.test.ts, bun-types.test.ts — neither touched by this PR). Latest commit adds socketCloseListener kUpgradeOrConnect guard, extracts onAbortCleanup for proper removal after 101, and adds stderr assertions to all three tests. All claude-bot and coderabbitai review issues resolved. Diff clean — no TODO/FIXME/HACK/XXX, only _http_client.ts and new 28450.test.ts touched. Three regression tests exercise upgrade event (bidirectional echo), IncomingMessage fields (crypto-verified Accept), and non-101 fallback (400 body) — tests 1-2 would fail on main.