test(proxy-stress): return deadPort()'s bind(0) port, not the connect-autobound one - #35400
test(proxy-stress): return deadPort()'s bind(0) port, not the connect-autobound one#35400robobun wants to merge 2 commits into
Conversation
…-autobound one #35269 made deadPort() hold a live TCP connection and return the client-side local port so a concurrent listen(0) couldn't steal it. That port is allocated by connect() (inet_bind_bucket fastreuse == -1), which Linux's __inet_hash_connect will happily reassign as the source port of a later connect() to a different 4-tuple. When that later connect() is to the dead port itself on 127.0.0.1, source == dest and the socket self-connects; the fetch client then reads back its own CONNECT request line and fails with Malformed_HTTP_Response. Return the server-side port instead: it was allocated by bind(0), so its bucket has fastreuse >= 0 and __inet_hash_connect skips it outright. The listen(0) allocator still skips it because the accepted socket (which stays established after server.close()) keeps it in the bind hash without SO_REUSEADDR. before: 1/2000 full-file runs, net.connect probe 3/30000 after: 0/5000 runs, probe 0/30000 (+ 0/30000 listen(0) collisions)
|
Warning Review limit reached
Next review available in: 1 minute 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 (1)
Comment |
|
Updated 3:50 AM PT - Jul 24th, 2026
✅ @robobun, your commit 9dcd231e5bedcaf93139c6a0e0164f0dd5c2f641 passed in 🧪 To try this PR locally: bunx bun-pr 35400That installs a local version of the PR into your bun-35400 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted test-helper fix for a well-diagnosed 1/2000 flake.
What was reviewed:
- Traced the port-source swap: sink's
bind(0)port is returned instead of holder's connect-autobound port; aftersink.close()nothing is listening, so callers still get ECONNREFUSED as expected. - Checked that
await acceptedP.promiseguaranteesacceptedis assigned before bothsink.close()and[Symbol.dispose](), so theaccepted?.destroy()→accepted.destroy()change is safe. - Verified all three call sites in
proxy-stress-errors.test.tsonly need a refused-connection port and don't depend on which side of the pair it came from.
Extended reasoning...
Overview
This PR modifies a single test helper, deadPort() in test/js/bun/http/proxy-stress-helpers.ts, which produces a 127.0.0.1 port guaranteed to refuse connections for the duration of a test. The prior implementation (from #35269) returned the client-side local port of a held loopback connection. That port was allocated by connect()'s autobind, whose inet_bind_bucket gets fastreuse == -1; Linux's __inet_hash_connect will still consider such a port for a later outgoing connect()'s source port, and on loopback that can produce a TCP self-connect (~1/10000). The fetch client then reads back its own CONNECT line and fails with Malformed_HTTP_Response instead of ECONNREFUSED, flaking proxy-stress-errors.test.ts.
The fix returns the server-side bind(0) port instead, keeps the accepted socket alive so the port stays in the bind hash (blocking both listen(0) and connect-autobind reuse), and adds an explicit await on the server's 'connection' event so accepted is guaranteed assigned before sink.close() and before dispose. The doc comment is rewritten to explain both why the sink port is safe and why the holder port is not.
Security risks
None. Test-only infrastructure; no production code, no external inputs, no auth/crypto.
Level of scrutiny
Low. This is a ~20-line change to a test helper, fixing a rare flake with a precise kernel-level root cause and strong empirical verification (0/2000 full-file runs, 0/30000 connect probes, 0/30000 listen-churn collisions, 841 pass under ASAN). The change preserves the helper's contract (port refuses connections, held until dispose) — I confirmed the three consumers in proxy-stress-errors.test.ts only rely on that contract.
Other factors
- The
let accepted!: net.Socketdefinite-assignment + unconditionalaccepted.destroy()is sound:await acceptedP.promiseruns before theDeadPortobject is returned, so dispose can never see it unassigned. This also closes a theoretical race in the old code wheresink.close()could fire before the connection callback. - Other
deadPort/getDeadPorthelpers in the repo (dgram.test.ts,websocket-utf16-headers.test.ts) are independent implementations and not affected. - No prior human review comments; CI build was still in progress at review time but this is a de-flake that strictly tightens the helper's guarantees.
There was a problem hiding this comment.
LGTM — targeted test-helper flake fix, no production code touched.
What was reviewed:
deadPort()now returns thebind(0)server port instead of theconnect()-autobound client port; the kernel-level rationale (Linux__inet_hash_connect/fastreuse→ loopback self-connect) checks out.- The new
acceptedPawait closes the race wheresink.close()could fire before the server-side'connection'handler assignedaccepted, so theaccepted!definite-assignment and non-optionalaccepted.destroy()are safe. - Checked all callers of this
deadPort(proxy-stress-errors.test.ts) — they only read.portand dispose, so the return-shape is unchanged; the identically-named helpers in websocket-utf16-headers/dgram tests are separate local functions.
Extended reasoning...
Overview
This PR modifies a single test-helper function, deadPort() in test/js/bun/http/proxy-stress-helpers.ts, to fix a ~1/2000 CI flake in proxy-stress-errors.test.ts. The previous implementation (from #35269) returned the client-side local port of a held TCP connection; on Linux, ports allocated by connect() get fastreuse == -1 and remain eligible for reuse by __inet_hash_connect, so a later connect(127.0.0.1:P) could be assigned source port P and self-connect on loopback — the fetch client would then read back its own CONNECT line and fail with Malformed_HTTP_Response instead of ECONNREFUSED. The fix returns the server's bind(0) port instead (whose bucket has fastreuse >= 0 and is skipped by both allocators), and keeps the accepted socket alive after server.close() so the port stays bound-but-not-listening.
Security risks
None. This is test-infrastructure-only code with no production surface.
Level of scrutiny
Low. The change is confined to one helper in a test support file, is ~15 net lines, and comes with detailed empirical verification (0/2000 full-file runs, 0/30000 self-connect probes, 0/30000 listen(0) collisions, 841 pass on ASAN). The mechanism is well-explained and matches known Linux TCP behavior.
Other factors
I verified the two additional correctness details in the diff: (1) awaiting acceptedP.promise before sink.close() guarantees accepted is assigned, so switching from let accepted: net.Socket | undefined + accepted?.destroy() to let accepted!: net.Socket + accepted.destroy() cannot NPE; (2) the return signature ({ port, [Symbol.dispose] }) is unchanged and all three call sites in proxy-stress-errors.test.ts only read .port and rely on using disposal. The doc comment was rewritten to record why the client-side port must not be used, satisfying the repo's "durable non-obvious content" comment guideline.
|
CI status:
Ready to merge. |
|
Independent data point for this fix. While hardening the fixture from #39621, which copied the current |
2 participants
Problem
proxy-stress-errors.test.tsflaked in CI on 5461e9d with:Reproduced locally at 1/2000 full-file runs.
Cause
#35269 changed
deadPort()to hold a live TCP connection and returnholder.address().port, the client-side local port, so a concurrentlisten(0)can't steal it. That works forlisten(0), but the port was allocated byconnect(), which gives itsinet_bind_bucketthefastreuse == -1marker. Linux's connect-time port allocator (__inet_hash_connect) only checks 4-tuple uniqueness for such ports, so a laterconnect(127.0.0.1:P)can itself be assigned source port P. On loopback that is a TCP self-connect: the fetch client's proxy connection succeeds, it writesCONNECT host:port HTTP/1.1..., reads those same bytes back, and picohttp reportsMalformed_HTTP_Response.A standalone
net.connectprobe against the currentdeadPort()hits this in ~3/30000 attempts.Fix
Return the server-side (
sink) port instead. It was allocated bybind(0), so its bucket hasfastreuse >= 0and__inet_hash_connectskips it entirely, ruling out self-connect. Theacceptedsocket (kept alive afterserver.close()) keeps the port in the bind hash withoutSO_REUSEADDR, solisten(0)still can't pick it either. Also wait for the server's'connection'event before closing it soacceptedis guaranteed to exist.Verification
no test proof · iteration 1 · docs-only change; test-proof not applicable