Repository navigation
Conversation
http.Server is backed by Bun.serve and never went through net.Server's onconnection path, so the maxConnections check in net.ts was never reached: with maxConnections = 2, every concurrent connection was accepted, every request served, and no 'drop' event fired. onServerConnection (the uws accept filter) now applies the same check net.Server uses: once the tracked-connection count reaches maxConnections, further accepts emit 'drop' with the peer/local address data and close the socket before the connectionListener runs.
WalkthroughChangesThe HTTP server now enforces HTTP connection limits
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:30 PM PT - Jul 21st, 2026
✅ @robobun, your commit e61b64774e4795e4245a7c9269e32ad11ac346f9 passed in 🧪 To try this PR locally: bunx bun-pr 35022That installs a local version of the PR into your bun-35022 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#34432 does include an equivalent check in The two implementations differ only in ordering: #34432 constructs the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.test.ts`:
- Around line 4125-4127: Bound the connection-count polling loop around
server.getConnections so it stops after a finite number of attempts or a bounded
wait window. After polling, assert the final connection count is 1 before
opening the replacement connection, while preserving the existing setImmediate
retry behavior.
- Line 4065: Add a separate fresh-server test case alongside the existing
server.maxConnections coverage that sets maxConnections to 0, then assert the
first connection is dropped, no request handler runs, and getConnections()
remains zero. Keep unset and positive-limit cases distinct to explicitly cover
each input space.
- Around line 4071-4080: Update openAndRequest and the corresponding promise
flow around the other affected socket setup to reject on client socket error
instead of using no-op "error" listeners. Preserve the existing close rejection
and response resolution behavior, ensuring connection failures cannot be
classified as a successful dropped outcome.
🪄 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: 8732d502-e762-4fac-99d3-741eff77d662
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/http/node-http.test.ts
Matches net.ts's ordering: a throwing 'drop' listener was leaving the socket open, so the native caller swallowed the exception and onNodeHTTPRequest went on to wrap and serve the over-limit connection (Node does not serve it). The address reads stay before close so the payload is populated.
There was a problem hiding this comment.
LGTM — the close-before-emit ordering fix addresses the last concern; the check now mirrors net.ts's maxConnections path exactly.
What was reviewed:
>= maxConnectionscomparison and!= nullguard matchsrc/js/node/net.ts:997;kTrackedConnections.sizeis the same countergetConnections()reads.socketHandle.remoteAddress/localAddressreturn{address, port, family}(same accessorsNodeHTTPServerSocketuses at :1839-1847), and are captured beforeclose().- Test now rejects on close-without-body and releases all clients + server in
finally; the intentional no-operrorhandler on the drop-path clients is sound (ECONNRESET is expected there).
Extended reasoning...
Overview
Adds a maxConnections check to onServerConnection in src/js/node/_http_server.ts (~20 lines) so http.Server enforces the limit and emits 'drop' like net.Server already does. The check reads addresses, calls socketHandle.close(), then emits 'drop' with the standard 6-field payload — the same shape and ordering as the sibling at src/js/node/net.ts:997-1008. A ~80-line test in node-http.test.ts verifies served count, drop count, drop payload, getConnections(), and slot recovery after freeing a connection.
Security risks
None. This is a connection-limit enforcement that only makes the server more restrictive; no auth, crypto, or parsing of untrusted data is involved. The address fields in the drop payload are read from the native handle via the same accessors already used by NodeHTTPServerSocket.
Level of scrutiny
Moderate — Node compat code in the accept path. But the change is a direct port of the existing net.ts pattern (same != null guard, same >= comparison, same close-then-emit ordering), and socketHandle.close() / .remoteAddress / .localAddress are already exercised elsewhere in this file. The counter (kTrackedConnections.size) is the same one getConnections() reports, so accounting is consistent.
Other factors
This PR has been through two prior review rounds from me: (1) test hang-on-close and missing try/finally cleanup — both fixed in 9175b86; (2) emit-before-close ordering allowing a throwing 'drop' listener to bypass the limit — fixed in 9f92ff4. Three CodeRabbit findings were declined with reasoning I agree with (the ECONNRESET-tolerant no-op error handler is intentional; the = 0 boundary goes through the same comparison; the unbounded poll matches file convention). All threads are resolved. The overlap with #34432 was acknowledged — this is the standalone version of one hunk from that larger PR.
On hosts where localhost prefers ::1, listen(0, 'localhost') binds the proxy to IPv6 while the client resolves to 127.0.0.1, so the test fails with ECONNREFUSED independent of the code under test. Bind and connect on the same literal address.
There was a problem hiding this comment.
LGTM — all three prior findings (test cleanup in finally, openAndRequest close-rejection, close-before-emit ordering) are addressed, and this run found nothing new.
What was reviewed:
- The
maxConnections != nullguard means the new branch is a no-op unless the user explicitly setsmaxConnections, so existing servers are unaffected. socketHandle.remoteAddress/.localAddress/.close()are all existing native accessors already used elsewhere in this file; the>=comparison and payload shape matchnet.ts:997-1008.onServerConnectionis registered unconditionally viaapplyServerCustomOptions, so the check covers every accept.- The
node-http-proxy.jsfixture change is a triviallocalhost→127.0.0.1hardening.
Extended reasoning...
Overview
This PR adds ~20 lines to onServerConnection in src/js/node/_http_server.ts so that http.Server#maxConnections is enforced the same way it already is for net.Server: once the tracked-connection set reaches the limit, the native handle is closed and a 'drop' event is emitted with the standard address payload, before any NodeHTTPServerSocket wrapper or 'connection'/'request' dispatch. It also adds a ~80-line test in node-http.test.ts covering served/drop counts, drop payload shape, getConnections(), and slot recovery, plus a two-line localhost → 127.0.0.1 binding fix in the node-http-proxy.js fixture.
Security risks
None. This is a resource-limiting feature (rejecting excess connections), not auth/crypto/permissions. The new code reads two address getters and calls .close() on a native handle — all pre-existing operations already used elsewhere in the same file. No user input is parsed.
Level of scrutiny
Medium-low. The accept path is hot, but the new branch is gated on maxConnections != null, and Server does not initialize maxConnections — it is undefined unless the user explicitly sets it. So for every existing server that hasn't set the option, this is a single != null check that falls through. The only affected users are those who set maxConnections and were previously getting silently-ignored behavior, which was the bug. The implementation is a direct copy of the sibling check in src/js/node/net.ts:997-1008 (same != null guard, same >= comparison, same payload keys, same close-before-emit ordering).
Other factors
I reviewed this PR twice previously and raised three findings (resource cleanup on assertion failure, openAndRequest hanging on clean close, and emit-before-close ordering allowing a throwing listener to bypass the limit). All three were fixed in commits 9175b86 and 9f92ff4 and the threads are resolved. CodeRabbit raised three more; two were declined with sound reasoning (intentional no-op error handler because the drop path can surface as ECONNRESET; unbounded poll matches file convention) and one was withdrawn. The bug-hunting system found nothing on the current revision. The test is hermetic (127.0.0.1, port 0, try/finally cleanup), asserts server-side invariants rather than timing, and the PR description confirms it fails under USE_SYSTEM_BUN=1 and passes under the debug build. The overlap with #34432 is acknowledged and non-blocking — whichever lands first, the other rebases.
|
Closing: the same behavior landed on main in #34432. _http_server.ts now checks server.maxConnections on the accept path, destroys the excess connection and emits 'drop' with the same address fields (src/js/node/_http_server.ts, around line 1216). The test this PR adds to test/js/node/http/node-http.test.ts (http.Server maxConnections destroys the excess and emits 'drop' like net.Server) passes unmodified against a debug build of main at 04148c8, twice in a row. |
2 participants
Problem
server.maxConnectionsdid nothing on anhttp.Server. WithmaxConnections = 2, every concurrent connection was accepted, every request answered,getConnections()counted them all, and no'drop'event fired. The same option works on a plainnet.Server(#953), buthttp.Serveris backed byBun.servedirectly and never goes throughnet.Server'sonconnectionpath where the check lives.Repro
Cause
src/js/node/_http_server.ts'sonServerConnection(the uws accept filter registered byapplyServerCustomOptions) wrapped every accepted socket in aNodeHTTPServerSocketand emitted'connection'unconditionally. ThemaxConnectionscheck insrc/js/node/net.ts'sonConnectionis only reached bynet.createServer()/tls.createServer(), not by theBun.serve-backed http server.Fix
Apply the same check
net.Serveruses, inonServerConnection: once the tracked-connection count reachesmaxConnections, emit'drop'with the same{localAddress, localPort, localFamily, remoteAddress, remotePort, remoteFamily}payload and close the native socket before aNodeHTTPServerSocketis created, so no'connection'/'request'is dispatched for the excess.Verification
New test in
test/js/node/http/node-http.test.tsopens two keep-alive connections (served), then two more (dropped), checksserved,'drop'count and payload,getConnections(), and that freeing one slot lets the next connection through. The same logic passes under Node v26.Related: #2793
[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file