Repository navigation
Conversation
…on unsatisfied-body FIN Node emits 'upgrade' after the parser.execute() that consumed the header block returns, with head = the remainder of that same chunk AFTER the declared body, the body already buffered on req, and req.complete true. Bun emitted 'upgrade' before the body was parsed with an empty head, then routed the same-chunk remainder to the socket's raw-data path. HttpParser now pre-computes head as the post-body slice (for both Content-Length and a chunked body that completes in-chunk) and reports bodyCompleteInHead; the dispatcher defers the 'upgrade' emit to the body fin so the listener observes req.complete. A new state bit suppresses the same-chunk remainder from onSocketData for that one parse call only. A peer FIN with the declared body unsatisfied left the tunnel-after-body socket half-open and never released the pending-request ref; onEnd now closes it (like Node's UpgradeStream), which drives socket 'close' and lets server.close() complete.
WalkthroughChangesNode Node HTTP upgrade body handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:21 AM PT - Jul 27th, 2026
❌ @robobun, your commit 57f25c7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35983That installs a local version of the PR into your bun-35983 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… + finally in tests The head-slice must agree with NodeHTTPResponse__createForJS's has_body (which excludes HEAD/TRACE) so a HEAD+Upgrade+Content-Length request keeps every post-header byte in head instead of slicing bytes that no channel then delivers. [skip size check]
[skip size check]
…de the liveness gate
The ConsumeMinimally fallback reassembly parses a view truncated to
maxFallbackSize, so a head sliced from it would not match the full-buffer
remainder the suppression later drops from onSocketData. Clear head there
so JS emits with an empty head and every post-body byte reaches
socket.on('data') like before; add a test covering the split-header +
large-tail shape.
Move the HEAD_PENDING clear inside the returnedData != nullptr gate so it
never touches httpResponseData after onClose has destructed it.
[skip size check]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http/node-http-upgrade-with-body.test.ts`:
- Around line 335-344: Replace the strict stderr equality assertion in this
subprocess test with the established combined assertion pattern used by the
sibling upgrade-body test: continue validating stdout and exitCode, but use an
asymmetric matcher for stderr so benign runtime diagnostics do not fail the
test.
🪄 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: 255be053-cfea-47a5-8b22-819bf88f4c86
📒 Files selected for processing (7)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpParser.hpackages/bun-uws/src/HttpResponseData.hsrc/js/internal/http.tssrc/js/node/_http_server.tssrc/jsc/bindings/NodeHTTP.cpptest/js/node/http/node-http-upgrade-with-body.test.ts
…r order, drop redundant onSocketData; fix split-header test race on Windows [skip size check]
|
Diff is ready at 57f25c7 (simplified: chunked pre-scan dropped, Content-Length only; src/packages 145 → 94 insertions). All 10 tests pass locally on debug (ASAN), release, and Windows debug; all CI build 83122: 148 lanes passed, 44 running; failures are |
…xt per-chunk [skip size check]
[skip size check]
There was a problem hiding this comment.
No new findings on 1d2149b, but this touches the HTTP parser's body-framing path and adds a chunked pre-scan that must mirror the real parse's error gates, so it's worth a human look.
Checked this round: the !deferredTransferEncodingError gate now keeps a non-chunked TE from entering the pre-scan; the per-chunk scanExt overflow check and validateNodeTrailerSection gate match the real parse; socket.once('close', resolveUpgrade) runs after the emit like the CONNECT path; the onEnd tunnel-after-body branch defers fin delivery to onClose (no double-dispatch).
Extended reasoning...
Overview
Changes span the uWS HTTP parser (HttpParser.h — new bodyCompleteInHead field and a chunked-body pre-scan that computes the post-body head slice), HttpContext.h (new HTTP_NODE_TUNNEL_HEAD_PENDING bit to suppress one same-chunk onSocketData; onEnd now closes on unsatisfied-body FIN instead of leaving the tunnel half-open), HttpResponseData.h (the new flag), NodeHTTP.cpp (passes bodyCompleteInHead to JS), and the JS side (_http_server.ts wraps the emit in emitUpgrade() and defers it via kDeferredUpgradeEmit; internal/http.ts consumes the deferred emit at the body fin, running emitEOFIncomingMessageOuter synchronously first so req.trailers is populated). ~500 lines across 7 files plus a 356-line test.
Security risks
The pre-scan re-parses the chunked body with local state before the real parse runs, so any gate the real parse enforces but the pre-scan misses would set bodyCompleteInHead=true for a body the real parse then rejects — stranding kDeferredUpgradeEmit until onClose fires it after 'clientError'. Three such gaps were found and fixed across earlier rounds (scanExt per-chunk, validateNodeTrailerSection, deferredTransferEncodingError). I did not find a fourth, but the pre-scan/real-parse lockstep is the fragile invariant here and a human should confirm no other real-parse error path exists between requestHandler return and the fin dataHandler. The head slice arithmetic is bounded by length and gated on remainingStreamingBytes <= length, so no OOB read.
Level of scrutiny
High. This is the node:http request-framing path — the same code that guards against request smuggling — and it introduces a second parse of the same bytes whose result must agree with the first. The onEnd semantic change (close instead of half-open on unsatisfied body) is a behavior change for the tunnel-after-body state.
Other factors
Three prior review rounds each surfaced a real correctness issue that was then fixed; the test file covers the CL/chunked/trailer/split/fallback-buffer/FIN matrix and asserts req.complete/readableLength/trailers at emit time. The PR description reports all test-http-upgrade-* and test-http-connect* still pass. The design choice to pre-scan (rather than, say, delay computing head until after the real parse's fin dispatch) is worth a maintainer's eye — it works, but keeping two chunked-decode passes in agreement is a maintenance cost.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Too much code. Simplify.
The chunked pre-scan had to mirror every gate the real parse applies
before the fin dispatch (per-chunk scanExt, validateNodeTrailerSection,
deferredTransferEncodingError) to stay consistent. Drop it: for a
chunked Upgrade body head is empty and the post-body remainder reaches
socket.on('data'), which is what Node's own upgrade-with-body test
concatenates with head anyway. bodyCompleteInHead is now only set for a
Content-Length body that fits in this chunk, on the fast path.
[skip size check]
|
Simplified in 57f25c7: dropped the chunked-body pre-scan (which had to mirror every gate the real parse applies and was ~35 lines of fragile lockstep). src/packages diff: 145 → 94 insertions. 10/10 tests pass, all |
There was a problem hiding this comment.
The 57f25c7 simplification (CL-only, no chunked pre-scan) resolves the pre-scan/real-parse consistency class I flagged on earlier revisions — the contentLengthStringLen && !transferEncoding.has && remainingStreamingBytes <= length gate now exactly matches what the real parse's CL branch will dispatch as fin=true, so kDeferredUpgradeEmit can't be stranded. No new issues on this revision, but this still spans the parser, onEnd close semantics, and a new state flag threaded through four layers, so worth a human sign-off.
What was checked on 57f25c7:
- CL-only head-slice gate vs. the real parse's CL dispatch — no divergence remains;
!ConsumeMinimallycorrectly excludes the fallback-buffer path. emitUpgrade()ordering:once('close', resolveUpgrade)after emit (matches CONNECT); the!handledbranch now returnsupgradePromiseinstead ofundefined, but destroy→close settles it.onEndTUNNEL_AFTER_BODY: close-only,onClosedelivers both fins;HEAD_PENDINGclear scoped inside thereturnedData != nullptrblock.- HEAD/TRACE exclusion is conservative (falls back to pre-PR behavior);
has_bodycomputation in NodeHTTPResponse.rs agrees with the JS branch selection.
Extended reasoning...
Overview
Node-compat fix for 'upgrade' with a request body: the head argument becomes the post-body remainder of the same TCP chunk (Content-Length only), req.complete/readableLength are populated before the listener runs via a deferred emit from onDataIncomingMessage's fin, and a peer FIN with an unsatisfied body now closes the tunnel-after-body socket instead of leaving it half-open. Touches HttpParser.h (new bodyCompleteInHead field + CL arithmetic on req->head), HttpContext.h (new HTTP_NODE_TUNNEL_HEAD_PENDING suppression flag, onEnd close for unsatisfied body), HttpResponseData.h (flag definition), NodeHTTP.cpp (pass bodyCompleteInHead through), internal/http.ts + _http_server.ts (deferred emit + head selection), plus a 356-line test file.
Review history
This is my fourth look. Earlier rounds flagged (all resolved): emit/close-listener ordering regression, redundant onSocketData in onEnd, and three pre-scan/real-parse consistency gaps in the chunked pre-scan (validateNodeTrailerSection, per-chunk scanExt, deferredTransferEncodingError). 57f25c7 dropped the chunked pre-scan entirely, which structurally eliminates that class — bodyCompleteInHead is now only set when the CL arithmetic guarantees the real parse's else if (contentLengthStringLen) branch will call dataHandler(..., true) on the same chunk.
Security risks
The head-slice touches fenceAndConsumePostPadded, but is gated on IsNodeHttp && !ConsumeMinimally and only reads remainingStreamingBytes (already validated <= STATE_SIZE_MASK) against length; the (unsigned int) narrowing of skip is safe because skip <= length and length is unsigned int. No new untrusted-input parsing. The onEnd change makes an unsatisfied-body FIN more aggressive (close instead of half-open), not less.
Level of scrutiny
High — HTTP parser and connection-lifecycle changes, even narrowly gated, warrant maintainer sign-off. The specific items I'd want a human to confirm: (1) the semantic decision to only pre-compute head for CL bodies (chunked remainder now goes to socket.on('data') instead — the PR argues Node's own test concatenates them anyway); (2) the onEnd close for HTTP_NODE_TUNNEL_AFTER_BODY — this is a behavior change from half-open to close for a state that was previously grouped with isConnectRequest; (3) the emitEOFIncomingMessageOuter synchronous call in the deferred branch (bypasses the nextTick that the non-deferred path uses).
Other factors
10 tests, all use test.concurrent, wire error/clientError to reject, release resources in try/finally, and the split-header test's once(server,'connection') race was fixed. The evidence shows 8/10 fail on the released binary and all pass on debug+ASAN. All test-http-upgrade-* and test-http-connect* continue to pass per the PR description.
|
Superseded by #45058. That change emits The second half of this pull request, the close on a FIN that comes before the end of the body, is not in #45058. |
Problem
Node emits
'upgrade'after theparser.execute()that consumed the header block returns: any declared body bytes present in that same chunk are already buffered onreq(withreq.complete === true), andheadis the chunk remainder after the body. Bun emitted'upgrade'before body parsing with an empty head, then routed the same-chunk remainder to the socket's raw-data path.Separately, a peer FIN with the declared body unsatisfied left the tunnel-after-body socket half-open and the pending-request ref unreleased, so
server.close()never completed.Fix
HttpParser.h: for a body-carrying Upgrade, pre-computereq->headas the post-body slice of this chunk (Content-Length arithmetic; for a chunked body, a local-state pre-scan of the same bytes) and setbodyCompleteInHeadwhen the body completes in-chunk.HttpContext.h: setHTTP_NODE_TUNNEL_HEAD_PENDINGwhenbodyCompleteInHeadand the dispatcher accepted the upgrade, and skip the oneonSocketDatacall for the same-chunk remainder (cleared at end of eachonData, so later packets flow normally).onEndnow closes the connection when the body never completed (Node'sUpgradeStreamwraps a socket whosesocketOnEndstillsocket.end()s here)._http_server.ts: passconnectHeadthrough ashead(native already sliced it). WhenbodyCompleteInHead, defer the'upgrade'emit to the body fin inonDataIncomingMessageso the listener observesreq.completeand the buffered body like Node.Verification
test/js/node/http/node-http-upgrade-with-body.test.tscovers: same-chunk CL / chunked / chunked-with-trailer (head="AFTER"), same-chunk no-remainder (head=""), split body (head="", later bytes -> socket data), later tunnel bytes after a same-chunk head, unsatisfied-body FIN (socket closes,server.close()completes), and a spawned process-exit check. All pass under this change and under Node v26; 8/10 fail or hang on main.All
test/js/node/test/parallel/test-http-upgrade-*andtest-http-connect*continue to pass.Supersedes #35974 (CL-only, JS-side slice) and #35063 (the onEnd close half): this handles chunked bodies and matches Node's emit timing (
req.complete/readableLengthat'upgrade'time).[review] gate passed · iteration 3 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 2 rejected · iteration 3
evidence per changed file