Conversation
A client that sends Expect: 100-continue withholds the request body until it receives the 100 Continue, so answering with a final status leaves the announced Content-Length unsent and the connection no longer framed. The server kept it alive anyway, and the parser still expected the body: the client's next request on that connection was consumed as the previous request's body, silently swallowing it. Mirror Node.js: record _expect_continue when the expectation is parsed, and clear shouldKeepAlive for a final response that was not preceded by writeContinue(), which answers Connection: close and ends the socket after 'finish'. Responses that did send the 100 Continue, and the 417 for any other expectation, keep the connection reusable as before.
|
Updated 11:16 PM PT - Jul 2nd, 2026
✅ @robobun, your commit 1c11253a25d4d8b2d21470b6c11208a4a46534ce passed in 🧪 To try this PR locally: bunx bun-pr 33293That installs a local version of the PR into your bun-33293 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe server now tracks ChangesExpect: 100-continue keep-alive fix
Related PRs: None identified. Suggested labels: node.js, http Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/_http_server.ts`:
- Around line 1461-1464: The keep-alive clear path in
clearKeepAliveIfContinueNotSent only updates shouldKeepAlive, so a checkContinue
response that explicitly sets Connection: keep-alive can still be treated as
reusable. Update the response handling in clearKeepAliveIfContinueNotSent and
the header flattening flow in renderNativeHeaders so the close transport state
is forced as well, and any explicit keep-alive Connection header is overridden
or removed before headers are finalized. Make sure the fix covers the related
continue/connection handling paths associated with renderNativeHeaders and the
explicit Connection header logic.
In `@test/js/node/http/node-http.test.ts`:
- Around line 3528-3530: The pending-read helper in the socket-based test only
rejects on error and end, so a raw socket close can leave until() waiting
indefinitely. Update the socket event wiring in the test helper to also fail on
close by adding a close handler alongside the existing error and end handlers,
and ensure the same pattern is applied to any awaited condition in this area so
every failure path rejects the pending promise.
🪄 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: 7186ae5e-d79b-4723-9b77-88dffeab3e37
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/http/node-http.test.ts
A raw socket can close without emitting 'end', which would leave until() waiting until the test timeout instead of failing with the bytes it had.
|
Re the suggested #20415 is about the server sending a Adding the auto-close keyword would silently close a real, still-open bug on merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — direct port of Node's _expect_continue/shouldKeepAlive rule with thorough positive/negative test coverage.
Extended reasoning...
Overview
This PR fixes node:http server behavior when a checkContinue handler rejects an Expect: 100-continue request without calling writeContinue(). Previously Bun kept the connection alive with the parser still expecting the announced body, causing the next pipelined request to be silently swallowed. The fix adds an _expect_continue flag on ServerResponse and a small helper clearKeepAliveIfContinueNotSent() that clears shouldKeepAlive — a direct 1:1 port of Node.js's writeHead logic. The helper is called from both writeHead() and renderNativeHeaders() to cover Bun's lazy-header-rendering fast path. Source change is ~20 lines in src/js/node/_http_server.ts; ~175 lines of tests added to test/js/node/http/node-http.test.ts.
Security risks
None. This is HTTP/1.1 connection-lifecycle framing. Closing a mis-framed connection after a rejected expectation is strictly safer than the previous behavior of keeping it alive and mis-parsing subsequent bytes. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Moderate. node:http keep-alive is production-critical, but the change is tightly scoped and purely additive: _expect_continue defaults to false and is only set to true on the existing Expect: 100-continue branch, so every other request path is unaffected. The helper reuses the existing shouldKeepAlive → kMustCloseConnection → Connection: close + socket-end machinery rather than adding new close logic. The dual call site (writeHead + renderNativeHeaders) is well-justified in the PR description and cannot disagree since _sent100 cannot change between them.
Other factors
- Five new raw-socket tests cover the full variant matrix: 417 via
writeHead, 417 viastatusCode=(no writeHead), 200 without writeContinue (all close), plus two negative cases proving keep-alive is preserved afterwriteContinue()and for non-100 expectations. All verified to pass verbatim on Node v26.3.0. - The first test pipelines a follow-up request and asserts it is never answered and the server FINs on its own — directly checking the swallowed-request bug shape.
- Both CodeRabbit comments were resolved: the explicit
Connection: keep-aliveoverride concern was withdrawn after the author demonstrated Node behaves identically; the test-helperclosehandler was added in 3dc080d. - PR description documents a regression sweep including the vendored Node
test-http-expect-continue-reuse-race.js/test-https-expect-continue-reuse-race.jstests (100 keep-alive reuses each), which would catch over-eager closing. - No CODEOWNERS entries for these paths. No bugs found by the bug hunting system.
CI status on 3dc080dThe two red lanes in build 68161 are both off this diff. Details so the next reader does not have to re-derive them:
Its upstream
That test requires only The change itself is green. On All 15 platforms' The build is still running, so I am leaving it to finish rather than superseding it with a retrigger. |
|
Build 68161 finished: 284 passed, 2 failed, and the two failures are the ones described above with nothing new. So I have pushed a single One loose end closed from that comment: I had argued the x64-asan abort could not involve this diff because If the re-roll comes back red on the same two lanes, I will leave it and defer to a maintainer rather than keep retriggering. |
There was a problem hiding this comment.
LGTM — small, well-tested Node.js parity fix that only affects Expect: 100-continue requests.
Extended reasoning...
Overview
This PR ports Node.js's _expect_continue / shouldKeepAlive interaction from lib/_http_server.js into Bun's node:http server. When a client sends Expect: 100-continue and the handler responds with a final status without first calling writeContinue(), the announced request body is never sent, so the connection is unframed and must be closed. The production change is ~20 lines across three sites in src/js/node/_http_server.ts: set _expect_continue = true where the expectation is parsed, initialize the field in the ServerResponse constructor, and add a clearKeepAliveIfContinueNotSent() helper invoked from both writeHead() and renderNativeHeaders() (the latter to cover the fast path where callWriteHeadIfObservable skips writeHead). Five new raw-socket tests in test/js/node/http/node-http.test.ts cover the variant matrix (417 via writeHead, 417 via statusCode only, 2xx without writeContinue, keep-alive preserved after writeContinue, and keep-alive preserved for non-100-continue expectations); all five are verified to pass on real Node v26.3.0.
Security risks
None. This is HTTP/1.1 connection-lifecycle management in the Node compat layer — no auth, crypto, permissions, or untrusted-input parsing is touched. The change is strictly more conservative than before: it closes connections that were previously left in a poisoned state where the next pipelined request would be silently consumed as the prior request's unsent body.
Level of scrutiny
renderNativeHeaders runs for every server response, so I looked carefully at blast radius. The new helper is gated on res._expect_continue, a new field that defaults to false and is only set to true in one place (when continueExpression matches the Expect header). For every request without Expect: 100-continue, the added code is a no-op and behavior is byte-identical. The helper only ever clears shouldKeepAlive, and it is placed before the _removedConnection / !hasConnection branches that read it, so ordering is correct. The double invocation (once in writeHead, once in renderNativeHeaders) is idempotent, and _sent100 cannot flip between them since writeContinue() throws once headers are sent. This is a direct 1:1 port of Node's own two-line check, well within "following established patterns with no ambiguity."
Other factors
Both CodeRabbit inline comments are resolved: the explicit Connection: keep-alive override case was verified against live Node v26.3.0 (Node keeps the connection alive there too, so matching is deliberate parity), and the test helper now rejects pending reads on close as well as error/end. No CODEOWNERS covers these files. The PR description documents a regression sweep across 29+ vendored Node keep-alive/pipeline/expect-continue tests, and CI shows the affected test file passing (133 pass, 0 fail) on the shard that carries it; the two red lanes are unrelated infrastructure/pre-existing flakes as detailed in the author's CI status comment.
|
The re-roll came back clean: build 68164 is 286 passed, 0 failed, including the two lanes that were red before ( Nothing outstanding on my side: CI is green, and the two review threads are resolved. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-03 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Repro
A client that sends
Expect: 100-continuewithholds the request body until it sees a100 Continue. Rejecting the expectation fromcheckContinue(the documented way to turn away a large upload early on auth/quota) therefore leaves the announcedContent-Lengthunsent:Bun keeps the connection alive while the parser is still armed for the 36 body bytes that will never arrive, so the client's next request on that connection gets consumed as the previous request's body: it is silently swallowed and never dispatched, and the connection stalls.
Cause
Node's
lib/_http_server.jsrecordsres._expect_continue = truewhen it parses the expectation, andwriteHeadthen does:Bun's
ServerResponsealready tracks_sent100and already honoursshouldKeepAlive === false(renderNativeHeadersanswersConnection: closeand setskMustCloseConnection, whosefinishlistener ends the socket), but nothing ever set_expect_continue, so the rule never fired.Fix
Set
_expect_continuewhere the expectation is parsed, and clearshouldKeepAlivefor a final response that was not preceded bywriteContinue(). Note this is not gated on the status code: any final response, 417 or 200, leaves the announced body unsent, and Node closes in both cases.The check runs in two places because Bun renders headers lazily:
ServerResponse.prototype.writeHead(Node's location, which also makesres.shouldKeepAliveobservablyfalseinside the handler) andrenderNativeHeaders, sincecallWriteHeadIfObservableskipswriteHeadentirely when the user never overrode it, sores.statusCode = 417; res.end()would otherwise miss the rule._sent100cannot change between the two (writeContinue()throws once headers are sent), so they always agree.Responses that did send the
100 Continue, and the default 417 for any other expectation (which never withholds a body), keep the connection reusable exactly as before.node:http2is untouched: a rejected h2 stream does not poison the connection.Verification
Five new raw-socket tests in
test/js/node/http/node-http.test.ts. Three of them fail on unpatched Bun, and all five pass verbatim on real Node v26.3.0 (the file requires Node parity):checkContinue→writeHead(417)Connection: close+ FINcheckContinue→statusCode = 417, nowriteHeadConnection: close+ FINcheckContinue→200withoutwriteContinue()Connection: close+ FINcheckContinue→writeContinue()then200Expect: meoww→ default 417The first test pipelines the request a client would send next and asserts it is never answered and that the server sends FIN on its own, so the swallowed-request shape is what is actually being checked.
Regression sweep
test/js/node/http/node-http.test.ts(only pre-existingissue#4295external-proxy failure, fails identically without the change)test-http-expect-continue-reuse-race.jsandtest-https-expect-continue-reuse-race.js, which each reuse one keep-alive socket for 100Expect: 100-continuerequeststest-http-expect-continue.js,test-http-expect-handling.js,test-http-sync-write-error-during-continue.js,test-http-write-callbacks.js, the threetest-http2-compat-expect-*tests