Skip to content

node:http: do not give a request the socket of a failed connection - #44191

Merged
Jarred-Sumner merged 6 commits into
mainfrom
robobun/ec7ddbdc/https-proxy-tunnel-error
Sep 30, 2026
Merged

Jarred-Sumner merged 6 commits into
mainfrom
robobun/ec7ddbdc/https-proxy-tunnel-error

Conversation

@robobun

@robobun robobun commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • An https request through a proxy emits only 'close' when the connection ends between the 200 of 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#createSocket gives the request the failed socket with the error (src/js/node/_http_agent.ts:354, :530). The TLS layer set _hadError on it, so the guard at src/js/node/_http_client.ts:1076 skips the 'error'.

Fix

  • Agent#createSocket passes only the error, as Node does. https.Agent#createConnection closes the proxy connection after every ERR_PROXY_TUNNEL, which the request did before. _http_client.ts does not change.
  • Correct: a request gets no socket that it did not listen to. 13 of 13 failure routes equal Node v26.3.0 in events, code and tick (base: 0).
  • Verified: 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.
  • Self-reviewed: 8 concerns raised, 8 addressed.

Background

Downsides

Notes

Reproduction

// bun repro.mjs  /  node repro.mjs
import net from "node:net";
import https from "node:https";
const proxy = net.createServer(s => {
  s.on("error", () => {});
  s.once("data", () => {
    s.write("HTTP/1.1 200 Connection established\r\n\r\n");
    s.once("data", () => s.end()); // the ClientHello
  });
});
await new Promise(r => proxy.listen(0, "127.0.0.1", r));
const agent = new https.Agent({ proxyEnv: { https_proxy: `http://127.0.0.1:${proxy.address().port}` } });
const req = https.get({ host: "example.invalid", port: 443, path: "/", agent });
req.on("error", e => console.log("error:", e.code, e.message));
req.on("close", () => {
  console.log("close");
  proxy.close();
});
node v26.3.0, and bun with this PR:
error: ECONNRESET Client network socket disconnected before secure TLS connection was established
close

bun 1.4.0, 1.4.1, 1.4.2, main 1313ca61d4 (3 of 3 runs each, linux-x64):
close

