Repository navigation
Conversation
…nding a random port
|
Warning Review limit reached
Next review available in: 4 minutes 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 (2)
WalkthroughServer.prototype.listen in the Node.js HTTP server compatibility layer now reads an Changesfd rejection in http.Server.listen
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ServerListen as Server.prototype.listen
Caller->>ServerListen: listen({ fd })
ServerListen->>ServerListen: extract fd from arg0
ServerListen->>ServerListen: check typeof fd === "number" && fd >= 0
ServerListen-->>Caller: throw EINVAL (errno=-22, syscall="listen")
Related Issues: None found in provided context. Related PRs: None found in provided context. Suggested labels: node:http, bug, test Suggested reviewers: None identified from provided context. Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Updated 2:24 AM PT - Jul 7th, 2026
❌ @robobun, your commit 42617ec has some failures in 🧪 To try this PR locally: bunx bun-pr 33578That installs a local version of the PR into your bun-33578 --bun |
|
Good catch, that's the same issue. Added |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/http/node-http-listen-fd.test.ts`:
- Around line 20-27: The listen-fd test only checks the error event’s code and
syscall, but the updated error contract also includes errno. Update the
assertion in the node-http-listen-fd test around the server.once("error")
handling to capture errno from the emitted error and include it in the expected
object alongside code and syscall, so the test fully verifies the listen failure
shape.
🪄 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: 9853ef00-6f19-4683-a6ec-2a5f8e4a9e3a
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/http/node-http-listen-fd.test.ts
There was a problem hiding this comment.
LGTM — the errno platform branch addresses my earlier note, and the guard is a clear improvement over silently binding a random port.
Extended reasoning...
Overview
Adds a guard in Server.prototype.listen (src/js/node/_http_server.ts) that reads fd from the options object and, when a numeric fd ≥ 0 is present, throws an EINVAL error inside the existing try/catch so it surfaces as an error event. Previously the option was ignored and the server silently bound a random ephemeral port. A new POSIX-gated test uses a donor net.Server to obtain a real listening fd and asserts the error shape (code, errno, syscall).
Security risks
None. This is a diagnostic-only guard that turns a silent misbehavior into an explicit, catchable error. No new I/O paths, no auth/crypto, no user-controlled data reaching a parser or syscall.
Level of scrutiny
Low. ~11 lines of production code adding an early-throw guard that routes through the existing error-emit path, plus a self-contained test. The change strictly narrows behavior on an input that was already unsupported — nothing that previously worked stops working. It also aligns node:http with what Bun's net.Server.listen({ fd }) already reports.
Other factors
- My earlier inline nit about the hardcoded
errno = -22on Windows was addressed with theprocess.platform === "win32" ? -4071 : -22branch, matching sibling built-ins. - CodeRabbit's suggestion to assert
errnoin the test was applied; both threads are resolved. - The bug-hunting pass found no issues.
- The test awaits the actual condition (races
errorvslistening), cleans up infinally, and is skipped on non-POSIX where the errno differs. - No CODEOWNERS cover the touched paths.
- The PR explicitly defers actual fd adoption (which would need a native uSockets listen-from-fd path) to a separate discussion; failing loudly is the right interim behavior.
There was a problem hiding this comment.
LGTM — small, well-scoped guard that turns a silent wrong-port bind into an explicit EINVAL, matching net.Server's behavior; both review nits addressed.
Extended reasoning...
Overview
This PR touches two files: a ~12-line addition to Server.prototype.listen in src/js/node/_http_server.ts that reads arg0.fd and throws an EINVAL-shaped error when a numeric fd ≥ 0 is passed, and a new POSIX-only test at test/js/node/http/node-http-listen-fd.test.ts that obtains a real listening fd from a donor net.Server and asserts the error event fires with { code: 'EINVAL', errno: -22, syscall: 'listen' }. The throw sits inside the existing try/catch, so it surfaces as an 'error' event rather than a synchronous throw — matching Node's listen-error semantics.
Security risks
None. No new input parsing, no auth/crypto/permissions surface, no external data handling. The only new branch inspects a user-supplied number and constructs a plain Error object.
Level of scrutiny
Low-to-moderate. node:http is production-critical, but this change is purely additive: it adds a guard on a code path that was previously broken (silently binding a random ephemeral port instead of the inherited socket, leaving the service unreachable — issue #22559). The new behavior is strictly better than silent failure and matches Bun's existing net.Server.listen({ fd }) EINVAL. No existing behavior is altered for callers who don't pass fd.
Other factors
- Both prior review comments (my Windows
errnonit →process.platform === 'win32' ? -4071 : -22, and CodeRabbit's suggestion to asserterrnoin the test) were addressed in commit eccae3a and the threads are resolved. - The bug-hunting system found no issues.
- No CODEOWNERS cover these paths.
- The test follows repo conventions (topic-specific file under
test/js/node/http/,isPosixgate, donor server for a real fd, awaits the actual event, cleans up infinally). - The PR explicitly scopes out actual fd adoption (which would need a native uSockets listen-from-fd path) as a separate discussion; that's a reasonable call and doesn't block this stopgap.
There was a problem hiding this comment.
LGTM — small, well-scoped fix that brings http.Server.listen({ fd }) in line with net.Server's existing EINVAL behavior; prior feedback on the platform-specific errno has been addressed.
Extended reasoning...
Overview
This PR touches src/js/node/_http_server.ts (~15 lines) to read the fd option from Server.prototype.listen's options object and, when present, throw an EINVAL error (routed through the existing try/catch to an 'error' event) instead of silently defaulting port to 0 and binding a random ephemeral port. A new POSIX-gated test at test/js/node/http/node-http-listen-fd.test.ts obtains a real listening fd from a donor net.Server and asserts the error shape (code, errno, syscall).
Security risks
None. No parsing of untrusted input, no auth/crypto/permissions surface. The change only adds an early-exit guard on a user-supplied option value that was previously ignored.
Level of scrutiny
Low-to-moderate. This is Node.js compat-layer JS, not native code — no memory-safety or GC concerns. The behavior change (silent wrong-port bind → explicit 'error' event) is strictly an improvement over a documented silent failure (issue #22559), and it mirrors what net.Server.listen({ fd }) already does in Bun, so no new policy is being introduced. The PR explicitly defers actual fd adoption (which would need a native uSockets listen-from-fd path) to a separate discussion, which is the right scoping.
Other factors
- Both prior review threads are resolved: my earlier note about the hardcoded POSIX errno was addressed in eccae3a (now
process.platform === 'win32' ? -4071 : -22, matching thechild_process.tsprecedent), and CodeRabbit's suggestion to asserterrnoin the test was applied. - No CODEOWNERS coverage on the modified files.
- The
fd >= 0gate matches Node's ownisValidFdsemantics (non-negative number) and correctly leavesfd: undefined/ absent untouched so existingport/pathcallers are unaffected. - Test uses
try/finallyfor donor cleanup, wires both'error'and'listening'to the awaited promise, and is skipped on Windows where the donor-fd fixture isn't meaningful.
|
CI status: the diff is green. Both builds (69490 and the re-roll 69582) passed the new The failing files are all in unrelated subsystems and differ between the two runs:
This change only touches Ready for a maintainer merge or manual retry on the red lanes. |
|
#44013 makes One difference: the test here listens on the descriptor of a live This PR stays open until a maintainer decides on #44013. |
Fixes #22559
What
http.Server.listen({ fd })(socket activation / inherited listener: systemdLISTEN_FDS, inetd, a privilege-dropping supervisor) silently ignored thefdoption. Thelisteningevent fired, but the server bound a fresh random ephemeral port on::instead of adopting the already-bound descriptor. The process reported listening, health checks that poll the process passed, and the deployment was unreachable on the socket the supervisor was holding, with no error anywhere.net.Server.listen({ fd })already reportsEINVALfor the same unsupported operation; only thenode:httppath failed silently.Repro
Cause
Server.prototype.listeninsrc/js/node/_http_server.tsnever readarg0.fd. With noport/path, the normalizer defaultedportto0and handed that toBun.serve, which bound an ephemeral TCP port.Bun.servehas no way to adopt an existing listening descriptor (uSockets exposes no listen-from-fd entry point), so the option had nowhere to go.Fix
Read
fdfrom the options object and, when present, throw the sameEINVAL(syscall: "listen") thatnet.Server.listen({ fd })reports. The throw goes throughlisten's existing try/catch, so it surfaces as anerrorevent, matching Node's listen error semantics and Bun's ownnode:netbehavior. Callers now learn immediately that the socket was not adopted instead of silently serving on the wrong port.Actually adopting an inherited listening fd would require a new native uSockets listen-from-fd path plus an
Address::Fdvariant in the server config; that is a larger, cross-cutting change worth a separate discussion.Verification
test/js/node/http/node-http-listen-fd.test.ts: a donornetserver provides a real listening fd;http.Server.listen({ fd })must emiterrorwith{ code: "EINVAL", syscall: "listen" }.main(emitslisteningon a random port, e.g.::43749).