Repository navigation
Conversation
…e end The idle sweep behind node:http server.close()/closeIdleConnections(), Bun.serve closeIdleConnections() and graceful stop() keyed off a per-socket isIdle flag that markDone() set when a response finished and onData cleared whenever bytes arrived. That destroyed connections still receiving a request body or the head of a pipelined request, and never reclaimed a connection whose body finished arriving after its response. Replace the flag with HttpResponseData::isIdle(), derived from a betweenRequests bit maintained at the points the node:http headers/request timeout tracking already used (message complete sets it; a parsed or partially buffered head clears it; connections start busy, like Node's ConnectionsList), plus the existing response-pending and pipelined-queue state. The timeout sweep reads the same bit instead of lastMessageStartMs == 0. Also complete a Content-Length: 0 message whose head was finished out of the fallback buffer: that pass skipped the body dispatch, so the empty fin chunk was never delivered and such a connection never counted as between requests.
|
Warning Review limit reached
Next review available in: 9 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 (12)
Comment |
|
Updated 3:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit a091992 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37889That installs a local version of the PR into your bun-37889 --bun |
|
Status Reproduced on bun 1.4.0 and on an unmodified debug build of main with one raw TCP connection per state and a single Fix and tests are in this PR; the new tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it redesigns how the uWS HTTP layer classifies idle connections (affecting both Bun.serve and node:http), touches the parser's fin-chunk delivery for the split-head Content-Length: 0 case, and carries an intentional behaviour change (res.end(); server.close() inside a handler no longer closes that handler's own connection), a human look would still be worthwhile.
What was reviewed:
betweenRequestsmaintenance inHttpContext::onData(set on fin before delivery, cleared on head parsed / partial head buffered) and its use inisIdle()/shouldCloseConnection()/isRequestTimedOut.HttpParser.hConsumeMinimallychange: withremainingStreamingBytes == 0the block emits an empty fin chunk (emittable = 0), matching the no-body branch and the single-read path.- Confirmed the removed
AsyncSocketData::isIdlefield /markDone(uwsRes)param /WebSocketDatawrite have no remaining readers;isConnectRequestinisIdle()preserves the prior CONNECT-tunnel behaviour for both server personalities.
Extended reasoning...
Overview
This PR replaces the per-socket AsyncSocketData::isIdle flag (set at response end, cleared on any incoming bytes) with a derived HttpResponseData::isIdle() computed from three independently-maintained facts: a new betweenRequests bit (set when a request message's body fin arrives, cleared when the next head is parsed or a partial head is buffered), HTTP_RESPONSE_PENDING, and nodeHttpQueuedPipelinedCount. App::closeIdle, the HTTP_CLOSE_WHEN_IDLE gate in shouldCloseConnection(), and the node:http isRequestTimedOut sweep all now key on this. It also fixes an adjacent parser gap where a Content-Length: 0 request completed out of the fallback buffer never received its empty fin chunk. Mechanical fallout: markDone() loses its uwsRes parameter (four call sites updated in libuwsockets.cpp plus two in HttpResponse.h), and the dead WebSocketData isIdle = false write is removed. Three test files gain matrix / regression coverage.
Security risks
None identified. The change is server-side connection lifecycle bookkeeping; it does not touch auth, TLS, input validation, or any parsing of untrusted bytes beyond the narrow Content-Length: 0 fin-delivery fix (which only adds an empty-string dataHandler call that the single-read path already made).
Level of scrutiny
High. packages/bun-uws/src/HttpContext.h / HttpResponseData.h / HttpParser.h are the core HTTP request/response state machine shared by every Bun.serve and node:http connection. The PR redefines what "idle" means for closeIdleConnections() / graceful stop() and changes when shouldCloseConnection() fires under HTTP_CLOSE_WHEN_IDLE. The description explicitly calls out a user-visible behaviour change (res.end(); server.close() from inside a handler no longer closes that connection immediately, matching Node) — that is the correct behaviour by the stated contract, but it is the kind of semantic shift a maintainer should sign off on.
Other factors
The PR is exceptionally well-argued and the test coverage is strong: the node-http matrix test enumerates all eight connection states with positive-signal outcomes (no sleeps), the split-head Content-Length: 0 regression is covered for both the idle sweep and the requestTimeout 408 path, and the Bun.serve test exercises closeIdleConnections() mid-upload plus graceful stop() drain. I traced the betweenRequests transitions against the onData requestHandler/dataHandler control flow and the post-parse partial-head check; the ordering (fin sets it before the chunk is delivered, so a res.end() inside req.on('end') sees the connection idle) is deliberate and covered by the bodydone row. I also verified that in the new HttpParser.h branch, remainingStreamingBytes == 0 forces emittable = 0, so the added dataHandler call is exactly the empty fin chunk the no-body branch already emits — no body bytes are consumed from the fallback buffer. Given the breadth of the state machine touched and the intentional behaviour change, deferring to a maintainer rather than auto-approving.
|
#48228 (draft) now covers the What this PR has that #48228 does not:
A merge of this branch with main now conflicts in 7 of its 12 files. |
|
Closing as superseded. Each part of this PR is on main or in a newer open PR. I ran the five tests that this PR adds on a debug build of main (2596ac5, Linux x64), without the source changes of this PR.
Two rules of this PR are not carried forward:
One difference in the |
Problem
closeIdleConnections()(node:http andBun.serve, so alsoserver.close()and gracefulstop()) destroys a keep-alive connection whose response was sent while the request body is still arriving. Closing a socket with unread inbound data resets it, which can discard the response the client was about to read.close()waits on that connection until the client or the keep-alive timeout ends it.Content-Length: 0request whose head arrived split across reads never counted as complete, so the nextrequestTimeoutsweep answered408 Request Timeouton an idle keep-alive connection (reproduced on 1.4.0; node keeps it).Fix
Content-Length: 0request in the same pass, as the no-body branch already did.res.end(); server.close()from inside a handler no longer closes the handler's own connection (the request is still being received), as in node.Bun.servetest that also fail without the fix. The existing http, serve and websocket suites and 50 vendored node tests were run; the only failures also fail on an unmodified build.Background
ConnectionsListtracks whether a message is currently being received;closeIdleConnections()and theheadersTimeout/requestTimeoutsweep both read that state, and a connection that has never sent a request counts as busy.Bun.servesit on uWS inpackages/bun-uws; one per-connection data block holds parser state, response-pending state and the pipelined-response count, so an idle definition built from those applies to both servers. Connections accepted bylisten()take this native path; node:http: only treat HTTP/1 fallback connections that are between requests as idle #37717 covers the JS fallback path (server.emit("connection"), http2allowHTTP1).stop()marks busy connections close-when-idle, and the same idle definition decides when that mark fires.Original description
What
http.Server#closeIdleConnections()(and thereforeserver.close()),Bun.serve'sserver.closeIdleConnections()and the close-when-idle mark left by a gracefulserver.stop()all go through uWSApp::closeIdle. It decided "idle" from a per-socket flag that meant "a response has finished and no bytes have arrived since". Node decides it from the request side: a connection is idle only between request messages (ConnectionsList::idle(), i.e. the last message completed and the next one has not started; a connection that has not sent its first request yet counts as busy), and only if the response assigned to it, if any, has finished.The two definitions disagree in both directions. One real TCP connection per state, one
closeIdleConnections()call, node v26.3.0 vs bun 1.4.0 / main:closeIdleConnections()called from inside the request handlerEvery kept connection still gets served afterwards (node and bun after), and
close()afterwards takes all of them. The "body still arriving" case is the harmful one: closing a socket that still has unread inbound data resets it, which can discard the response the client was about to read. The "body finished afterwards" case leavesclose()waiting on a connection that is idle by every definition until the client or the keep-alive timeout closes it.This is the same divergence #37717 fixes for the JS fallback path (
server.emit("connection"), http2allowHTTP1); this PR is the native path for connections accepted bylisten(). Once both are in, the two matrix tests can be folded into one parameterized over the transport.Cause
AsyncSocketData::isIdlewas set inmarkDone()(response end) and cleared at the top ofonData(any bytes). Nothing re-armed it when a request message completed, and a response ending mid-message set it regardless. The request-side state node keys on already existed on the node:http ext block (lastMessageStartMs, stamped at accept / head parsed / partial head buffered and zeroed at message complete, used by the headersTimeout/requestTimeout sweep), it just was not what the idle sweep looked at.Fix
HttpResponseDatagets abetweenRequestsbit, maintained at exactly the points the node:http timing already used, for both server personalities: the data handler's fin sets it (before the chunk is delivered, so a response completing from inside the delivery already sees the connection idle), a parsed request head clears it, a partial head left in the fallback buffer after a read clears it, and it starts out false (node registers the parser as active at setup, so a connection that never sends anything isheadersTimeout's problem, not an idle one).isIdle()is derived from it:betweenRequests && !HTTP_RESPONSE_PENDING && nodeHttpQueuedPipelinedCount == 0.App::closeIdleand theHTTP_CLOSE_WHEN_IDLEgate (shouldCloseConnection()) use it; theisIdleflag, its maintenance inmarkDone()/resetResponseState()/onData, themarkDone(uwsRes)parameter that existed only to set it, and the deadWebSocketDatawrite are gone. A connection that became a CONNECT/Upgrade tunnel has no further request messages, so it is judged by response state alone: node:http tunnels keep the response they were created from pending (never idle, as before), a Bun.serve CONNECT that was answered stays reclaimable (as before).isRequestTimedOut) readsbetweenRequestsinstead oflastMessageStartMs == 0, so the idle sweep and the timeout sweep share one definition of "between requests", which is how node'sConnectionsListserves bothcheckConnections()andcloseIdleConnections().lastMessageStartMsis now purely the timestamp.HttpParser: aContent-Length: 0request whose head is completed out of the fallback buffer (head split across reads) never got its empty fin chunk: the fallback pass skips the body dispatch and the caller only streams a body whileremainingStreamingBytesis non-zero. Without the fin such a connection would never return tobetweenRequests; today the same gap leaves the request's timeout window open, so the nextrequestTimeoutsweep answers408 Request Timeouton an idle keep-alive connection (reproduced on 1.4.0, node keeps the connection). It now completes the message in that pass too, the same way the no-body branch next to it always has; the single-read path already delivered this chunk, so consumers see nothing new.Why this is the right shape rather than patching the flag: "idle" is now computed from three facts that are each maintained in one place (message boundaries, response in flight, responses queued), and it is the definition node uses, so the rows above match by construction rather than by approximation. For Bun.serve the declared contract ("not currently sending a request or waiting for a response", #37074) was already this; the implementation now matches it, which also fixes the graceful
stop()analogue of the last table row (it no longer waits foridleTimeouton a connection whose upload finished after the response).Behaviour change worth knowing about:
res.end(); server.close()from inside a request handler no longer closes that handler's own connection; the request is still being received at that point (the handler runs from headers-complete), so, exactly as in node, the connection is left tokeepAliveTimeoutor the client. The existing test suites did not depend on the old behaviour (see below). Unchanged: a response whose bytes are still draining keepsHTTP_RESPONSE_PENDING, so it is still kept where node would destroy it mid-flush;closeIdle's iteration over the group is untouched (its cached-nexthazard is tracked separately).Tests
test/js/node/http/node-http.test.ts: the matrix above (plus aContent-Length: 0split-head row for the parser change), each state confirmed through something the server does after reaching it, each verdict read as a positive signal (a response, or the connection going away) so there are no timers; thenclose()over the kept ones. Plus the in-handler case and a test that'upgrade'/'connect'tunnels survive the sweep. The same code passes verbatim under node v26.3.0. On the unfixed build the matrix fails on exactly thebody/reused/bodydonerows and the in-handler test fails on its second request.test/js/node/http/node-http-server-timeouts.test.ts: after a split-headContent-Length: 0exchange, a second connection is left to be reaped byrequestTimeout; once it has been, the first connection must still serve a request (unfixed: it has been answered 408 and closed by the same sweep).test/js/bun/http/bun-server.test.ts: two early-answered uploads;closeIdleConnections()must close 0 (unfixed: 2), the first connection must still serve a request behind the rest of its upload, and a gracefulstop()must resolve once the second upload's body arrives.Also run against this build: the full
node-http.test.ts, the rest ofnode-http-server-timeouts.test.ts,node-http-backpressure.test.ts, the rest oftest/js/node/http/,bun-server.test.ts,serve.test.ts,websocket-server.test.ts, and 50 vendored node tests coveringserver-close-idle/close-all, everyheaders-timeout/request-timeoutvariant, keep-alive, pipelining, CONNECT and upgrade. The only failures are ones that fail identically on an unmodified build in this environment (the http proxy test, IPv6requestIP, root-range port,/bun:info,#6583,localhostupload abort, and a handful of 5 s / 10 s debug-build timeouts that reproduce on an unmodified debug build; thenode:httpfuzz fixture'sserver.close()was timed at ~85 ms with this change).