Repository navigation
Conversation
…r empty `start()` took the socket branch for every `unix` of the `app/server.ts` export that was not `undefined`. That branch hides the export's `hostname`, `host` and `port` and sets no "localhost" default. `Bun.serve()` reads `null`, `false` and an empty string as no socket, so the dev server listened on every interface, on port 3000. The socket branch is now taken only for a value that `Bun.serve()` uses. The value that was tested is passed as an own `unix` option, so `Bun.serve()` does not read the export's `unix` a second time.
|
Updated 5:15 AM PT - Oct 24th, 2026
❌ @robobun, your commit 8faa6dc has 2 failures in
🧪 To try this PR locally: bunx bun-pr 46717That installs a local version of the PR into your bun-46717 --bun |
|
Status
|
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Select listener and validate serving behavior src/js/internal/bake.ts, test/cli/bake/bake.test.ts |
bun bake reads the exported unix value once and selects the Unix socket only when it is non-falsy and neither hostname nor port is specified. Tests cover falsy values, getter reads, and TCP serving when either flag is supplied. |
Suggested reviewers: alii
Priority:
Merge Risk: 
Minimal · up to 89a73
-
The listener-selection change is mergeable after normal checks; no actionable issue remains identified.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, cause, fix, scope, verification results, tests, limitations, and remaining issues. Although it does not use the exact template headings, it provides the required information in equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/internal/bake.ts— Developers with a stale socket file get six identical bind attempts on the same path, then an uncaught error with a stack trace instead of bake's one-line error. In theuseUnixbranch the port argument to serveOnFreePort is undefined (src/js/internal/bake.ts:491) and options() ignores the port it is handed (bake.ts:477-482), yet a unix listen failure carries code EADDRINUSE (src/runtime/server/mod.rs:724), so serve_cli.ts:112-124 retries five more times with the same socket. Fix: skip the port retry loop when the options use a unix socket, and route the final listen error through BakeError sobun bakeprintserror: ...and exits as it does for a TCP address.Why this was flagged
Trigger: app/server.ts exports
unix: "./app.sock"and the file exists from a previous run killed without cleanup; the developer runsbun bakewith no --host/--port. Entry: start() calls serveOnFreePort at src/js/internal/bake.ts:465 with portundefined(bake.ts:491). Bun.serve() fails to listen on the socket and throws a SystemError whose code isEADDRINUSE(src/runtime/server/mod.rs:719-728). serveOnFreePort treats that as a busy TCP port (src/js/internal/serve_cli.ts:112) and calls Bun.serve(withId(defaultPort++)) five more times, but options() in the useUnix branch (bake.ts:477-482) definesportas undefined and keeps the sameunix, so every retry binds the same path. After the sixth failure the error reaches the catch at bake.ts:493;addressFromExportis true becauseuseUnixis true (bake.ts:498-499), so the raw error is rethrown (bake.ts:504) and main() rethrows non-BakeErrors (bake.ts:522): the process dies with an unhandled error and stack trace rather than theerror:line and exit code the TCP path gives. The base branch behaves the same.Verification: pre-existing. Trigger: app/server.ts exports a non-empty
unixstring, no --host/--port, and the socket path is already bound or a stale file from a killed run. The port argument isundefined(src/js/internal/bake.ts:491) and the options factory ignoresportin the useUnix branch (bake.ts:477-482), so every attempt yields the same socket path. The base commit has byte-identical behavior on this exact path.
…ugh spawnBake With `--port` or `--host`, `bun bake` makes no socket for the `unix` of the export, so the test needs no POSIX-only socket path. `spawnBake` takes extra environment variables, and the new tests use it in place of their own `Bun.spawn` call.
|
Reply to the additional finding in the review (a Confirmed, and main behaves the same. With a stale Not changed in this PR. The retry is in The two test nits are done in 8faa6dc, and the three threads have replies. |
There was a problem hiding this comment.
LGTM — the second commit addressed both test nits, and the fix itself is small and matches what Bun.serve() does.
What was reviewed:
hasUnixat src/js/internal/bake.ts:460 againstget_stringish(src/jsc/JSValue.rs:1141) and theunixread in src/runtime/server/ServerConfig.rs:1057: undefined/null/false/"" are no socket on both sides;0,NaNand symbols reachBun.serve()the same way as before.- The own
unixwrite at bake.ts:482 is the only other reader of the export'sunix, so the getter is read exactly once;serveOnFreePortdoes not re-read it. - Error classification at bake.ts:498: with
unix: nulland no flags,addressFromExportis now false, so a listen failure on the default address is correctly non-fixable. - New tests:
PORT=0is read by ServerConfig.rs:619,server.addressis the DTO from SocketAddress::into_dto with anaddressfield, and the rows assert the bound address rather than the banner.
Extended reasoning...
The change touches start() in the builtin src/js/internal/bake.ts (six lines: read unix once, a presence predicate matching Bun.serve's get_stringish, and an own unix property in the socket branch) plus seven new subprocess tests in test/cli/bake/bake.test.ts. It narrows an accidental wildcard listen for unix: null/false/"" to loopback; no auth, crypto or injection surface is involved, and bun bake is gated behind an experimental feature flag. Neither path has a CODEOWNER, the hunt ran dry with no findings, and the second commit resolved both inline nits from the prior review. The remaining gap for non-primitive values whose string form is empty was already noted as pre-existing and the author documents it as deliberately held back.
Problem
bun bakelistens on every interface when theapp/server.tsexport has aunixthatBun.serve()does not use:null,falseor"". The banner still sayshttp://localhost:3000/.start()takes the socket branch forunix !== undefined(bake.ts:459) and hides the export'shostnameandport.Bun.serve()then readsunixagain withget_stringish(ServerConfig.rs:1057), finds none, and binds TCP with no hostname.Fix
unixthatBun.serve()uses. Pass that value as an ownunixoption, soBun.serve()does not read the export again.test/cli/bake/bake.test.ts. Release build of main: 87 pass, 5 fail. This branch: 92 pass, 0 fail.Background
bun bakecallsBun.serve()with an object that inherits from the export, soBun.serve()reads every option bake does not define itself.get_stringish, which readsunix,hostnameandhost,undefined,null,falseand""are no value. 477ec6f (bun bake: build plugins from bake.config.ts #45828) is the same guard for an emptyhostname.bun bakekeepunix,hostnameandportinapp/server.ts?Downsides
unix: [],new String(""), the samehostnameshapes): 20 of 91 probed exports, 31 before.internal/bake.jsand the release binary's text grow 95 bytes. A socket export makes 8Object.definePropertycalls per attempt, not 7.get unix()reads a#privatefield now serves (main throws), andObject.prototype.unixnow makes a socket, not a wildcard listener.Notes
How it was found. In review of #44013, which adds
fdbesideunixin the same expression. That PR's body says this bug is reported apart from it. No user reported it: the tracker has no issue, and a stable build runsbun bakeonly withBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1.Reproduce on 1.4.4 (release, Linux x64)
app/server.ts:export default { unix: null, fetch: (_request, server) => Response.json(server.address) };BUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1 PORT=0 bun bake, with no--portand no--host.url: http://localhost:N/line, thenss -ltnp | grep "pid=<pid>,".The listener is
*:N, and a fetch of the URL answers{"address":"::"}. With this change it is[::1]:Nand::1. The same holds forunix: ""andunix: false. A realistic shape is{ unix: process.env.SOCK ?? "", hostname: "127.0.0.1" }withSOCKunset: on 1.4.4 it binds*:3000and answers on the machine's LAN address.The larger change, held back. Read
unix,hostnameandhostthe wayBun.serve()reads them, and pass each as an own option (+153 lines, commitefeb1e7f0aonrobobun/52c6767f/bake-address-once). It takes the wildcard rows from 31 of 91 to 4, fixesunix: []andhostname: [], and stopsBun.serve()reading any address option from the export. It costs a copy ofget_stringishand of the lookup that stops beforeObject.prototype, in JS, and +783 bytes. It is held back becausebun bakeis being reworked (the five PRs closed on 2026-10-21) andServerFilein the design repo has no address option. If the keys stay, I will send it.Measured (release builds of
a8fcd319beand of this branch, Linux x64)app/server.tsfiles through the real CLI and readsssandserver.address. Of the 14 shapes in the report: 8 before, 3 after.0.0.0.0themselves (hostname: "0.0.0.0",hostname: "0").internal/bake.js: 14,339 to 14,434 bytes (wc -c). Release binarytext: 107,667,648 to 107,667,743 (size).data,bssand the file size are equal.Object.definePropertyperBun.serve()attempt, from a preload hook: plain export 7 before and after, socket export 7 before and 8 after.unixper start with a socket export, from a getter: 2 before, 1 after.hostname,hostandportare 1 each, before and after.start(): 443 bytecode instructions before, 451 after (BUN_JSC_dumpGeneratedBytecodes).Tests. Five fail on main:
unix: null,unix: "",unix: false,unix: ""withhostname: "127.0.0.1", and the read count. Two pin behaviour this change keeps:--portand--hoststill come before the export's socket. Each clause of the fix was deleted in turn, and each deletion fails at least one test. The address rows assertserver.addressfrom a request, not the banner: the banner sayslocalhostfor a wildcard listener.Not addressed from the self-review: the existing test
bun bake listens on localhost when the export of app/server.ts has an empty hostnameasserts the hostname of the banner, so it cannot see a wildcard listener. It is the test of another guard, and this change leaves it as it is.Debug+ASAN with
--timeout 90000: 91 pass, 1 fail. That one failure,bun --watch-kill-signal ... bake --check sends that signal before a restart, also times out with main'sbake.tson this machine. Also ranbun-serve-args.test.tsandbun-serve-html-entry.test.ts: 81 pass, 0 fail.Sites of the same class, not in this change
unix,hostnameorhostwhose string is empty. The larger change above covers these.websocketis read bybun bake(bake.ts:369) and again byBun.serve()(ServerConfig.rs:989). It fails closed and sets no address.serve_cli.ts:113and again inServerConfig.rs:618. html dev server: keep an explicit port, warn when the default port moves #42020 owns that line.node:http:server.listen({ path: "" })listens on::, where Node throwsERR_INVALID_ARG_VALUE. Its fix is Node's throw, not this one read.Bun.serve({ unix })callers are handled separately.Also unchanged: a socket path that is in use (a stale file too) is tried six times by
serveOnFreePort(serve_cli.ts:112), thenbun bakeprints theEADDRINUSEerror and waits for an edit. Main does the same. The wait is the rule of #45828 for an address that the export gives.Order against #44013. That PR renames this expression to
useSocketand addsfd. Either can land first. Merging this branch into it leaves one conflict region, the predicate. The resolution isconst useSocket = (hasUnix || exported?.fd != null) && flags.hostname === undefined && flags.port === undefined, plusown("fd", exportedFd)besideown("unix", exportedUnix).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/bake/bake.test.ts