node:http: ServerResponse write()/end() observe a handler-destroyed socket - #35207
node:http: ServerResponse write()/end() observe a handler-destroyed socket#35207robobun wants to merge 4 commits into
Conversation
WalkthroughChangesServerResponse completion handling now matches Node-like behavior for standalone responses and closed native handles. New tests cover lifecycle events, state transitions, output buffering, and callback timing. The proxy test uses ServerResponse completion
Proxy test host normalization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:51 AM PT - Jul 23rd, 2026
✅ @robobun, your commit bb6251a9f7eef23688791547892183b738f8ce5c passed in 🧪 To try this PR locally: bunx bun-pr 35207That installs a local version of the PR into your bun-35207 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked the two suggested issues against
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes observable behavior in ServerResponse.prototype.write() / end() — the most heavily exercised path in the node:http compat layer — so it's worth a human look.
What was reviewed:
- Verified
handle.aborted≡Flags::SOCKET_CLOSED(NodeHTTPResponse.rs:764), so it is a strict subset ofclosed_or_completedand the removed early return is indeed redundant. - Checked that
hasServerResponseFinished(viathis.finished) prevents a secondend()from re-emitting'prefinish'. - Confirmed the new closed-path block mirrors the existing fall-through block (same
_header/req._dump()/markResponseEndedNTsequence, minus'finish'/_callPendingCallbacks). - Test awaits real events, no sleeps, placed alongside the related abort-events test.
Extended reasoning...
Overview
Two files: src/js/node/_http_server.ts (~18 lines net) and a new test case in test/js/node/http/node-http-server-abort-events.test.ts. The change makes ServerResponse.prototype.write() return false (instead of true) and ServerResponse.prototype.end() transition to finished + emit 'prefinish' (instead of returning true with no state change) when the native handle's closed_or_completed flag is set. It also removes the handle?.aborted early return at the top of end().
Security risks
None. This is a Node.js-compat state-machine fix with no user-controlled parsing, auth, or crypto involvement.
Level of scrutiny
High. ServerResponse.prototype.write and end are the hot path for every node:http server response. The change is small but alters three observable behaviors: write() return value on a dead socket, end() return value (true → this — the old value was outright wrong), and event emissions on the closed path. The closed_or_completed mask covers both socket_closed and request_has_completed; only the former is exercised by the new test, and removing the handle?.aborted early return means callWriteHeadIfObservable and the trailer block now run before the closed check where they previously didn't on an aborted handle.
Other factors
The reasoning against Node.js's OutgoingMessage._writeRaw / end() is sound and cited in-source. I confirmed on the native side that get_aborted returns SOCKET_CLOSED, so the removed guard is provably subsumed by the later closed_or_completed check. The new closed-path block is a copy of the existing fall-through block minus the 'finish' / callback / _callPendingCallbacks scheduling, which matches the described Node behavior (the finish callback is dropped by _writeRaw's conn.destroyed branch). The PR reports the surrounding node-http suites and test-http-outgoing-* / test-http-writable-true-after-close parallel scripts pass unchanged. Given the surface area, a maintainer familiar with the node:http state machine should sign off.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/node/http/node-http-server-abort-events.test.ts`:
- Around line 71-75: Update the response write/end assertions around res.write
and res.end to pass callbacks, verify both callbacks remain uncalled after
res.close, and replace the weak endResult type assertion with an identity
assertion that endResult is res. Apply the same checks to the corresponding
second test location.
In `@test/js/node/http/node-http.test.ts`:
- Around line 2882-2884: Replace the explanatory regression comment near the
writeHead()/end() regression test with only the issue URL for `#25632`, removing
the surrounding behavioral explanation.
- Around line 2888-2896: Update the response assertion around res.outputData to
verify its buffered content includes the rendered 403 header block and response
body, rather than only checking that outputData.length is nonzero. Preserve the
existing finished and writableEnded assertions while replacing outputDataLength
with content-specific checks.
- Around line 2903-2908: Make the test containing the res2.end callback
assertion asynchronous, await one process.nextTick after calling res2.end, then
assert cbCalled directly rather than scheduling an unawaited next-tick
expectation. Preserve the existing finished and writableEnded assertions.
🪄 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: ab978210-1d03-4278-b83d-df9c6f1de865
📒 Files selected for processing (4)
src/js/node/_http_server.tstest/js/node/http/node-http-proxy.jstest/js/node/http/node-http-server-abort-events.test.tstest/js/node/http/node-http.test.ts
There was a problem hiding this comment.
No bugs found, but this changes observable ServerResponse lifecycle behavior in three places (new 'prefinish' emission on a closed handle, end() callback no longer nextTick'd for standalone responses, write() return flipped to false), so it's worth a human look.
What was reviewed:
- Checked that
writableEndedderives fromfinished(_http_outgoing.ts:850), so the newfinished = truecovers both. - Checked that
hasServerResponseFinishedguards double-end()after the new closed-handle branch setsfinished. - Checked the removed
handle?.abortedearly-return is subsumed by theclosed_or_completedflag check (socket_closed | request_has_completed). - The
req._dump()block in the new branch mirrors the existing success path at_http_server.ts:3224-3227verbatim.
Extended reasoning...
Overview
The PR touches src/js/node/_http_server.ts (ServerResponse.prototype.end and .write), plus two test files and a hostname pin in a proxy test fixture. Three behavior changes: (1) write() on a closed/completed native handle now returns false instead of true; (2) end() on a closed/completed handle now sets _header, dumps the request, sets finished = true, schedules _ended, emits 'prefinish', and returns this (previously returned true — wrong type — with no state transition); (3) the !handle standalone-response branch now unconditionally delegates to OutgoingMessagePrototype.end instead of a conditional shortcut that skipped the state transition when _header was already set and invoked the callback via nextTick. The redundant handle?.aborted early return is removed.
Security risks
None. This is Node.js-compat state-machine bookkeeping on the response object; no auth, crypto, path handling, or untrusted-input parsing is touched.
Level of scrutiny
Medium-high. ServerResponse.prototype.end() is on the hot path of every node:http server, and each of the three changes alters observable behavior: a new synchronous 'prefinish' emission where none fired before, a return-value flip that backpressure-aware callers can observe, and a callback that standalone-response users (light-my-request / fastify inject) previously received via nextTick and now won't. All three are argued against Node v26.3.0 semantics with repros in the PR body, and the listed test suites (node-http*, test-http-outgoing-*, test-http-pipeline-*, test-http-server-response-standalone, test-http-writable-true-after-close) reportedly pass unchanged — but the blast radius of end() control-flow edits warrants a maintainer's eye on whether the removed handle?.aborted guard had any callers that reach it without closed_or_completed set, and whether emitting 'prefinish' synchronously from inside the closed-handle branch can re-enter anything.
Other factors
My earlier inline comment (nextTick assertion running outside the test's scope) and all four CodeRabbit findings were addressed in 892d0a1 and the threads are resolved. The new tests are well-shaped: the abort-events test awaits res 'close' before asserting, wires every failure to captured state, and asserts the full event sequence; the standalone test now awaits a nextTick before asserting cbCalled === false. CI build #78294 is in progress.
…ocket When a request handler calls `req.socket.destroy()` and then writes to the response, ServerResponse.write() returned true and ServerResponse.end() silently left finished/writableEnded false. Node.js's OutgoingMessage._writeRaw() returns false once the assigned socket is destroyed, and OutgoingMessage.end() still sets finished=true and emits 'prefinish' (the 'finish' callback passed to _send is dropped on the destroyed-socket branch, so 'finish' never fires). Align the ServerResponse native-handle path with that: - write(): return false on a closed/completed handle instead of true. - end(): drop the redundant handle?.aborted early return; on a closed/completed handle set _header, dump the request, set finished=true, emit 'prefinish', and return `this` without calling into the native handle or scheduling 'finish'/the end callback.
…tableEnded With no native handle (light-my-request, `new ServerResponse(req)`), calling writeHead() then end() returned early without setting `finished` because the `!handle` branch only delegated to OutgoingMessage.end() when `_header` was unset. Route through OutgoingMessage.end() unconditionally so the end() state transition and outputData buffering match Node.js. Fixes #25632
The proxy fixture listened on and dialled `localhost`, so on a host whose resolver prefers IPv6 the server binds `::1` and the client dials `127.0.0.1` (or vice versa), refusing the connect. Pin both ends to `127.0.0.1` to match the `exampleSite` it talks to.
- abort-events test: pass callbacks to write()/end() and assert both remain uncalled after 'close'; assert end() returned `res` itself. - standalone #25632 test: make async and await one nextTick before asserting the end() callback was not invoked; assert the rendered 403 header block and body are buffered in outputData rather than just a nonzero length.
156378c to
bb6251a
Compare
There was a problem hiding this comment.
No bugs found, and my earlier nit (unawaited nextTick assertion) was addressed. Deferring to a human because this inverts observable Node-compat behavior in _http_server.ts: write() on a closed handle now returns false (previously true, with a comment claiming that matched Node), end() on a closed handle now emits 'prefinish' and runs req._dump(), and the handle?.aborted early-return is gone so more of the end() path (trailers, callWriteHeadIfObservable) executes before bailing.
Checked: the 'prefinish'-on-destroyed-socket emit was compared against Node's OutgoingMessage.end and matches; the req._dump() block is copied verbatim from the fall-through path; test-http-writable-true-after-close asserts res.writable, not the write() return, so the true→false flip doesn't affect it.
Extended reasoning...
Overview
The PR touches src/js/node/_http_server.ts (ServerResponse.prototype.write/end), plus two test files and a localhost→127.0.0.1 fix in node-http-proxy.js. Three behavior changes:
write()on aclosed_or_completednative handle returnsfalseinstead oftrue(callback dropped).end()on aclosed_or_completedhandle sets_header, dumpsreq, setsfinished, emits'prefinish', and returnsthis— instead of the previous silentreturn true(which also had the wrong return type).end()with no native handle unconditionally delegates toOutgoingMessage.prototype.end, dropping thewriteHead()-already-called shortcut and theprocess.nextTick(callback)shortcut.- The
if (handle?.aborted) return thisearly return at the top ofend()is removed as redundant.
Security risks
None identified — this is state-machine bookkeeping in the Node HTTP compat layer, no auth/crypto/parsing of untrusted structure.
Level of scrutiny
High. _http_server.ts is one of the most stateful, event-order-sensitive files in the runtime, and this change inverts observable behavior that was previously commented as Node-matching ("node.js will return true if the handle is closed"). The new closed-handle end() branch adds a synchronous emit('prefinish') and a req._dump() call that weren't there before; both look correct per the cited Node semantics and are copied from the existing fall-through, but the interaction with pipelining, keep-alive teardown, and framework 'prefinish' listeners is the kind of thing a human familiar with this file should sign off on. The handle?.aborted removal means callWriteHeadIfObservable and the trailer path now run on an aborted handle before the closed_or_completed bail — likely fine (the trailer call is ?.-guarded), but worth a second pair of eyes.
Other factors
Tests are solid: the abort-events test awaits res.close, asserts the exact event sequence ['res.prefinish', 'res.close'] with no 'finish', and covers write-return, end-return-identity, callback suppression, and finished/writableEnded. The standalone-response test now awaits nextTick before asserting cbCalled === false (my earlier comment). PR body cites Node v26.3.0 verification and lists the full set of parallel/http tests that still pass. All prior review comments (mine and CodeRabbit's) are resolved.
|
Diff is green; CI noise is unrelated.
Ready for review; the remaining CI red is fleet/flake, not this change. |
|
Re-checked against current main (165dc9f) now that #25632 is closed. Still needed: both tests added here fail on main unchanged.
What #25632 was closed on is the narrower case of a standalone response calling |
2 participants
What
Three
ServerResponse.prototype.end()paths returned without ever settingfinished/writableEnded, andwrite()on a dead socket reportedtrue:req.socket.destroy()and then writes to the response:res.write()returnstrue(reporting the bytes as buffered) andres.end()is a silent no-op that leavesfinished/writableEndedatfalse.new ServerResponse(req)(no native handle; light-my-request / fastify inject) that callswriteHead()beforeend(): the!handlefallback returnedthiswithout settingfinishedonce_headerwas rendered.Repro
Cause
ServerResponse.prototype.endhad anif (handle?.aborted) return thisearly return, and bothwrite()andend()had aflags & closed_or_completedcheck that returnedtruewithout updating any writable-side state. Separately, the!handlebranch skipped theOutgoingMessage.end()delegation when_headerwas already set (writeHead already called) and returned without settingfinished.Node.js's
OutgoingMessage._writeRaw()returnsfalseonce the assigned socket is destroyed, andOutgoingMessage.end()setsfinished = trueunconditionally; on a destroyed socket thefinishcallback handed to_sendis dropped by theconn.destroyedbranch, so'prefinish'fires but'finish'never does.Fix
In
src/js/node/_http_server.ts:write(): returnfalseon a closed/completed native handle instead oftrue; the write callback is dropped (never invoked), matching Node.js.end()with a closed/completed native handle: drop the redundanthandle?.abortedearly return. Set_header, dump the request, setfinished = true, emit'prefinish', and returnthiswithout calling into the native handle or scheduling'finish'/ the end callback.'close'is emitted by the existing socket-close path.end()with no native handle: always delegate toOutgoingMessage.prototype.end, sofinishedis set and the header/body are buffered intooutputDataregardless of whetherwriteHead()was called first. The previous fallback also invoked the end callback vianextTick, which Node.js does not do (it registers it on'finish').Verification
New test in
test/js/node/http/node-http-server-abort-events.test.tsdestroysreq.socketfrom inside the handler, then assertswrite() === false,finished === true,writableEnded === true, and the event sequence['res.prefinish', 'res.close'](no'finish').New test in
test/js/node/http/node-http.test.tscovers the standalonewriteHead()+end()path:finished/writableEndedaretrue,outputDatais populated, and the end callback is not invoked via nextTick.Both fail on main; both pass with this change and match Node v26.3.0.
node-http.test.ts,node-http-backpressure.test.ts,node-http-transfer-encoding.test.ts,node-http-server-socket-end-drain.test.ts, and thetest-http-outgoing-*/test-http-pipeline-*/test-http-server-response-standalone/test-http-writable-true-after-closeparallel scripts all pass unchanged.Fixes #25632
[review] gate passed · iteration 4 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 2 rejected · iteration 4
evidence per changed file