Repository navigation
Conversation
Bun.serve({ port, unix }) bound only the unix socket and dropped the
port without a diagnostic. The sibling conflict, unix with hostname,
already throws. Throw "Cannot specify both port and unix" for a
non-zero port. Port 0 asks for no specific port and keeps its current
behavior.
node:http's listen({ port, path }) now binds the port and ignores the
path, as node does, so it does not reach the new check.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 7:10 PM PT - Sep 6th, 2026
❌ @robobun, your commit fec129a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41660That installs a local version of the PR into your bun-41660 --bun |
…t portable
listen({ path, host }) forwarded both to Bun.serve as hostname + unix,
which throws. Node's pipe branch never reads host, so drop it there.
The Bun.serve test asserted on server.url, whose getter throws for a
Windows drive path (pre-existing, see #35806). Assert reachability over
the unix socket instead.
| if ((typeof port === "number" || typeof port === "string") && socketPath) { | ||
| socketPath = undefined; | ||
| } else if (socketPath) { | ||
| host = undefined; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) When path wins the precedence check, port is not cleared, so a truthy non-number/non-string port (e.g. true, [8080], new Number(80)) is still forwarded to Bun.serve alongside unix, where the new ServerConfig guard now throws — listen({ port: true, path }) bound the socket on base but emits 'error' after this PR, and Node binds the pipe. Fix: set port = undefined in the else if (socketPath) branch so the pipe path never forwards a port, matching src/js/node/net.ts:3745.
Extended reasoning...
listen({ port: true, path: sock }): line 548 sets port = true; line 552 skips (not undefined/null); line 578 skips (not string); line 586 typeof true is "boolean" so the first branch is false; line 588 takes else if (socketPath) and clears host but leaves port = true and socketPath = sock. kRealListen then calls Bun.serve({ port: true, unix: sock, ... }). In ServerConfig::from_js, get_truthy("port") returns Some(true), to_number(true) is 1, has_port = true (line 1087); the unix block then hits the new has_port guard and throws "Cannot specify both port and unix" (line 1125-1129). The throw is caught at line 627 and routed to emitListenErrorNextTick, so the server emits 'error' instead of 'listening'. On the base branch the same inputs reached Bun.serve, port was parsed into args.address and then silently overwritten by unix, so the socket bound. Node's net.Server.listen only takes the TCP branch when typeof options.port is 'number' or 'string'; any other port value falls through to the pipe branch which never reads port, so Node…
Verification: nit — real regression this PR introduces, but the triggering inputs (port: true, [8080], new Number(80)) are exotic; unlikely a real caller hits it. Mechanism traced and verified: src/js/node/_http_server.ts with listen({ port: true, path: sock }): - L548 port = arg0.port → true - L552 (port === undefined && "port" in arg0) || port === null → false, skipped - L574 `port ===…
|
Status Reproduced on bun 1.4.3 (linux x64): const s = Bun.serve({ port: 48123, unix: "/tmp/app.sock", fetch: () => new Response("ok") });
console.log(s.port, String(s.url)); // undefined unix:///tmp/app.sock, nothing listens on 48123With this PR the call throws The new tests in CI is red on lanes this diff does not touch: |
Problem
Bun.serve({ port: 8080, unix: "/tmp/app.sock", fetch })binds only the unix socket. The port is dropped with no diagnostic:server.portisundefinedand nothing listens on 8080.src/runtime/server/ServerConfig.rs:1115. Theunixbranch overwritesargs.addressafterportwas parsed into it. The sibling conflict,unixwithhostname, already throwsCannot specify both hostname and unixa few lines above.Fix
Cannot specify both port and unixwhenunixis given with a non-zeroport. Port 0 asks for no specific port, so it keeps its current behavior (the unix socket wins). In-tree tests rely onport: 0withunix.node:http'sServer.listenforwardedport,hostandunix: socketPathtogether toBun.serve. It now follows Node'snet.Server.listen: when the options carry aportkey (a number, a string,undefinedornull), the port wins and the path is not forwarded. When the path wins,hostis not forwarded (Node's pipe branch never reads it, andlisten({ path, host })emitted'error'in bun before).node:netalready applies the port rule insrc/js/node/net.ts. Verified against node v26.3.0.unixexclusive withhostnameandport(packages/bun-types/serve.d.ts,XOR<HostnamePortServeOptions, UnixServeOptions>). The runtime now enforces that contract.test/js/bun/http/serve.test.ts(new test) andtest/js/node/http/node-http.test.ts(five new cases). All fail on bun 1.4.3 and pass with the fix, on linux x64 and Windows x64. Also ranbun-serve-args.test.tsand both full files.Background
ServerConfig::from_jsparses theBun.serveoptions object into anAddress, which is eitherTcp { hostname, port }orUnix(path). Each option is read in sequence, so a later option can silently replace an earlier one.node:httpin bun is built onBun.serve.Server.prototype.listeninsrc/js/node/_http_server.tsnormalizes the Node argument shapes and callsBun.serveinkRealListen.Server.listen(options)checksoptions.portbeforeoptions.path. Aportkey of any accepted type takes the TCP branch andpathis never looked at. The pipe branch reads onlypathand the pipe-specific options.Notes
Found by an internal audit of places where bun picks a transport mode from conflicting input.
The first revision of the
Bun.servetest asserted onserver.url. Its getter throwsTypeError: Invalid URLfor a Windows drive path (unix://C:\...), which is the pre-existing #40182 (fix proposed in #35806). The test now assertsserver.portisundefinedand fetches over the unix socket instead.Two pre-existing failures in
serve.test.tsin my linux container (#7187root range port,/bun:infoloopback clients) also fail on main there. They depend on the container running as root and on its network interfaces. They are unrelated to this change and pass in CI.Self-reviewed: 4 concerns raised, 4 addressed (match the
net.tsport precedence rule including string ports and Node'sundefined/nullnormalization, keep theport: 0+unixboundary asserted next to the throw test, usetempDirin the new test, do not cite #14299 as precedent). Review follow-ups addressed:listen({ path, host })parity, Windows-portable test assertion.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/bun/http/serve.test.ts