Repository navigation
Conversation
bsd_create_listen_socket walks every getaddrinfo result (IPv6 then IPv4) so that a host whose v6 stack is disabled can still bind the v4 loopback. It was treating every bind failure as a reason to try the next address, so when `localhost` resolved to both ::1 and 127.0.0.1, a second `server.listen(port, "localhost")` that hit EADDRINUSE on ::1 would silently bind 127.0.0.1 instead of surfacing the error. Two dev servers both claimed localhost:5173 (Vite's port probe never saw a conflict). Keep the fallback for errors that mean the family is unusable, but treat EADDRINUSE and EACCES as terminal so the caller sees them, matching Node's listen(port, host) semantics. Fixes #15560
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
WalkthroughChangesThe change makes Localhost bind handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:34 PM PT - Aug 1st, 2026
❌ @robobun, your commit a296d26 has 2 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.d542c541-ac86-4f5d-8201-f966a6dfd5b7🧪 To try this PR locally: bunx bun-pr 36710That installs a local version of the PR into your bun-36710 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #30363: that one is a separate bug. |
There was a problem hiding this comment.
No issues found, but this changes the fallback semantics of the shared TCP listen path (Bun.listen/Bun.serve/net.Server/http.Server), so it's worth a human sign-off.
What was reviewed:
*erroris captured inus_internal_bind_and_listenbeforebsd_close_socketruns, soclose()clobbering errno doesn't affect the fatal check.- The
*error = 0reset before each attempt keeps a stale errno from an earlier iteration from tripping the early exit whenbsd_set_reuse/IPV6_V6ONLYfails without writing*error. LIBUS_ERRmaps toWSAGetLastError()on Windows and the fatal set usesWSAEADDRINUSE/WSAEACCES, so the platform gate is consistent.- Tests still pass (though don't exercise the fallback) on hosts where
localhostresolves to a single family — no false failure introduced.
Extended reasoning...
Overview
The PR modifies bsd_create_listen_socket in packages/bun-usockets/src/bsd.c to stop iterating the getaddrinfo result list when bind() (or listen()) fails with EADDRINUSE/EACCES, instead of falling through and binding the other loopback family. It adds a small helper bsd_bind_error_is_fatal, platform-gated errno constants, and a *error = 0 reset before each attempt. Two regression tests are added (net and http) that listen twice on (port, "localhost") and assert the second listen fails with EADDRINUSE.
Security risks
None identified. This tightens listen behavior (fails earlier rather than silently binding a different address); it does not relax any check, expose new surface, or handle untrusted input differently.
Level of scrutiny
Medium-high. The C change itself is small and reads correctly — LIBUS_ERR expands to errno/WSAGetLastError() so *error holds the real bind/listen errno, it's captured before bsd_close_socket, and the pre-attempt *error = 0 prevents a stale value from short-circuiting when bsd_bind_listen_fd fails in a path that doesn't write *error (bsd_set_reuse, IPV6_V6ONLY setsockopt). But this is the single shared sink for every TCP listen in Bun, and it is a deliberate behavior change: a second server on (port, "localhost") that previously bound 127.0.0.1 after ::1 reported EADDRINUSE will now fail. That matches Node and fixes #15560, but it's the kind of user-visible semantics shift a maintainer should confirm rather than a bot.
Other factors
The tests are hermetic (local servers, port: 0 for the first bind, error events wired to reject, cleanup in finally) and follow the file's existing patterns. On CI hosts where localhost resolves to only one family the tests still pass — they just don't exercise the multi-family fallback — so no flake is introduced. The bug hunting system reported no findings. Given the shared-path scope and cross-platform errno handling, deferring to a human reviewer.
The addrinfo-loop fix in bsd.c exposed that the Windows pass of test-net-listen-shared-ports.js depended on the old fallback: worker2 only reached 'listening' because bind(::1) failing over to 127.0.0.1 masked the port conflict. On Linux the same test already failed on main for the same reason (the primary's RoundRobinHandle holds the port the worker then tries to re-bind). Bind both the primary's RoundRobinHandle and the worker's listenOnPrimaryHandle with reusePort so every cluster participant can share the port (SO_REUSEPORT on Linux, SO_REUSEADDR on Windows), which is the same approach _http_server.ts already uses for http.Server in a cluster worker. The full cluster round-robin rework is in #30603.
There was a problem hiding this comment.
I didn't find bugs, but this touches the shared native listen path (bsd_create_listen_socket feeds Bun.serve, Bun.listen, net.Server, and http.Server) and adds a reusePort: true cluster workaround whose Windows behavior depends on how LIBUS_LISTEN_REUSE_PORT degrades there — worth a human look, and CI on the post-cluster-fix commits should be green first.
What was reviewed:
bsd.c: the early-return correctly closes the fd and freesaddrinfoon both loops;*error = 0before each bind prevents a stale errno from a prior iteration being read as fatal.net.tscluster path: the forcedreusePortonly applies in the!isPrimary && !exclusivebranch, so non-cluster andexclusive: truelistens still honor the caller's flag.- Checked that
bsd_close_socketruns before the fatal-error return so errno isn't clobbered — it writes to*errorfirst, so the saved value survives the close.
Extended reasoning...
Overview
The PR changes bsd_create_listen_socket in packages/bun-usockets/src/bsd.c to stop iterating getaddrinfo results when bind() fails with EADDRINUSE or EACCES, instead of falling through to the next address family. It also forces reusePort: true on the primary's RoundRobinHandle server and on cluster workers' net.Server bind (matching what _http_server.ts already does), and adds regression tests to node-net.test.ts and node-http.test.ts.
Security risks
None identified. The change tightens error reporting (surfaces EADDRINUSE/EACCES instead of silently binding a different address), which is the safer direction.
Level of scrutiny
High. bsd_create_listen_socket is the single sink for every TCP listen in Bun — Bun.serve, Bun.listen, net.Server, http.Server, the dev server. A regression here breaks every server. The bsd.c change itself reads correctly (fd closed, freeaddrinfo called, *error zeroed before each attempt so a stale value can't trip the fatal check), but the behavioral change and its cross-platform interaction with the cluster workaround warrant human review.
Other factors
- The cluster
reusePort: truechange is explicitly a workaround for Bun's incomplete handle-passing (tracked in #30603). On Windows,bsd_set_reuseportreturnsENOTSUPandbsd_set_reuseswallows it unlessDISALLOW_REUSE_PORT_FAILUREis set — so whetherreusePort: trueactually enables address sharing on Windows depends on whether the JS-side option also setsLIBUS_LISTEN_REUSE_ADDR. The PR description assertstest-net-listen-shared-ports.jsnow passes on Windows, but the last CI status in the timeline is for the pre-cluster-fix commit (c997ea7) where that test timed out on both Windows targets; CI for the follow-up commits should be confirmed. - The comment-cop bot flagged earlier verbose comments; those were tightened in later commits and the threads are resolved.
- Tests are well-formed (error events wired to reject, cleanup in
finally, no sleeps) and cover bothnet.Serverandhttp.Serverentry points.
|
On the Windows Verified on a local Windows x64 build at 2620482: and |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-usockets/src/bsd.c`:
- Around line 1240-1250: Update bsd_bind_listen_fd so every failure path,
including bsd_set_reuse() and the IPv6 IPV6_V6ONLY setsockopt() failure, assigns
the corresponding socket error to *error before returning. Preserve the existing
caller flow so bsd_bind_error_is_fatal() always receives the actual failure code
rather than zero.
In `@test/js/node/http/node-http.test.ts`:
- Around line 1315-1318: Update the finally blocks in
test/js/node/http/node-http.test.ts lines 1315-1318 and
test/js/node/net/node-net.test.ts lines 1894-1897 to await server shutdown
callbacks. Await a.close() and conditionally await b.close() only when b is
listening, preserving cleanup for failed-listen cases and applying the same
change to both tests.
In `@test/js/node/net/node-net.test.ts`:
- Around line 1873-1878: Update the regression comment above the affected test
to contain only the issue URL, removing the explanatory implementation details
while leaving the test behavior unchanged.
- Around line 1879-1893: Update both regression tests at
test/js/node/net/node-net.test.ts:1879-1893 and
test/js/node/http/node-http.test.ts:1298-1314 to resolve localhost with { all:
true } before binding, verify that both IPv4 and IPv6 addresses are available,
and skip with an explicit reason when either family is absent. Keep the existing
bind and EADDRINUSE assertions unchanged for dual-family hosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2da0835-4ebe-4cf8-8c35-aa66f94fb3eb
📒 Files selected for processing (5)
packages/bun-usockets/src/bsd.csrc/js/internal/cluster/RoundRobinHandle.tssrc/js/node/net.tstest/js/node/http/node-http.test.tstest/js/node/net/node-net.test.ts
|
Build #87468 ran the full suite on a296d26. The only lanes that failed are unrelated to this diff:
None of the new or changed tests in this PR failed. In particular (Earlier builds #87288/#87369 never ran: fleet-wide agent outage, every job Ready for review. |
|
Heads-up from #37720, which touches the same two lines in |
Fixes #15560.
Repro
Before:
B bound { address: '127.0.0.1', port: <same>, family: 'IPv4' }After (and in Node):
B EADDRINUSECause
bsd_create_listen_socketcallsgetaddrinfo(host)and walks every result, IPv6 first then IPv4, so that hosts with a disabled v6 stack can still bind the v4 loopback. It treated everybind()failure as a reason to move on to the nextaddrinfo, so whenlocalhostresolved to both::1and127.0.0.1, a secondlisten(port, "localhost")that gotEADDRINUSEon::1would silently bind127.0.0.1instead of surfacing the error. Two dev servers both "listened on localhost:5173" and Vite's port probe never saw a conflict.Node resolves the host once via
dns.lookupand binds that single address, so the second listener seesEADDRINUSE.Fix
Keep walking the
addrinfolist for errors that mean the family is unusable (EAFNOSUPPORT,EADDRNOTAVAIL, socket() failures), but stop and return the error whenbind()reportsEADDRINUSEorEACCES: those mean the port is owned, and binding the other loopback family is not a correct fallback.packages/bun-usockets/src/bsd.cis the shared sink forBun.listen,Bun.serve,net.Serverandhttp.Server, so the one change covers all of them.node:cluster follow-up
Stopping the addrinfo fallback exposed that
test-net-listen-shared-ports.json Windows only passed because of it: the primary'sRoundRobinHandlebinds the requested port and each worker then re-binds it itself, so worker2 was reaching'listening'by quietly taking127.0.0.1after::1reported in-use. On Linux the same test was already failing onmain(the primary's dual-stack wildcard blocks both loopback addresses outright)._http_server.tsalready handles this forhttp.Serverin a cluster worker by binding withreusePort. Do the same fornet.Server: the primary'sRoundRobinHandleand the worker'slistenOnPrimaryHandlenow passreusePort: true, so the port is shared viaSO_REUSEPORT(Linux) /SO_REUSEADDR(Windows) instead of via the addrinfo fallback.test-net-listen-shared-ports.jsnow passes on both Linux and Windows. The full cluster round-robin rework (primary stops accepting,port: 0resolved once) is tracked in #30603.Verification
Both new tests fail on main with
second listen bound {"address":"127.0.0.1",...}.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts