Conversation
…s FIN For payloads above the 16KB cork buffer, res.write() holds the unsent tail by reference (pending_pinned_write) instead of copying into the uWS AsyncSocketData buffer. socket.end() only consults getBufferedAmount(), which does not see that tail, so it shuts the socket down with the tail still outstanding: an 8 MiB write delivered ~2.5 MiB then FIN. On socket.end() with an attached response, first spill the held tail into the backpressure buffer so getBufferedAmount() counts it. Clear HTTP_RESPONSE_PENDING along with setting HTTP_CONNECTION_CLOSE so HttpContext::onWritable performs the deferred shutdown once the buffer has drained, even when res.end() was never called (in Node both writes go through one net.Socket Writable, so the FIN is already ordered after every queued byte).
|
Updated 11:10 PM PT - Jul 21st, 2026
❌ @robobun, your commit fea03f2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35027That installs a local version of the PR into your bun-35027 --bun |
…ESPONSE_PENDING Clearing HTTP_RESPONSE_PENDING in deferShutdownUntilResponseDrains removed the guard two other HttpContext sites read while the spilled body is still draining (onEnd's httpAllowHalfOpen gate and onData's pipelining branch). Leave the bit alone and add HTTP_NODE_SOCKET_ENDED so HttpContext::onWritable's shutdown gate can fire for a socket.end() that was deferred behind buffered bytes without changing any other consumer's semantics. The bit is connection-scoped so it survives resetResponseState().
WalkthroughChangesNode HTTP shutdown now preserves socket-end state, spills pending pinned writes before FIN checks, and validates response delivery across content-length, chunked, and pipelined scenarios when Node HTTP shutdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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 `@src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp`:
- Around line 251-253: Condense each non-obvious comment to three lines or fewer
while preserving its rationale: update the shutdown-state comment at
src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp lines 251-253, the
pinned-tail/FIN comment at test/js/node/http/node-http-pinned-write.test.ts
lines 343-347, and the framing comment at
test/js/node/http/node-http-pinned-write.test.ts lines 391-394; make no code
changes.
🪄 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: 3ce53087-91fe-4fbe-8219-1682dadf3441
📒 Files selected for processing (5)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponseData.hsrc/jsc/bindings/node/JSNodeHTTPServerSocket.cppsrc/runtime/server/NodeHTTPResponse.rstest/js/node/http/node-http-pinned-write.test.ts
…-keepalive test socket.end() can run inside HttpContext::onData's corked region (e.g. from a req 'data' listener). A sub-cork-buffer spill would land in the cork buffer, which getBufferedAmount() does not count; uncork first so every queued byte is visible to the probe. The test now asserts the exact body length for each framing (matching Node.js byte-for-byte) and adds a variant that sends two pipelined keep-alive requests in one write, guarding the HTTP_RESPONSE_PENDING semantics the onData dispatch branch relies on.
The bit only needs to live until onWritable drains this response's buffer; while HTTP_RESPONSE_PENDING is set (the scenario it exists for), the two resetResponseState() callers are unreachable, so it never needs to survive a reset. Keeping it connection-scoped let it leak into a subsequent keep-alive response and satisfy onWritable's shutdown gate while that response still has a pinned-write tail outstanding.
There was a problem hiding this comment.
fea03f2 addresses my last concern — HTTP_NODE_SOCKET_ENDED is out of HTTP_CONNECTION_SCOPED, so resetResponseState() clears it and it can't leak into a subsequent response. I re-checked the current diff and found nothing further; deferring to a maintainer for the uWS socket-lifecycle change.
What was reviewed:
onWritableshutdown gate: the new OR branch fires only withHTTP_CONNECTION_CLOSE(set alongside the bit) and only for the response that calledsocket.end().spill_pending_pinned_write_for_endreuses the existing spill path (spillBodyTail) already exercised byres.end()and second-write-before-drain.- The new
uncork()before the buffered-amount probe: onData's trailinguncork()becomes a no-op (no cork slot), and the early flush is what makes sub-cork-buffer writes visible to the probe.
Extended reasoning...
Overview
Fixes a node:http compat truncation: res.write(large); req.socket.end() (no res.end()) sent FIN before the zero-copy pinned tail drained. Touches packages/bun-uws/src/HttpContext.h (onWritable shutdown gate), HttpResponseData.h (new HTTP_NODE_SOCKET_ENDED bit), JSNodeHTTPServerSocket.cpp (spill + uncork before the buffered-amount probe), a thin Rust export in NodeHTTPResponse.rs, and three new describe.each cases in node-http-pinned-write.test.ts.
Prior review status
I flagged two issues on earlier revisions, both fixed: (1) clearing HTTP_RESPONSE_PENDING broke onEnd's half-open gate and onData's pipelined-dispatch check → replaced with a dedicated bit in 1e51745; (2) that bit was in HTTP_CONNECTION_SCOPED and could leak into a later response → dropped in fea03f2. On the current head I traced the remaining reader (onWritable) and the two resetResponseState() callers and found no further hazard: the bit is set only alongside HTTP_CONNECTION_CLOSE, and while HTTP_RESPONSE_PENDING is set neither reset caller is reachable.
Security risks
None identified. Server-side write ordering; no parsing of untrusted input, no auth/crypto.
Level of scrutiny
High — this is bun-uws HTTP server socket lifecycle (cork state, buffered-amount accounting, shutdown sequencing) on a hot node:http compat path. Two earlier revisions had real ordering bugs, which is a signal the interaction surface is subtle.
Other factors
Tests cover Content-Length, chunked, and pipelined keep-alive; the PR body lists the surrounding test files as passing. The Rust export is a one-line wrapper over an existing helper. Given the subsystem, a maintainer familiar with the uWS/node:http integration (coderabbit suggested cirospaciari) should give final sign-off.
|
CI on build 77396: the diff is green (the new |
…oses (#35034) ### Problem ```js const server = http.createServer((req, res) => { res.writeHead(200, { 'Content-Length': String(BIG.length) }); res.write(BIG); // 8 MiB res.end(); }); // raw net client does: c.end('GET / HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n') // node: body bytes 8388608 // bun: body bytes 2621440 (truncated at the first kernel send), then connection drops ``` A raw-socket client that sends its request with `socket.end(...)` (half-closes its write side) receives only what the kernel accepted on the first `send()`; the rest of the response body is dropped. Same result without `res.end()`, and for `server.httpAllowHalfOpen = true`. Debug trace at onEnd: `state=0x77 buf=5767168 pipelined=0` (res.end called, HTTP_RESPONSE_PENDING cleared, 5.8 MiB still in AsyncSocketData::buffer) or `state=0x7b buf=0` (no res.end, pinned write held in `NodeHTTPResponse::pending_pinned_write`); in both cases the half-open gate does not hold and the fall-through `asyncSocket->close()` discards the unsent bytes. ### Cause `HttpContext::onEnd<true>` only defers close for `httpAllowHalfOpen && (HTTP_RESPONSE_PENDING || pipelined)`; the fall-through close runs even when `AsyncSocketData::buffer` (or a zero-copy `res.write()` tail being drained via the onWritable callback) still holds response bytes. Node's `socketOnEnd` issues `socket.end()`, which in Node's single-Writable model queues FIN behind every byte already handed to `res.write()`, so nothing is dropped. ### Fix * `onEnd<true>`: also defer close while `getBufferedAmount() > 0` or an onWritable callback is armed (covers the pinned tail). The connection is shut down from the existing `shouldCloseConnection()` gates once those bytes have flushed. * `onWritable<true>`'s close gate: with `!httpAllowHalfOpen` and the peer's FIN received, shut the socket down once the buffer has drained even if `res.end()` was never called (Node's `socket.end()` semantics). A writable event that moves zero bytes after FIN (EPIPE, peer gone) closes immediately so the deferred connection does not spin between onWritable and onEnd. * `NodeHTTPResponse::on_drain`: return `false` only while a pinned drain made partial progress (so onWritable waits instead of evaluating its close gate against a `bufferedAmount` that does not count the pinned tail); zero progress falls through to the close gate, and once drained the native callback is disarmed so onEnd does not read a stale shim as pending output. `Bun.serve` is unaffected (the new close-gate terms are `if constexpr (IsNodeHttp)`). ### Verification New `describe.each` in `test/js/node/http/node-http-backpressure.test.ts` covers three variants (default + end, default + no end, httpAllowHalfOpen + delayed end-after-drain); all three receive 2621440 bytes on main and the full 8 MiB with the fix, matching Node.js. `node-http-backpressure.test.ts`, `node-http-pinned-write.test.ts`, `node-http-server-socket-end-drain.test.ts`, `test-http-server.js`, `test-http-1.0.js` pass; the EPIPE spin showed up first as `node-http-uaf.test.ts` timing out in CI build 77357 and is eliminated by the zero-progress close. ### Relation to #35027 \#35027 is the `req.socket.end()` initiator (server-side JS triggers the FIN); this PR is the peer-FIN initiator (client half-closes after its request). Different triggers, no file conflicts beyond the `onWritable` close-gate hunk both touch. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 2 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/63] gen cpp.rs (cppbind) [2/63] gen generated_host_exports.rs generated_host_exports.rs: 91 exports (host=3, lazy=10, generic=78, rust=0); 237 extern-C blocks audited [3/63] gen JS modules (bundle-modules) Preprocess modules (9420ms) Bundle modules (60ms) Postprocesss modules (35ms) Bundle Functions (683ms) Generate Code (9ms) [10.23s] Bundled "src/js" for development 2210 kb 165 internal modules 13 native modules 90 internal functions across 19 files [3/50] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) [37/50] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-15.cpp.o FAILED: obj/unified/UnifiedSource-src_jsc_bindings_webcore-15.cpp.o /usr/bin/ccache /usr/lib/llvm-21/bin/clang++ -march=nehalem -O0 -g3 -gz=zstd -glldb -fsanitize=address -fno-exceptions -fno-c++-static-destructors -fno-rtti -f ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (95ba710) test/js/node/http/node-http-backpressure.test.ts: (pass) backpressure > should handle backpressure [14.69ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [24.12ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [21.54ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [12.22ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [8.84ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [8.88ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() after drain, httpAllowHalfOpen [7.14ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts bun test v1.4.0 (afac658) test/js/node/http/node-http-backpressure.test.ts: (pass) backpressure > should handle backpressure [496.43ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [425.44ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [239.88ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [249.10ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [237.24ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [163.07ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision afac658 features baseline 22 deps, 106 codegen, 1169 objects in 671ms ninja: Entering directory `/workspace/bun/build/release' [1/1231] install /workspace/bun bun install v1.4.0-canary.1 (95ba710) Checked 124 installs across 170 packages (no changes) [6.00ms] [2/1231] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (95ba710) Checked 1 install across 2 packages (no changes) [1.00ms] [3/1231] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (95ba710) Checked 129 installs across 147 packages (no changes) [4.00ms] [4/1231] gen ErrorCode+*.h [5/1231] gen bindgenv2 [6/1231] fetch libjpeg-turbo [libjpeg-turbo] up to date [7/1231] fetch picohttpparser [picohttpparser] up to date [8/1231] fetch zlib [zlib] up to date [9/1231] fetch tinycc [tinycc] up to date [10/1230] gen .bind.ts → GeneratedBindings.cpp [11/1230] subst deps/zlib/zlib.h [12/1230] gen ProcessBindingConstants.lut.h Generating /workspace/bun/build/release/codegen/Proc ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-uws/src/HttpContext.h | 58 ++++++++++++------ packages/bun-uws/src/HttpResponseData.h | 11 ++-- src/runtime/server/NodeHTTPResponse.rs | 16 ++++- src/uws_sys/Response.rs | 6 ++ test/js/node/http/node-http-backpressure.test.ts | 78 ++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 22 deletions(-) ``` </details> **gate history** · 5 passed · 1 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-uws/src/HttpContext.h 14 16 0 packages/bun-uws/src/HttpResponseData.h 2 1 0 src/runtime/server/NodeHTTPResponse.rs 17 14 0 src/uws_sys/Response.rs 2 2 0 test/js/node/http/node-http-backpressure.test.ts 3 5 0 ``` </details> <!-- robobun:evidence:end -->
|
Note on the state of main for this case, from the check that closed #35054. Main at 36cd151 (with #43557) no longer truncates:
With 1 MiB (it fits the socket buffers) Repro// WRITE=one|many CLOSE=end|destroySoon
import { once } from "node:events";
import http from "node:http";
import net from "node:net";
const SIZE = 8 << 20;
const { WRITE = "one", CLOSE = "end" } = process.env;
const server = http.createServer((req, res) => {
res.writeHead(200);
if (WRITE === "one") res.write(Buffer.alloc(SIZE, 0x61));
else {
const chunk = Buffer.alloc(8 * 1024, 0x61);
for (let i = 0; i < SIZE / chunk.length; i++) res.write(chunk);
}
if (CLOSE === "end") req.socket.end();
else req.socket.destroySoon();
});
await once(server.listen(0, "127.0.0.1"), "listening");
const c = net.connect(server.address().port, "127.0.0.1");
await once(c, "connect");
let bytes = 0;
c.on("data", d => (bytes += d.length));
c.on("error", () => {});
c.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
const watchdog = setTimeout(() => {
console.log("no close after 15 s, bytes=" + bytes);
process.exit(1);
}, 15000);
await new Promise(resolve => c.once("close", resolve));
clearTimeout(watchdog);
console.log("closed, bytes=" + bytes);
process.exit(0);This branch conflicts with main. |
|
Closing: #44244 replaces this PR. It is stacked on #44243.
|
Problem
Since #34511,
res.write()payloads larger than the 16 KB cork buffer take a zero-copy path:tryWriteBodywrites what the kernel accepts and the unsent tail is held by reference inNodeHTTPResponse::pending_pinned_write, not copied intoAsyncSocketData::buffer.req.socket.end()(jsFunctionNodeHTTPServerSocketEnd->shutdownAfterResponseDrains) only consultsAsyncSocket::getBufferedAmount(), sees 0, and shuts the socket down immediately. In Node.jsres.write()andsocket.end()go through onenet.SocketWritable, so the FIN is already ordered behind every queued byte.Fix
shutdownAfterResponseDrains()first spills the current response'spending_pinned_writeintoAsyncSocketData::buffer(newBun__NodeHTTPResponse_spillPendingPinnedWrite, the same spill pathres.end()already uses), sogetBufferedAmount()counts it anddeferShutdownUntilResponseDrainshands the close toHttpContext::onWritable.deferShutdownUntilResponseDrainsuncorks before probinggetBufferedAmount():socket.end()can run insideHttpContext::onData's corked region (e.g. from areq'data'listener), and a sub-cork-buffer spill or priorres.write(small)would otherwise be invisible to the probe.HttpResponseData::HTTP_NODE_SOCKET_ENDEDbit is set alongsideHTTP_CONNECTION_CLOSE, andHttpContext::onWritable's shutdown gate also checks it:socket.end()means no more response bytes will ever be written, so the gate fires once the buffer has drained even whenres.end()was never called.HTTP_RESPONSE_PENDINGis left alone soonEnd'shttpAllowHalfOpengate andonData's pipelined-dispatch branch keep their existing semantics. The bit is per-response (cleared byresetResponseState()) so it cannot leak into a subsequent keep-alive response and trip the gate while that response still has a pinned tail outstanding.Verification
New
describe.eachintest/js/node/http/node-http-pinned-write.test.tscovers Content-Length, chunked, and two pipelined keep-alive requests in one write; each receives ~2.5 MiB on main and the full 8 MiB with the fix, matching Node.js byte-for-byte (the test asserts the exact body length per framing).node-http-pinned-write.test.ts(12/12),node-http-backpressure.test.ts,node-http-server-socket-end-drain.test.ts,node-http-nested-cork.test.ts,node-http-transfer-encoding.test.tsandnode-http.test.tspass (the onerequest via http proxyfailure there is pre-existing on released bun in this environment).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/http/node-http-pinned-write.test.ts