Conversation
For wss:// through an HTTP proxy the WebSocket client's own socket handle is detached: the connection to the proxy belongs to the upgrade client, which stays alive after the upgrade to feed the socket into the TLS tunnel, and only closes it when the peer does. So every teardown that the client initiates itself (ws.terminate(), failing the connection on a protocol error, finalization, context stop) dispatched the close event but left the proxy connection open, together with the upgrade client and its event loop keep-alive, until the proxy or the server dropped it. Give the tunnel a close_socket() that closes the proxy connection, and call it from cancel()/finalize()/drop_connection_without_callback() where the direct-connection path closes tcp. The upgrade client takes its socket handle out of the tunnel when it tears down, so a close arriving after the peer already closed the connection is a no-op. The close runs the upgrade client's handle_close synchronously, so the paths that can now free the upgrade client and the tunnel from inside their own callbacks (handle_data, handle_writable, tunnel write) hold refs across the re-entrant call.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesThe WebSocket client now retains proxy tunnels during cleanup and closes proxy sockets explicitly. Proxy tunnel writes and upgrade handling preserve object lifetimes across re-entrant callbacks. Tests cover HTTP and HTTPS termination, protocol errors, socket closure, and subprocess exit behavior. Proxy tunnel teardown
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e connection never opens
|
Updated 5:32 AM PT - Aug 11th, 2026
✅ @robobun, your commit ada3ee906dd8372c849e6ee6ab353195519242d9 passed in 🧪 To try this PR locally: bunx bun-pr 37487That installs a local version of the PR into your bun-37487 --bun |
There was a problem hiding this comment.
I re-reviewed after ada3ee9 and didn't find any bugs; the earlier nit about wiring onerror to reject the awaited opened promise is addressed. Because this change threads a new synchronous close path through the tunnel/upgrade-client refcount graph (re-entrant handle_close freeing objects mid-call, with ScopedRef guards as the only thing keeping them alive), a human pass over the lifetime reasoning would still be worthwhile.
What was reviewed:
retain_tunnel()/RetainedTunnel— the tunnel ref is taken beforeclear_data()releases the struct's own ref, andclose_socketruns after the tunnel is detached from the WebSocket, soon_close → fail → cancelre-entry seesproxy_tunnel = None.close_sockettaking the socket viaptr::replacebefore closing, anddetach_upgrade_clientnow nullingsockettoo, so the peer-initiated close path leavesclose_socketa no-op.- New
ScopedRefguards inWebSocketProxyTunnel::write,handle_data(Done state) andhandle_writable— checked that each guards the specific ref that the re-entrant close can now drop. - Tests: the
connect()helper wires error/close to rejectopened; the subprocess test asserts"open\n"before the close line so a pre-open failure can't pass.
Extended reasoning...
Overview
The PR fixes a socket leak in tunnel mode (wss:// through an HTTP CONNECT proxy): client-initiated teardown (terminate(), protocol errors via cancel(), finalize(), drop_connection_without_callback()) previously shut down the inner TLS session but never closed the proxy TCP connection, because the WebSocket's own tcp is detached in tunnel mode and the socket belongs to the still-alive upgrade client. The fix adds WebSocketProxyTunnel::close_socket() and calls it from the three teardown paths, holding a scoped ref on the tunnel across clear_data(). Because closing the socket synchronously runs the upgrade client's handle_close — which drops the socket ref that keeps the upgrade client alive in the Done state and drops its tunnel ref — the frames that can now be on the stack when this happens (handle_data Done arm, handle_writable, WebSocketProxyTunnel::write) each gain a ScopedRef guard.
Files: src/http_jsc/websocket_client.rs (+~45/-~20), WebSocketProxyTunnel.rs (+~35/-~5), WebSocketUpgradeClient.rs (~35 lines reshuffled in handle_writable + a guard in handle_data), and ~170 lines of new tests.
Security risks
None identified. This is teardown of an outbound client connection; no new input parsing, no trust decisions. The TLS-hop arm uses FastShutdown (close_notify without waiting for the peer's), which is the appropriate behaviour for an abrupt teardown and matches the direct path.
Level of scrutiny
High. This is native code in the most-blocked review category (memory safety): intrusive refcounts, re-entrant callbacks that can free self, and unsafe raw-pointer projections whose soundness rests on the aliasing/lifetime comments. The PR description is very thorough about which refs each call can drop and why each guard is placed where it is, and the ASAN test run and the re-entrancy test cases (invalid frame delivered as tunnel data, so cancel runs from inside the upgrade client's own handle_data) give good coverage — but the correctness argument is subtle enough that a maintainer familiar with this refcount graph should confirm it.
Other factors
- The comment-cop bot flagged the multi-line comments; the author shortened them and what remains reads as ownership/re-entrancy documentation rather than workaround justification.
- My earlier nit about the tests'
onerror = () => {}leavingawait openedwith no rejection path was addressed in ada3ee9 via the sharedconnect()helper. - Test coverage looks solid: both terminate timings (in-onopen and after), HTTPS-proxy hop, both delivery paths for the protocol-error case, and a spawned hang-guard test that asserts the process exits on its own with the expected stdout.
…s in the proxy tests (#40624) ### Problem - `websocket-proxy.test.ts` and its twin `test/js/first_party/ws/ws-proxy.test.ts` check outcomes loosely: `toContain` on the message list, "some error fired" on the failure paths, no check that a proxy header reached the proxy, no `expect()` in the ping regression test, and NO_PROXY children that exit 0 or 1. - No test checks that the proxy was used. With this container's ambient `NO_PROXY=localhost,127.0.0.1,...`, 16 of the 19 twin tests pass with the proxy bypassed and the other 3 fail. - The file was reported as a 163 s test on darwin aarch64 (build 106656). The runner log shows 199 ms there, and 117 to 208 ms across the last five main builds. No speed change is needed. ### Fix - `proxy-test-utils.ts` exposes the CONNECT request it already parses through an `onConnectRequest` hook, folds the TLS proxy into `createConnectProxy({ tls: true })`, and exports `startRecordingProxy` (port, requests, connection count), `connectRequest`, `startEchoServer` and the client event helpers. - Every tunneling test starts its own recording proxy and asserts the CONNECT request (target, headers, `Proxy-Authorization`) next to one exact event list: `["connected", message, { code: 1000, reason: "", wasClean: true }]`. Failure paths assert the error message and close event (1006 "Proxy connection failed", 1015 "TLS handshake failed") and close an unexpected open at once. - The NO_PROXY children print their events. The test compares stdout, stderr, exit code and the proxy's connection count (0 bypassed, 1 used). The twin clears the ambient NO_PROXY, and "explicit proxy wins" checks that the agent's proxy saw no connection. - Verified: `bun bd test` on both files and the two other importers of the helper, 57 pass. Release: 0.27 s for both files (0.22 s + 0.17 s before). Every new assertion fails under a mutation of its input (details below). ### Background - `createConnectProxy` is a Node `net` (or `tls`) server that parses one CONNECT request, answers 407, 403 or 200, and pipes the client to the target. The hook runs after the parse and before the answer. - NO_PROXY also applies to an explicit `proxy:` option, and bun reads `no_proxy` before `NO_PROXY` (`src/dotenv/env_loader.rs:374`). #37439 moves the clearing into the harness. This PR keeps the module-level clearing in both files until it lands. <details><summary>Notes</summary> **The 163 s figure.** Buildkite build 106656, job `:darwin: any aarch64 - test-bun` (01a04234-72d1): the runner log has the file at `[299/3162]`, `Ran 38 tests across 1 file. [199.00ms]`, and the Buildkite timestamps around it span 206 ms. The other darwin aarch64 job of that build did not run the file. Darwin x64 in the same build: 221 ms. Darwin aarch64 in the last five main builds: 106385 185 ms, 106512 208 ms, 106596 119 ms, 106656 199 ms, 106692 117 ms. The per-test sum of the file is about 0.16 s, so the reported number is most likely the junit suite time read in the wrong unit. **Local timing (this container).** Release (`USE_SYSTEM_BUN=1 bun test`): websocket-proxy 0.24 to 0.39 s before, 0.21 to 0.28 s after; ws-proxy 0.16 to 0.18 s before; both files together 0.26 to 0.28 s after (5 runs). Debug (`bun bd test`): websocket-proxy 6.1 to 6.4 s before, 6.0 s after; ws-proxy 5.3 s after; both together with the CI env (`BUN_GARBAGE_COLLECTOR_LEVEL=1 BUN_JSC_randomIntegrityAuditRate=1.0`) 8.5 s, 3 runs. About 2.7 s of the debug time of websocket-proxy is module load (`https-proxy-agent`, `ws`, `docker/index.ts`) plus `beforeAll`. The slowest tests are the four NO_PROXY children (0.3 s each in debug, they already run concurrently). Nothing else is converted to `concurrent`: the TLS tunnel regression tests depend on event loop ordering (the `setImmediate` spin and the held burst), and there is nothing to gain on a 0.2 s file. **Mutations checked against the new assertions** (each run on a copy of the file with the release binary, all fail in under 15 ms): - client sends no custom header: `ws:// through proxy with custom headers` fails on the recorded CONNECT request. - `NO_PROXY=other.host.com` in a "direct" case: fails on stdout and `proxyConnections: 1`. - ambient `no_proxy=127.0.0.1` in the "proxied" case: fails on stdout and `proxyConnections: 0`. - server never pings: `pongs: 0`. - right credentials in `proxy wrong credentials returns error`, the CA given in `fails without CA certificate` (both files): fail with the open and clean close in the event list. Before this change the same mutation hung until the 5 s timeout. - twin, `explicit proxy option takes precedence over agent` with the explicit option pointing at the agent's proxy: fails on `explicitRequests: []`. - twin, process-level `no_proxy=127.0.0.1` left in place: 13 of 19 tests fail on `requests: []` (before this change all 16 through-proxy tests passed that way). **Scope.** Three test files, no runtime change. `createTLSConnectProxy` is gone: its only callers were the two rewritten files, and `createConnectProxy({ tls: true })` replaces it. The Docker Squid section of websocket-proxy.test.ts is untouched (it cannot run here). The constructor checks in both `proxy API` sections now use port 1 (nothing needs to listen: `close()` follows at once) instead of the shared fixtures, which are gone. `tsc -p test/tsconfig.json` reports the same classes of errors on these files before and after (the test tsconfig resolves the DOM `WebSocket` constructor and `@types/ws`, so every `proxy:` option already errors on main; the helper's `socket.on("data")` Buffer typing is unchanged). **Review.** A self-review of the first revision found the recorder in the wrong layer (a second CONNECT parser in the test file, unusable by the twin and by the TLS proxy) and three sessions still on the unrecorded shared proxy. This revision moves the recording into the helper, migrates those sessions and the HTTPS proxy tests, and applies the same assertions to the twin. **Open PRs that touch these files:** #37439 (proxy env isolation, edits the NO_PROXY env code in both files), #37638 (adds tests after `rejects invalid proxy URL`), #37504 and #37487 (add tests after the ping test in websocket-proxy.test.ts). The NO_PROXY block and the ping test are rewritten here, so those PRs need a small rebase. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 3 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/first_party/ws/ws-proxy.test.ts' 'test/js/web/websocket/websocket-proxy.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/first_party/ws/ws-proxy.test.ts test/js/web/websocket/websocket-proxy.test.ts bun test v1.4.1 (731aa92) test/js/first_party/ws/ws-proxy.test.ts: (pass) ws package proxy API > accepts proxy option as string (HTTP proxy) [30.17ms] (pass) ws package proxy API > accepts proxy option as string (HTTPS proxy) [4.89ms] (pass) ws package proxy API > accepts proxy option with object containing url [2.64ms] (pass) ws package proxy API > accepts proxy URL with credentials [3.22ms] (pass) ws package proxy API > can combine proxy with headers and protocols [4.25ms] (pass) ws package proxy API > rejects invalid proxy URL [4.66ms] (pass) ws package through HTTP CONNECT proxy > ws:// through HTTP proxy [493.11ms] (pass) ws package through HTTP CONNECT proxy > ws:// through HTTP proxy with auth [192.90ms] (pass) ws package through HTTP CONNECT proxy > proxy auth failure returns error [102.68ms] (pass) ws package through HTTP CONNECT proxy > proxy wrong credentials returns error [85.41ms] (pass) ws package wss:// through HTTP proxy (TLS tunnel) > wss:// through HTTP proxy [113.43ms] (pass) ws package through HTTPS proxy (TLS proxy) > ws:// through HTTPS proxy with CA certificate [246.55ms] (pass) ws package through HTTPS proxy (TLS proxy) > ws:// through HTTPS proxy with rejectUnauthorized: false [130.83ms] (pass) ws package through HTTPS proxy (TLS proxy) > ws:// through HTTPS proxy fails without CA certificate [92.43ms] (pass) ws package with HttpsProxyAgent > ws:// through HttpsProxyAgent [133.63ms] (pass) ws package with HttpsProxyAgent > wss:// through HttpsProxyAgent with rejectUnauthorized [111.82ms] (pass) ws package with HttpsProxyAgent > HttpsProxyAgent with authentication [101.82ms] (pass) ws package with HttpsProxyAgent > HttpsProxyAgent with agent.proxy as URL object [116.92ms] (pass) ws package with HttpsProxyAgent > explicit proxy option takes precedence over agent [113 ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/first_party/ws/ws-proxy.test.ts | 606 +++++++----------------- test/js/web/websocket/proxy-test-utils.ts | 210 ++++++--- test/js/web/websocket/websocket-proxy.test.ts | 641 ++++++++------------------ 3 files changed, 494 insertions(+), 963 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/first_party/ws/ws-proxy.test.ts 1 1 0 test/js/web/websocket/proxy-test-utils.ts 2 3 0 test/js/web/websocket/websocket-proxy.test.ts 7 11 0 ``` </details> <!-- robobun:evidence:end -->
|
This bug still reproduces on 1.4.3-canary.1+09bb54630 (main as of 2026-09-13). The rewrites of the WebSocket client since the last push here did not fix it. The repro opens 20
On main at a8e4e90, This branch conflicts with main. #40002, #40018 and #40055 rewrote the three source files around |
This branch has not been deployed
2 participants
What
For a
wss://WebSocket connected through an HTTP CONNECT proxy (new WebSocket(url, { proxy }), i.e. tunnel mode), a teardown initiated by the client itself never closed the connection to the proxy.ws.terminate()and every failure that goes throughWebSocket::cancel()(for example a protocol error in a received frame) firedclose(1006 / 1002) in JS, but the TCP connection to the proxy stayed open until the proxy or the server dropped it, and so did the upgrade client behind it, whose keep-alive holds the event loop:Direct
wss://andws://through a proxy close the socket immediately. (Callingterminate()synchronously insideonopenhappened to work, because at that point the tunnel still holds its backref to the upgrade client, which closes the socket when the tunnel is shut down; anything after the open event leaked.)Why
In tunnel mode the
WebSocketstruct'stcpis detached: the socket to the proxy belongs to theWebSocketUpgradeClient, which stays alive in its done state purely to pump socket bytes into theWebSocketProxyTunnel, and it only closes that socket when the peer does (handle_close/handle_end).WebSocket::cancel()shut the tunnel's TLS session down (a fast shutdown, which performs no I/O) and then closedtcp, which is a no-op on a detached handle. Nothing closed the proxy socket, andWebSocketProxyTunnelhad no close path at all.finalize()anddrop_connection_without_callback()skipped their close the same way (is_closed()is true for a detached handle).The clean
ws.close()path is deliberately unchanged: after the close frame is flushed it leaves the connection for the server to close, which is what the directwss://path does too, and the lingering upgrade client is what flushes an encrypted close frame that is still buffered under backpressure.How
WebSocketProxyTunnel::close_socket()closes the proxy connection.WebSocket::cancel(),finalize()anddrop_connection_without_callback()call it where the direct path closestcp, afterclear_data()has detached the tunnel from the WebSocket (so the upgrade client's teardown, which shuts the tunnel down again, cannot reach back into the WebSocket), holding their own ref on the tunnel across it sinceclear_data()releases the struct's ref. A plain hop to the proxy is reset like the!SSLarm oftcp.close(); a TLS hop (https proxy) gets a close_notify but is closed without waiting for the peer's reply, becausehandle_closeis the only thing that releases the upgrade client.detach_upgrade_client), soclose_socket()is a no-op when the connection was closed from the peer side first (the existinghandle_close -> tunnel.shutdown -> ws.fail -> cancelchain), or in theterminate()-inside-onopenwindow described above.handle_closesynchronously, which releases the socket ref that is all that keeps the upgrade client alive in the done state and drops its ref on the tunnel. The frames this can now happen inside of hold refs across the re-entrant call: the upgrade client'shandle_data(done state) andhandle_writable, which previously readthis.stateafter draining the tunnel, andWebSocketProxyTunnel::write, whose&SslWrapperlives in the tunnel allocation.Verification
New tests in
test/js/web/websocket/websocket-proxy.test.tsobserve the close from the proxy's end of the connection:ws.terminate()after the open event (fails before: times out waiting for the proxy socket to close), and synchronously insideonopen(the window above, passes before and after).ws.terminate()through an HTTPS proxy (the TLS hop arm ofclose_socket).terminate()(spawnedbun -e; hangs before).Debug (ASAN) build: the four new cases fail on the unpatched build by timing out and pass with the patch; the new tunnel tests pass 10/10 in a loop;
websocket-proxy.test.ts,websocket-proxy-tunnel-upgrade-leak,websocket-proxy-tunnel-client-leak,websocket-proxy-close-reentrancy,test-ws-bidir-proxy,ws-proxy.test.tsand the websocket client suites pass (the only failures seen locally are thewebsocket.test.jscases that needws.postman-echo.com, which this environment cannot reach, identically on the unpatched build).cargo clippy -p bun_http_jscis clean.