test(websocket): record the CONNECT request and tighten the assertions in the proxy tests - #40624
Conversation
Assert the exact event sequence of every proxied session (greeting, echo, clean close) and the exact error message and close code of the failure paths. Record the CONNECT request each test's proxy receives and check the proxy headers and credentials the client sent. The NO_PROXY children print what they observe and the test compares their stdout, stderr, exit code and the number of connections the proxy saw. The ping regression test counts the pongs the server receives.
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: the change is complete at 0a2091f. The self-review findings (recorder in the wrong layer, three sessions still on the unrecorded proxy, the untouched twin) and the one review thread (dead What the PR does: the CONNECT request is recorded inside Measured before the change: websocket-proxy.test.ts takes 117 to 208 ms on the darwin aarch64 lane in the last five main builds (106385, 106512, 106596, 106656, 106692), 0.22 to 0.39 s with the release binary here, and 6.1 s with the debug build (2.7 s of that is module load and Verification: CI (builds #106768, #106889, #106892): the jobs red on every retry are debian 13 x64-asan on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only refactor that strictly strengthens assertions and consolidates boilerplate.
What was reviewed:
startRecordingProxy's secondconnectionlistener passively observes the samedataevents as the tunnel handler (broadcast, not consumed) and detaches after\r\n\r\n, so it cannot interfere with tunnel bytes.- Ordering assumptions are deterministic:
echoed()'s["connected", msg, close]order holds because the server sends the greeting inopenbefore any echo; the ping test readspongsonly afterserverClosedresolves, and the pong frame precedes the close frame on the wire. - No assertion was weakened — every original
toContain/"some error fired" check is replaced by an exacttoEqualon the full event list plus the recorded CONNECT request;failingSessioncloses an unexpected open so failures diff instead of timing out. - NO_PROXY
test.eachuses thunks sowsPortis read afterbeforeAll, spreadsbunEnvwithno_proxy: undefined, drains stdout/stderr/exited concurrently, and each concurrent case owns its own recording proxy port.
Extended reasoning...
Overview
This PR refactors a single test file, test/js/web/websocket/websocket-proxy.test.ts, collapsing ~390 lines of repeated Promise.withResolvers/onopen/onmessage/onclose boilerplate into shared helpers (clientEvents, echoSession, failingSession, echoed, failed, connectRequest, startRecordingProxy). Every rewritten test now asserts an exact ordered event array via .toEqual instead of toContain or "an error fired", and additionally asserts the CONNECT request line and headers the proxy actually received. The four NO_PROXY subprocess tests are folded into one test.each that asserts a combined {stdout, stderr, exitCode, proxyConnections} object. The ping-over-TLS-tunnel regression test now asserts the server received exactly one pong and closed with 1000. No native or shipped code is touched.
Security risks
None. This is a test-only change with no modifications to runtime code, no new dependencies, no network egress beyond the existing local 127.0.0.1 servers, and no credential handling beyond the pre-existing hardcoded proxy_user:proxy_pass test fixture.
Level of scrutiny
Low-to-moderate. Test-only changes to a single file, no CODEOWNERS entry for the path, no outstanding reviewer objections, and the bug hunt exited on dry_streak. The main risks for a test-tightening PR are (a) silently weakening an existing assertion and (b) introducing flakiness via ordering or race assumptions. I checked both: every original assertion is preserved or strengthened, and the ordering assumptions ("connected" before echo, pong before close frame, serverClosed awaited before reading pongs) are backed by TCP ordering and the server's open-before-message sequencing rather than timing.
Other factors
The refactor closely follows the repo's REVIEW.md and test/CLAUDE.md conventions: using disposal registered before assertions, {...bunEnv, no_proxy: undefined} spread, Promise.all on stdout/stderr/exited, test.each for the parameterized matrix, no throw inside event callbacks (all failure paths funnel through close → resolve), and failingSession closes an unexpectedly-opened socket so a broken invariant produces a diff rather than a timeout. The noProxy values in the test.each table are thunks specifically so wsPort (assigned in beforeAll) is read at test time, not table-construction time. The PR description documents mutation testing of each new assertion, which matches the "prove the test fails for the right reason" requirement.
|
Updated 8:19 AM PT - Aug 27th, 2026
❌ @robobun, your commit 0a2091f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40624That installs a local version of the PR into your bun-40624 --bun |
…ighten ws-proxy.test.ts too
proxy-test-utils.ts already parses the CONNECT request it answers. It now
exposes that request through an onConnectRequest hook, folds the TLS proxy
into createConnectProxy({ tls: true }) so an HTTPS proxy can record too, and
exports startRecordingProxy, connectRequest, startEchoServer and the client
event helpers for both proxy test files.
websocket-proxy.test.ts starts a recording proxy in every test that tunnels,
including the three that still used the shared unrecorded proxy and the HTTPS
proxy tests, and drops the shared proxy fixtures. ws-proxy.test.ts gets the
same exact event lists and CONNECT request assertions, clears the ambient
NO_PROXY like its twin, and checks that the agent's proxy sees no connection
when an explicit proxy option wins.
robobun
changed the title
…nnectProxy({ tls: true })
There was a problem hiding this comment.
LGTM — the dead createTLSConnectProxy export I flagged was removed in 0a2091f, and grep confirms the remaining importer (websocket-syscall-fault.test.ts) only uses createConnectProxy/startProxy, which are intact.
Checked that folding the TLS proxy into createConnectProxy({ tls }) preserves the original handler behavior (auth path was already absent from the deleted TLS variant, and no caller passes both tls: true and requireAuth). The new clientEvents/failingSession helpers wire close/error to resolve the awaited promise, so failure paths can't hang. The port: 1 literals are constructor-only tests that close() before any dial, so nothing listens or connects there.
Extended reasoning...
Overview
Test-only refactor across three files: proxy-test-utils.ts gains onConnectRequest/tls options on createConnectProxy, a per-test startRecordingProxy disposable, and structured event collectors (clientEvents, echoSession, failingSession, echoed, failed, connectRequest). Both websocket-proxy.test.ts and ws-proxy.test.ts drop file-scoped shared proxies in favor of per-test using recorders and assert a single .toEqual covering the ordered client event stream plus the exact CONNECT request the proxy received. The NO_PROXY subprocess tests collapse into a test.each asserting stdout + connection count. Net ~470 lines removed. No src/ code touched.
Security risks
None. This is test infrastructure only — no runtime code, no auth/crypto/permissions paths. The proxy helper is a test-local net/tls server bound to 127.0.0.1 on port 0.
Level of scrutiny
Moderate for a test refactor: the main risks are (a) breaking other importers of the shared helper and (b) weakening existing coverage. For (a), grep confirms only three importers of proxy-test-utils; the two rewritten in this PR and websocket-syscall-fault.test.ts, which imports only createConnectProxy/startProxy — both still exported with compatible signatures. For (b), the refactor moves from .toContain/sawError booleans to exact .toEqual on ordered event lists and recorded CONNECT requests, which is strictly stronger per REVIEW.md's "assert the strongest invariant" rule. The failingSession helper closes on unexpected open so a mutation that makes a should-fail path succeed surfaces as an assertion diff instead of a timeout.
Other factors
My only prior finding (dead createTLSConnectProxy alias after its last callers were rewired) was addressed by commit 0a2091f; grep on HEAD finds zero references. The PR description's claim that the alias "stays for other importers" is now stale, but the code is what merges. No outstanding third-party CHANGES_REQUESTED reviews in the timeline. The change aligns with the repo's test conventions (per-test using disposal, port: 0, awaiting the close event rather than sleeping, test.each for the NO_PROXY matrix, ambient env cleared and restored).
2 participants
Problem
websocket-proxy.test.tsand its twintest/js/first_party/ws/ws-proxy.test.tscheck outcomes loosely:toContainon the message list, "some error fired" on the failure paths, no check that a proxy header reached the proxy, noexpect()in the ping regression test, and NO_PROXY children that exit 0 or 1.NO_PROXY=localhost,127.0.0.1,..., 16 of the 19 twin tests pass with the proxy bypassed and the other 3 fail.Fix
proxy-test-utils.tsexposes the CONNECT request it already parses through anonConnectRequesthook, folds the TLS proxy intocreateConnectProxy({ tls: true }), and exportsstartRecordingProxy(port, requests, connection count),connectRequest,startEchoServerand the client event helpers.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.bun bd teston 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
createConnectProxyis a Nodenet(ortls) 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.proxy:option, and bun readsno_proxybeforeNO_PROXY(src/dotenv/env_loader.rs:374). test: isolate proxy tests from ambient proxy environment variables #37439 moves the clearing into the harness. This PR keeps the module-level clearing in both files until it lands.Notes
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) plusbeforeAll. The slowest tests are the four NO_PROXY children (0.3 s each in debug, they already run concurrently). Nothing else is converted toconcurrent: the TLS tunnel regression tests depend on event loop ordering (thesetImmediatespin 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):
ws:// through proxy with custom headersfails on the recorded CONNECT request.NO_PROXY=other.host.comin a "direct" case: fails on stdout andproxyConnections: 1.no_proxy=127.0.0.1in the "proxied" case: fails on stdout andproxyConnections: 0.pongs: 0.proxy wrong credentials returns error, the CA given infails 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.explicit proxy option takes precedence over agentwith the explicit option pointing at the agent's proxy: fails onexplicitRequests: [].no_proxy=127.0.0.1left in place: 13 of 19 tests fail onrequests: [](before this change all 16 through-proxy tests passed that way).Scope. Three test files, no runtime change.
createTLSConnectProxyis gone: its only callers were the two rewritten files, andcreateConnectProxy({ tls: true })replaces it. The Docker Squid section of websocket-proxy.test.ts is untouched (it cannot run here). The constructor checks in bothproxy APIsections now use port 1 (nothing needs to listen:close()follows at once) instead of the shared fixtures, which are gone.tsc -p test/tsconfig.jsonreports the same classes of errors on these files before and after (the test tsconfig resolves the DOMWebSocketconstructor and@types/ws, so everyproxy:option already errors on main; the helper'ssocket.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.[stamp-90s] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file