Conversation
WalkthroughModifies the ChangesmaxRequestsPerSocket dropRequest fix and tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:41 PM PT - Jul 17th, 2026
❌ @robobun, your commit 30c1c77 has some failures in 🧪 To try this PR locally: bunx bun-pr 33472That installs a local version of the PR into your bun-33472 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line removal aligning maxRequestsPerSocket with Node's documented behavior, with solid test coverage.
Extended reasoning...
Overview
The production change is a single deletion in src/js/node/_http_server.ts: removing socket.destroy() from the reachedRequestsLimit branch (plus comment adjustments). The rest of the diff is test coverage in test/js/node/http/node-http.test.ts: a shared pipelineRequests helper, two new tests (pipelined and sequential over-limit requests), and a rework of the existing Connection: close test that previously depended on the server-initiated destroy to resolve.
Security risks
None identified. maxRequestsPerSocket defaults to 0 (disabled), so this only affects servers that explicitly opt in. The over-limit path still does no user-handler work — it emits dropRequest and answers 503 immediately. The connection now stays open with Connection: close advertised rather than being torn down, which is exactly Node.js's documented contract ("will set the Connection header value to close, but will not actually close the connection"). This is not a DoS-protection knob; it exists for connection rotation, and the new behavior matches the reference implementation.
Level of scrutiny
Low-to-moderate. The behavioral change is one deleted line with a direct citation to Node's parserOnIncoming and the Node.js docs. maxRequestsOnConnectionReached was already being set correctly, so renderNativeHeaders already emits Connection: close on these responses — only the premature teardown was wrong. The reworked existing test now resolves by counting arrived responses instead of waiting for a server-side close that no longer happens, which is the necessary adjustment.
Other factors
The new tests await observable conditions (response count) rather than timers, wire close/error to resolve/reject so a regression produces an assertion failure instead of a hang, use port: 0, and clean up in finally. The author reports the 402 test-http-*/test-https-* Node parallel tests have identical pass/fail sets before and after. No CODEOWNERS cover this path and no prior reviewer comments are outstanding.
|
Pushed one more test after the review: an over-limit request that carries a body ( That path is distinct from the GET case. The 503'd request's body still has to come off the wire, otherwise the parser never reaches the request pipelined behind it. Three pipelined POSTs with a |
There was a problem hiding this comment.
I didn't find any issues and the Node-compat reasoning is solid, but removing the over-limit socket.destroy() changes server connection lifetime with an acknowledged resource trade-off — worth a human sign-off.
Extended reasoning...
Overview
The production change is a single-line deletion in src/js/node/_http_server.ts: dropping socket.destroy() from the reachedRequestsLimit branch so that pipelined requests past maxRequestsPerSocket each get their own 503 + 'dropRequest' event, matching Node's documented parserOnIncoming behavior. The rest is comment updates and four well-constructed tests in test/js/node/http/node-http.test.ts (pipelined GETs, pipelined POSTs with bodies, sequential requests, and a rework of the existing Connection: close assertion).
Security risks
No injection/auth/data-exposure concerns. The relevant risk is resource exhaustion: without the destroy(), a client that ignores Connection: close can keep the socket open and keep collecting 503s. The PR explicitly analyzes this and argues (convincingly) that (a) Node does exactly this and reaps via keepAliveTimeout, and (b) Bun's node:http server already passes idleTimeout: 0 so no keep-alive socket is reaped today regardless — so this doesn't open a hole that wasn't already open. That analysis reads correct to me, but it's a judgment call about acceptable server behavior that a maintainer should confirm.
Level of scrutiny
Medium-high. The diff is tiny, but it removes a defensive connection teardown from the node:http server request-dispatch path. Connection-lifecycle changes in server code deserve a human look even when the compat justification is clear.
Other factors
- The fix matches Node's source (
parserOnIncomingdoes not destroy on this path) and Node's docs verbatim. - Tests are thorough: they cover the single-segment pipeline, the body-carrying pipeline (distinct parser path), and the non-pipelined sequential case; the helper resolves on
closeso a regression produces an assertion failure rather than a timeout. - Author verified the 402
test-http-*/test-https-*parallel tests have identical pass/fail sets before and after. - No prior human reviews or outstanding comments to address.
|
Agreed that the connection-lifetime change is the part worth a human look, so here is the measurement behind the claim in the PR description rather than just the assertion. No import http from "node:http";
import net from "node:net";
const srv = http.createServer((q, r) => { q.resume(); r.end("ok"); });
srv.keepAliveTimeout = 500;
await new Promise(r => srv.listen(0, "127.0.0.1", r));
const t0 = Date.now();
const closedAfterMs = await new Promise(res => {
const s = net.connect(srv.address().port, "127.0.0.1");
s.on("data", () => {}); // must read, or the FIN never surfaces as 'close'
s.on("error", () => {});
s.on("close", () => res(Date.now() - t0));
s.on("connect", () => s.write("GET /a HTTP/1.1\r\nHost: h\r\n\r\n"));
setTimeout(() => { s.destroy(); res(-1); }, 8000);
});
console.log(closedAfterMs === -1 ? "never reaped (waited 8s)" : `reaped after ${closedAfterMs}ms`);
srv.close(); srv.closeAllConnections();So a client that holds a keep-alive socket open can already pin it indefinitely on The missing idle reaping is a real gap and I'd be happy to open a separate issue or take a shot at it, but it's a behavior change for every keep-alive connection in The alternative I considered and rejected was deferring the close until the read buffer drains. It is timing-dependent, and it still truncates a pipeline that arrives across two TCP segments, which is the same bug report with a slightly different client. |
|
Moved the Two reasons:
The new file is 4/4 green with the fix and 3/4 red without it. Happy to fold it back into |
There was a problem hiding this comment.
No bugs found and the fix matches Node's parserOnIncoming exactly, but since this removes an explicit socket teardown in the HTTP server request-limit path — and you've already flagged the connection-lifetime tradeoff yourself — I'd like a maintainer to sign off on that decision.
Extended reasoning...
Overview
The functional change is a single-line deletion in src/js/node/_http_server.ts: removing socket.destroy() from the reachedRequestsLimit branch so that every pipelined request past maxRequestsPerSocket receives its own 503 and 'dropRequest' event, matching Node.js's documented behavior. The rest of the diff is comment rewording, a new focused test file with four tests, and a pure deletion of one migrated test from node-http.test.ts.
Security risks
No injection, auth, or data-exposure surface. The relevant risk axis is resource exhaustion: without the destroy(), a client that ignores Connection: close can hold the socket open and keep collecting 503s. The PR's analysis is convincing that this exposure already exists for every keep-alive connection in Bun's node:http server (which passes idleTimeout: 0 and never enforces keepAliveTimeout), so the change doesn't create a new class of vulnerability — it moves over-limit sockets into the same bucket as every other keep-alive socket. Still, deliberately removing a teardown in server connection-management code is the kind of tradeoff a maintainer should ratify.
Level of scrutiny
Medium-high. The diff is tiny and mechanically correct, but it lives in the HTTP server's connection-lifecycle path, which is production-critical, and it changes when a socket is closed rather than what bytes are written. The author explicitly noted the connection-lifetime aspect is "worth a human look," and I agree.
Other factors
Test coverage is thorough (pipelined GETs, sequential requests, POSTs with bodies, and the pre-existing Connection: close assertion), verified against Node v26.3.0, and the author reports the 402-test node parallel http suite is unchanged. The reasoning about why deferring the close until buffer-drain would reintroduce the same bug is sound. This is close to approvable on the merits; I'm deferring only because the author themselves flagged it and because connection-teardown semantics in the HTTP server warrant explicit maintainer agreement.
|
Correction to my earlier comment, and a revert of the file split that went with it. I claimed const s = createServer((q, r) => r.end("ok"));
s.listen(0, "localhost", () => {
console.log("server bound:", JSON.stringify(s.address()));
const req = request({ hostname: "localhost", port: s.address().port, path: "/" }, res => {
res.resume();
res.on("end", () => { console.log("OK status", res.statusCode); s.close(); });
});
req.on("error", e => { console.log("ERROR:", e.message); s.close(); });
req.end();
});Node fails the same way, so this is a non-hermetic test rather than a runtime bug, and it only shows up on hosts whose resolver returns With that file runnable end to end, the separate test file had no reason to exist, so the four tests are back in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 3515-3643: These socket-spawning tests are independent and should
run concurrently instead of sequentially. Update the four new test cases to use
it.concurrent (or the equivalent concurrent test form in this file) in
node-http.test.ts, keeping the same bodies and assertions, so each test still
creates its own server and uses its own dynamic port without shared state.
🪄 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: 8574e214-2ed5-4d99-94ce-5ba07ee1fbb8
📒 Files selected for processing (3)
src/js/node/_http_server.tstest/js/node/http/node-http-proxy.jstest/js/node/http/node-http.test.ts
There was a problem hiding this comment.
The one-line fix and tests look correct and match Node's documented behavior, but since this removes an active socket teardown from a resource-limiting path (and leans on the pre-existing keepAliveTimeout gap you documented), a maintainer should sign off on the connection-lifetime tradeoff.
Extended reasoning...
Overview
The functional change is a single-line deletion in src/js/node/_http_server.ts: removing socket.destroy() from the maxRequestsPerSocket over-limit path so that every pipelined request past the limit gets its own 503 + dropRequest event, matching Node's parserOnIncoming and its documented behavior. The rest of the diff is test coverage (three new pipelining tests plus a rework of the existing Connection: close test in node-http.test.ts) and an unrelated hermetic-test fix in node-http-proxy.js (localhost → 127.0.0.1).
Security risks
maxRequestsPerSocket is a resource-limiting knob. Previously Bun forcibly tore down the connection once the limit was exceeded; after this change the connection stays open and keeps answering 503s until the client closes it or an idle timeout fires. The author correctly points out that Bun's node:http server currently passes idleTimeout: 0 and never reaps keep-alive sockets anyway, so this doesn't open a new class of exposure — over-limit sockets just join the same unbounded-lifetime bucket every other keep-alive socket is already in. That argument is sound, but it's still a deliberate relaxation of teardown behavior on a limiting path, and the author themselves flagged it as "the part worth a human look."
Level of scrutiny
This is production HTTP server request-dispatch code. The mechanical change is trivial and clearly matches upstream Node, but the semantic change (connection lifetime after the request cap is hit) is a judgment call a maintainer should make — particularly whether to land this as-is and track the missing keepAliveTimeout reaping separately, or require both to land together.
Other factors
The tests are well-constructed: port: 0, error handlers wired to reject, completion driven by counting response status lines rather than sleeps, and a close fallback so a mid-pipeline teardown surfaces as an assertion failure instead of a hang. The node-http-proxy.js change is a straightforward test-hermeticity fix. No prior human review on the thread; no bugs found by the bug-hunting pass.
…n 503 The over-limit handler wrote one 503 and then destroyed the socket, which tore down the connection while requests pipelined behind it were still in the read buffer. Only the first request past the limit got a response and a 'dropRequest' event. Node's parserOnIncoming does not touch the connection here: each over-limit request gets its own 'dropRequest' and its own 503, and the response already advertises Connection: close.
A dropped request's body still has to be read off the wire, or the parser never reaches the request pipelined behind it.
node-http.test.ts carries an unrelated proxy test that cannot connect in a sandbox with HTTP_PROXY set, which makes the whole file un-runnable there. The per-option layout matches node-http-maxHeaderSize.test.ts and friends.
The file uses bun:test directly, like its sibling focused files.
proxyServer.listen(0, "localhost") binds ::1 wherever the resolver prefers IPv6, while the client dials 127.0.0.1 and the connect is refused. Node fails the same way there, so the test is not hermetic rather than the runtime being wrong. exampleSite() already binds 127.0.0.1.
It was split out only because that file could not run to completion; with the proxy test pinned it can, so the tests go back beside the rest of the node:http server coverage.
The sequential test had its own copy of the connect/accumulate/close/error scaffolding; only the point at which the next request is written differed. Also run the four tests concurrently, they share no state.
There was a problem hiding this comment.
I didn't find any bugs, but this removes the socket teardown on the maxRequestsPerSocket over-limit path — a connection-lifetime change to a resource-limiting mechanism that (as you noted yourself) is worth a human look, especially given Bun doesn't yet reap idle keep-alive sockets.
Extended reasoning...
Overview
The runtime change is a single line: remove socket.destroy() from the reachedRequestsLimit branch in src/js/node/_http_server.ts, so that every pipelined request past maxRequestsPerSocket gets its own 503 + 'dropRequest' event instead of the connection being torn down after the first one. This matches Node's parserOnIncoming and its documented behavior. Alongside it: four new tests in node-http.test.ts (pipelined GET, pipelined POST-with-body, sequential-after-limit, and a rework of the existing Connection: close assertion), plus an unrelated hermeticity fix pinning node-http-proxy.js to 127.0.0.1.
Security risks
maxRequestsPerSocket is a resource-limiting knob. Removing the forced teardown means a client that ignores Connection: close can keep the socket open and keep collecting 503s. The author's argument — that Bun already passes idleTimeout: 0 and never reaps idle keep-alive sockets, so this moves over-limit connections into the same bucket every other keep-alive connection is already in rather than opening a new hole — is well-reasoned and empirically demonstrated in the PR thread. It also matches Node exactly. But it's still a deliberate loosening of a limit-enforcement path that a maintainer should sign off on, particularly the decision to defer the idle-reaping gap to a separate change.
Level of scrutiny
Medium-high. The diff is tiny, the Node-compat justification is solid, and test coverage is thorough (three of the four new tests fail on main). But this is the core node:http server request path, and the author explicitly flagged the connection-lifetime aspect as "the part worth a human look." That's the right call.
Other factors
- CodeRabbit's two review comments (dedupe socket-collection helper, use
it.concurrent) were both addressed in commit 2eb1eaa and marked resolved. - The
node-http-proxy.jslocalhost→127.0.0.1 change is a straightforward test-hermeticity fix and low risk on its own. - No prior human reviews on the PR; CI build was still in progress at the time of the last timeline entry.
2eb1eaa to
e2b4ca4
Compare
Not closing the socket at all left the close to whenever GC finalized the Bun.serve wrapper: `Server.prototype.close` drops the JS reference and `stop()` downgrades it to a Weak, so `schedule_deinit` -> `app.close()` only runs once the wrapper is collected. With no keep-alive reaping in Bun, a connection whose requests all completed could stay open indefinitely, and test-http-keep-alive-drop-requests.js only passed when a GC happened to run. Ending the socket on the over-limit response's "finish" is too early - the requests pipelined behind it have not been parsed yet. Defer it instead, so the current read is drained, every buffered request gets its own 503, and only then does the connection end.
Replacing the sequential test left the "sequential" branch of the helper without a caller.
Unrelated flakes on install/run/spawn lanes (spawn.test.ts timeout, spawn-pipe-leak, bun-install, no-orphans), all with retries; none touch the node:http diff.
There was a problem hiding this comment.
Both of my earlier concerns are addressed and I found nothing further, but the deliberate divergence from Node's documented "will not actually close the connection" behavior (as a workaround for the pre-existing keep-alive-reaping gap) is a design tradeoff worth a maintainer's sign-off.
Extended reasoning...
Overview
The runtime change is small: in src/js/node/_http_server.ts, the reachedRequestsLimit branch replaces socket.destroy() with a one-shot setImmediate(endSocketAfterDroppedRequests, socket), guarded by a new kEndAfterDroppedRequests symbol so it's scheduled once per socket. A new module-level helper and symbol are added. test/js/node/http/node-http.test.ts gains a sendRequests helper and four it.concurrent tests covering pipelined GETs, pipelined POSTs with bodies, the Connection: close header on the 503, and server-initiated connection close. test/js/node/http/node-http-proxy.js pins both ends to 127.0.0.1 (unrelated hermeticity fix).
Security risks
None identified. The change affects only the over-limit path when maxRequestsPerSocket is set (default 0/off). Deferring the close via setImmediate rather than destroying immediately does not create a new resource-exhaustion vector — the socket is still ended deterministically after one event-loop turn, and the author measured that idle keep-alive connections were already unreaped on main regardless of this option.
Level of scrutiny
Medium-high. This is production node:http server code on a connection-lifetime path, and the fix rests on a uWS implementation detail (all requests in a single read buffer are dispatched synchronously within one onData, so setImmediate runs strictly after the last pipelined request). The PR also intentionally diverges from Node's documented behavior — Node keeps the connection open and answers indefinitely with 503s; this ends it after draining the current buffer — as a pragmatic workaround for Bun's missing keepAliveTimeout reaping. That tradeoff is clearly stated in the PR description and comments, but it's the kind of Node-compat call a maintainer should ratify.
Other factors
Both of my earlier inline comments were addressed with measured evidence: the GC-dependent close in test-http-keep-alive-drop-requests.js was fixed by the deferred-end approach (52ab594), and the dead "sequential" mode in the test helper was removed (3a9ffbc). CodeRabbit's dedup/concurrency suggestions were also applied. The four new tests are hermetic (own server on port 0, error wired to reject, resolve on close), and the author reports the vendored Node parallel tests are unchanged. The remaining commit since my last review (45698bcf) is a CI retrigger with no code change.
Status: diff is green, CI red is infra/unrelatedBuild 75052 (at
Earlier builds (69181, 69271, 74720) each went red on a different set of unrelated install/spawn/napi/fs/webview flakes. Locally I've used my one empty retrigger and I'm not going to push empty commits for queue backlogs or unrelated flakes. This is ready for a maintainer; the one call worth a human eye is the connection-lifetime tradeoff described in the PR body (deterministic teardown vs. Node's "never close"), which the automated reviews also flagged for sign-off. |
…eline Resolve conflict in _http_server.ts: keep the deferred-end approach (setImmediate after the read buffer is fully parsed) and make it pipeline-aware so a queued 503 behind an async response closes after the last queued response drains rather than mid-pipeline.
On main the first queued 503 carries kMustCloseConnection, so socket.end() runs on its 'finish' and advanceResponsePipeline bails before the second queued 503 is written. The deferred end now marks the last queued response instead, so the whole pipeline drains.
|
Merged main (post-#32488) and resolved the conflict in #32488 added an New test covers that path (async handler so the 503s are queued behind an in-flight response): fails on main with node-http.test.ts: 136 pass. |
…arked response The deferred end snapshotted the queue tail at check-phase time and marked it with kMustCloseConnection. An over-limit request arriving in a later read while the first response was still in flight grew the queue past that tail, so when the marked response finished the socket closed before the later 503 was replayed: N dropRequest events, <N 503s on the wire. Move the close decision out of per-response marks. The setImmediate now just flips kEndAfterDroppedRequests from "scheduled" to "fired", and onResponseFinishHandleSocket ends the connection when the pipeline has drained (the same queue-empty check it already makes before arming the keep-alive timeout). The two states keep a 503's own 'finish' during the per-request drainMicrotasks() from closing the socket before the next pipelined request is dispatched. Tests: - the async-handler test now outlives two check phases so the deferred end observes a queued pipeline - a new test pins the later-read case: /a+/b in one read, then /c after the deferred end has fired, then the handler responds
…x-requests-per-socket-pipeline
Status update: redesigned the deferred end after two more review catchesBoth about the post-#32488-merge reconciliation. Both verified with probes, both fixed in
New approach: stop marking per-response entirely. Tests: one new (later-read, barrier-driven, no wall-clock timing); the async-handler test hardened to The previous CI builds (69181, 69271, 74720) each went red on a different set of install/spawn/napi/fs/webview flakes; none referenced |
… early Racing the second 'dropRequest' against the socket closing turns a regression that ends the connection before /c is dispatched into an assertion failure instead of a hang on the missing event. The finally now releases the handler barrier and destroys the socket, so a failure does not leave /a parked and the connection un-reapable.
There was a problem hiding this comment.
No bugs found this pass, but this changes connection-teardown semantics in the node:http server and deliberately diverges from Node's documented "will not actually close the connection" behavior — that tradeoff (which the PR body itself flags) is a maintainer call, so deferring rather than auto-approving.
Checked: the tri-state kEndAfterDroppedRequests flag against mid-dispatch drainMicrotasks() closes — the === symbol guard holds. The new socket.end() in onResponseFinishHandleSocket sits after both the in-flight and queued-pipeline early returns, so it can't fire while responses are pending. Earlier concerns (later-read truncation, untested queue branch, test cleanup on the barrier path, comment-length cap) are all addressed in the current diff.
Extended reasoning...
Overview
The PR replaces the immediate socket.destroy() in the maxRequestsPerSocket over-limit path with a deferred close-on-drain: a tri-state kEndAfterDroppedRequests symbol (true = scheduled, symbol-itself = fired) is set on the socket, endSocketAfterDroppedRequests runs at setImmediate, and onResponseFinishHandleSocket ends the connection once the pipelined-response queue is empty. Six new tests in node-http.test.ts cover single-segment pipelines, POST bodies, async handlers (two check phases out), a barrier-driven later-read case, and server-initiated close. node-http-proxy.js is a two-line hermeticity fix (localhost → 127.0.0.1).
Security risks
None identified. This is request-limit accounting on an already-accepted connection; no auth, crypto, or untrusted-input parsing changes. The over-limit path already existed — only the teardown timing changed.
Level of scrutiny
High. _http_server.ts is a hot path in a hostile-userland builtin, and connection lifecycle here interacts with the pipelined-response queue, drainMicrotasks() inside the per-request dispatch loop, and the keep-alive timeout added in #32488. Two earlier iterations of this fix had real correctness bugs (later-read truncation via stale tail-marking; an untested load-bearing branch), both caught in review and both fixed by the current close-on-drain redesign. That churn is a signal the interactions are subtle.
Other factors
The PR explicitly trades away Node's documented "will not actually close the connection" for deterministic teardown after the buffered pipeline drains. That's defensible (Bun's keep-alive reaping only just landed, and the reporter asked for exactly this), but it's a Node-compat design choice a maintainer should sign off on rather than an auto-approve. All prior inline review threads are resolved; test coverage is thorough and barrier-driven with proper failure wiring and cleanup.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
With
server.maxRequestsPerSocketset, only the first request past the limit gets a 503 and a'dropRequest'event. Bun then destroys the connection, so any requests the client pipelined behind it are never answered.Node answers every request past the limit: each one gets its own 503 and its own
'dropRequest'.Repro
The same gap shows up without pipelining: after the limit is reached, a second request on the socket gets no response at all, because the connection is already gone.
Cause
src/js/node/_http_server.tscalledsocket.destroy()immediately after writing the over-limit 503:Requests are dispatched one at a time as the parser walks the socket's read buffer, so closing the handle there discards everything still sitting behind the current request.
Node's
parserOnIncomingdoes not touch the connection on this path:It relies on the
Connection: closeheader thatmaxRequestsOnConnectionReachedalready puts on the response. This is the documented behaviour: "When the limit is reached it will set theConnectionheader value toclose, but will not actually close the connection, subsequent requests sent after the limit is reached will get503 Service Unavailableas a response."Bun already sets
maxRequestsOnConnectionReached, so theConnection: closeheader was being advertised correctly and only the teardown was wrong.Fix
Drop the
socket.destroy(), and end the connection once the response pipeline has drained. In the over-limit branch:The deferred callback flips the flag from
trueto the symbol itself ("the current read is fully parsed") and ends the connection if nothing is in flight or queued. OtherwiseonResponseFinishHandleSocketends it once the pipeline drains, at the same queue-empty point it already checks before arming the keep-alive timeout. No per-response mark can go stale (a later read growing the queue past a marked tail) because nothing is marked.The two flag states are needed because
drainMicrotasks()in the per-request dispatch can fire a 503's own'finish'before the next pipelined request is dispatched. Closing on the rawtrueflag there re-introduces the original truncation with a synchronous handler.Why the close has to be deferred rather than dropped or immediate
Three placements, only the third works:
socket.destroy()). Tears the connection down mid-pipeline. This is the bug."finish", viakMustCloseConnection.'finish'fires before the next pipelined request is dispatched, so the pipeline is still truncated.Not closing at all was the first version of this PR and is wrong for Bun specifically. Node gets away with never closing because
keepAliveTimeoutreaps the socket. Bun had no keep-alive reaping (the merge that landed #32488 added it), andserver.close()→stop()downgrades the JS reference to a Weak, soschedule_deinit→app.close()only runs once the wrapper is GC-finalized. That lefttest-http-keep-alive-drop-requests.jspassing only when a GC happened to run.Cost
This gives up node's documented "will not actually close the connection": a client that keeps sending past the limit now gets hung up on instead of an endless stream of 503s. In exchange the connection teardown is deterministic, which is what the reporter asked for ("keep draining the already-buffered pipelined requests ... and only then close").
Verification
Six tests in
test/js/node/http/node-http.test.ts:200/503/503and twodropRequestevents carrying the rightreq.urland the same socketConnection: closeassertion, reworked so it no longer waits on the server-initiated close it was observingFive of those fail on
mainwith["200", "503"]or["200"].test-http-keep-alive-drop-requests.jsandtest-http-keep-alive-pipeline-max-requests.jspass 5/5 each. The 402test-http-*/test-https-*tests intest/js/node/test/parallelhave the same failure set before and after.Also in here
test/js/node/http/node-http-proxy.jslistened onlocalhostand then dialledlocalhost. Where the resolver prefers IPv6 the server binds::1, the client dials127.0.0.1, and the connect is refused, sorequest via http proxy, issue#4295fails on those hosts. Node fails the same way, so the test is not hermetic rather than the runtime being wrong. Pinned both ends to127.0.0.1, matching theexampleSite()it talks to. Two lines, easy to split out if preferred.[review] gate passed · iteration 6 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 6
evidence per changed file