node:http: release a request whose body fin was buffered while paused once the response ends

[robobun]

Problem

• A node:http server request stays counted as in flight forever when the last chunk of its body arrived while the IncomingMessage was paused and the response was ended afterwards. server.close() never calls back (the server's 'close' event needs the in-flight count to reach zero), the process it keeps alive never exits, and the native response object leaks (IS_REQUEST_PENDING is a self-reference). Node v26 closes cleanly; reproduces on bun 1.4.0 and main.
• Easy to hit without an explicit pause(): a body chunk larger than the IncomingMessage highWaterMark that nobody reads pauses the connection mid-segment (Node's readStop semantics), so the rest of the same segment (remaining chunks + terminating chunk) is buffered natively while paused.
• Cause, src/runtime/server/NodeHTTPResponse.rs:
  • on_buffer_request_body_while_paused(last = true) sets IS_DATA_BUFFERED_DURING_PAUSE_LAST, releases body_read_ref, and deliberately leaves body_read_state == Pending so JS can still drain the buffered tail (drainRequestBody).
  • should_request_be_pending() read that Pending as "body still arriving", so res.end() -> on_request_complete() kept IS_REQUEST_PENDING. write_or_end's dump-equivalent does not apply either, because it keys on body_read_ref still being held.
  • Nothing re-evaluates later: uws delivers nothing further for that body (it nulls its handler at the fin and again at end()), and the later ondata = undefined from _dump()/_destroy() only flips the state to Done.
  • The socket-close fallback in handle_abort_or_timeout only reaches the connection's current response, so as soon as the keep-alive connection serves one more request the strand is permanent.

Fix

• should_request_be_pending() treats a body whose fin was buffered while paused as complete (Pending and not IS_DATA_BUFFERED_DURING_PAUSE_LAST is the only "still arriving" state). The request is then released where every other body state is released: at res.end() via on_request_complete(), or for an upgrade tunnel with a body at the fin itself (the existing mark_request_as_done_if_necessary() call in on_buffer_request_body_while_paused, matching what the unbuffered fin already does in on_data_or_aborted).
• mark_request_as_done() no longer frees buffered_request_body_data_during_pause in that state while the connection is still open: the IncomingMessage still owns that tail and drains it on its next _read() (possibly after the response ended). It is freed when drained, when the reader lets go (set_on_data's clear branch, reached from _dump() / _destroy() / a re-arm attempt that follows _read()'s drain), or with the box. Closed/upgraded connections free it immediately as before.
• Why this is correct: once the fin has been seen there is nothing left for the accounting to wait for. Every other consumer of the flag already treats it as "body complete" (hasBody reports done, pause()/resume()/ondata refuse to re-arm), and the accounting release at res.end() is what Node does too (resOnFinish drops the request from the server's queue regardless of whether its body was consumed). Releasing the accounting does not affect JS's ability to read the tail because the wrapper holds its own reference to the box; only the buffer's lifetime had to be decoupled.
• The clear_on_data() reached from mark_request_as_done() on this new path is a no-op: uws already nulled the connection's handler when it delivered the fin (and again in end()), and ondata is cleared on this request's own wrapper (armed_this_value).
• Tests: test/js/node/http/node-http-req-socket-pause.test.ts, four new tests, each also checked against Node v26 (all pass there):
  • unread body, response ended later, second keep-alive request, server 'close' fires: times out on main.
  • reader that starts reading as the response ends still gets the buffered tail, then server 'close' fires: times out on main, and fails (tail missing) if the tail is freed at release.
  • same with the next request pipelined in the same segment (covers the variant where the tail was drained before res.end()): times out on main.
  • upgrade request with such a body, reader attached later gets the whole body: passes on main by design, fails (tail missing) if the tail is freed at the fin-time release; covers the tunnel arm.
• Also run on this build: the handoff repro exits with server closed; node-http.test.ts, node-http-backpressure*.test.ts, node-http-transfer-encoding, node-http-server-abort-events, node-http-connect, node-http-with-ws, node-http-ondata-reregister-leak, node-http-server-socket-end-drain, node-http-uaf, node-http-server-timeouts, and 42 vendored test-http-{pause,no-read-no-dump,expect-continue,keep-alive,pipeline,upgrade-server-with-body,...} tests. The only failures are identical without this diff (the proxy test's localhost resolution in this container and a few subprocess tests exceeding 5 s on the ASAN build). cargo clippy -p bun_runtime is clean.
• Related: node:http: keep receiving the request body after the response has ended #38196 (keep receiving the body after the response ended) names this same predicate body_still_arriving() and leaves this strand to a separate fix; in its model a paused reader can also get the fin buffered after res.end(), and this change is what releases that request. node:http: deliver a pipelined POST's body when the previous response is still in flight #34761 (pipelined successor's handler slot) is independent.

Background

• Request body delivery: uws keeps one HttpResponseData per connection with a single body-data handler slot. NodeHTTPResponse arms it with on_data_shim (deliver to JS ondata) or, while the IncomingMessage is paused, on_buffer_paused_shim, which appends chunks to buffered_request_body_data_during_pause; JS later fetches that buffer with drainRequestBody() from IncomingMessage._read(). uws clears the slot itself after the chunk flagged as the fin.
• body_read_state is None (no body), Pending (uws may still call back, or a buffered tail is still readable) or Done; IS_DATA_BUFFERED_DURING_PAUSE_LAST records that the fin was among the buffered chunks. body_read_ref is the event-loop keep-alive held while chunks are still expected.
• IS_REQUEST_PENDING is a self-reference on the response box plus one unit of the server's in-flight request count; mark_request_as_done() drops both, and should_request_be_pending() decides whether an ended/tunneled response may drop them yet. server.close() in node:http completes through the native server's all-requests-done promise, which is gated on that count.

Repro from the report (hangs on bun 1.4.0, exits with "server closed" on Node and with this change)

import { once } from "node:events";
import { createServer } from "node:http";
import { connect } from "node:net";
let n = 0;
const server = createServer((req, res) => { const i = ++n; if (i === 1) setTimeout(() => res.end("ok1"), 100); else res.end("ok2"); });
server.listen(0, "127.0.0.1"); await once(server, "listening");
const client = connect(server.address().port, "127.0.0.1");
let data = ""; client.on("data", d => (data += d)); await once(client, "connect");
const big = Buffer.alloc(70000, "x").toString();
client.write("POST / HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunked\r\n\r\n" + big.length.toString(16) + "\r\n" + big + "\r\n3\r\nend\r\n0\r\n\r\n");
while (!data.includes("ok1")) await new Promise(r => setTimeout(r, 5));
client.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
while (!data.includes("ok2")) await new Promise(r => setTimeout(r, 5));
client.destroy(); server.close(() => console.log("server closed"));

BUN_DEBUG_NodeHTTPResponse=1 before: onData(70000) -> doPause -> onBufferRequestBodyWhilePaused(3, false) -> onBufferRequestBodyWhilePaused(0, true) -> end('ok1') -> onRequestComplete, and no markRequestAsDone() for the first request. After: markRequestAsDone() follows onRequestComplete directly, and the later drainBufferedRequestBodyFromPause 3 shows the tail still being handed to JS.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 30 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: 333d244b-729d-499b-a20d-e4fc56850cf1

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 01b1e25.

📒 Files selected for processing (2)

• src/runtime/server/NodeHTTPResponse.rs
• test/js/node/http/node-http-req-socket-pause.test.ts

---

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

[robobun]

^{Updated 2:19 PM PT - Aug 13th, 2026}

✅ @robobun, your commit 01b1e255d1ddfc5ef5ea9aed346c9ccafa191188 passed in Build #94785! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38207

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

bun-38207 --bun

[robobun]

Status: fix pushed, waiting on CI and review.

• Reproduced with the report's script on bun 1.4.0 and on a debug build of main (timeout 15 bun repro.mjs exits 124 without printing server closed; Node v26 prints it and exits 0). BUN_DEBUG_NodeHTTPResponse=1 confirms the first request never reaches markRequestAsDone() once its fin was buffered while paused.
• With this change the repro prints server closed and exits 0, and the four new tests in test/js/node/http/node-http-req-socket-pause.test.ts pass; three of them time out on main (the fourth guards the buffered tail on the upgrade path and fails if that tail is freed at release). All four also pass as-is under Node v26.
• While writing the tests I ran into two unrelated pre-existing problems (a pipelined successor's body handler being dropped by the previous response's end(), and upgrade-with-body requests losing their body for a reader attached later); both are being tracked separately and are not touched here.

[github-actions]

This PR may be a duplicate of:

1. node:http: deliver a pipelined POST's body when the previous response is still in flight #34761 - Contains the same should_request_be_pending() rewrite (a body_pending local combining body_read_state == Pending with !IS_DATA_BUFFERED_DURING_PAUSE_LAST, substituted into both the TUNNELED and ENDED branches), editing the identical lines for the same pending-request accounting.
2. node:http: keep delivering the request body after a synchronous res.end() #35489 - Rewrites the same should_request_be_pending() TUNNELED/ENDED branches around the buffered-during-pause fin and adds the very same buffered_request_body_data_during_pause.clear_and_free() to set_on_data()'s clear branch, i.e. the same predicate plus free-site relocation.

🤖 Generated with Claude Code

[claude]

I reviewed this PR and didn't find any bugs. Because it changes native HTTP server request-lifecycle accounting (when mark_request_as_done runs and what it frees, plus a new unconditional buffer free in set_on_data's refuse-to-arm branch), a human look would still be worthwhile.

What was reviewed:

• should_request_be_pending(): traced that IS_DATA_BUFFERED_DURING_PAUSE_LAST is only set alongside body_read_ref unref, so treating it as body-complete for accounting matches every other consumer of the flag.
• Buffer lifetime in mark_request_as_done(): verified the retained tail is later reachable via _read() → drainRequestBody(), and is freed either there, in set_on_data's clear branch, or in deinit().
• set_on_data new clear_and_free(): confirmed _read() always calls drainRequestBody() before assigning handle.ondata, so the buffer is empty by the time this branch is reached with a callable; _dump/_destroy reach it with undefined, where freeing is intended.
• Tests await observable events (server 'close', marker in response bytes, body promise) rather than sleeping; the four scenarios cover unread/read-late × keep-alive/pipelined/upgrade.

Extended reasoning...

Overview

The PR fixes a request-accounting leak in node:http server: when a request body's fin chunk arrives while the IncomingMessage is paused (native buffering path), should_request_be_pending() previously read body_read_state == Pending as "body still arriving" and kept IS_REQUEST_PENDING set forever after res.end(), stranding the server's in-flight count and the response object's self-reference. Two files change: src/runtime/server/NodeHTTPResponse.rs (three code sites, one comment) and four new tests in test/js/node/http/node-http-req-socket-pause.test.ts.

The Rust change has three parts: (1) should_request_be_pending() now treats Pending && IS_DATA_BUFFERED_DURING_PAUSE_LAST as body-complete, factored into a shared body_pending local for both the TUNNELED and ENDED arms; (2) mark_request_as_done() skips freeing buffered_request_body_data_during_pause when the fin was buffered and the socket is still open (so JS can still drainRequestBody()); (3) set_on_data's refuse-to-arm branch now frees that buffer, catching the _dump/_destroy/re-arm-after-drain paths so the retained buffer doesn't leak until deinit().

Security risks

None identified. This is server-side request accounting and buffer lifetime management with no auth, crypto, or user-controlled parsing changes. The buffer being retained longer is bounded (it holds only the tail of one request's body that arrived while paused), and every release path was traced to a free (drain, set_on_data clear, or deinit()).

Level of scrutiny

High. This is native HTTP server lifecycle code — refcount balancing (IS_REQUEST_PENDING is a self-reference plus a server counter unit), buffer ownership across the JS/native boundary, and interaction with the shared per-connection uws HttpResponseData handler slot. The correctness of retaining the buffer in mark_request_as_done() depends on the invariant that _read() drains before it re-arms ondata, and that clear_on_data_callback() (called unconditionally from mark_request_as_done() on the new path) is safe when uws has already nulled its handler. The PR description argues each of these convincingly and I verified them against the source, but the reasoning is subtle enough that a maintainer familiar with the pipelined-request / keep-alive edge cases in this file should confirm.

Other factors

The four new tests are well-constructed: they use a raw TCP socket to control segment framing, await response markers rather than sleeping, exercise a follow-on keep-alive request so socket-close cannot mask the leak, and each is documented as also passing on Node v26. The PR description lists an extensive set of related test files run on the build. The comment update in handle_abort_or_timeout correctly reflects that the paused-fin case is no longer the reason REQUEST_HAS_COMPLETED can co-exist with Pending (the custom-ondata case in write_or_end remains). No CODEOWNERS entry covers these paths.

[cirospaciari]

@robobun please run the tests this PR adds or changes on Node.js v26.3.0 and report whether each one passes or fails.
They must pass on both Bun and Node v26.3.0, so fix any case that only passes on Bun.

[robobun]

@cirospaciari I ran test/js/node/http/node-http-req-socket-pause.test.ts (the only test file this PR changes) unmodified on Node v26.3.0 with node --test. A small resolve hook maps bun:test onto node:test and node:assert (below). All 7 tests pass, 30 of 30 runs. The 4 tests this PR adds:

Test	Node v26.3.0
request whose whole body arrived while it was paused ... > is released once the response ends even though the body is never read	pass
... > still hands the rest of the body to a reader that starts reading as the response ends	pass
... > is released once the response ends when the next request was pipelined behind it	pass
upgrade request whose whole body arrived while it was paused still hands the whole body to a reader attached later	pass

The 3 tests that were already in the file (req.socket emits 'pause' once an unread request body fills the IncomingMessage buffer, body reading from 'pause' still delivers every byte and 'end', req.socket emits 'pause' on every body-bearing keep-alive request, not just the first) also pass there.

On Bun the file is green on every lane of this PR's CI run for the head commit 01b1e25 (build #94785), including the ASAN lane. No test only passes on Bun, so there is nothing to change.

A negative control (one toEqual changed to a wrong value) fails under the same hook with ERR_ASSERTION, so the hook does not hide failures.

How to rerun on Node

// bun-test-node.mjs
import { registerHooks } from "node:module";
registerHooks({
  resolve(specifier, context, next) {
    if (specifier === "bun:test") return { url: import.meta.url, shortCircuit: true };
    return next(specifier, context);
  },
});
import assert from "node:assert";
export { describe, it, test, before as beforeAll, after as afterAll, beforeEach, afterEach } from "node:test";
export function expect(value) {
  return {
    toBe: e => assert.strictEqual(value, e),
    toEqual: e => assert.deepStrictEqual(value, e),
    toContain: e => assert.ok(value.includes(e)),
    toBeGreaterThan: e => assert.ok(value > e),
    toBeGreaterThanOrEqual: e => assert.ok(value >= e),
    toBeLessThan: e => assert.ok(value < e),
  };
}

cp test/js/node/http/node-http-req-socket-pause.test.ts /tmp/node-http-req-socket-pause.test.mts
node --import ./bun-test-node.mjs --test --test-reporter=spec /tmp/node-http-req-socket-pause.test.mts

✔ req.socket emits 'pause' once an unread request body fills the IncomingMessage buffer (16.600931ms)
✔ body reading from 'pause' still delivers every byte and 'end' (3.238522ms)
✔ req.socket emits 'pause' on every body-bearing keep-alive request, not just the first (4.787984ms)
▶ request whose whole body arrived while it was paused, answered later on a keep-alive connection
  ✔ is released once the response ends even though the body is never read (2.757649ms)
  ✔ still hands the rest of the body to a reader that starts reading as the response ends (1.851139ms)
  ✔ is released once the response ends when the next request was pipelined behind it (2.646178ms)
✔ request whose whole body arrived while it was paused, answered later on a keep-alive connection (7.731327ms)
✔ upgrade request whose whole body arrived while it was paused still hands the whole body to a reader attached later (1.381006ms)
ℹ tests 7
ℹ suites 1
ℹ pass 7
ℹ fail 0

The .mts copy is only so that Node treats the file as ESM with type stripping. The file content is unchanged.