Conversation
…equest body completes An HTTP Upgrade request that carries a body (Content-Length or chunked) wedged the process when the client half-closed before the body completed: HttpContext::onEnd kept the connection half-open for HTTP_NODE_TUNNEL_AFTER_BODY the same way it does for a live CONNECT tunnel, so the NodeHTTPResponse body-read ref and the server's pending-request count were never released. server.close() never completed and the event loop never drained. Node's UpgradeStream wraps a raw socket whose socketOnEnd listener is still attached while the body is being parsed, so a mid-body FIN ends and closes the raw socket and the UpgradeStream destroys with it. Once the body finishes the connection becomes a proper tunnel (isConnectRequest) and stays half-open on FIN as before.
|
Updated 8:09 AM PT - Jul 22nd, 2026
❌ @robobun, your commit bb9d44c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35063That installs a local version of the PR into your bun-35063 --bun |
WalkthroughChangesNode HTTP EOF lifecycle
Possibly related PRs
Suggested reviewers: 🚥 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: 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.test.ts`:
- Around line 3471-3474: Shorten the comment near the UpgradeStream test to a
concise lifecycle invariant: a FIN received during body transmission must close
the accepted Upgrade socket. Remove the explanatory PR context about
socketOnEnd, pending-request accounting, and process hangs, keeping the comment
within three lines.
🪄 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: 4a7587bc-fdc8-4cf9-bde2-9222724b095e
📒 Files selected for processing (2)
packages/bun-uws/src/HttpContext.htest/js/node/http/node-http.test.ts
|
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}
There was a problem hiding this comment.
Beyond the inline nit, I traced the state machine: once the body completes the data handler clears HTTP_NODE_TUNNEL_AFTER_BODY and sets isConnectRequest, so the body-completed half-open path is unchanged; and onClose<IsNodeHttp> already runs both socketData last=true and inStream last=true for this flag, so the close here does release the body-read ref and pending-request count as the description claims.
Extended reasoning...
Verified the PR's mechanism claims against the surrounding code rather than taking them on faith. The two states are mutually exclusive at the point onEnd runs (the flag is cleared exactly when isConnectRequest is set in the data handler's fin path), so splitting the condition does not change behavior for the completed-body case. The new branch's uncorkWithoutSending() + close() matches the existing fallthrough shape at the bottom of onEnd. Deferring rather than approving because this is FIN-handling control flow in vendored uWS with node:http compat semantics — worth a human look even though I found no correctness issue.
socket.write() inside the upgraded socket's 'end' listener ran with the JS Duplex still writable but the native handle already closed; handle.write() returned 0, the callback was stashed in #pendingCallback, and nothing ever flushed it. Node fails the write with ERR_STREAM_WRITE_AFTER_END. Also flush #pendingCallback from #onClose so a backpressured write whose drain never arrives still reports failure. Test comment trimmed and the assertion switched to a combined object per review.
node-http.test.ts has an unrelated pre-existing failure
('request via http proxy, issue#4295' ECONNREFUSED) in the
gate environment.
#pendingCallback is only ever stashed against a live handle; the socket
closing before drain is ERR_STREAM_DESTROYED('write'), not write-after-end.
There was a problem hiding this comment.
No bugs found; both earlier nits (stderr assertion, $ERR_STREAM_DESTROYED for the backpressure-then-close flush) are addressed. Deferring to a human because this changes uWS connection-lifecycle behavior for node:http Upgrade — the ref-release path now depends on onEnd → close() → onClose running the tunnel-after-body cleanup, which is the kind of lifetime/ref-count change that warrants a maintainer look.
What was reviewed:
onEndsplit: verifiedonClose(HttpContext.h:241-248) already dispatchessocketData/inStreamwithlast=trueforHTTP_NODE_TUNNEL_AFTER_BODY, so the newclose()path releases the body-read ref as claimed._writeguard:handle.closedis an existing property read elsewhere in this file; the callback is failed instead of stashed, so it can't leak.#onCloseflush: only reachable for a write stashed whilehandle.closed === false, soERR_STREAM_DESTROYED('write')is the right code;$ERR_STREAM_DESTROYEDis a declared builtin.- Test: combined-object assertion, drains all pipes concurrently, spawn timeout guards the pre-fix hang; separate-file placement and the 15s per-test ceiling were considered and are fine given the 4.5s ASAN wall time.
Extended reasoning...
Overview
Three files: packages/bun-uws/src/HttpContext.h splits onEnd's half-open decision so isConnectRequest (CONNECT / body-complete Upgrade) stays half-open while HTTP_NODE_TUNNEL_AFTER_BODY (Upgrade whose body never completed) uncorks and closes; src/js/node/_http_server.ts fails a tunnel _write callback with ERR_STREAM_WRITE_AFTER_END when handle.closed (instead of stashing it for a drain that never comes) and flushes any stashed #pendingCallback with ERR_STREAM_DESTROYED('write') in #onClose; a new spawned-child test asserts the exact event order matches Node v26.3.0 and that the process exits.
Security risks
None identified. This is server-side connection teardown for a peer-initiated FIN — no new parsing of untrusted bytes, no auth/crypto surface. Closing sooner rather than staying half-open is strictly less exposure.
Level of scrutiny
High. The uWS HttpContext::onEnd change alters when a socket is closed vs kept half-open, and correctness depends on onClose<IsNodeHttp> running the socketData last=true + inStream last=true cleanup for HTTP_NODE_TUNNEL_AFTER_BODY — I verified it does (HttpContext.h:240-248), but this is exactly the ref-count/lifetime category REVIEW.md flags as most-blocked. The JS-side changes are small defensive fixes to stop callback leaks and follow existing patterns in the file.
Other factors
I reviewed this PR twice previously; both nits were addressed (combined-object stderr assertion; ERR_STREAM_DESTROYED instead of ERR_STREAM_WRITE_AFTER_END for the backpressure-then-close flush). This run found no bugs. Verifier agents ruled out concerns about test-file placement and the per-test timeout override. The mechgate evidence block's "release without fix: all passed" is noise — its baseline commit (b9825e4) already contains the HttpContext.h fix, so it wasn't actually testing without the fix. Given the change is a behavioral shift in native connection lifecycle for node:http compat rather than a mechanical fix, a maintainer should confirm the Node-semantics claim (mid-body FIN on an UpgradeStream closes rather than half-opens) and that no other consumer of the old combined isConnectRequest || TUNNEL_AFTER_BODY half-open path relied on it.
There was a problem hiding this comment.
No new issues found; all three earlier nits (stderr assertion, ERR_STREAM_DESTROYED for the backpressured-then-closed callback, past-tense comment) are addressed. Deferring to a human because the HttpContext::onEnd change alters native socket lifecycle for the Upgrade-with-body path — small and well-reasoned, but not mechanical.
What was reviewed:
onEndsplit:isConnectRequeststill stays half-open;HTTP_NODE_TUNNEL_AFTER_BODYnow closes — confirmedonClose<IsNodeHttp>already runs the tunnel-after-body cleanup (socketData+inStreamlast=true), so refs release._writehandle.closedguard and#onClosepending-callback flush —handle.closedis an existing property (used at :1475, :1800);$ERR_STREAM_DESTROYEDis defined in ErrorCode.ts.- Test asserts the exact Node event sequence via a spawned subprocess with combined-object assertion;
timeout: 10_000bounds the pre-fix hang.
Extended reasoning...
Overview
Three files: packages/bun-uws/src/HttpContext.h splits the onEnd handling of isConnectRequest vs HTTP_NODE_TUNNEL_AFTER_BODY (previously OR'd together) so a mid-body FIN on an accepted Upgrade closes the socket instead of staying half-open; src/js/node/_http_server.ts adds a handle.closed check in _write (fail the callback with ERR_STREAM_WRITE_AFTER_END instead of stashing it for a drain that never comes) and flushes any stashed #pendingCallback in #onClose with ERR_STREAM_DESTROYED("write"); a new subprocess test asserts the exact Node v26 event sequence.
Security risks
None identified. This narrows a half-open path to close sooner on peer FIN; no new user-controlled input is parsed, no auth/crypto surface touched.
Level of scrutiny
Medium-high. The C++ change is a two-branch split inside a templated uWS socket handler (onEnd<IsNodeHttp>) and changes lifecycle behavior for a real (if narrow) node:http path. I verified onClose<IsNodeHttp> already handles HTTP_NODE_TUNNEL_AFTER_BODY cleanup, so asyncSocket->close() correctly releases the body-read ref and pending-request count via that existing path. The uncorkWithoutSending() before close() matches the fall-through pattern just below it. The JS changes are small and use existing primitives (handle.closed, $ERR_STREAM_DESTROYED). Still, native socket-lifecycle changes in the HTTP server are exactly the kind of change a maintainer should sign off on — a subtle regression here (e.g. an Upgrade-with-body client that legitimately expects half-open before body completion) would be hard to spot from the diff alone.
Other factors
All three of my earlier inline comments were addressed (combined-object stderr assertion in c5b6e98, ERR_STREAM_DESTROYED for the backpressured-close case in adb4ed3, present-conditional comment wording in ffd3d4b). The bug hunting system found nothing this run. The test was moved to its own file (node-http-upgrade-body-fin.test.ts) in 35a1f16 rather than staying in node-http.test.ts; that's a deliberate author choice I'm not blocking on. The auto-generated evidence block's "release without fix: all passed" line appears to have run against a branch commit (3fe03ad) that already contained the fix, so it doesn't contradict the PR's claim that the test hangs pre-fix.
The client net.Socket is readOnly (writable ended, readable waiting for the server FIN) when the server-side assertions have already completed; under ASAN load the event loop can stay alive past the spawn timeout waiting for that FIN. The server-side refs are proven released by server.close() having fired.
There was a problem hiding this comment.
No further issues from me — the earlier nits (stderr assertion, ERR_STREAM_DESTROYED vs ERR_STREAM_WRITE_AFTER_END, comment tense) are all addressed. Deferring the approval itself since this changes native uWS socket-lifecycle behavior in HttpContext::onEnd and the node:http tunnel state machine, which is worth a maintainer's eyes.
What was reviewed:
onEndsplit: confirmedonClose<IsNodeHttp>already handlesHTTP_NODE_TUNNEL_AFTER_BODYcleanup (socketData last=true + inStream last=true), soasyncSocket->close()releases the body-read ref and pending-request count as the comment claims.uncorkWithoutSending()beforeclose()matches the existing pattern at the tail ofonEndand inonClose._write's newhandle.closedguard and#onClose's#pendingCallbackflush — verified the only assignment to#pendingCallbackis on the!flushed && handle.ondrain && !handle.closedpath, soERR_STREAM_DESTROYEDthere andERR_STREAM_WRITE_AFTER_ENDat write-time are the right split.- Test: subprocess pipes drained concurrently, combined-object assertion,
port: 0, spawn timeout guards the pre-fix hang.
Extended reasoning...
Overview
Three files: packages/bun-uws/src/HttpContext.h splits the onEnd FIN handler so an accepted-Upgrade-with-incomplete-body closes instead of staying half-open (previously it shared the CONNECT-tunnel branch); src/js/node/_http_server.ts adds a handle.closed guard in _write so a write during the tunnel's 'end' listener fails with ERR_STREAM_WRITE_AFTER_END instead of stashing a callback that will never drain, and flushes any already-stashed #pendingCallback with ERR_STREAM_DESTROYED('write') in #onClose; a new test file spawns a fixture that half-closes mid-body and asserts the exact event/error-code sequence matches Node v26.3.0.
Security risks
None identified. This is server-side connection-lifecycle handling on an already-accepted socket; the change closes a connection that previously stayed half-open, which is strictly less resource retention. No parsing of untrusted input was added.
Level of scrutiny
High. HttpContext::onEnd is the native uWS FIN handler for every node:http server socket, and the distinction between isConnectRequest (established tunnel, half-open allowed) and HTTP_NODE_TUNNEL_AFTER_BODY (upgrade accepted but body incomplete, must close) is a subtle protocol-state decision that the pre-PR code deliberately merged in the other direction. The reasoning in the PR body (Node's UpgradeStream still has socketOnEnd attached until the body completes) is sound and I verified onClose already runs the tunnel-after-body cleanup, but a maintainer familiar with the node:http/uWS compat layer should confirm the state-machine change and that no other path (SSL variant, backpressured writes on the tunnel-after-body socket) is affected.
Other factors
I've been through three prior rounds on this PR (stderr-empty assertion, ERR_STREAM_DESTROYED vs WRITE_AFTER_END for the backpressured-then-closed callback, comment tense) and all were addressed in subsequent commits. The bug hunting system found nothing this run. The test is a hang-guard fixture (10s spawn timeout) that asserts the child exits on its own with the exact event list, and the PR evidence block shows it passes on ASAN+debug and release with the fix. The gate evidence shows the release build passed without the fix too (the ASAN-without-fix build failed to compile, so the "fails on main" leg didn't actually run the test on a working unfixed binary) — worth noting but the mechanism is well-explained enough that this isn't blocking on its own.
…letes server.close() resolving proves pending_requests reached zero (the ref the pre-fix half-open path stranded); the fixture then exits explicitly so ASAN CI lanes are not at the mercy of natural-exit timing. Without the fix the fixture blocks at await gotSocketClose and the spawn timeout kills it.
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-body-fin.test.ts`:
- Line 66: Update the stderr assertion in the fixture test to require exact
emptiness rather than merely rejecting lowercase “error” substrings. Replace the
broad string-content matcher with the exact empty-string invariant while
preserving the existing bunEnv-based test setup.
🪄 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: 7e288135-373f-4e08-b33a-cb00712945c1
📒 Files selected for processing (3)
packages/bun-uws/src/HttpContext.hsrc/js/node/_http_server.tstest/js/node/http/node-http-upgrade-body-fin.test.ts
There was a problem hiding this comment.
All four of my earlier inline nits are addressed and the bug-hunting pass found nothing new. I'm not approving on my own because the split in HttpContext::onEnd changes EOF handling for every node:http server connection in the HTTP_NODE_TUNNEL_AFTER_BODY state, and the uWS + node:http-compat state machine is subtle enough to warrant a look from someone who owns it.
What was reviewed:
onEndsplit: confirmedonClose<IsNodeHttp>already runs the tunnel-after-body cleanup (socketData+inStreamlast=true), so the refs the half-open path stranded are released on close._writehandle.closedguard and#onCloseflushing#pendingCallback— checked against the only assignment site (line 2019) and#onDrain; no double-invoke path.- Candidate concerns ruled out: separate test file vs.
node-http.test.ts(spawned hang-guard fixture; several sibling files in this dir follow the same pattern), and the 30s per-test timeout (bounds a 20s spawn timeout for the pre-fix hang case).
Extended reasoning...
Overview
Three files: packages/bun-uws/src/HttpContext.h splits the onEnd EOF handler so HTTP_NODE_TUNNEL_AFTER_BODY (Upgrade whose body never completed) now closes the socket instead of staying half-open like isConnectRequest; src/js/node/_http_server.ts adds a handle.closed fast-fail in _write and flushes #pendingCallback in #onClose; a new spawned-fixture test asserts the exact event sequence Node v26.3.0 produces and that server.close() completes.
Security risks
None identified. This narrows a half-open path to a close; no new input parsing, no auth/crypto surface.
Level of scrutiny
Moderate-to-high. The production diff is small (~30 lines) and the reasoning is well-grounded in Node's UpgradeStream behavior, but HttpContext::onEnd is on the hot path for every node:http server connection's EOF, and the interaction between HTTP_NODE_TUNNEL_AFTER_BODY, the body-completion flip to isConnectRequest in the data handler, and onClose's cleanup is a state machine that the uWS/node:http-compat owners (suggested reviewers: cirospaciari, Jarred-Sumner) are best placed to sign off on.
Other factors
I left four inline nits on earlier revisions (stderr-exact-empty assertion, wrong error code for the backpressured-then-closed callback, two past-tense bug-history comments) and all four were addressed in follow-up commits. The evidence block's "fails without fix" gate looks broken — the "release without fix" run reports b3f3f2f5d (a PR commit) passing and the ASAN run failed to build — so there is no automated proof the test fails on main, only the PR text's claim. That is a harness issue rather than a code issue, but it means the USE_SYSTEM_BUN=1-style check REVIEW.md asks for was not demonstrated here.
A CONNECT tunnel's backpressured write can already have reached the kernel by the time the socket closes; erroring its callback with ERR_STREAM_DESTROYED surfaced as an unexpected error log entry in test-http-set-global-proxy-from-env-https-request.mjs and test-https-proxy-request.mjs. The _write handle.closed guard covers the write-after-native-close case this PR actually needs.
There was a problem hiding this comment.
I didn't find any issues this pass, and all four earlier nits are resolved (the #onClose pending-write flush was dropped entirely in bb9d44c rather than re-coded, which is fine — that path is back to pre-PR behavior and out of scope for this fix). Deferring to a human reviewer since this changes the uWS onEnd FIN-handling state machine for node:http upgrade connections.
What was reviewed:
HttpContext::onEnd: confirmedonClose<IsNodeHttp>already runs theHTTP_NODE_TUNNEL_AFTER_BODYcleanup (socketData+inStreamwithlast=true), soasyncSocket->close()here releases the body-read ref and pending-request count as the comment claims.- The body-completed path is unchanged: the data handler flips
HTTP_NODE_TUNNEL_AFTER_BODY→isConnectRequeston fin, so a completed-body upgrade still stays half-open on FIN. _writehandle.closedguard: only reached inside the!flushed && handle.ondrainbranch, so the normal write path is untouched; the stashed-then-destroyed callback case remains at pre-PR behavior after bb9d44c.- Test fixture: pipes drained concurrently, spawn timeout bounds the pre-fix hang, combined-object assertion with asymmetric stderr; the separate-file placement and per-test timeout were examined and are reasonable for a hang-guard subprocess test.
Extended reasoning...
Overview
Three files: packages/bun-uws/src/HttpContext.h splits onEnd's handling of HTTP_NODE_TUNNEL_AFTER_BODY (accepted Upgrade whose request body never completed) out of the isConnectRequest half-open branch and closes the socket instead, so onClose runs the tunnel-after-body cleanup and releases the stranded body-read ref / pending-request count. src/js/node/_http_server.ts adds a handle.closed guard in NodeHTTPServerSocket#_write so a write inside the upgraded socket's 'end' listener fails with ERR_STREAM_WRITE_AFTER_END instead of stashing a callback for a drain that will never fire. test/js/node/http/node-http-upgrade-body-fin.test.ts is a new spawned-fixture hang test that asserts the exact event sequence Node v26.3.0 produces and that server.close() completes.
Security risks
None identified. The change narrows behavior (closes a connection that previously stayed half-open) on a server-side FIN path; no new user input parsing, no auth/crypto surface.
Level of scrutiny
Medium-high. The diff is small and well-argued, but it edits the native uWS HTTP context's FIN-handling state machine for node:http compat — connection-lifecycle / refcount territory where a wrong branch strands refs or double-closes. The onEnd handler is templated on IsNodeHttp and this branch is inside the IsNodeHttp block, so Bun.serve is unaffected, but the interaction between HTTP_NODE_TUNNEL_AFTER_BODY, isConnectRequest, and the onClose cleanup contract is subtle enough that a human familiar with the uWS/node:http glue should sign off.
Other factors
Four rounds of my own nits (stderr assertion, #onClose flush error code, two comment-phrasing items) have all been addressed; the #onClose flush was ultimately dropped rather than re-coded to ERR_STREAM_DESTROYED, which just leaves that unrelated backpressure-then-peer-close path at its pre-PR behavior. CodeRabbit's remaining suggestion (exact-empty stderr) was correctly declined per REVIEW.md. The candidate issues raised and refuted this run (per-test timeout, standalone test file) are both defensible for a hang-guard subprocess test. CI for the latest commit (bb9d44c) is still building per the robobun status comment.
|
CI on bb9d44c (build #77774): the new test |
This branch has not been deployed
2 participants
Problem
When an Upgrade request carries a body and the client half-closes before that body completes,
HttpContext::onEndtreatedHTTP_NODE_TUNNEL_AFTER_BODYthe same as a live CONNECT tunnel and kept the connection half-open. Nothing on that path ever released theNodeHTTPResponsebody-read ref or the server's pending-request count, so the event loop never drained andserver.close()never completed.Fix
Node's
UpgradeStream(used for an Upgrade request whose body has not finished) wraps a raw socket whosesocketOnEndlistener is still attached. A mid-body FIN there callssocket.end(), the raw socket closes, and theUpgradeStreamdestroys with it. Only once the body has actually finished does the connection become a tunnel that can stay half-open.HttpContext::onEndnow handles the two states separately:isConnectRequest(CONNECT, body-less Upgrade, or Upgrade-with-body after the body completed) still stays half-open, whileHTTP_NODE_TUNNEL_AFTER_BODY(body never completed) closes the socket.HttpContext::onClosealready runs the tunnel-after-body cleanup (socketDataandinStreamwithlast=true), which releases the body-read ref and the pending-request count.With the socket now closed by the time the upgraded socket emits
'end', asocket.write()inside that listener must fail like Node's does._writenow calls its callback withERR_STREAM_WRITE_AFTER_ENDwhen the native handle reports closed instead of stashing the callback for a drain that will never arrive.Verification
New test in
test/js/node/http/node-http-upgrade-body-fin.test.tsspawns a child that sends a partial upgrade body, half-closes, writes inside the upgraded socket's'end'listener, and waits for the socket to emit'close'andserver.close()to complete. The event list['upgrade','end','write-cb:ERR_STREAM_WRITE_AFTER_END','error:ERR_STREAM_WRITE_AFTER_END','close']matches Node v26.3.0. Without the fix the child hangs at theclosewait and the spawn timeout kills it.The body-completed case is unchanged: after the body finishes the connection switches to
isConnectRequestand stays half-open on FIN, so the server can still write back.[review] gate passed · iteration 4 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 1 rejected · iteration 4
evidence per changed file