Repository navigation
node:http: do not give a request the socket of a failed connection - #44191
Conversation
…ateConnection() A proxy that answers the CONNECT with a status that is not 200 can hold the connection open. createConnection() left that connection to the request. Close it in the code that opened it, also when the status line has no usable code.
createSocket() gave its callback the failed socket together with the error. A request then got a socket that it never listened to. When the TLS layer had set _hadError on that socket (the peer ended the connection during the handshake), the request skipped its 'error' event and emitted only 'close'. Pass only the error, like Node.js. The request emits 'error' and then 'close' on the next tick, and leaves what a custom createConnection() passes as its second argument alone.
…ued createSocket() callback The request emits 'error' and 'close' on the process.nextTick turn where Node.js emits them. A createSocket() override that calls back with an error and a socket, for a request that waited behind maxSockets, reaches the request as the error only.
|
Status: ready for review. The tests of this PR pass on every CI lane. The build is red because of one test that this PR does not touch. CI, build 121492 at 6c55f8a
How to reproduce the bug Run the script under "Reproduction" in the Notes of the description with
The tests in |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. WalkthroughSocket-creation failures no longer pass leftover socket values to requests. Proxy-tunnel errors now destroy the proxy socket, including when the error has a status code. Tests cover proxy disconnects, rejected CONNECT requests, callback arguments, diagnostics, and request event ordering. ChangesProxy and socket error handling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk was identified in the reviewed changes. The proxy error and cleanup behavior is covered by the added tests. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:20 PM PT - Sep 28th, 2026
❌ @robobun, your commit 6c55f8a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44191That installs a local version of the PR into your bun-44191 --bun |
Problem
httpsrequest through a proxy emits only'close'when the connection ends between the200of the CONNECT and the end of the TLS handshake. Node emits'error'first:ECONNRESET,Client network socket disconnected before secure TLS connection was established.Agent#createSocketgives the request the failed socket with the error (src/js/node/_http_agent.ts:354,:530). The TLS layer set_hadErroron it, so the guard atsrc/js/node/_http_client.ts:1076skips the'error'.Fix
Agent#createSocketpasses only the error, as Node does.https.Agent#createConnectioncloses the proxy connection after everyERR_PROXY_TUNNEL, which the request did before._http_client.tsdoes not change.test/js/node/http/node-http-proxy-url.test.ts: 23 tests fail without the fix, 22 also under Node. Also 100 vendored Node tests.Background
createConnection, which this PR takes.createConnectioncallback(err, x)stays broken.Downsides
https.Agent#createConnectioncaller gets a destroyed socket. Node leaves it open.agent.createSocket()calls back with(err)only. A request with no'error'listener throws thisECONNRESET.https-proxy-agent,tunneland a directhttpsrequest still reportERR_SOCKET_CLOSED(node:net, node:tls: socket teardown events that still differ from Node (spurious 'end', ERR_SOCKET_CLOSED in place of the real error) #43381, item 4).Notes
Reproduction
The proxy ends its side with
end()after it read the ClientHello. Adestroy()with unread bytes is a reset, which takes another path through the client and was reported before. The proxy does not have to be at fault: a proxy that relays the close of a target that hangs up during the handshake gives the same result.The report also says that the process never exits. No handle is left open: both sockets are closed and
process.getActiveResourcesInfo()is empty. The awaited promise never settles, and Bun does not exit on an unsettled top-level await (#33283).Who hits it
node:https:new https.Agent({ proxyEnv }), the global agent underNODE_USE_ENV_PROXY=1, andhttp.setGlobalProxyFromEnv().http://proxy connection that ends with the200or after it, and anhttps://proxy connection that ends during its own TLS handshake or after its200.maxSockets.Agent#createConnectionthat calls back with(err, socket). With a second argument that has nodestroy()the process ended withTypeError: socket.destroy is not a function. The request destroyed a live socket that it was given this way. Node leaves the second argument alone.Not this bug:
https-proxy-agent,tunneland a directhttpsrequest, where the request writes to the TLS socket before the handshake is done. They reportERR_SOCKET_CLOSEDwhere Node reportsECONNRESET, before and after this PR. That is item 4 of #43381, and #43392 is the open PR for it.The earlier decision in #31587
The review thread of #31587 found that a proxy which refuses the CONNECT and holds the connection left that connection open, as in Node. It named two ways: forward the socket from the Agent to the request (taken, d677423), or close the connection in
cleanupAndPropagatefor every error (set aside as further from upstream). The forwarding applies to every failed socket creation, not only to a refused CONNECT. So the request also gets the sockets that the TLS layer already marked, and loses its'error'. With this PR the Agent is Node's text again, and the one line that differs from Node is the condition incleanupAndPropagate. The test that #31587 added for the held connection passes with no change to its assertions.Node has the same guard since nodejs/node#61770. With an agent whose
createSocketcalls back with(err, socket)and a socket that the peer ended during the TLS handshake, Node v26.3.0 also emits only'close'. So the forwarding cannot go upstream as it is. Node v26.3.0 leaves the held connection open.Measurements
Release builds of 1313ca6 and of the three source expressions of this PR on it, linux-x64, and Node v26.3.0.
_http_agent.ts,https.tsand_http_client.tsare the same at 1313ca6 and at the base of this branch. Every count is the same in 5 of 5 runs._http_agent.js-13 B,https.js-33 B,_http_client.js0 B (wc -c,cmp: identical). bun binary text+data -46 B (size: text 80662588 to 80662542, data 110424).onSocketReady47,onSocketCreatedForPending45,cleanupAndPropagateImpl36 instructions (base 48/47/38, JSC bytecode dump).nextTickentries / listener registrations on the socket /bindcalls, from the moment the Agent has the socket to the'socket'event: http 2/9/1, keep-alive 1/5/1, https 2/10/1, tunnel 2/9/0. Base and PR are identical.createSocketreported an error 0 of 800 times.nextTickentries / listener registrations on the failed socket, from the failure callback to the'close'of the request: FIN 1/0 (base 2/1, node 1/0), held 407 1/0 (base 3/1, node 1/1).nextTickturn (base 0 of 13). FIN routes:'error'and'close'at turn 1/1 (node 1/1, base no'error','close'at turn 2). The message equals Node's on 11 of 13. The other 2 are texts of the TLS library, the same before and after.createConnectioncaller seessocket.destroyed === truein its callback (nodefalse, basefalse).createConnectioncb(err, x): 16 of 16 rows equal Node (base 0 of 16),destroy()is called onx0 times (node 0, base 8), 0 rows end in an uncaught exception (base 10). The callback ofagent.createSocket()gets 1 argument after a failure (node 1, base 2).The 13 routes: FIN after the ClientHello,
200with the FIN,https://proxy FIN during its handshake,https://proxy200then FIN, held 407, 500, RST after the200, bytes that are not TLS after the200, certificate error, end before the200, tunnel timeout, request queued behindmaxSockets, global agent withNODE_USE_ENV_PROXY=1.The 7 callers: a request through the agent, a request queued behind
maxSockets,agent.createSocket(), a subclass that passes on only the error, thecreateConnectionoption of a request,agent.createConnection()with a callback and without one.Behavior that changes besides the fix
'close'of a request comes onenextTickturn earlier after every failed socket creation, on the turn where Node emits it.destroy()on what a customcreateConnectionpasses with an error.createSocketand calls back with(err, socket)for a request that waited behindmaxSockets: the request emits'error', then'close', and does not calldestroy()on that socket. Before, it emitted only'close'when the socket had_hadError, and destroyed the socket. The subclass owns that socket now, as in Node.HTTP/1.1 abc), a direct caller ofcreateConnectiongets a destroyed socket, as in Node.Left as it is
req.onSocket(socket, err)called directly with a socket that has_hadError, and a subclass that overridescreateSocketand calls back with(err, socket)for a request that did not wait: only'close', also in Node.createConnectionoption of a request that reports an error through its callback:'error'and no'close', also in Node.req.destroy(err),req.abort(), an aborted signal andagent.destroy()emit no'error'and no'close', and the proxy connection stays open. When the proxy closes, the request reportsERR_PROXY_TUNNEL. Node v26.3.0 does the same.tls.connect()rejects (https: handle invalid TLS options in proxied requests nodejs/node#66096). Both are separate work.WRONG_VERSION_NUMBER,self signed certificate).agent.sockets[name]is deleted after a failed creation. Node keeps an empty array.Tests
test/js/node/http/node-http-proxy-url.node.mtsruns under Node and under Bun. 22 new tests: 4 ways a proxy connection ends, a destroyed and an aborted request,new http.ClientRequest,http.setGlobalProxyFromEnv(),diagnostics_channel, a queued request, 5 tests of a refused CONNECT, 7 of a creation callback with an error and a second argument. 6 of them also assert thenextTickturn of'error'and'close'.test/js/node/http/node-http-proxy-url.test.ts: the global agent withNODE_USE_ENV_PROXY=1, in a child process._http_agent.ts:35017 tests fail,:5241 test fails,https.ts5 tests fail. Withreq.onSocketonenextTicklate, the 6 turn assertions fail.node-http-proxy-url.test.tsandnode-http.test.tson every lane, and both pass at the first attempt: Linux glibc and musl, macOS and Windows, each on x64 and arm64, and the ASAN lane. The Node run reports 32 of 32 on each lane.closefor the reproduction.node-http.test.ts(263 pass),node-https-agent-checkserveridentity-reuse.test.ts(30),node-http-agent-free-socket.test.ts(9),node-http-client-request-gc.test.ts,trace-events.test.ts, the two Agent tests ofmodule-graph-io.test.ts.test-https-proxy-request-*,test-http-proxy-request-*,test-http-agent*,test-https-agent*,test-http-client-abort*,test-http-set-global-proxy-from-env-*,test-tls-over-http-tunnel, and more).test-http-agent-keepalive.jsfails on a debug build with and without this change.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http.test.ts