node:http: enforce server headersTimeout and requestTimeout - #33061
Open
Jarred-Sumner wants to merge 14 commits into
Open
Jarred-Sumner wants to merge 14 commits into
Jarred-Sumner wants to merge 14 commits into
Conversation
Collaborator
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements node:http Changesnode:http headersTimeout/requestTimeout enforcement
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/bun-uws/src/HttpContext.h`:
- Around line 147-163: The timeout re-arm logic in tryArmNodeReceiveTimeout is
resetting the full request/header timeout every time resetTimeout(), pause(), or
resume() calls back into it, which turns the phase deadline into a renewable
idle timeout. Update HttpResponseData to remember an absolute expiry or
remaining budget when the socket first enters NodeReceivePhase::Headers or
NodeReceivePhase::Body, and have tryArmNodeReceiveTimeout re-arm
us_socket_timeout using the remaining time instead of the original configured
seconds. Ensure the phase-start bookkeeping is tied to nodeReceivePhase and the
existing HttpResponse::resetTimeout()/pause()/resume() flow.
- Around line 631-645: The receive-timeout branch in HttpContext::onClientError
handling is incorrectly gated by the HttpResponseData state check, which skips
timeout error reporting and socket teardown after writeHead()/write()/end() has
started. Update the node receive-timeout logic so onClientError(...) and the
forced close path always run when nodePhase is not None, and limit only the
canned HTTP 408 write in that block to cases where the response has not already
started; use the existing nodePhase, httpResponseData->state,
us_socket_is_closed, and asyncSocket->close flow to locate and adjust the
condition.
In `@test/js/node/http/node-http.test.ts`:
- Around line 3754-3760: Trim the block comment in node-http.test.ts to Bun’s
3-line limit by keeping only the durable invariant about Server.headersTimeout /
Server.requestTimeout behavior; remove the repeated explanation and
implementation-specific timing notes, and leave the concise summary near the
related test block so it stays aligned with the test names.
- Around line 3803-3816: The waits in this HTTP test only resolve, so failures
on socket/server paths can hang until timeout instead of failing immediately.
Update the promise setup around clientError, handlerCalled, aborted,
receivedFirstResponse, and closed in node-http.test.ts so each awaited condition
also rejects on error, unexpected close, abort, or other failure events. Use the
existing event handlers and promise variables in the test to wire reject paths
alongside resolve paths, and keep the assertions in the same flow so the test
fails at the first bad event.
🪄 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: 77d42951-1b00-4761-9f60-6abbfa145f82
📒 Files selected for processing (18)
packages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpContextData.hpackages/bun-uws/src/HttpErrors.hpackages/bun-uws/src/HttpParser.hpackages/bun-uws/src/HttpResponse.hpackages/bun-uws/src/HttpResponseData.hsrc/js/builtins.d.tssrc/js/internal/http.tssrc/js/node/_http_server.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeHTTP.cppsrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/uws_sys/App.rssrc/uws_sys/libuwsockets.cpptest/js/node/http/node-http.test.ts
Comment on lines
+3754
to
+3760
| // node:http Server.headersTimeout / Server.requestTimeout enforcement. | ||
| // Node answers an expired receive deadline with a canned | ||
| // "HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n" (only when no | ||
| // 'clientError' listener is installed) and emits 'clientError' with | ||
| // ERR_HTTP_REQUEST_TIMEOUT. The deadline check granularity is | ||
| // connectionsCheckingInterval in Node and ~4s in Bun, so these use small | ||
| // configured values and only await events. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trim this block comment to Bun’s 3-line limit.
This is 7 lines and mostly repeats what the test names already say. Keep only the durable invariant here.
As per coding guidelines, "Keep code comments to 3 lines max".
🤖 Prompt for 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.
In `@test/js/node/http/node-http.test.ts` around lines 3754 - 3760, Trim the block
comment in node-http.test.ts to Bun’s 3-line limit by keeping only the durable
invariant about Server.headersTimeout / Server.requestTimeout behavior; remove
the repeated explanation and implementation-specific timing notes, and leave the
concise summary near the related test block so it stays aligned with the test
names.
Source: Coding guidelines
Comment on lines
+3803
to
+3816
| const { promise: clientError, resolve: onClientError } = Promise.withResolvers<Error>(); | ||
| server.on("clientError", (err, socket) => { | ||
| onClientError(err); | ||
| socket.destroy(); | ||
| }); | ||
| try { | ||
| server.listen(0, "127.0.0.1"); | ||
| await once(server, "listening"); | ||
| const { port } = server.address() as AddressInfo; | ||
| const socket = connect(port, "127.0.0.1"); | ||
| socket.on("error", () => {}); | ||
| const closed = new Promise<void>(resolve => socket.on("close", () => resolve())); | ||
| socket.write("GET / HTTP/1.1\r\nHost: localhost\r\nX-Partial: "); | ||
| const err: any = await clientError; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject the awaited promises on socket/server failure paths.
These waits only resolve. If the server/socket takes the wrong path, the test hangs until the 20s test timeout instead of failing at the first bad event. Wire error/unexpected close to reject for clientError, handlerCalled, aborted, receivedFirstResponse, and the closed waits.
Suggested pattern
- const { promise: clientError, resolve: onClientError } = Promise.withResolvers<Error>();
+ const { promise: clientError, resolve: onClientError, reject: onClientErrorFailure } =
+ Promise.withResolvers<Error>();
server.on("clientError", (err, socket) => {
onClientError(err);
socket.destroy();
});
+ server.on("error", onClientErrorFailure);
const socket = connect(port, "127.0.0.1");
- socket.on("error", () => {});
- const closed = new Promise<void>(resolve => socket.on("close", () => resolve()));
+ const { promise: closed, resolve: onClosed, reject: onClosedFailure } =
+ Promise.withResolvers<void>();
+ socket.on("close", onClosed);
+ socket.on("error", onClosedFailure);As per coding guidelines, "Await conditions, wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks."
Also applies to: 3828-3848, 3868-3881, 3942-3945
🤖 Prompt for 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.
In `@test/js/node/http/node-http.test.ts` around lines 3803 - 3816, The waits in
this HTTP test only resolve, so failures on socket/server paths can hang until
timeout instead of failing immediately. Update the promise setup around
clientError, handlerCalled, aborted, receivedFirstResponse, and closed in
node-http.test.ts so each awaited condition also rejects on error, unexpected
close, abort, or other failure events. Use the existing event handlers and
promise variables in the test to wire reject paths alongside resolve paths, and
keep the assertions in the same flow so the test fails at the first bad event.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
Author
|
Pushed 4cf7897: the receive deadlines are now absolute per message instead of inactivity timers, matching Node.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/js/node/http/node-http.test.ts (1)
3933-3943: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject the awaited socket/server promises on failure paths.
closedandhandlerCalledonly resolve here, whilesocket.on("error", () => {})swallows the actual failure signal. If the connection or request takes the wrong path, these tests wait for the full 30s timeout instead of failing immediately. Please wireerror/ unexpectedcloseintorejectfor these waits, the same way the earlier timeout tests in this file needed. Based on learnings, Bun reviewers reject tests that only resolve awaited conditions and require wiring every failure event to reject.Also applies to: 3955-3981
🤖 Prompt for 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. In `@test/js/node/http/node-http.test.ts` around lines 3933 - 3943, The awaited socket/server waits in the HTTP timeout tests only resolve and ignore failure signals, so they can hang until the full timeout instead of failing fast. Update the promise setup around the socket flow and the request handler flow (the closed and handlerCalled waits in the node-http tests) to reject on error and unexpected close paths, and remove any error handlers that silently swallow failures. Use the existing test helpers/patterns in this file for wiring rejected promises so the affected cases fail immediately when the wrong event occurs.Source: Coding guidelines
🤖 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 `@packages/bun-uws/src/HttpContext.h`:
- Around line 145-149: The new timeout comments in HttpContext should be
shortened to meet the 3-line limit by keeping only the invariant about deadline
calculation and removing the extra narrative detail. Update the comment blocks
around the deadline logic in HttpContext so they are concise, retain the
distinction between headers/body and requestTimeout, and apply the same cleanup
to the related comment range in the same class.
In `@packages/bun-uws/src/HttpParser.h`:
- Around line 73-79: The NodeReceivePhase comment in HttpParser.h is too long
and exceeds the repository’s 3-line comment limit. Trim the block to the durable
invariant only, keeping the explanation around NodeReceivePhase and
hasNodeReceiveTimeouts while removing the extra examples and situational detail;
update the comment near the NodeReceivePhase enum so it stays concise and under
3 lines.
In `@packages/bun-uws/src/HttpResponseData.h`:
- Around line 130-136: The comment attached to the receive deadline state in
HttpResponseData should be shortened to meet the 3-line maximum. Trim the
explanatory text around nodeArmedReceivePhase, nodeMessageStarted, and
nodeMessageStartMs so it still identifies the absolute-per-message deadline
behavior, but in a condensed form that fits the repo comment guideline.
---
Duplicate comments:
In `@test/js/node/http/node-http.test.ts`:
- Around line 3933-3943: The awaited socket/server waits in the HTTP timeout
tests only resolve and ignore failure signals, so they can hang until the full
timeout instead of failing fast. Update the promise setup around the socket flow
and the request handler flow (the closed and handlerCalled waits in the
node-http tests) to reject on error and unexpected close paths, and remove any
error handlers that silently swallow failures. Use the existing test
helpers/patterns in this file for wiring rejected promises so the affected cases
fail immediately when the wrong event occurs.
🪄 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: 9f5e695b-a853-4d62-8275-041ddccfe533
📒 Files selected for processing (4)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpParser.hpackages/bun-uws/src/HttpResponseData.htest/js/node/http/node-http.test.ts
Comment on lines
+73
to
+79
| /* node:http receive phase of a socket (only meaningful when | ||
| * hasNodeReceiveTimeouts is set): Headers from connection (or the first | ||
| * byte of the next keep-alive message) until a message's header section | ||
| * is fully parsed, Body until its body is fully received, None while a | ||
| * handler/response is in flight or the keep-alive socket is idle after a | ||
| * completed exchange. CONNECT tunnels stream forever and have no phase. */ | ||
| enum class NodeReceivePhase : unsigned char { None, Headers, Body }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Shorten the phase comment to the repository limit.
This new block is 6 lines; keep only the durable invariant.
Proposed cleanup
- /* node:http receive phase of a socket (only meaningful when
- * hasNodeReceiveTimeouts is set): Headers from connection (or the first
- * byte of the next keep-alive message) until a message's header section
- * is fully parsed, Body until its body is fully received, None while a
- * handler/response is in flight or the keep-alive socket is idle after a
- * completed exchange. CONNECT tunnels stream forever and have no phase. */
+ /* node:http receive phase when hasNodeReceiveTimeouts is set.
+ * Headers lasts until parsed; Body until received; CONNECT/idle use None. */As per coding guidelines, “Keep code comments to 3 lines max.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* node:http receive phase of a socket (only meaningful when | |
| * hasNodeReceiveTimeouts is set): Headers from connection (or the first | |
| * byte of the next keep-alive message) until a message's header section | |
| * is fully parsed, Body until its body is fully received, None while a | |
| * handler/response is in flight or the keep-alive socket is idle after a | |
| * completed exchange. CONNECT tunnels stream forever and have no phase. */ | |
| enum class NodeReceivePhase : unsigned char { None, Headers, Body }; | |
| /* node:http receive phase when hasNodeReceiveTimeouts is set. | |
| * Headers lasts until parsed; Body until received; CONNECT/idle use None. */ | |
| enum class NodeReceivePhase : unsigned char { None, Headers, Body }; |
🤖 Prompt for 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.
In `@packages/bun-uws/src/HttpParser.h` around lines 73 - 79, The NodeReceivePhase
comment in HttpParser.h is too long and exceeds the repository’s 3-line comment
limit. Trim the block to the durable invariant only, keeping the explanation
around NodeReceivePhase and hasNodeReceiveTimeouts while removing the extra
examples and situational detail; update the comment near the NodeReceivePhase
enum so it stays concise and under 3 lines.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-uws/src/HttpContext.h (1)
159-201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLet disabled receive deadlines fall back to socket idle timeouts.
When
nodeRemainingReceiveSeconds()returns0,tryArmNodeReceiveTimeout()still returnstrue, soHttpResponse::setTimeout()never falls back toidleTimeout. WithheadersTimeout/requestTimeoutdisabled, a userreq.setTimeout()during an active receive phase can be silently disarmed.Proposed fix
- if (messageStarted && phase == httpResponseData->nodeArmedReceivePhase) { + unsigned int remainingSeconds = nodeRemainingReceiveSeconds(s, httpResponseData, phase); + if (remainingSeconds == 0) { + httpResponseData->nodeArmedReceivePhase = NodeReceivePhase::None; + return false; + } + if (messageStarted && phase == httpResponseData->nodeArmedReceivePhase) { /* Both deadlines are absolute from the message start: received * bytes never extend them, so a client trickling data slowly * cannot hold the socket past them. */ return true; } @@ } httpResponseData->nodeArmedReceivePhase = phase; - us_socket_timeout(s, nodeRemainingReceiveSeconds(s, httpResponseData, phase)); + us_socket_timeout(s, remainingSeconds); return true;🤖 Prompt for 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. In `@packages/bun-uws/src/HttpContext.h` around lines 159 - 201, The receive-timeout path in nodeRemainingReceiveSeconds() and tryArmNodeReceiveTimeout() is treating a disabled deadline as an armed timeout, which prevents fallback to the socket idle timeout. Update tryArmNodeReceiveTimeout() so that when nodeRemainingReceiveSeconds() computes 0 for the current phase, it returns false and leaves HttpResponse::setTimeout() to apply the legacy idleTimeout instead; keep the receive-phase arming logic in HttpContext.h and nodeRemainingReceiveSeconds() consistent with this behavior.
🤖 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 `@packages/bun-uws/src/HttpContext.h`:
- Around line 608-611: Condense the new comment block in HttpContext.h to fit
the repository’s 3-line limit by keeping only the core invariant about writable
state and timeout behavior. Update the comment near the writable rearm logic and
tryArmNodeReceiveTimeout so it is shorter, removes extra explanation, and
preserves only the essential note that the node:http receive deadline timer must
remain armed while a message is being received.
---
Outside diff comments:
In `@packages/bun-uws/src/HttpContext.h`:
- Around line 159-201: The receive-timeout path in nodeRemainingReceiveSeconds()
and tryArmNodeReceiveTimeout() is treating a disabled deadline as an armed
timeout, which prevents fallback to the socket idle timeout. Update
tryArmNodeReceiveTimeout() so that when nodeRemainingReceiveSeconds() computes 0
for the current phase, it returns false and leaves HttpResponse::setTimeout() to
apply the legacy idleTimeout instead; keep the receive-phase arming logic in
HttpContext.h and nodeRemainingReceiveSeconds() consistent with this behavior.
🪄 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: 6c0b34ce-c326-41a8-ae5f-bbfeeb2fa421
📒 Files selected for processing (3)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponse.htest/js/node/http/node-http.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/js/node/http/node-http.test.ts (2)
4169-4175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim these comments to the 3-line limit.
Both new block comments exceed Bun’s comment-length guideline; keep only the durable invariant about the coalesced boundary and restarted deadline. As per coding guidelines, “Keep code comments to 3 lines max.”
Also applies to: 4226-4230
🤖 Prompt for 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. In `@test/js/node/http/node-http.test.ts` around lines 4169 - 4175, Trim the new block comments in the http tests to fit Bun’s 3-line limit by removing the long timing narrative and keeping only the stable invariant. Update the comments around the receive-phase transition and the headersTimeout restart to mention the coalesced body/headers boundary and that the second message’s deadline starts on its first byte. Apply the same shortening to the matching comment block near the other referenced test case.Source: Coding guidelines
4176-4203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWire failure paths into these awaited conditions.
firstResponse,secondResponse,clientError, andclosedmostly only resolve while socket errors are swallowed. If the server/socket takes the wrong path, these tests wait for the 20–30s timeout instead of failing immediately. Reject the pending resolvers fromserver/socketerrorevents and unexpectedclosepaths. As per coding guidelines, “Await conditions, wire EVERY failure event (error,close,abort, process exit) to reject the awaited promise.”Also applies to: 4231-4268
🤖 Prompt for 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. In `@test/js/node/http/node-http.test.ts` around lines 4176 - 4203, The awaited test promises in the HTTP coalesced-packet case currently only resolve on the happy path, so failures can hang until timeout. Update the test around firstResponse, secondResponse, clientError, and closed to reject pending promises from server and socket error events, and also on unexpected close/abort paths, using the existing Promise.withResolvers setup and the socket/server handlers in node-http.test.ts to fail fast when the wrong path is taken.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@test/js/node/http/node-http.test.ts`:
- Around line 4169-4175: Trim the new block comments in the http tests to fit
Bun’s 3-line limit by removing the long timing narrative and keeping only the
stable invariant. Update the comments around the receive-phase transition and
the headersTimeout restart to mention the coalesced body/headers boundary and
that the second message’s deadline starts on its first byte. Apply the same
shortening to the matching comment block near the other referenced test case.
- Around line 4176-4203: The awaited test promises in the HTTP coalesced-packet
case currently only resolve on the happy path, so failures can hang until
timeout. Update the test around firstResponse, secondResponse, clientError, and
closed to reject pending promises from server and socket error events, and also
on unexpected close/abort paths, using the existing Promise.withResolvers setup
and the socket/server handlers in node-http.test.ts to fail fast when the wrong
path is taken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83ec05bc-ea60-473c-a243-afd2c80f2b11
📒 Files selected for processing (2)
packages/bun-uws/src/HttpContext.htest/js/node/http/node-http.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/http/node-http.test.ts (1)
4169-4174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTrim these block comments to Bun’s 3-line limit.
Both comments exceed the repository’s max comment length. Keep only the durable invariant; the timing detail can live in the test body/PR description.
Suggested tightening
- // Chunked framing is only known after the request is dispatched, so the - // receive phase passes through None inside the handler; the message-start - // time must survive that. The headers take 10s of the 13s requestTimeout, - // so the absolute deadline expires 3s into the stalled body — long before - // the client finishes the body at +16s. A deadline re-based at header - // completion would instead let the request complete with a 200. + // Chunked framing briefly clears the receive phase in the handler. + // Preserve message-start time so requestTimeout is not re-based at header completion. @@ - // 4_294_967_297_000 ms is 2**32 + 1 seconds; narrowed naively to a 32-bit - // count of seconds it wraps to a 1-second deadline. Negative contract - // bounded by an awaited condition: a sibling server whose 6s headersTimeout - // 408-closes first proves a wrapped 1s deadline (plus a full 4s timer - // sweep) would already have reaped the huge-timeout connection. + // 4_294_967_297_000 ms is 2**32 + 1 seconds. + // A sibling 6s headersTimeout bounds the negative contract for wraparound.As per coding guidelines, "Keep code comments to 3 lines max".
Also applies to: 4359-4363
🤖 Prompt for 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. In `@test/js/node/http/node-http.test.ts` around lines 4169 - 4174, Trim the block comments in the affected test cases to Bun’s 3-line maximum by keeping only the lasting invariant and removing the long timing explanation. Update the comments near the request timeout assertions in node-http.test.ts so they stay brief and compliant, while preserving the intent around the message-start deadline behavior.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@test/js/node/http/node-http.test.ts`:
- Around line 4169-4174: Trim the block comments in the affected test cases to
Bun’s 3-line maximum by keeping only the lasting invariant and removing the long
timing explanation. Update the comments near the request timeout assertions in
node-http.test.ts so they stay brief and compliant, while preserving the intent
around the message-start deadline behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fe31ded-bb9d-4c43-812b-740dd295fc07
📒 Files selected for processing (3)
packages/bun-uws/src/HttpContext.hsrc/js/node/_http_server.tstest/js/node/http/node-http.test.ts
Collaborator
Author
|
Confirmed and addressed in e4756ba.
Verified manually against a debug build with a 9s |
Server.headersTimeout and Server.requestTimeout were validated and stored
but never enforced: the node:http server is created with idleTimeout 0, so
nothing ever closed a connection with stalled request headers or a stalled
request body.
Match Node's behavior (verified against node v26.3.0): from connection (or
the end of the previous keep-alive exchange) until a message's headers are
complete, the per-socket uWS timer is armed with headersTimeout; while its
body is incomplete, with requestTimeout. When one of those deadlines
expires the server emits 'clientError' with ERR_HTTP_REQUEST_TIMEOUT
("Request timeout"), writes a canned
"HTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\n" if nothing was
written for that message yet, and closes the socket. Handlers, responses,
and idle keep-alive sockets keep having no deadline, and both values
plumb through as 0 to disable. The behavior is gated on a node-only uWS
context option so Bun.serve idleTimeout/keep-alive/upgrade semantics are
unchanged.
…e message Like Node, both deadlines are absolute per message rather than inactivity timers: receiving more bytes never extends them, and requestTimeout spans the header section too. The clock starts at the message's first received byte (or connection open while nothing has arrived yet).
…etTimeout(), drains, and partial CONNECT headers Like Node, requestTimeout keeps applying to a message whose body is still outstanding after the handler already responded, headersTimeout applies to a CONNECT request until its header section completes, expiry always emits 'clientError' (only the canned 408 is suppressed once response bytes were written), and neither deadline can be disarmed by req.setTimeout() or by the writable (drain) path zeroing the socket timer.
…oundary When one packet carries the tail of a message's body together with the next message's bytes, the receive phase moves Body->Headers (or Body->Body) without passing through None, so tryArmNodeReceiveTimeout kept treating it as the same message: the second message inherited the first message's start time (premature 408) or, in the same-phase case, never had its deadline armed at all. Reset the per-message start state when the parser signals the end of a message's body, so every message's headersTimeout/requestTimeout are measured from its own first byte.
…wing headersTimeout/requestTimeout values of 2**32 seconds or more wrapped when narrowed to a 32-bit second count (e.g. 4_294_967_297_000 ms became a 1-second deadline). Clamp to the documented 940-second cap on the JS side for both options.
…r receive-phase gap Chunked framing is only marked after the request handler returns, so the receive phase passes through None inside the handler and the Headers->Body transition treated the body as a new message, re-basing requestTimeout at header completion. Tie the message-start state to the actual message boundary (the fin reset) instead of the armed phase.
…er, not a clear/set flag The per-message start time was a sticky nodeMessageStarted flag, set lazily inside tryArmNodeReceiveTimeout and cleared only by the fin reset in onData's data handler. That boundary can be skipped (an explicit Content-Length: 0 request whose headers complete on the parser's buffered fallback path never delivers the empty fin chunk) and undone (a synchronous res.write() from a 'data' listener re-arms while the body's final chunk is still being delivered, before remainingStreamingBytes is decremented). Either way the next keep-alive request inherits the previous message's start and is spuriously 408'd. Replace the flag with the one unskippable begin point: onData stamps the packet's arrival time and fenceAndConsumePostPadded<false> copies it into nodeMessageStartMs as it begins each message's request line -- the first message after open or keep-alive idle, every coalesced or pipelined follower in the same packet, and the partial first packet of split headers, while leaving a buffered-header or body continuation alone. tryArmNodeReceiveTimeout becomes a pure reader so nothing reached from a handler can re-base a deadline, and nodeMessageStarted / nodeArmedReceivePhase are deleted. Bun.serve is unaffected: the stamp is gated on the node receive deadlines.
…must not claim the socket timer With requestTimeout: 0 the Body phase still claimed the per-socket timer: tryArmNodeReceiveTimeout() computed a 0-second deadline, disarmed the timer via us_socket_timeout(s, 0) and returned true, so setTimeout()/resetTimeout() never fell through to the legacy idle timeout and a handler's req.setTimeout(ms, cb) was silently swallowed for the whole body phase (same for the Headers phase when both timeouts are 0). Fold the disabled-deadline check into nodeReceivePhase(): a phase whose configured deadline is 0 returns None, so tryArmNodeReceiveTimeout() returns false without touching the timer and onTimeout() attributes the expiry to the user idle timeout instead of emitting a 408/'clientError'.
…ntent-Length end path The Content-Length end path reset the socket timer before markDone(), so the still-pending response made the receive phase read as None and disarmed the timer. When the next keep-alive request's partial header bytes were already buffered (pipelined behind an async handler), nothing re-armed afterwards and its headersTimeout never fired. Order markDone() before resetTimeout(), as the chunked path already does.
… timeout For an https node server the receive-deadline timer is armed at TCP accept, before the TLS handshake. If the handshake never completes, its expiry was taking the headersTimeout/requestTimeout path: emitting 'clientError' with ERR_HTTP_REQUEST_TIMEOUT and writing a 408 into an SSL stream that never handshaked. Node's HTTP layer only attaches after the handshake, so it emits neither. Gate the 408/'clientError' path in onTimeout on the handshake having completed (isAuthorized, set only by onHandshake); a pre-handshake expiry falls through to the plain close, still bounded by the deadline armed at open.
…he canned 408 after the interim 100 Matches Node v26: socketOnError only suppresses the canned 408 once the in-flight response's own header was sent (_headerSent); an interim 1xx does not set it.
Jarred-Sumner
force-pushed
the
claude/node-http-server-timeouts
branch
from
July 2, 2026 06:37
9464e8d to
5b38098
Compare
Contributor
There was a problem hiding this comment.
No further issues found — all prior review comments have been addressed with regression tests. Deferring to a human because this threads a new per-socket receive-deadline state machine through the core uWS HTTP context/parser/response paths, which is complex enough to warrant a maintainer look.
Extended reasoning...
Overview
This PR wires node:http / node:https Server.headersTimeout and Server.requestTimeout into uWS as per-socket receive deadlines. It touches the vendored uWS core (HttpContext.h, HttpParser.h, HttpResponse.h, HttpResponseData.h, HttpContextData.h, App.h, HttpErrors.h), the Rust server runtime (mod.rs, server_body.rs, App.rs), C++/JS bindings (NodeHTTP.cpp, libuwsockets.cpp, _http_server.ts, internal/http.ts), a new error code (ERR_HTTP_REQUEST_TIMEOUT), and ~970 lines of tests in node-http.test.ts.
Security risks
The feature is the security control (slowloris protection). The new code is gated on a node-only hasNodeReceiveTimeouts context flag so Bun.serve behavior is unchanged. The main risk class was gaps that let a client disarm/inherit/bypass a deadline; over the review cycle roughly a dozen such gaps (early-response disarm, CONNECT short-circuit, onWritable direct disarm, Body→Headers inheritance, chunked transient re-basing, CL:0 fallback skip, req.setTimeout interaction, TLS handshake attribution, pipelined-behind-async-handler, disabled-deadline swallowing user timeout) were found, each confirmed against real Node v26 and fixed with a regression test. No new issues were found in the current head.
Level of scrutiny
High. This is a state machine sharing the single per-socket uSockets timer slot with the legacy idle timeout and user req.setTimeout(), whose correctness depends on exact ordering across onOpen/onHandshake/onData/onWritable/onTimeout/internalEnd/markDone and the parser's per-message stamping. The number of subtle edge cases fixed during review demonstrates that this is not mechanical, and the changes live in the hot path of every node:http server connection.
Other factors
Test coverage is now extensive (25+ cases including HTTPS, keep-alive, pipelining, chunked, CONNECT, Expect: 100-continue, backpressure/drain, coalesced packets, disabled/wraparound values, and TLS-handshake stalls), each verified to fail with the relevant hunk reverted. All prior inline comments (mine and CodeRabbit's) are resolved. A duplicate-PR bot flagged overlap with #32488. Given the scope and the iteration history, a maintainer should sign off on the final design (especially the parser-level nodeMessageStartMs stamping and the resetTimeout/markDone ordering change in HttpResponse.h).
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 4, 2026
## What
A hardening and robustness pass across the runtime: input validation,
bounds checking, protocol-state handling, and object-lifetime
correctness in ~200 files. It contains 122 individual fixes and ~190 new
tests (171 new `test`/`it` blocks, several parameterized, across 79
existing test files). No new API is introduced; every behavioral change
below has a test unless explicitly noted, and each is aligned with Node,
the relevant RFC/spec, or the upstream reference implementation.
## Potentially breaking / behavior-visible changes
Read this section first. Everything else in the PR preserves behavior
for valid inputs.
- **`Bun.serve` `request.url` is only synthesized from a structurally
valid `Host`.** For every HTTP/1.x request (`Bun.serve` and node-compat
servers alike), a `Host` value that is empty or contains bytes outside
`uri-host [":" port]` (RFC 3986 authority: alphanumerics, `.-:_~%[]` and
sub-delims) is never used as the authority of the synthesized
`request.url`; `request.url` falls back to the request target (e.g.
`/path`) and the request is still served. No request is rejected on the
basis of the `Host` field value, and a valid `Host` still round-trips
into `request.url` exactly. Why: `request.url` should never carry an
authority that cannot come back out of `new URL()`.
- **`fetch` rejects request lines it cannot legally serialize.** A URL
path/host (or, for proxied requests, the full href) containing a control
character, space, or DEL now fails with `InvalidURL` before any bytes
are written (RFC 9112 request-line grammar; normal `fetch()` input is
percent-encoded by the URL parser and unaffected). Also: a redirect
whose `Location` resolves to a non-http(s) scheme now fails with
`UnsupportedRedirectProtocol` (Fetch spec, matches undici); a `101`
arriving on the pre-tunnel leg of a proxied request is treated as an
unrequested upgrade; connections whose identity was accepted by a
per-request `checkServerIdentity` callback are never entered into or
taken from the keep-alive pool.
- **An own `__proto__` key from data files and macros is printed as a
computed key.** The `json`, `jsonc`, `json5`, `toml`, `yaml`, and
CSS-module loaders — and objects returned from Bun macros — now emit
`["__proto__"]: ...` so importing such data yields an own `__proto__`
property (like `JSON.parse`) instead of a prototype assignment. Who
notices: only code importing data with a `__proto__` key; matches
esbuild's JSON loader and Node semantics.
- **`node:url` legacy `url.parse` lookup tables no longer inherit from
`Object.prototype`** (both `node:url` and the browser fallback), the
hostless/slashed lookups use the lowercased protocol, and `url.parse(s,
true).query` is a null-prototype object (empty query included). All
three match Node's `lib/url.js` exactly. Who notices: code doing
`query.hasOwnProperty(...)` or parsing schemes named like `toString:`.
- **WebSocket client: missing negotiated subprotocol fails the
handshake.** Per RFC 6455 §4.1, if `new WebSocket(url, ["a"])` requested
subprotocols and the server's 101 omits `Sec-WebSocket-Protocol`, the
connection now closes with 1002 instead of opening with `ws.protocol ===
""`. Matches browsers, `ws`, and undici. Connections that request no
subprotocol are unaffected.
- **HTTP/2 (client and server) enforces RFC 9113 message framing.**
Trailer blocks must carry END_STREAM and no pseudo-headers;
`content-length` must be `1*DIGIT`, non-duplicated, and equal to the
DATA actually received (CONNECT exempt) — violations get
RST_STREAM(PROTOCOL_ERROR) instead of being delivered. With
`maxSessionMemory` exceeded, new peer streams are refused with
REFUSED_STREAM (retryable), and reset streams promptly release their
native state — Node/nghttp2 parity throughout. The all-streams teardown
helper now throws a `TypeError` for a non-numeric error code instead of
coercing it per stream.
- **`node:http2` HTTP/1 fallback (`allowHTTP1`) frames responses like
Node.** Header-name matching is case-insensitive; HEAD and
close-delimited responses don't get an auto `Transfer-Encoding:
chunked`/terminating chunk; `writeHead` now throws
`ERR_HTTP_INVALID_STATUS_CODE` / `ERR_INVALID_CHAR` like Node's
`ServerResponse`. Re-entrant `sendTrailers()` raises
`ERR_HTTP2_TRAILERS_ALREADY_SENT` in the same order Node does.
- **`node:http(s)` proxy `CONNECT` endpoint is validated with
`validateHeaderValue` in release builds** (previously a debug-only
assertion), so an invalid host/port surfaces as the same error Node
throws.
- **Glob: walking through a self-referential directory symlink
completes.** With `followSymlinks`, a link that resolves to one of its
own live ancestors is descended exactly once (like `find -L`, glibc
`fts`, node-glob); sibling/cousin links to the same target are still all
visited. One pre-existing test changed: it previously asserted the walk
failed with `ENAMETOOLONG` after the path grew past the limit; it now
asserts the scan completes.
- **Resolver: an `exports`/`imports` target whose expansion would exceed
the OS path limit is a normal resolution error** (`Invalid module
specifier` / `Invalid package target`, as Node models it) instead of a
hard failure.
- **Shell:** template arrays nested deeper than 100 levels throw a clear
error instead of recursing without bound; an interpolated string equal
to `if`/`then`/`elif`/`else`/`fi` is treated as data, never as a
reserved word (POSIX: reserved words are only recognized literally);
`$.escape` now quotes strings containing tab, CR, or `?` (word
delimiters / glob metacharacters).
- **`bun pack` / `bun publish` include/exclude matches npm-packlist.**
With a `"files"` field, the non-overridable defaults (`.git`, `.npmrc`,
`node_modules`, lockfiles) are now applied inside the `files` traversal
too; conversely `.hg` moved to the *overridable* default-ignore list, so
`"files"` can re-include it — exactly npm's split.
- **`bun upgrade` verifies the downloaded artifact against the digest
the GitHub Releases API reports** for that asset, and fails with a
retryable error on mismatch. If the API reports no (or an unrecognized)
digest, behavior is unchanged.
- **install:** an `integrity` string carrying several space-separated
digests (legal SSRI) is now parsed correctly and verified against the
strongest algorithm present (see Deviations); a stored `bun.lockb` with
a non-0/1 byte in a boolean slot fails validation instead of being
reinterpreted; lifecycle scripts for *registry* packages always come
from the installed `package.json` (never from lockfile bytes), matching
what Bun writes and what npm does; isolated installs apply the same
name/alias shape validation as hoisted installs; bin links reached
through a subdirectory get the same resolved-containment check the
dotted forms already had (npm only links files inside the package
folder).
- **`node:fs`:** `mode` arguments are no longer masked to `0o777`
(setuid/setgid/sticky pass through to the syscall, like Node);
`copyFile`/`cp` create the destination with the source's permission bits
(libuv parity); on Windows, `cp` copies directory junctions/symlinks via
the unprivileged-create + junction-fallback helpers and rewrites
`\\?\UNC\` targets to `\\server\share` form (libuv parity), so copying a
tree with junctions works without elevation; on macOS the
`clonefile`/`openat` paths use `NOFOLLOW` so the copy matches the
`lstat` classification (`dereference:false`).
- **Web plumbing observable from JS:** record conversion (`new
Headers(obj)`, fetch init, `URLSearchParams`, …) snapshots the key list
once and re-resolves keys mutated by a converter, exactly as Web IDL
specifies (deleted keys skipped, replaced values re-read) — released
Bun/Node/WebKit order preserved; `TextDecoder.decode` over a
`SharedArrayBuffer` or resizable buffer view snapshots the bytes first;
consuming a `Blob`/`Response` body no longer empties *other* objects
sharing the same byte store (transfer only when sole owner); deeply
nested serialized arrays in `structuredClone` data hit the same
recursion cap objects already had; the SIMD `decodeURIComponent` fast
path decodes non-ASCII input as UTF-8 (with U+FFFD for ill-formed
sequences) instead of throwing/garbling.
- **N-API / V8 API:** `napi_create_arraybuffer` returns zeroed memory
(Node contract); `napi_get_typedarray_info`/`napi_get_dataview_info`
report the view's real `byte_offset`; `v8::String::Utf8Length` returns
the exact byte count `WriteUtf8` will produce for ill-formed UTF-16;
`v8::Number::New` canonicalizes NaN payloads.
- **Dev-only endpoints check `Host`/`Origin`:** the inspector (`bun
--inspect`) HTTP/WebSocket endpoint applies its Host/Origin checks
before the `/json`* discovery routes and rejects non-matching DNS-name
`Host` values with 400 (Node inspector semantics); the bake/dev-server
internal routes require an allowed Host and same-origin for the
error-report/sourcemap endpoints; internal HMR pub/sub topics are
namespaced so user `publish`/`subscribe` topic strings can never collide
with them.
- **markdown:** reference-link expansion is charged against md4c's exact
output budget (`16 × min(input, 64 KiB)` scale); once exhausted, further
references degrade to literal bracketed text — no error — exactly as
md4c does.
- **Misc small behavior corrections:** `checkPrime` validates the
candidate before the options (Node's order); HKDF rejects non-secret
`KeyObject`s with `ERR_CRYPTO_INVALID_KEY_OBJECT_TYPE` (current Node);
Ed25519 sign/verify with a wrong-length key errors/returns false; X25519
JWK import honors `kty`/`crv`/`use`/`key_ops`/`ext`; SPKAC helpers
return false/empty for empty or whitespace-only input (Node);
`CookieMap.delete` of a `__Host-`/`__Secure-` cookie emits `Secure` so
browsers accept the expiry; postgres `escapeIdentifier` rejects embedded
NUL (`pg` parity); valkey/redis pending commands reject with "Connection
closed" on disconnect and out-of-band push frames never consume an
unrelated command's promise (RESP3); semver strings consisting only of
`v`/`=`/whitespace parse as `*` (node-semver); `Bun.wrapAnsi` measures
rows whose seam joins grapheme clusters (combining marks/ZWJ/VS16) the
way npm `wrap-ansi` does; bash/zsh completions handle script names
containing `:` and other special characters; the Docker images verify
(not just decode) the release checksum signature.
## Changes by area
- **HTTP server (uWS / `Bun.serve` / `node:http`)** —
`packages/bun-uws/HttpParser.h`, `packages/bun-usockets`,
`src/runtime/webcore/Request.rs`, `src/runtime/server`: the
`request.url` `Host` handling above (URL synthesis in `Request.rs` only
— the HTTP parser's `Host` handling is unchanged and every request is
served); CONNECT requests are framed as an opaque tunnel regardless of
`Transfer-Encoding`/`Content-Length` (RFC 9110 §9.3.6); TLS socket
relocation updates the loop's spill/last-error owner pointers so
bookkeeping never points at a moved socket; the bake-only route
additionally requires an allowed `Host`.
- **fetch / HTTP client / webcore** — `src/http/lib.rs`,
`src/http/ssl_config.rs`,
`src/runtime/webcore/{Request,Blob,TextDecoder}.rs`,
`src/jsc/bindings/webcore/*`,
`src/jsc/bindings/decodeURIComponentSIMD.cpp`, `src/url/lib.rs`,
`src/runtime/api/BunObject.rs`: everything in the highlights, plus:
`write_request` failures propagate their real error instead of being
reported as out-of-memory; partial-header/1xx short reads no longer
re-feed already-consumed bytes to the parser; `SSLConfig` now actually
applies `secureOptions` and the client-renegotiation limit/window it was
already accepting; `URLSearchParams` no longer drops a pair whose value
has a malformed percent sequence (WHATWG: never drop, decode lazily);
`AbortSignal` native listeners deregistered by an earlier abort callback
are not invoked with a stale context; the compression helpers
(`Bun.gzipSync` et al.) read the options object before coercing the
input buffer (so a getter can't invalidate the captured slice) and no
longer register a deallocator for an empty result's dangling sentinel
pointer (an invalid free at GC time under debug allocators).
- **node compat** — `src/runtime/node/*`, `src/js/node/*`,
`src/node-fallbacks/url.js`, `src/jsc/ipc.rs`, `src/runtime/socket`:
everything in the highlights, plus: `fs` path arguments from typed
arrays are always pinned for the call's duration; `BlockList`
structured-clone carries only an opaque per-instance nonce resolved
through a live table (round-trip unchanged); IPC advanced-mode frame
lengths are range-checked before span arithmetic, serialization failures
leave no partial frame in the send queue, and a received fd is closed if
its message fails to parse; TLS-upgrade `initialData` is copied to an
owned buffer before use; `node:wasi` interprets rights bitfields as
unsigned u64 (per the ABI) and no longer reports success for a
`path_open` that threw internally; the inspector/debugger endpoint
changes above.
- **HTTP/2 & WebSocket client** —
`src/runtime/api/bun/h2/connection.rs`, `h2_frame_parser.rs`,
`src/http_jsc/*`: everything in the highlights, plus: 1xx interim
responses are not misclassified as trailers; refused streams still
HPACK-decode the discarded block (RFC 9113 §4.3) and advance
`last_stream_id` so pipelined RST_STREAMs don't become connection
errors; the frame parser no longer holds an exclusive stream reference
across calls back into JS (`options`/header getters, `toString`
coercions) — engine-side stream eviction is deferred until the dispatch
unwinds; the trailers failure path still ends the stream with
FRAME_SIZE_ERROR + graceful GOAWAY so in-flight streams stay retryable;
the deflate plumbing gains a per-VM slot (no behavior change yet).
- **install / pack / bunx / upgrade / create** — `src/install/*`,
`src/runtime/cli/{pack,create,upgrade}_command.rs`, `src/semver`,
`packages/bun-release`, `packages/bun-vscode`: everything in the
highlights, plus: SSRI option suffixes (`?...`) are stripped from digest
payloads; a GitHub dependency whose resolved ref would not form a single
well-formed folder name is refused with a clear error (real refs/SHAs
always pass); the trusted-dependency lookup moved off the extraction
worker thread (no user-visible change); `bun create`'s `package.json`
rewrite uses the CLI arena so the parsed AST outlives its uses; the npm
installer package validates archive entry paths stay inside the
destination; the VS Code lockfile preview escapes interpolated text and
the debug adapter's session id comes from `crypto.randomBytes`.
- **resolver / bundler / parsers / macros / sourcemap / markdown** —
`src/resolver`, `src/bundler`, `src/parsers/{json,json5,yaml}.rs`,
`src/ast/e.rs`, `src/js_parser/lexer.rs`, `src/js_parser_jsc/Macro.rs`,
`src/jsc/RuntimeTranspilerStore.rs`, `src/jsc/bindings/BunPlugin.cpp`,
`src/sourcemap`, `src/md`, `src/paths`, `src/standalone_graph`: the
`__proto__`, resolver-limit, and markdown items above, plus: a
native-plugin `onLoad` source buffer now has exactly one owner (its free
callback was registered twice); `onResolve` callback lists are
snapshotted (GC-visible) before user callbacks run, so a callback
registering more plugins can't perturb the in-progress dispatch;
barrel-import scheduling copies its alias seeds instead of holding
references into a map the BFS mutates; embedded bytecode caches are
handed to `ResolvedSource` as a genuinely owned allocation; the lazy
sourcemap decompression cache became `OnceLock`-based (shared-reference
safe); the lexer's SIMD long-string fast path advances past scanned
bytes (removes a quadratic re-scan on unterminated literals);
`is_parent_or_equal` uses a true prefix check instead of substring
containment.
- **shell / glob / CLI / wrapAnsi** — `src/shell_parser`,
`src/runtime/shell`, `src/glob/GlobWalker.rs`,
`completions/bun.{bash,zsh}`, `src/jsc/bindings/wrapAnsi.cpp`: the
highlights above; the glob walker's followed-link tracking is scoped to
the live ancestor chain (DAG revisits still enumerate); `wrapAnsi` also
caches row widths so the seam fix comes with fewer full-row rescans.
- **crypto** —
`src/jsc/bindings/{ncrypto.cpp,node/crypto/*,webcrypto/*}`: the
highlights above, plus: the sign/verify job copies signature bytes out
of the JS view before the async job runs; `ECDH.convertKey` and
`prepareAsymmetricKey` capture buffer spans only after argument
coercions that can run user JS; deserialized `CryptoKey`s re-validate
that the algorithm matches the key class (and an empty key payload is
rejected); an OOM while encoding returns after throwing.
- **sql / valkey / s3** — `src/sql`, `src/sql_jsc`,
`src/js/internal/sql`, `src/runtime/valkey_jsc`, `src/valkey`,
`src/s3_signing`: the highlights above, plus: postgres `CopyData`
payload length is computed per the wire protocol (was one byte short)
and `PortalSuspended`/`Copy*` messages are consumed instead of
desynchronizing the stream; MySQL zero-length
`AuthSwitchRequest`/`LocalInfileRequest` packets are clean protocol
errors instead of a length underflow; prepared-statement caches are
keyed by the full statement name, not a 64-bit hash (a hash hit is
verified by equality); the distributed-transaction name is type-checked;
the S3 region used to synthesize a host must be host-safe and endpoint
parsing is index-safe on odd endpoint strings.
- **JSC bindings / N-API / V8 / sqlite / misc** —
`src/jsc/bindings/{napi.cpp,v8/*,ZigException.cpp,ZigGlobalObject.cpp,CookieMap.cpp,sqlite/JSSQLStatement.cpp}`,
`src/jsc/rare_data.rs`, `src/runtime/webview`: the N-API/V8 items above;
stack-trace population skips out-of-range frame indices instead of
asserting; the native microtask trampoline is hidden from stack traces;
`bun:sqlite` detects a database closed re-entrantly from inside a bind
coercion and throws "Database has closed"; the macOS webview bridge
type-checks the objects a page posts to its internal message handler.
- **dev server / bake / build & CI** — `src/runtime/bake/*`,
`dockerhub/*`, `.github/workflows/update-vendor.yml`: the dev-endpoint
gating and HMR topic namespacing above; the dev-server terminal error
report blanks non-UTF-8 bytes (not just encoded C1); the React SSR
flight-data inliner escapes the fully decoded string instead of
per-chunk (a boundary-split escape character could previously produce a
malformed inline script); Docker images use `gpg --verify`; the
vendor-update workflow passes matrix values through `env:`.
## Deviations / decisions
- **Integrity: strongest-single-digest verification.** When an
`integrity` field carries several digests, Bun verifies the strongest
supported algorithm present; when several digests of that same algorithm
are present, the first is the one verified. npm/ssri accepts a match on
*any* digest of the chosen algorithm — keeping the full set needs
plumbing through the manifest cache/lockfile types and is left for a
follow-up.
- **No response-decompression size cap in `fetch`.** A per-response
decompressed-body limit was implemented and then deliberately removed
from this PR ("Keep fetch response decompression unbounded"): Node
imposes none, and any cap is a behavior change for legitimate large
responses. The net diff has no decompression change.
- **markdown reference expansion degrades instead of erroring**,
matching md4c exactly (see highlights). No error is ever surfaced.
- **Record conversion keeps the specification's per-property order** —
`[[GetOwnProperty]]`/`Get` interleaved with value conversion, so a
`toString` that mutates a sibling property is observed and a deleted one
skipped, the same as released Bun, Node, and WebKit. The property table
is never held across user code.
- Dead code removed because these changes made it unreachable:
`cache::Entry.external_free_function` (plus `Entry::new` and the free
branch of `Entry::deinit`) and `AlreadyBundled::bytecode_slice`; the
`bun_wyhash` dependency of `sql_jsc` is dropped.
- Deeper, behavior-visible sibling work in the same areas was split into
its own PRs so it can be reviewed on its own terms: #33054 (`node:https`
TLS option set), #33055 (websocket dispatch re-entrancy), #33056
(FileSystemRouter/resolver entry locking), #33060 (`child_process`
uid/gid), #33061 (`node:http` server headers/request timeouts). Nothing
from those PRs is claimed here.
- The changes to CI workflow files, shell completion scripts,
Dockerfiles, and the VS Code extension have no automated-test harness; a
few lifetime/ordering corrections have no deterministic observation from
JS (they are covered by the existing suites and the sanitizer jobs) and
are noted as such instead of shipping a non-asserting test.
## How it was verified
- `bun bd` (full debug build) and `cargo check` clean with zero
warnings; `bun run rust:check-all` passes 10/10 targets (the change set
includes Windows- and macOS-gated code); clippy lints raised on the
touched files were addressed.
- ~190 new regression tests in existing test files (171 `test`/`it`
blocks across 79 files, several parameterized). Each was verified to
fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test
<file>` (except the handful noted above with no JS-observable
assertion); the touched suites were run in full to confirm no
regressions.
- HTTP/2 changes were exercised against a dedicated h2 conformance suite
(`test/js/node/http2/h2-conformance.test.ts`) and Node's own http2
tests; the `node:http` Host behavior was checked against Node's
conformance test for accepted host values; the record-conversion
ordering was checked against WebKit/Node observable order.
- rustfmt / clang-format / prettier / oxlint clean over the changed
files.
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Jarred-Sumner
added a commit
that referenced
this pull request
Sep 26, 2026
…inish, lifecycle) (#43557) One pull request for the open `node:http` server pull requests. Each root cause is fixed once, and each pull request's tests are carried over. Node is the reference: every scenario was run under Node and under Bun from one script, and the outputs were compared. Fixes #4733 Fixes #18613 Fixes #40350 Fixes #43155 Fixes #43297 Fixes #43513 Fixes #43527 Fixes #25632 Fixes #31301 Fixes #43027 Fixes #43163 Fixes #43342 Fixes #43344 Fixes #43370 Fixes #43490 Fixes #43512 Fixes #43519 Each of these has a repro that is wrong on Bun 1.4.3, right on this branch, and the same as Node. | Issue | Not closed by this PR, because | | --- | --- | | #30501 (msal-node keeps Bun alive at exit) | Probably fixed. The repro copies the teardown of msal-node. The package itself was not run. | | #14430 (yarn: "does not support SSL") | Probably fixed. `response.hasOwnProperty("socket")` is now `true`. yarn itself was not run. | | #39681 (`server.setTimeout` callback after destroy) | Probably fixed. The repro is the deterministic case of #39686. The script in the issue depends on timing and on Windows. | | #43455 (`req.complete`, nine flows) | Partially addressed. Flows 1, 2, 3, 5 and 7 are fixed, and flow 6 was already right. Flow 8 (a socket timeout while the body of an Upgrade request arrives) and flow 9 (a request that `stream.pipeline()` destroyed never reports `complete`) are not. Flow 4 differs only in `_readableState.ended`. | ### What changes for users | Area | Before | After (same as Node) | | --- | --- | --- | | `req.pause()` | The socket stops at once. `req.complete` stays `false` for a small body. | The body is received until the buffer is full. Then the socket stops. | | `res.end()` before the body arrives | `req` gets `'end'` and `'close'` at once, and the body is lost | The request completes when its body really ends | | `res.destroy()` in the middle of a body | `'end'` with bytes missing | `aborted`, then `ECONNRESET` | | `socket.destroy()` inside the `'request'` listener | The body that came with the head is dropped | That body is still delivered | | `'finish'` and the `end()` callback | Fire when `end()` buffers the bytes | Fire when the last bytes have left the socket | | A response that closes the connection | The server half-closes and waits for the peer | The socket closes right behind the FIN | | `'drain'` after a later write flushed the backlog | Lost. `pipe(res)` could hang. | Emitted | | A pipelined request whose body continues after the previous response ends | Body dropped, no response, `server.close()` hangs | Delivered | | CONNECT and Upgrade tunnel sockets | Keep reading when paused or full | Stop reading. `_read()` starts them again. | | A tunnel write that waits for a drain when the client goes away | Its callback, the callbacks of the writes behind it and the `end()` callback never run | They run with an error before `'close'` | | Upgrade request with a body, paused in its listener | The body flows away | The request keeps its body | | `ws` on a reused keep-alive socket | Writes after the Upgrade could stall | Sent | | A raw `socket.write()` behind a response that still drains (the 400 for a bad pipelined request, the reply of a `'clientError'` listener) | Lands in the middle of that response | Sent after it | | `server.close()` | Could report closed while connections were open | Waits for every connection. An idle tunnel does not keep the process alive. | | `closeAllConnections()` on a listening server | Also stops the listener and destroys tunnels and WebSockets | Destroys only the HTTP connections | | `Proxy-Connection: close` (node:http only) | Ignored. The connection stays open. | Ends the connection, like `Connection: close` | | A response larger than 16 KB, also in `Bun.serve` and over TLS | Up to 4 `send()` calls for each chunk. Slower than Node in most cases. | One write for the writes of one tick. 1.1x to 2.7x the requests per second of main, and faster than Node. | | `socket.destroy()` and then `res.end()` in a listener | (this PR, earlier) `req` ended as if it were complete | `'aborted'`, then `ECONNRESET` | | `emit('connection')` or http2 `allowHTTP1`: the response ends while the listener still reads the body | The rest of the body is dropped | The body is complete | | `httpValidation: "relaxed"`, `Content-Length` or `Transfer-Encoding` in trailers | Accepted | `HPE_INVALID_CONTENT_LENGTH`, `HPE_INVALID_TRANSFER_ENCODING` | | `req.complete` inside `'connect'`, and inside `'upgrade'` without a body | `false` | `true` | | `optimizeEmptyRequests`: `socket.parser.incoming` after the response | Keeps the request alive on an idle connection | `null` | | A HEAD or OPTIONS request with `Content-Length` | The body is dropped, and `req.complete` is `true` before it comes | The request has its body | | `res.end(chunk)` after the client went away | `finished` and `writableEnded` stay `false`, no `'prefinish'` | The response ends | | An HTTP/1.0 request with an `Expect` header | `100 Continue`, `'checkContinue'`, `'checkExpectation'` or a 417 | A plain `'request'` | | The idle sweep of `close()` and `closeIdleConnections()` | Could destroy a connection that was still receiving a request, or whose response was still draining | Closes only idle connections | ### Design | Piece | What it is | | --- | --- | | Request body state | `None / Pending / Complete / Aborted / Upgraded / Detached`. Only the last chunk sets `Complete`. One function, `leave_pending`, is the only other way out of `Pending`. | | Read flow control | One path: `push()` returning false stops the socket, `_read()` starts it. Both native pause buffers are removed: no read is copied and replayed. | | "This read is parsed" signal | `notifyWhenReadParsed()` sets a uws state bit. uws delivers a `readParsed` event after the read. It replaces a `setImmediate`. | | Close during a parse | One uws bit defers a close to the end of the current message. | | Response finish | A response is finished when it has ended and the socket has fully drained. | | Idle connection | One rule, `HttpResponse::closeIfIdle()`. A connection is idle when it receives no request (head or body) and no response is in flight, queued or undrained. The sweep of `close()` and `closeIdleConnections()` both use it. | | Idle tunnel | A tunnel at read EOF with nothing left to send. uws reports it to the server through the connection filter (`-3`, `+3`, `-4`). It still counts for `'close'`, but it does not hold the event loop, like a libuv handle in that state. | | Server `'close'` | One native close promise per `listen()`. `close()` records whether its sweep left nothing open. Then a `listen()` in the same tick cannot hold `'close'` back, as in `net.Server._emitCloseIfDrained`. | | Raw socket writes | While uws holds response bytes (its buffer, the zero-copy tail of a `res.write()`, the cork buffer), a raw write goes through `AsyncSocket::write`, the path a 1xx line takes. So the order on the wire is the order of the calls. | | Upgrade verdict | One scanner and one verdict, shared by the parser and the dispatcher. | | llhttp | Updated from 9.3.0 to 9.4.2, as Node v26.5.0 vendors it, plus one local patch (see below). Node v26.5.1 and later vendor 9.4.3. That update is not in this PR. | The parser changes also tighten request framing so that it agrees with llhttp in more cases. There is no new API surface. ### A pause holds from the next read The copy of the rest of a read (`nodeHttpPausedSpill`), its replay from a posted task and the nested parse are removed. Like in Node, the rest of the read that caused a pause is still parsed, and the socket stops at the next read. usockets reads up to 512 KB in one call. libuv reads 64 KB. | One paused, unread request (client sends 64 MB) | Bytes held | | --- | --- | | Node 25.6 | 131,018 | | This pull request | 524,234 | The price is in one case. A client sends 512 KB of small pipelined requests (19,418 of them) and never reads. Each handler answers with its own 64 KB body: | Handler | Runtime | Requests dispatched | RSS | | --- | --- | --- | --- | | Answers at once | Node 26.3 | 2,425 | +177 MB | | Answers at once | main | 41 | +9 MB | | Answers at once | This PR | 2,425 | +171 MB | | Answers one tick later | Node 26.3 | 4,850 | +336 MB | | Answers one tick later | main | 2,426 | +181 MB | | Answers one tick later | This PR | 4,850 | +330 MB | Release builds on Linux x64. This PR now does what Node does. main held fewer responses, mostly for a handler that answers at once. On macOS one read can return all 512 KB. There, Bun 1.4.3 already reached +951 MB for the handler that answers one tick later, and Node reached +1,294 MB. `server.maxRequestsPerSocket` bounds it. ### Performance #### Responses larger than 16 KB are faster, and now faster than Node On main, a response that did not fit the 16 KB uWS cork buffer released the cork. After that, each piece was its own `send()`: the buffered head, the chunk-size line, the data, the `\r\n` and the last chunk. Over TLS, each 2-byte piece was also its own record. Two changes fix that, for `Bun.serve` and for node:http: | Change | Effect | | --- | --- | | A write that does not fit goes out with the cork buffer and its framing in one vectored write | No copy is added. Over TLS, the records of all the pieces share the write batch that one `SSL_write` loop already had. | | The cork buffer holds 128 KB, up from 16 KB. Only a write of 16 KB or less is copied into it, as before. | Several writes in one tick go out in one write, like in Node. A longer write still goes out without a copy. | The bytes on the wire are the same. The vectored write uses `sendmsg()` with the flags that `send()` uses. Write syscalls for one response: | Response | Node 26.3 | main | This PR | | --- | --- | --- | --- | | 4 x `res.write(16 KB)` | 1 | 16 | 1 | | 40 x `res.write(2 KB)` | 1 | 16 | 1 | | `res.end(64 KB)` | 1 | 2 | 1 | | 256 KB file, `.pipe(res)` | 4 | 16 | 5 | Throughput (req/s, the mean of 2 rounds). Node v26.3.0, main `97246d044e`, this PR `fe0ed1fbea`, with the method below: | Case | Node | main | This PR | main / Node | PR / Node | PR / main | | --- | --- | --- | --- | --- | --- | --- | | http, 4 x `res.write(16 KB)` | 17,735 | 6,983 | 19,160 | 0.39x | 1.08x | 2.74x | | https, 4 x `res.write(16 KB)` | 11,720 | 6,110 | 15,510 | 0.52x | 1.32x | 2.54x | | http, 40 x `res.write(2 KB)` | 9,879 | 6,140 | 14,980 | 0.62x | 1.52x | 2.44x | | https, 40 x `res.write(2 KB)` | 6,981 | 5,541 | 11,780 | 0.79x | 1.69x | 2.13x | | http, `res.end(64 KB)` | 18,535 | 16,528 | 19,889 | 0.89x | 1.07x | 1.20x | | http, 256 KB file `.pipe(res)` | 2,684 | 2,375 | 2,719 | 0.88x | 1.01x | 1.15x | | https, `res.end(64 KB)` | 11,894 | 14,586 | 16,426 | 1.23x | 1.38x | 1.13x | | http, GET hello (control) | 55,994 | 70,989 | 71,958 | 1.27x | 1.29x | 1.01x | main was slower than Node in six of these eight cases. This PR is faster than Node in all eight. `Bun.serve`, measured on `a771572a8d`, before the larger cork buffer (req/s, the mean of 2 rounds): | Case | main | PR | Change | | --- | --- | --- | --- | | Direct stream, 4 x 16 KB | 6,826 | 12,479 | +83% | | 64 KB string | 16,944 | 20,145 | +19% | | TLS, 64 KB string | 15,045 | 16,892 | +12% | | hello (control) | 83,957 | 83,137 | -1.0% | These runs are on loopback, where the kernel send buffer is 2.6 MB and the work of the receiver runs inside `send()`. That is the best case for fewer writes. A new connection over a real network takes about 46 KB in its first write on Linux. The rest waits in the socket buffer, as it would after separate writes. #### Small responses are unchanged A small response is already one `recvfrom` and one `sendto` on both builds. `perf` puts 66% of the time of a hello-world server in the kernel, on both builds. CI release builds on Linux x64: main `97246d044e` (the merge base) against this PR `8834cd0787`. Both use the same WebKit. The server runs on one pinned core. `oha` sends 64 connections for 5 s after a 2 s warm-up. There are 2 rounds, and the order of the builds alternates. "Change" compares the means of the two rounds. Framework servers from `bun-perf-tester` (req/s): | Server | main, round 1 | main, round 2 | PR, round 1 | PR, round 2 | Change | | --- | --- | --- | --- | --- | --- | | express | 49,803 | 51,103 | 49,968 | 50,263 | -0.7% | | fastify | 61,214 | 61,105 | 60,705 | 60,668 | -0.8% | | node:http | 70,934 | 71,405 | 73,178 | 71,053 | +1.3% | | elysia | 84,696 | 85,096 | 85,027 | 84,882 | +0.1% | | `Bun.serve` | 89,020 | 89,099 | 88,168 | 88,373 | -0.9% | node:http paths that this PR changes (req/s): | Case | main, round 1 | main, round 2 | PR, round 1 | PR, round 2 | Change | | --- | --- | --- | --- | --- | --- | | GET hello | 70,226 | 70,341 | 70,151 | 72,359 | +1.4% | | POST, 16 KB body | 47,446 | 47,789 | 48,191 | 48,785 | +1.8% | | 64 KB response in four writes | 6,885 | 6,894 | 6,834 | 6,868 | -0.6% | | Pipelined keep-alive, depth 8 | 94,063 | 93,294 | 92,909 | 93,809 | -0.3% | p99 latency (ms), the higher of the two rounds: | Server | main | PR | | --- | --- | --- | | express | 1.94 | 1.91 | | fastify | 1.52 | 1.55 | | node:http | 1.11 | 1.12 | | elysia | 0.98 | 0.96 | | `Bun.serve` | 0.82 | 0.83 | RSS (MB), one pass of 8 s of load: | Server | Build | Start | Under load | 5 s idle | 15 s idle | | --- | --- | --- | --- | --- | --- | | express | main | 39 | 94 | 61 | 57 | | express | PR | 40 | 92 | 60 | 57 | | fastify | main | 41 | 91 | 58 | 55 | | fastify | PR | 41 | 91 | 58 | 55 | | node:http | main | 20 | 64 | 43 | 40 | | node:http | PR | 20 | 65 | 45 | 41 | | elysia | main | 28 | 46 | 36 | 35 | | elysia | PR | 29 | 46 | 37 | 36 | | `Bun.serve` | main | 14 | 30 | 22 | 22 | | `Bun.serve` | PR | 14 | 30 | 22 | 22 | Every change is within 2%. fastify and `Bun.serve` hello are lower in both rounds, by about 1%. `Bun.serve` hello shows the same -1.0% in the control row above, so a small real cost there is possible. The RSS pass ran at the same time as the throughput runs, on other cores. The commits after `8834cd0787` change tests and add one version check to the node:http dispatcher. They were not measured. ### Supersedes | Theme | Pull requests | | --- | --- | | Request body | #43592 #43579 #38196 #43518 #43602 #43408 #43427 #43597 #43555 #43456 #43466 | | Tunnels | #43570 #43485. #43596 is a duplicate of #43570. | | Parser | #43182 #43161 #43326 #43327 #40505 #43363 #42532 #42194 | | Response write | #39386 #43371 #43548 #43499 #43496 #43464 #42008 #43549 | | Response finish | #40351 #43021 #41822 #43473 #42068 #35207 #43425 #43503 | | Lifecycle | #43413 #39686 #43028 #42727 #42622 #42610 #35837 #35839 #37825 #37749 #43376 #35268 | | JS API | #41691 #41738 #38036 #42462 #36527 #39718 #37964. #42947 merged on its own. | The close drain, `resetAndDestroy()`, the pending write callback handling and the response `'close'` ordering come from #42622 and #42727 by @steipete. The diagnosis and the tests for the stalled `ws` writes come from his #42610. Not included: | Pull request | Reason | | --- | --- | | #33061 | main already enforces `headersTimeout` and `requestTimeout` | | #41672 | It makes `http.createServer({ key, cert })` stop serving TLS. That needs a product decision. | | #37543 | A type refactor with no tests and no user-visible change | | #35465 | It makes `http.Server` extend `net.Server`. Only the prototype chains were joined. The `net.Server` constructor never ran, so `_handle` and `_connections` were `undefined`, and `_emitCloseIfDrained()` emitted `'close'` on a listening server. The server is backed by uWS, not `node:net`. | | The `AutoFlusher` removal in #42622 | It makes `flushHeaders()` flush at once. That is a performance change with no relation to the rest. | ### Tests | Check | Result on a debug build (macOS arm64) | Head | | --- | --- | --- | | Every test file that this PR touches (28 files) | 1,866 pass, 2 fail. The 2 failures are `serve.test.ts` "bounds memory when proxying ... to a stalled client". They fail the same way on a debug build of main. | `83af4da4a3`, run before the last commit of main came in | | `test/js/third_party/express` (9 files) and the `body-parser` test | 299 pass, 0 fail | `83af4da4a3`, run before the last commit of main came in | | Node 25.6 against Bun, 32 scenarios from two scripts (event order, framing, lifecycle) | No regression against Bun 1.4.3 | `0ff1a23f61` | | Every vendored Node `test-http-*` and `test-https-*` file, plus the `test-net-*` and `test-tls-*` files for pause, write, end and close | 535 of 537 exit 0. `test-http-agent-keepalive.js` and `test-https-timeout.js` fail on that debug build. Both pass on every CI lane. | `daee05fcfd` (before the rebase) | | The tests that depend on what the kernel takes in one send, on Windows Server 2019 x64 and Windows 11 arm64 | pass | `3eef223328` (x64), `a29289bcc1` (arm64) | | CI build 120191 (Linux, macOS and Windows, release and ASAN) | every lane passed | `daee05fcfd` (before the rebase) | Each new test fails on Bun 1.4.3, or on the commit before its fix for a fault that this branch introduced. The two tests over the limit are `node-http-connect.test.ts` ("tests should run on bun") and `node-http-syscall-fault.test.ts` ("racing a queued drain"). Each starts a debug subprocess that needs more than 5 s on this machine. Both pass on CI. ### Changes in the last push The branch is rebased on main (`daee05fcfd` was the head before). It is now linear. Four regressions against main, each with a test that fails without its fix: | Case | main | Before this push | Now (same as Node) | | --- | --- | --- | --- | | `emit('connection')` or http2 `allowHTTP1`: `res.end()` on a request that nobody reads | `'end'`, `'close'` | No events | `'end'`, `'close'` | | The same server, an unread 32 MB body | 0 bytes held | 32 MB held | 0 bytes held | | A NUL in a header value with `httpValidation: "relaxed"` (client, `HTTPParser`, `emit('connection')` server) | Accepted | The process spins forever | `HPE_INVALID_HEADER_TOKEN` | | `Connection: close`, body in the same read as the head, a 20 KB response before the body is read | `'end'` with an empty body | No events on `req` | `'end'` with the body, `'close'` | | An empty line on an idle keep-alive connection, then `server.close()` | 0 s | About 6 s | 0 s | | Fix | Where | | --- | --- | | The finish listener of a fallback connection dumps an unread request, like Node's `resOnFinish` | `http1_server_fallback.ts` | | llhttp patch: `llhttp__internal__c_test_lenient_flags_20` is false for a NUL. The relaxed state does not consume a NUL, and the next state sent it back there. 9.4.3 has the same loop. | `llhttp.c`, noted in its `README.md` | | A node:http socket that `onData` is parsing gets the close gate of `onData`, also when a large write released the cork | `HttpResponse.h` `uncorkCompletedResponse()` | | A read that starts no message leaves an idle connection idle | `HttpContext.h` `onData` | The open review threads are fixed in `8834cd0787`: `closeAllConnections()`, `Proxy-Connection: close`, five comments cut to one line, and the test of two overlapping listeners, which now waits on events. With the generation gate in `emitCloseServer` removed, that test fails in both cases. `AsyncSocketData` keeps its bools together, which takes it from 56 to 48 bytes per socket. `http.Server` no longer extends `net.Server` (see "Not included"). The special case for it in `Ipc.ts` is gone too. `child.send(msg, httpServer)` still throws `ERR_INVALID_HANDLE_TYPE`, and its test stays. <details><summary>Changes since the first revision (2988a61)</summary> Merged with main at `c8e1f6fa5b`. The one conflict was #43708 (`req.socket` emits `'end'` and `'error'`). Its state bit `HTTP_NODE_PEER_ENDED` moved to bit 22, because bit 19 is `HTTP_NODE_NOTIFY_READ_PARSED` here. Its 15 tests run in `node-http-server-abort-events.test.ts` next to the tests of this branch (103 pass). CI on `2988a610c` had ten red tests from four causes. They are fixed: - `ed882e5e99`: `write()` to a response without a body (HEAD, 204) does not wait for unsent bytes. - `811f817704`: an idle tunnel does not hold the event loop after `server.close()`. Four vendored Node tests timed out on every platform. - `0697deec11`, `d428824c08`, `7aec062d14`: the write callback tests use a body that backs up a loopback socket, and accept what Winsock does. - `c7af1c1615`: two tests from main asserted the old `close()` contract. Review findings, each reproduced against Node v26.3.0 and fixed with a test that fails without the fix: - `d4d2b783ea`, `2cc79939bb`, `45141abca9`, `9a63dbc470`, `80a23a1822`: the idle rule. A keep-alive connection is idle again when its body ends after its response. A connection that owes a queued pipelined response, that still receives a request head or body, or whose response still drains is not idle. - `87d8904aa3`, `7eca4f7806`: `close(cb)` followed by `listen()` in the same tick reports `'close'`, also for an https server whose only connection was idle. - `3fd7b25382`: a paused pipelined request behind a response that still drains stops the connection. The first revision read 512 MiB of 512 MiB into memory. - `2d1a5e2d8d`, `33e4683a8a`, `539cb41eb9`, `197c7dfc29`, `0490e3540b`: raw socket writes stay behind every unsent response byte. The cases were a CONNECT pipelined behind a response that still drains (its `200` landed at offset 2.6 MB of a 64 MiB body), a zero-length tunnel write (it hung the tunnel), the 400 replies above, the zero-copy tail of a large `res.write()`, the cork buffer, and Windows 11, where the kernel takes the whole response and refuses the next send. - `0abd39c133`: an upgrade from the request's `'end'` listener keeps the body bytes out of the WebSocket. The connection closed with 1006 right after the 101. - `5aafc61aa6`: the callback of a small `res.write()` that the kernel refuses at the uncork runs on the drain. Reproduced on Windows 11 only. - `a29289bcc1`: a tunnel write that waits for a drain settles its callbacks when the connection closes. Known differences from Node that this PR leaves: - A handler that calls `res.end()` and then `server.close()` closes its keep-alive connection at once. Node waits for the `keepAliveTimeout`. - A pipelined Upgrade behind a response that still drains is served as a plain request. - An `end()` on a tunnel with no write pending, while the response before the CONNECT still drains, closes both directions after the flush. Node half-closes. - A raw `req.socket.write(big)` and `req.socket.end()` with no `res.end()` sends every byte but no FIN. main loses bytes here. - A CONNECT socket that is given back with `server.emit('connection', socket)` answers only the first of several pipelined requests. main answers none. - A large write from an `'upgrade'` listener stalls while the body of that Upgrade request is still pending. A second `listen()` on a listening server does not throw. Both are the same on main. - A tunnel write that fails because the client went away fails its callbacks but emits no `'error'`. Node emits `ECONNRESET`. A new `'error'` could end a process that has no listener for it. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 8 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch.stream.test.ts, test/js/node/url/url.test.ts, test/js/node/tls/tls-syscall-fault.test.ts, test/js/node/net/node-net-server.test.ts, test/js/node/http/node-http.test.ts, test/js/node/http/node-http-syscall-fault.test.ts, test/js/node/http/node-http-server-close-drain.test.ts, test/js/node/http/node-http-connect.test.ts, test/js/node/http/node-http-backpressure.test.ts, test/js/node/child_process/child_process_ipc_handle.test.ts, test/js/bun/http/serve.test.ts, test/js/bun/http/serve-syscall-fault.test.ts, test/js/bun/http/bun-server.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Collaborator
|
I checked if main (9f70da0) makes this PR obsolete. It does not. main covers most cases. It differs from Node in the cases below, and this PR has code for them. The server ran on a debug build of main and on Node v26.10.0, on Linux x64. The client was the same Node script for both. Different from Node
Causes on main
Same as Nodemain runs Node's
The test block of this PR (5b38098) runs on main with no source change from this PR: 25 of 27 pass. The other 2 wait for #43557 lists this PR as covered by main. That is correct only for this last list. Results for the cases that are the sameThese rows use
Note: Linux gives a connection with no data to the server after 1 s ( Tests on this build of main:
Two tests of the block assert a behaviour that Node v26.10.0 does not have:
Script for three rows of the first table// Run with `node repro.mjs` and with `bun repro.mjs`. It takes about 21 s.
import http from "node:http";
import net from "node:net";
function probe(name, options, idle) {
const server = http.createServer({ connectionsCheckingInterval: 100, ...options }, (req, res) => res.end("ok"));
let error = "no 'clientError'";
const emit = server.emit;
server.emit = function (event, ...args) {
if (event === "clientError") error = `'clientError' ${args[0].code}`;
return emit.apply(this, [event, ...args]);
};
server.listen(0, "127.0.0.1", () => {
const socket = net.connect(server.address().port, "127.0.0.1");
let received = "";
let firstByte;
let reported = false;
const report = state => {
if (reported) return;
reported = true;
clearTimeout(giveUp);
const status = received.split("\r\n")[0] || "no bytes";
console.log(`${name}: ${state} ${Date.now() - firstByte} ms after the first byte, ${status}, ${error}`);
socket.destroy();
server.close();
};
const giveUp = setTimeout(report, idle + 21000, "still open");
socket.on("connect", () =>
setTimeout(() => {
firstByte = Date.now();
socket.write("GET / HTTP/1.1\r\nHost: localhost\r\nX-Partial: ");
}, idle),
);
socket.on("data", chunk => (received += chunk));
socket.on("error", () => {});
socket.on("close", () => report("closed"));
});
}
probe("headersTimeout 20000", { headersTimeout: 20000, requestTimeout: 30000 }, 0);
probe("both timeouts 0", { headersTimeout: 0, requestTimeout: 0 }, 0);
probe("headersTimeout 3000, first byte 2500 ms after connect", { headersTimeout: 3000, requestTimeout: 6000 }, 2500);Node v26.10.0: main (9f70da0, debug build): A release build of 367d939 (2026-09-18) prints the same as main, with 1625 ms, 12023 ms and 12025 ms. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Server.headersTimeoutandServer.requestTimeout(node:http/https) were validated and stored on the server object, but never enforced: the node:http server is created withidleTimeout: 0, so a connection with stalled request headers, a stalled request body, or that never sent a byte was kept open indefinitely.This wires both values into uWS as per-socket receive deadlines:
headersTimeout(default 60s).requestTimeout(default 300s).'clientError'withERR_HTTP_REQUEST_TIMEOUT("Request timeout"), writesHTTP/1.1 408 Request Timeout\r\nConnection: close\r\n\r\nwhen nothing has been written for that message yet, and closes the socket.0disables either value.All of it is gated on a node-only uWS context option (
setNodeReceiveTimeouts), soBun.serveidleTimeout, keep-alive, and upgrade behavior is untouched (Bun.serve sockets keep the legacy 10s pre-request timeout).How it was verified
Behavior was derived from real Node (v26.3.0) fixtures and matched case by case: stalled first headers, zero-byte connection, stalled body after dispatch (handler runs, then
'clientError'+req 'aborted'+ 408), stalled second keep-alive request, fully-received request with a slow handler (not killed), idle keep-alive socket (not reaped by these timeouts), both values0(disabled), and a user'clientError'listener taking over teardown.Known bounded deviations from Node: expiry granularity is uWS's ~4s timer sweep rather than
connectionsCheckingInterval(Node's default check interval is 30s, so both are coarse); the deadline is re-armed per received chunk (inactivity-based) instead of an absolute per-message deadline; the canned 408 is written from native code even when a synchronous'clientError'listener has not yet destroyed the socket (same as the existing native 400/431 parse-error responses); values are applied atlisten()time; per-socket values above ~940s are capped (uWS timer slot range).Tests
New cases in
test/js/node/http/node-http.test.ts("server headersTimeout/requestTimeout enforcement"): http+https stalled headers => exact 408 bytes + close;'clientError'ERR_HTTP_REQUEST_TIMEOUT; stalled body => handler dispatched then aborted with a 408; stalled second keep-alive request; fully-received request with a slow handler still gets its 200; timeouts disabled with 0. All except the two negative-contract cases fail on the previous behavior (USE_SYSTEM_BUN=1).Also re-ran: the full
test/js/node/http/suite, the vendoredtest-http(s)-server-*timeout*, keep-alive, upgrade and CONNECT node parallel tests,test/js/bun/http/serve.test.ts, and the Bun.serve websocket suites.