node:http: keep delivering the request body after a synchronous res.end()

[robobun]

Fixes #4733.
Fixes #18613.

Repro

import http from "http";
import fs from "fs";

const server = http.createServer((req, res) => {
  const out = fs.createWriteStream("/tmp/output");
  req.pipe(out);
  req.once("end", () => console.log("request ended"));
  res.end("ok");
});
server.listen(3000);
// curl --data "testing" 127.0.0.1:3000

Node writes testing to /tmp/output and logs request ended. Bun before this change emits end/close but never fires data, so /tmp/output is empty.

Cause

Two independent native discards dropped the body as soon as res.end() ran:

1. write_or_end::<true> (and maybe_stop_reading_body) released the body-read ref whenever !HAS_CUSTOM_ON_DATA, which the dispatcher clears for every request; the check fired regardless of whether the IncomingMessage was being consumed.
2. uws's markDone() (reached from end()/endStream()) nulls the connection's inStream, so even without step 1 no further body chunks would be delivered.

Node's resOnFinish only dumps the body when nothing is consuming it, and it runs on the 'finish' event (nextTick after end()), so a consumer attached after res.end() in the same tick is still honoured.

Fix

• emitResponseFinish (the 'finish' listener, Bun's resOnFinish) now runs the req._dump() gate, exactly where Node does. ServerResponse.prototype.end no longer touches _dump().
• write_or_end::<true> no longer discards the body on keep-alive; it only discards for Connection: close/HTTP 1.0, where uws closes the socket before any later segment could reach the parser. After raw_response.end(), inStream is re-armed while the body is still Pending so chunks keep flowing into the IncomingMessage. The re-arm skips SOCKET_CLOSED (end() can destruct the HttpResponseData synchronously) and preserves an existing buffered-pause shim.
• do_pause/do_resume no longer refuse while ENDED and the body is still Pending, so push()->false backpressure reaches the socket after a synchronous res.end().
• on_buffer_request_body_while_paused(last=true) now sets body_read_state = Done and defers mark_request_as_done until drain_buffered_request_body_from_pause has handed the buffered tail to JS, so the tail is not freed before the consumer sees it.
• on_data_or_aborted(last=true) re-evaluates IS_REQUEST_PENDING unconditionally; run_callback above can drain nextTicks and unref body_read_ref via _destroy -> set_on_data(undefined) before the tail re-reads it.
• set_on_data's clear branch and mark_request_as_done release body_read_ref and call mark_request_as_done_if_necessary() so a mid-upload socket close after the response ended cannot strand vm.active_tasks.
• maybe_stop_reading_body guards on_data_get_cached against an empty this_value.

Node semantics

req._dumped now matches Node v26 for every ordering: consumer before res.end(), pipe/on('data') after res.end() in the same tick, resume() after res.end(), no consumer (dumped on 'finish'), and pause-then-end (test-http-pause-no-dump). Verified by running each scenario side-by-side against Node.

Verification

New describe("request body still flows after res.end() was called in the handler") in test/js/node/http/node-http.test.ts covers all of the above plus a keep-alive reuse case (three POSTs on one socket, server.close() resolves), a mid-upload socket-destroy (process reaches beforeExit), Connection: close, and a chunked body split across TCP segments. 10 of the 12 fail on main and all pass with this change; Node v26 passes them identically.

Also ran node-http.test.ts, node-http-backpressure.test.ts, node-http-server-*.test.ts, express.json.test.ts, and the upstream test-http-{pause,pause-no-dump,no-read-no-dump,pause-resume-one-end,pipe-fs,req-res-close,request-end,request-large-payload,incoming-pipelined-socket-destroy,keep-alive,1.0-keep-alive,keep-alive-pipeline-max-requests,client-keep-alive-release-before-finish,server-keepalive-end,pipeline-requests-connection-leak,outgoing-end-cork}.js. No new failures.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f6f257b3-4869-4e1b-93c3-c42e278db87f

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba2d02 and f1ae3c9.

📒 Files selected for processing (4)

• src/js/node/_http_server.ts
• src/runtime/server/NodeHTTPResponse.rs
• test/js/node/http/node-http-proxy.js
• test/js/node/http/node-http.test.ts

Walkthrough

Changes

HTTP request body completion

Layer / File(s)	Summary
Response finish body draining src/js/node/_http_server.ts	Moves request-body dumping from ServerResponse.end into response finish handling before socket and keep-alive processing.
Pending body lifecycle handling src/runtime/server/NodeHTTPResponse.rs	Updates pause/resume gating, callback re-arming, teardown reference release, and pending-body completion handling.
Request body delivery coverage test/js/node/http/node-http.test.ts	Tests body delivery and event ordering after synchronous response completion, including chunked uploads, socket closure, and Connection: close cases.

Possibly related PRs

• oven-sh/bun#34356: Adjusts socket resumption when ending responses with unread request bodies.
• oven-sh/bun#34740: Overlaps in native request-body pause, resume, and pending-state handling.
• oven-sh/bun#35207: Also changes request-body dumping and response completion ordering.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change set addresses #4733 by keeping IncomingMessage data flowing and emitting end/close events after synchronous res.end().
Out of Scope Changes check	✅ Passed	The code and tests stay focused on request-body delivery, teardown, and HTTP semantics, with no obvious unrelated changes.
Title check	✅ Passed	The title clearly and concisely matches the main change: preserving request body delivery after a synchronous res.end().
Description check	✅ Passed	The description includes the problem, fix, and verification details, satisfying the template's intent despite different headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Found 1 issue this PR may fix:

1. http body won't be received if res.end is called too early #18613 - Describes the same bug: res.end() called early in a node:http handler causes the request body's data events to never fire

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #18613

🤖 Generated with Claude Code

[claude]

Beyond the inline nits, two lifetime/ref concerns in NodeHTTPResponse.rs were examined and ruled out: (1) the abort/timeout handlers do not need re-arming alongside the post-end() on_data re-arm — they're registered on the socket/connection, not on inStream, so markDone() nulling inStream doesn't clear them; (2) maybe_stop_reading_body on the UPGRADED/SOCKET_CLOSED branches still releases correctly — with ondata set the body stays Pending and body_read_ref is later dropped via on_data_or_aborted(last=true) → mark_request_as_done, so no ref leaks.

Extended reasoning...

This PR changes request/response lifecycle in the native HTTP server — re-registering a uws on_data callback on raw_response after end()/end_stream() has run, gated on body_read_state == Pending with IS_REQUEST_PENDING as the keep-alive. That's exactly the kind of lifetime-sensitive change that should get human eyes even though the reasoning in the PR description is sound and test coverage is good. The two inline findings are nits (test hygiene; _dump() now preceding a throw-capable native call). Deferring rather than approving; the note above records the additional concerns the finder/verifier pass already checked so they aren't re-explored.

[Jarred-Sumner]

@robobun this needs to match Node.js semantics identically.

[robobun]

Reworked so the _dump() gate runs in emitResponseFinish (the 'finish' listener), exactly where Node's resOnFinish runs it. req._dumped now matches Node v26 for every ordering: consumer before/after res.end() in the same tick, pipe() after res.end(), resume() after res.end(), no consumer, and pause-then-end. Also fixed the review findings around ref release on the re-armed inStream path and buffered-pause tail delivery. Pushed in d444b8f + ebf5a3b.

[robobun]

^{Updated 8:43 PM PT - Jul 31st, 2026}

✅ @robobun, your commit 8e926f027d31f9b0768c708fb7f38b3c5023122b passed in Build #86503! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 35489

That installs a local version of the PR into your bun-35489 executable, so you can run:

bun-35489 --bun

[robobun]

Rebased onto main at df49a6e and re-verified on 8e926f0.

The _dump() gate runs in emitResponseFinish (the 'finish' listener), matching Node's resOnFinish: req._dumped is identical to Node v26 for every consumer/res.end() ordering, and the buffered-pause paths the re-arm opens are covered on every terminal transition.

node-http.test.ts is 155/0 with the fix; 10 of the 12 new cases fail on main. node-http-backpressure.test.ts, node-http-server-*.test.ts, express.json.test.ts, and the upstream test-http-{pause,pause-no-dump,no-read-no-dump,pipe-fs,req-res-close,pause-resume-one-end}.js all pass.

Ready for review.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/runtime/server/NodeHTTPResponse.rs (2)

680-731: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Ref-release on teardown looks correct.

Releasing body_read_ref in mark_request_as_done() fixes the leaked-ref/vm.active_tasks imbalance for the "socket closed mid-upload" case (SOCKET_CLOSED can make should_request_be_pending() return false while body_read_state is still Pending, e.g. via the set_closed()-only path). One nit: body_read_state itself is left at Pending here rather than transitioned to Done/None, so get_has_body() could report a stale "pending" bit indefinitely after teardown. Given the object is already torn down at this point, this is likely inconsequential, but worth a quick check that nothing downstream re-reads body_read_state after this point in a way that matters.

🤖 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 `@src/runtime/server/NodeHTTPResponse.rs` around lines 680 - 731, Review
teardown handling in mark_request_as_done and downstream reads of
body_read_state, especially get_has_body. Ensure the body-read state is
transitioned from Pending to its appropriate terminal state during teardown so
stale pending status cannot be observed, while preserving the existing
body_read_ref.unref behavior and avoiding duplicate release.

---

2032-2073: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Update the ordering comment and relax the end-time callback gate

• req._dump() now runs on the 'finish' nextTick, so the comment saying ServerResponse.prototype.end clears ondata before reaching this path is stale.
• set_on_data still rejects Flags::ENDED even when body_read_state is Pending; that can drop a deferred req.on("data")/req.pipe() attached after synchronous res.end() and stop post-end body delivery.

🤖 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 `@src/runtime/server/NodeHTTPResponse.rs` around lines 2032 - 2073, Update the
ordering comment in the IS_END body-read handling to reflect that req._dump()
runs on the 'finish' nextTick, rather than claiming ServerResponse.prototype.end
clears ondata first. Adjust set_on_data so Flags::ENDED does not reject
registration while body_read_state is Pending, allowing deferred req.on("data")
and req.pipe() callbacks to receive post-end body data. Preserve the ended
rejection for non-pending body states.

🤖 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.

Outside diff comments:
In `@src/runtime/server/NodeHTTPResponse.rs`:
- Around line 680-731: Review teardown handling in mark_request_as_done and
downstream reads of body_read_state, especially get_has_body. Ensure the
body-read state is transitioned from Pending to its appropriate terminal state
during teardown so stale pending status cannot be observed, while preserving the
existing body_read_ref.unref behavior and avoiding duplicate release.
- Around line 2032-2073: Update the ordering comment in the IS_END body-read
handling to reflect that req._dump() runs on the 'finish' nextTick, rather than
claiming ServerResponse.prototype.end clears ondata first. Adjust set_on_data so
Flags::ENDED does not reject registration while body_read_state is Pending,
allowing deferred req.on("data") and req.pipe() callbacks to receive post-end
body data. Preserve the ended rejection for non-pending body states.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 30cce976-e54d-4082-9564-a46a4d1237ac

📥 Commits

Reviewing files that changed from the base of the PR and between 27b2df9 and 8ba2d02.

📒 Files selected for processing (3)

• src/js/node/_http_server.ts
• src/runtime/server/NodeHTTPResponse.rs
• test/js/node/http/node-http.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/runtime/server/NodeHTTPResponse.rs:1632-1645 — d444b8f's buffer-non-empty guard only covers the self-call: line 1632 still sets body_read_state = Done immediately, so in the reverse ordering — fin lands on the paused shim before a deferred res.end() — write_or_end::<true> → on_request_complete() (:2117 → :1476) sees ENDED && !Pending, runs mark_request_as_done(), and clear_and_free()s the undelivered pause-buffer tail (:712-713). do_resume then early-returns at :1445-1448 and 'end' still fires via IS_DATA_BUFFERED_DURING_PAUSE_LAST — silent body truncation. Have should_request_be_pending() also return true while buffered_request_body_data_during_pause is non-empty (or defer :1632 until the drain hands the tail to JS) so all mark_request_as_done_if_necessary() callers are covered atomically.

  Extended reasoning...

  What the bug is

  Commit 0d9c2f2 added self.body_read_state.set(BodyReadState::Done) at :1632 in on_buffer_request_body_while_paused(last=true). Commit d444b8f then guarded the local mark_request_as_done_if_necessary() call at :1639-1645 (skip while the pause buffer is non-empty) and added a drain-time call at :1419-1425. But :1632 flips global state that every other caller of mark_request_as_done_if_necessary() observes via should_request_be_pending() (:651-652: if ENDED { return body_read_state == Pending }). One such caller — on_request_complete() at :1476, reached from write_or_end::<true> at :2117 — has no buffer-emptiness guard. So when the body's fin arrives on the paused shim before a deferred res.end() runs, res.end() frees the buffered tail before the consumer can drain it.

  Step-by-step trace

  1. Async/nextTick handler: req.pipe(slowSink); await X; res.end('ok'). A body chunk arrives → onDataIncomingMessage → push() returns false → do_pause sets IS_DATA_BUFFERED_DURING_PAUSE and arms on_buffer_paused_shim.
  2. The body's fin (plus any tail bytes) arrives on the paused shim — always reachable on Windows (pause_socket() is cfg-skipped at :1388-1391) and on POSIX when coalesced in the same recv() batch. on_buffer_request_body_while_paused(chunk, last=true) appends chunk to buffered_request_body_data_during_pause (:1624-1625), sets IS_DATA_BUFFERED_DURING_PAUSE_LAST (:1628), sets body_read_state = Done (:1632), unrefs body_read_ref (:1633-1635), and — buffer non-empty — skips the self-call at :1639-1645. ENDED is not yet set, REQUEST_HAS_COMPLETED is not yet set.
  3. The deferred res.end('ok') fires → write_or_end::<true>. :2083 sets Flags::ENDED. :2101 checks body_read_state == Pending (it's Done) so the re-arm block is skipped. :2117 calls on_request_complete().
  4. on_request_complete() (:1468-1477): REQUEST_HAS_COMPLETED not set → :1473 sets it, :1476 calls mark_request_as_done_if_necessary(). At :727, IS_REQUEST_PENDING is set and should_request_be_pending() at :651-652 evaluates ENDED → (body_read_state == Pending) = false → mark_request_as_done() runs.
  5. mark_request_as_done(): :703 clear_on_data_callback nulls the JS ondata slot; :712-713 buffered_request_body_data_during_pause.clear_and_free() frees the tail chunk appended in step 2; :723 derefs.
  6. The slow consumer catches up → _read() → do_resume. :1445-1448 (REQUEST_HAS_COMPLETED || ENDED) && body_read_state != Pending → return FALSE before :1462's drain (and the buffer is empty anyway). get_has_body still ORs in the done bit from IS_DATA_BUFFERED_DURING_PAUSE_LAST, so JS runs emitEOFIncomingMessage → req emits 'end' with the tail silently missing.

  Why nothing existing prevents it

  should_request_be_pending() (:633-655) has no clause for buffered_request_body_data_during_pause or IS_DATA_BUFFERED_DURING_PAUSE_LAST; d444b8f's guard is local to :1639-1645 and cannot influence :1476. The re-arm block at :2101-2116 is gated on Pending, so it neither re-arms nor drains here. Pre-PR this was safe because on_buffer_request_body_while_paused did not touch body_read_state (the diff shows :1632 is a pure addition), so at step 4 should_request_be_pending() saw Pending and returned true, leaving the buffer intact for do_resume to drain.

  Relationship to the open comment at :1644

  That comment describes the fin-after-res.end() ordering (ENDED already set when the fin lands, self-call at the old :1625 frees the buffer). d444b8f's :1639-1645 guard addresses that self-call site. This report is the fin-before-deferred-res.end() ordering, reaching mark_request_as_done() via a different call site (on_request_complete() at :2117) that has no such guard. Same root cause (:1632 flips body_read_state before the drain), different unguarded caller — d444b8f patched only one of them.

  Impact

  Silent request-body truncation with a clean 'end' event on the exact backpressure path this PR opens (async handler + slow consumer where the upload finishes before the deferred response). A req.pipe(fs.createWriteStream(...)) upload handler that awaits before responding writes a truncated file with no error surfaced. This is a regression introduced by :1632 on a path this PR made reachable.

  Fix

  Either:

  • have should_request_be_pending() also return true when buffered_request_body_data_during_pause is non-empty (or IS_DATA_BUFFERED_DURING_PAUSE_LAST is set with an undrained buffer) — this covers every mark_request_as_done_if_necessary() caller atomically and lets the local :1639-1645 guard be dropped; or
  • defer self.body_read_state.set(BodyReadState::Done) from :1632 to drain_buffered_request_body_from_pause (set it just before :1424's mark_request_as_done_if_necessary(), once the tail has been handed to JS).

  The first is preferable per REVIEW.md's "fix bugs at the layer that owns the violated invariant": the invariant is "request is not done while undelivered body bytes exist", and should_request_be_pending() is the single predicate every caller consults.

• 🔴 src/runtime/server/NodeHTTPResponse.rs:1439-1449 — The relaxed do_resume gate now drains the pause buffer and returns it to NodeHTTPServerSocket.#resumeSocket() even after ENDED — but a synchronous res.end() has already run detachSocket(socket) (dispatcher :1084), so this._httpMessage is null and #resumeSocket()'s if (req) req.push(resumed) at :1872 is skipped; the drained bytes go only to this.push() on the socket duplex (no HTTP consumer, discarded by super.resume()). Back in IncomingMessage._read(), hasBufferedDataDuringPause is now clear (do_resume already .replace(Vec::new())'d the buffer at :1412 and cleared the flag at :1459), so the :400-404 drainRequestBody() recovery is skipped — the mid-body chunks are silently lost while the later fin still fires 'end'. Simplest fix: when ENDED|REQUEST_HAS_COMPLETED is set, have do_resume re-arm the shim but skip :1459/:1462 and return TRUE, so _read()'s own handle.drainRequestBody() (which pushes to the IncomingMessage directly) does the drain.

  Extended reasoning...

  What the bug is

  When backpressure from req.pipe(slowSink) buffers non-fin body chunks in the native pause buffer after a synchronous res.end(), the consumer's next _read() drains that buffer via do_resume — but the drained bytes are routed through socket._httpMessage?.req, which the dispatcher already nulled via detachSocket(). The bytes go only to the socket duplex's readable side (which the HTTP path never reads), so they are discarded. The IncomingMessage later receives the tail via the re-armed on_data_shim and emits 'end', so the consumer sees a complete stream with a hole in the middle — silent request-body corruption.

  This is distinct from the open thread on on_buffer_request_body_while_paused at :1644 (that one is the fin case where mark_request_as_done() freed the buffer, now addressed by the .is_empty() gate in d444b8f). This is the non-fin case: body_read_state is still Pending, so do_resume proceeds past the new gate at :1445-1448 and drains — to the wrong JS target.

  Step-by-step proof

  1. Handler: req.pipe(slowSink); res.end('ok') on a keep-alive request with a large body.
  2. write_or_end::<true> sets ENDED, keeps the body Pending (ondata set, not Connection: close), re-arms on_data_shim at :2110, and on_request_complete() sets REQUEST_HAS_COMPLETED. handle.finished → true.
  3. The dispatcher returns from the 'request' emit, sees handle.finished at _http_server.ts:1077, and runs http_res.detachSocket(socket) at :1084 → socket._httpMessage = null (:3457) and socket[kRequest] = null (:3462). req.socket is not touched, so IncomingMessage._read() can still reach the socket.
  4. A body chunk arrives on on_data_shim → onDataIncomingMessage → req.push(chunk) returns false (slowSink is full) → socket.pause() (internal/http.ts:261-266, gated on !socket.writableEnded which is false on keep-alive) → do_pause. The relaxed gate at :1362-1365 lets ENDED + Pending through; :1373-1374 arms on_buffer_paused_shim.
  5. Further non-fin chunks land in buffered_request_body_data_during_pause — always on Windows (pause_socket() is cfg-skipped at :1378-1381), and on POSIX for chunks in the same recv() batch that uws keeps parsing after the shim swap. The fin has not yet arrived, so body_read_state stays Pending.
  6. slowSink drains → IncomingMessage._read() fires → _http_incoming.ts:376 calls socket.resume() → NodeHTTPServerSocket.resume() (:2037-2039) → #resumeSocket() (:1843-1878).
  7. #resumeSocket(): handle.response is still the live NodeHTTPResponse (currentResponseObject on JSNodeHTTPServerSocket is only cleared by onClose/detach() or reassigned by a new request, not by on_request_complete). response.resume() → do_resume. body_read_state == Pending, so the new gate at :1445-1448 does not early-return. :1457 re-arms on_data_shim. :1459 clears IS_DATA_BUFFERED_DURING_PAUSE. :1462 → drain_buffered_request_body_from_pause() → :1412 .replace(Vec::new()) → returns the buffer.
  8. Back in #resumeSocket() (:1866-1875): const message = this._httpMessage → null (step 3). const req = message?.req → undefined. :1872 if (req) req.push(resumed) → skipped. Only :1875 this.push(resumed) runs, pushing to the socket duplex.
  9. resume() :2039 calls super.resume(), putting the socket duplex into flowing mode with no 'data' listener — the pushed bytes are discarded.
  10. Back in _read() at :390, handle.hasBody is re-read: get_has_body (:780) tests buffered_request_body_data_during_pause.len() > 0 for the hasBufferedDataDuringPause bit — now 0 (drained in step 7). The recovery at :400-404 (handle.drainRequestBody() → this.push(drained)) is skipped.
  11. Later chunks + fin arrive on the re-armed on_data_shim → onDataIncomingMessage → req.push(...) → emitEOFIncomingMessage. The consumer receives the tail and 'end' — with the step-5 bytes missing from the middle.

  Why nothing existing prevents it

  • #resumeSocket()'s only route to the IncomingMessage is this._httpMessage?.req; there is no fallback to this[kRequest] (also nulled) or any other back-pointer.
  • _read()'s recovery at :400-404 keys on hasBufferedDataDuringPause, which do_resume clears at :1459 in the same call that hands the buffer to the wrong target.
  • The PR's tests use 12-byte bodies against a 16 KB HWM, so push() never returns false and this path is uncovered.

  Why this is newly reachable

  Pre-PR, do_pause returned FALSE unconditionally on ENDED (old :1350-1355), so the pause buffer was never populated after res.end(); and do_resume returned FALSE unconditionally on ENDED (old :1419-1425), so #resumeSocket() never received a drained buffer post-end (and _read()'s :400-404 fallback, which pushes to this directly, was the only drain path). This PR opens both — correctly, for the feature — but the drain-return path was designed for the pre-detach case where _httpMessage is still set.

  Impact

  Silent request-body truncation whenever, after a synchronous res.end() on keep-alive, a body chunk triggers push()→false and further non-fin chunks buffer before the consumer's next _read(). req.pipe(fs.createWriteStream(...)) — the PR's own headline repro — with a body larger than the Readable HWM hits this on Windows deterministically and on POSIX under coalesced recvs. The consumer sees 'end' and treats the upload as complete; there is no error. Per REVIEW.md this is data loss on the exact path the PR opens.

  How to fix

  Two options; the first is the smallest change to this PR:

  1. In do_resume, when ENDED|REQUEST_HAS_COMPLETED is set and body_read_state == Pending, re-arm on_data_shim (as now) but skip :1459 and :1462 — return TRUE without draining. IncomingMessage._read() then sees hasBufferedDataDuringPause at :390 and drains via handle.drainRequestBody() at :401, which pushes to this (the IncomingMessage) directly. This mirrors how the pre-PR code delivered the pause buffer.
  2. Alternatively, have #resumeSocket() route the drained buffer to the IncomingMessage that owns the handle when _httpMessage is null — but both _httpMessage and kRequest are cleared by detachSocket(), so this needs a new back-pointer (e.g. stash the IncomingMessage on the native handle) and is a larger change.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/runtime/server/NodeHTTPResponse.rs:1405-1413 — The deferred mark_request_as_done_if_necessary() here runs from JS-side _read() after the slow consumer catches up — potentially after request N+1 on the same keep-alive connection has already dispatched and written its own inStream/userData onto the shared per-connection HttpResponseData. mark_request_as_done() → clear_on_data_callback (:703 → :2260-2264) then calls raw_response.clear_on_data() on N's still-live raw_response, gated only on SOCKET_CLOSED/UPGRADED — not on REQUEST_HAS_COMPLETED like the sibling set_on_data clear branch at :2293 — so uws_res_on_data(NULL, NULL) writes data->userData = nullptr; data->inStream = nullptr and silently drops N+1's request body (and leaves onTimeout/onAborted to fire with a null ctx). Fix: add && !flags.contains(Flags::REQUEST_HAS_COMPLETED) to the gate at :2260, mirroring :2293.

  Extended reasoning...

  What the bug is

  The fix for the buffered-tail truncation (previous review round) deferred mark_request_as_done_if_necessary() from on_buffer_request_body_while_paused(last=true) (:1623-1629) into drain_buffered_request_body_from_pause (:1406-1412), so it runs when JS eventually calls handle.drainRequestBody() from IncomingMessage._read(). But by the time the slow consumer catches up and _read() fires, a later request on the same keep-alive connection may already own the shared per-connection HttpResponseData. mark_request_as_done() → clear_on_data_callback() (:703 → :2260-2264) calls raw_response.clear_on_data() on N's still-live raw_response, and HttpResponse::onData(nullptr, nullptr) (packages/bun-uws/src/HttpResponse.h:940-943) writes data->userData = nullptr; data->inStream = nullptr — silently dropping every subsequent body chunk for N+1 (HttpContext.h:511 gates delivery on inStream) and leaving a later onAborted/onTimeout on N+1 to fire with a null ctx pointer (userData is shared with those callbacks per HttpResponseData.h:172-173).

  The specific code path

  Newly reachable via the relaxed do_pause gate (:1354-1357) plus the :1411 deferral. Handler does req.pipe(slowSink); res.end('ok'):

  1. write_or_end::<true> sets ENDED, re-arms on_data_shim at :2079, calls on_request_complete() which sets REQUEST_HAS_COMPLETED (body_read_state is still Pending, so IS_REQUEST_PENDING stays set).
  2. A body chunk arrives → onDataIncomingMessage → push() returns false (slowSink full) → pipe calls req.pause() → do_pause. The relaxed gate at :1354-1357 now permits ENDED && body_read_state == Pending, so it arms on_buffer_paused_shim.
  3. N's body fin arrives on the paused shim — always on Windows (pause_socket() is skipped per #[cfg(not(windows))] at :1370-1373), or on POSIX when coalesced into the same recv() batch (uws keeps parsing the recv buffer through the newly-armed shim). on_buffer_request_body_while_paused(chunk, last=true) appends the chunk, sets IS_DATA_BUFFERED_DURING_PAUSE_LAST, sets body_read_state = Done, unrefs body_read_ref, and — because the buffer is non-empty — defers mark_request_as_done_if_necessary().
  4. uws's body fin path nulls inStream (HttpContext.h:543) and continues: N+1's headers arrive on the same keep-alive connection (Windows: any later recv before slowSink drains; POSIX: same recv buffer if pipelined). The dispatcher creates M, and handle.ondata = onDataIncomingMessage → set_on_data reaches :2321 (M's ENDED=false, body_read_state=Pending) and writes inStream = on_data_shim, userData = M.ctx on the shared HttpResponseData.
  5. Later, slowSink drains → N's readable resumes → _read() → handle.drainRequestBody() on N (get_has_body() at :776-778 sets bit 3 while buffered_request_body_data_during_pause is non-empty). drain_buffered_request_body_from_pause hands the buffered tail to JS, then :1411 calls mark_request_as_done_if_necessary(). N has IS_REQUEST_PENDING set and should_request_be_pending() (ENDED && body_read_state==Done) is false → mark_request_as_done() runs.
  6. :703 calls clear_on_data_callback. At :2260, N's flags have neither SOCKET_CLOSED nor UPGRADED (and body_read_state == Done != None passes the outer gate at :2255), so :2262-2264 dereferences self.raw_response — which was never nulled on this path (only :572 (upgrade), :1259 and :1311 (both abort-only) null it) — and calls clear_on_data() on the live per-connection HttpResponseData that N+1 now owns.

  Why nothing existing prevents it

  clear_on_data_callback gates only on N's own SOCKET_CLOSED/UPGRADED flags. It has no guard equivalent to the "don't touch a pipelined request's userData" comments in do_pause/do_resume (:1359-1361, :1437-1439), and — critically — is asymmetric with set_on_data's clear branch at :2293-2295, which does gate its raw_response.clear_on_data() on !REQUEST_HAS_COMPLETED for exactly this hazard. N has REQUEST_HAS_COMPLETED set (from step 1), so a matching gate would have skipped the clear.

  Pre-PR this was unreachable: (a) do_pause refused while ENDED, so the paused shim never armed after res.end(); (b) drain_buffered_request_body_from_pause did not call mark_request_as_done_if_necessary(). Every mark_request_as_done() on N therefore ran synchronously inside a uws inStream callback (or inside res.end()), before uws could dispatch N+1. The :1411 deferral to a later JS turn is what breaks this invariant.

  Impact

  Silent request-body loss for the next keep-alive request on the same connection (HttpContext.h:511 gates body delivery on inStream), plus a potential null-deref crash if N+1 later times out or aborts (on_timeout_shim at :300-303 / on_aborted_shim dereference the ctx pointer with no null check). The trigger is narrow — backpressure after a synchronous res.end(), fin landing on the paused shim, and N+1 dispatched before the slow sink drains — but on Windows (where pause_socket() is a no-op) the fin-on-paused-shim leg is not a race, and req.pipe(slowSink); res.end() is precisely the pattern this PR exists to support.

  Fix

  Mirror set_on_data's clear branch: add && !flags.contains(Flags::REQUEST_HAS_COMPLETED) to the gate at :2260 in clear_on_data_callback. Once REQUEST_HAS_COMPLETED is set, uws's markDone() has already nulled N's inStream on the connection and any later inStream/userData there belongs to a subsequent request; N has no business touching it. Alternatively, null self.raw_response once N's body fin has been observed with REQUEST_HAS_COMPLETED set, so N can no longer reach the shared connection at all.

[claude]

No new findings on f1ae3c9, but this reworks the node:http server request-lifecycle state machine (ref-counting, per-socket vs per-request inStream/currentResponseObject, buffered-pause shim across keep-alive) and has needed several rounds of subtle fixes — worth a human pass before merge.

What was reviewed:

• should_request_be_pending now hoists the _LAST && !buffered.is_empty() check above every terminal flag (SOCKET_CLOSED/UPGRADED/TUNNELED/ENDED), closing the last parallel-arm truncation.
• set_on_data(undefined) allows clear_on_data() while pre-clear state is Pending, and clear_on_data_callback gates both the cached-slot write and the native clear on !REQUEST_HAS_COMPLETED — the N-clobbers-N+1 keep-alive hazard stays covered on both sides.
• write_or_end::<true>'s paused-shim re-arm re-pause_socket()s on POSIX so end()'s resume_socket() cannot leave the buffering shim on an un-paused fd.
• mark_request_as_done now unrefs body_read_ref; traced the added unref against every existing unref site — no double-unref path found.

Extended reasoning...

Overview

The PR changes src/runtime/server/NodeHTTPResponse.rs (request-body lifecycle after a synchronous res.end(): should_request_be_pending, do_pause/do_resume, write_or_end::<true>'s post-markDone() inStream re-arm, set_on_data's clear branch, clear_on_data_callback, on_buffer_request_body_while_paused, drain_buffered_request_body_from_pause, mark_request_as_done) and src/js/node/_http_server.ts (moves the req._dump() gate from ServerResponse.prototype.end to emitResponseFinish, matching Node's resOnFinish). Twelve new tests in node-http.test.ts and a one-line hostname fix in node-http-proxy.js.

Security risks

No auth/crypto surface. The class of concern here is resource exhaustion: an earlier revision could accumulate an entire request body unbounded in buffered_request_body_data_during_pause on a resumed fd; that was closed by re-pause_socket() after the paused-shim re-arm and by letting the clear branch disarm the shim while body_read_state == Pending. The other hazard was per-socket state bleed on keep-alive (request N clearing N+1's inStream/cached ondata), addressed by the !REQUEST_HAS_COMPLETED guard in clear_on_data_callback.

Level of scrutiny

High. This is the production node:http server hot path, with cross-cutting changes to a refcounted state machine (body_read_ref, IS_REQUEST_PENDING, deref()), per-socket uws state shared across pipelined requests, and platform-divergent behavior (pause_socket() is a no-op on Windows). The PR has taken eight review rounds to converge, each surfacing a distinct ref-leak / truncation / unbounded-buffer / N-vs-N+1 interaction — that history alone argues for a maintainer read of the final shape rather than bot approval.

Other factors

• f1ae3c9 addresses my last finding exactly as suggested (hoisting the _LAST && !buffered.is_empty() check to the top of should_request_be_pending), and the bug-hunting pass on this revision found nothing new.
• One unresolved comment-cop nag remains on the new comment block at should_request_be_pending:637 — cosmetic, not a blocker.
• Test coverage is good (12 new cases including keep-alive reuse, mid-upload socket destroy, chunked body split across segments, Connection: close), but the Windows-specific paused-shim paths and the slow-consumer + client-close-after-fin race that motivated f1ae3c9 are not directly exercised by an automated test.

[claude]

All prior findings from earlier rounds are now addressed (the last one — SOCKET_CLOSED bypassing the buffered-tail guard — is fixed by the new _LAST && !buffered.is_empty() check ahead of every terminal flag in should_request_be_pending). This run only surfaced a nit on per-test timeouts. That said, this touches the native node:http body-read lifecycle across enough interacting axes (uws inStream re-arm after markDone(), per-socket currentResponseObject vs per-request state on keep-alive, paused-shim buffering with Windows/POSIX pause_socket divergence, body_read_ref/IS_REQUEST_PENDING balancing) that a human maintainer should sign off on the final shape.

Extended reasoning...

Overview

Changes span src/runtime/server/NodeHTTPResponse.rs (body-read lifecycle after res.end(): should_request_be_pending, do_pause/do_resume gates, write_or_end::<true> re-arming inStream, set_on_data clear branch, clear_on_data_callback, mark_request_as_done, paused-shim buffering), src/js/node/_http_server.ts (moving the req._dump() gate from ServerResponse#end to emitResponseFinish, matching Node's resOnFinish), 12 new tests in node-http.test.ts, and a hostname fix in node-http-proxy.js.

Security risks

An earlier revision introduced a client-controllable unbounded buffer (paused shim armed on a resumed fd); that and the req.pause(); res.end() no-consumer variant were both fixed mid-review. The final diff re-pause_socket()s when re-arming the paused shim and allows clear_on_data() while pre-clear body_read_state == Pending, so the dump path disarms the shim. I did not find a remaining unbounded-growth path this round, but the interaction surface is large enough that I would not certify it.

Level of scrutiny

High. This is production-critical native code on the node:http server request path. Over five review rounds each surfaced a distinct real bug (N clobbering N+1's inStream/cached ondata on keep-alive, buffered fin tail freed before drain on ENDED/TUNNELED/SOCKET_CLOSED, unbounded paused-shim accumulation, stranded IS_REQUEST_PENDING on pipeline destroyer). All are now addressed, but the density of cross-layer invariants argues for a maintainer pass.

Other factors

Test coverage is solid for the primary fix (12 new tests, verified against Node v26 per the description; keep-alive reuse and mid-upload socket-destroy run in subprocesses and assert clean exit). The edge cases that drove the later fix commits — backpressure-triggered paused shim after res.end(), N+1 dispatch racing N's deferred mark_request_as_done, socket close with a buffered fin tail — are not directly exercised by the new tests (they need slow sinks / coalesced recv batches / Windows). The one inline finding this run is a style nit on 20_000 per-test timeouts; the file already has several, so it is non-blocking.

[robobun]

Closing in favour of #38196, which fixes the same res.end() body-discard mechanism (write_or_end releasing the body read state, uws's markDone() dropping the data handler, _dump() decided at res.end() instead of 'finish') against current main, and additionally keeps req.complete / readableEnded / 'end' / 'close' tied to the body actually arriving, keeps pause() / resume() working after the response, and delivers the body of Connection: close / HTTP/1.0 requests (which this branch discarded explicitly). This branch has also been conflicting with main since the beginning of the month.

The tests from this PR were carried over to #38196 (11 of the 12 cases, in node-http.test.ts); the Connection: close case was replaced there by tests asserting Node's behaviour, since Node does not emit 'end' for a body that is cut off by the close.