The proxy ends its side with end() after it read the ClientHello. A destroy() 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

  • The built-in proxy support of node:https: new https.Agent({ proxyEnv }), the global agent under NODE_USE_ENV_PROXY=1, and http.setGlobalProxyFromEnv().
  • An http:// proxy connection that ends with the 200 or after it, and an https:// proxy connection that ends during its own TLS handshake or after its 200.
  • A request that waited behind maxSockets.
  • No proxy: a custom Agent#createConnection that calls back with (err, socket). With a second argument that has no destroy() the process ended with TypeError: 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, tunnel and a direct https request, where the request writes to the TLS socket before the handshake is done. They report ERR_SOCKET_CLOSED where Node reports ECONNRESET, 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 cleanupAndPropagate for 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 in cleanupAndPropagate. 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 createSocket calls 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.ts and _http_client.ts are the same at 1313ca6 and at the base of this branch. Every count is the same in 5 of 5 runs.

  • builtin JS text: _http_agent.js -13 B, https.js -33 B, _http_client.js 0 B (wc -c, cmp: identical). bun binary text+data -46 B (size: text 80662588 to 80662542, data 110424).
  • success path: 0 changed bytecode instructions. onSocketReady 47, onSocketCreatedForPending 45, cleanupAndPropagateImpl 36 instructions (base 48/47/38, JSC bytecode dump).
  • successful request, nextTick entries / listener registrations on the socket / bind calls, 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. createSocket reported an error 0 of 800 times.
  • failed creation, nextTick entries / 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).
  • 13 of 13 failure routes equal Node v26.3.0 in events, error code and nextTick turn (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.
  • held non-200 CONNECT: the proxy connection is closed for 7 of 7 callers (node 0, base 2) and for 5 of 5 status lines without a code. A direct createConnection caller sees socket.destroyed === true in its callback (node false, base false).
  • custom createConnection cb(err, x): 16 of 16 rows equal Node (base 0 of 16), destroy() is called on x 0 times (node 0, base 8), 0 rows end in an uncaught exception (base 10). The callback of agent.createSocket() gets 1 argument after a failure (node 1, base 2).

The 13 routes: FIN after the ClientHello, 200 with the FIN, https:// proxy FIN during its handshake, https:// proxy 200 then FIN, held 407, 500, RST after the 200, bytes that are not TLS after the 200, certificate error, end before the 200, tunnel timeout, request queued behind maxSockets, global agent with NODE_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, the createConnection option of a request, agent.createConnection() with a callback and without one.

Behavior that changes besides the fix

  • 'close' of a request comes one nextTick turn earlier after every failed socket creation, on the turn where Node emits it.
  • The request does not call destroy() on what a custom createConnection passes with an error.
  • A subclass that overrides createSocket and calls back with (err, socket) for a request that waited behind maxSockets: the request emits 'error', then 'close', and does not call destroy() 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.
  • After a CONNECT reply with a status line that has no usable code (for example HTTP/1.1 abc), a direct caller of createConnection gets 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 overrides createSocket and calls back with (err, socket) for a request that did not wait: only 'close', also in Node.
  • The createConnection option of a request that reports an error through its callback: 'error' and no 'close', also in Node.
  • A tunnel that is still pending: while the proxy does not answer the CONNECT, req.destroy(err), req.abort(), an aborted signal and agent.destroy() emit no 'error' and no 'close', and the proxy connection stays open. When the proxy closes, the request reports ERR_PROXY_TUNNEL. Node v26.3.0 does the same.
  • Two changes of Node's tunnel code after v26.3.0 are not ported here: the limit on the headers of the CONNECT reply (nodejs/node 84e367579e) and the handling of TLS options that tls.connect() rejects (https: handle invalid TLS options in proxied requests nodejs/node#66096). Both are separate work.
  • Error messages of the TLS library differ from Node's (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.mts runs 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 the nextTick turn of 'error' and 'close'.
  • test/js/node/http/node-http-proxy-url.test.ts: the global agent with NODE_USE_ENV_PROXY=1, in a child process.
  • linux-x64, debug build. With the fix: 32 of 32 in the dual file (Node v26.3.0: 32 of 32) and 4 of 4 in the wrapper. Without the fix: the 22 and the 1 fail.
  • Each source edit alone, reverted on a debug build: _http_agent.ts:350 17 tests fail, :524 1 test fails, https.ts 5 tests fail. With req.onSocket one nextTick late, the 6 turn assertions fail.
  • CI build 121492 at 6c55f8a ran node-http-proxy-url.test.ts and node-http.test.ts on 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.
  • Windows x64, without the fix: the canary a7c73fd prints only close for the reproduction.
  • Unchanged and passing on linux-x64: 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 of module-graph-io.test.ts.
  • Vendored Node tests: 100 of 101 pass (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.js fails 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

…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.
@robobun

robobun commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

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

  • test/js/node/http/node-http-proxy-url.test.ts and test/js/node/http/node-http.test.ts pass on every lane at the first attempt: Linux glibc and musl, macOS and Windows, each on x64 and arm64, and the ASAN lane. The Node v26.3.0 run of the new tests reports 32 of 32 on each lane.
  • The one failed job is debian 13 x64-asan, with test/js/bun/spawn/spawn.test.ts ("an idle reader stopped at the highwater mark does not keep the process alive"). The child prints a LeakSanitizer warning about ptrace to stderr, and the test expects an empty stderr. This PR changes src/js/node/_http_agent.ts and src/js/node/https.ts only.
  • The same test fails, or passes only after retries, on that lane in the builds of other branches. Examples where it failed: 121486, 121487, 121474, 121464.

How to reproduce the bug

Run the script under "Reproduction" in the Notes of the description with bun repro.mjs and with node repro.mjs. A local proxy answers the CONNECT with 200 and ends the connection when the ClientHello arrives.

  • Node v26.3.0 and this branch print error: ECONNRESET Client network socket disconnected before secure TLS connection was established, then close.
  • Bun 1.4.0, 1.4.1, 1.4.2 and main print only close (3 of 3 runs each, linux-x64).

The tests in test/js/node/http/node-http-proxy-url.test.ts fail without the source change (22 in the file that also runs under Node, 1 in the wrapper) and pass with it.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 5e852a4b-5093-4fc9-84ef-e41911e56e10

📥 Commits

Reviewing files that changed from the base of the PR and between d5e6c4c and 6c55f8a.

📒 Files selected for processing (1)
  • test/js/node/http/node-http-proxy-url.node.mts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


Walkthrough

Socket-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.

Changes

Proxy and socket error handling

Layer / File(s) Summary
Socket-creation error callbacks
src/js/node/_http_agent.ts, test/js/node/http/node-http-proxy-url.node.mts
Socket-creation failures reach requests without forwarding a leftover socket value. Tests cover callback arguments and queued requests.
Proxy tunnel failure and cleanup
src/js/node/https.ts, test/js/node/http/node-http-proxy-url.node.mts, test/js/node/http/node-http-proxy-url.test.ts, test/js/node/http/node-http.test.ts
Proxy-tunnel errors destroy the proxy socket. Tests cover early proxy closure, rejected CONNECT requests, diagnostics, and error and close events. The updated comment describes connection cleanup behavior.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6c55f

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)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: preventing requests from receiving sockets from failed connections.
Description check ✅ Passed The description provides detailed problem context, implementation changes, verification results, tests, limitations, and behavior changes. It does not use the template headings exactly, but it fully c…

Comment @coderabbitai help to get the list of available commands.

Comment thread test/js/node/http/node-http-proxy-url.node.mts
Comment thread test/js/node/http/node-http-proxy-url.node.mts
Comment thread src/js/node/_http_agent.ts
Comment thread src/js/node/https.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/node/http/node-http-proxy-url.node.mts
@robobun

robobun commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:20 PM PT - Sep 28th, 2026

❌ @robobun, your commit 6c55f8a has 1 failures in Build #121492 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 44191

That installs a local version of the PR into your bun-44191 executable, so you can run:

bun-44191 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@Jarred-Sumner
Jarred-Sumner merged commit e70cca7 into main Sep 30, 2026
4 of 5 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the robobun/ec7ddbdc/https-proxy-tunnel-error branch September 30, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants