node:net: fail the write when the flush at the end of the open dispatch gets a fatal send error - #43026
node:net: fail the write when the flush at the end of the open dispatch gets a fatal send error#43026robobun wants to merge 4 commits into
Conversation
… fatal send error A write from inside the 'connection' or 'connect' listener runs under the native open dispatch. When its send is short, on_open flushes the rest of buffered_data_for_node_net once the listener returns. It discarded the errno that internal_flush reports for a fatal send (EPIPE, ECONNRESET). It then saw the buffer empty, because the fatal path drops the bytes, and dispatched the drain callback. node:net completed the write as delivered: the callback succeeded and 'finish' fired for bytes that were never sent. on_writable already fails the write in this case on POSIX. Move that delivery into fail_fatal_flush and call it from the tail of on_open too. Windows keeps the legacy contract in both places. write_or_end also queued the bytes of a write whose first send was fatal, although JS fails that write. The same flush then sent them again. Do not queue them.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 3 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 3 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 minute for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 2:35 AM PT - Sep 17th, 2026
✅ @robobun, your commit aecd9e37a5c4f469cfd914edcb95ac46ae4b9be8 passed in 🧪 To try this PR locally: bunx bun-pr 43026That installs a local version of the PR into your bun-43026 --bun |
|
Status: the fix and the tests are pushed. CI is green on How I reproduced it
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked: fail_fatal_flush taking &self across call_error_handler — socket is a Cell and the dispatching frame (on_open's ref / on_writable's ScopeExit) already keeps the wrapper alive, so it matches the pre-refactor inline code; the wrote >= -1 change in write_or_end — -1 (closed / rejected-TLS would-block) still queues the remainder as before; and the TLS on_open tail — unchanged because it sits under the existing !SSL guard, so tls.Server/tls.connect listeners keep their prior behaviour rather than gaining a new failure mode.
Extended reasoning...
The change is a focused fix in the node:net socket runtime plus tests. One inline finding (a pre-existing double 'error' on fd-adopted sockets that the new on_open path now also reaches) is already posted. The additional items above were examined from the diff and surrounding code and do not appear to introduce new defects: the helper's &self use reads only a Cell field after user JS, under a frame that already holds the wrapper reference exactly as the inlined on_writable code did; the write_or_end condition change only excludes fatal (< -1) sends and leaves the legacy -1 path intact; and TLS sockets are excluded by the pre-existing !SSL gate rather than by this PR. Given this touches socket lifecycle and error delivery on a hot path, a human maintainer's look is still warranted; this note only records what was ruled out.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this version and found no bugs; since the change adds a second native-to-JS error dispatch (with a nested close) at the tail of on_open in src/runtime/socket/socket_body.rs, a human look at that lifecycle path is still worthwhile.
What was reviewed:
fail_fatal_flushis a byte-for-byte lift of the formeron_writableblock; its&selfacrosscall_error_handlermatches the file's R-2 convention (mutated fields areCell/JsCell), and theon_opencaller already re-enters JS from the same spot (drain_callback/call_error_handler), so no new keep-alive requirement is introduced.- The
wrote >= -1gate inwrite_or_endonly affects thebuffer_unwritten_datapath (write_or_end_bufferedwith an empty buffer);-1(closed/rejected) still buffers, and TLS never produces< -1sincewrite_check_errorsets no fatal errno for SSL sockets. - Windows gating:
fail_fatal_flushand both call sites are#[cfg(not(windows))], so no dead-code warning on that target; the Windows drain contract is unchanged. - The new server test's busy-wait is bounded (10s) and asserts
sawResetexplicitly, so a missed marker fails rather than silently passes.
Extended reasoning...
Overview
The PR touches one Rust file (src/runtime/socket/socket_body.rs) and two test files. In Rust: (1) the POSIX fatal-flush delivery previously inlined in on_writable becomes fail_fatal_flush(&self, handlers, this_value, errno), (2) the on_open tail now checks internal_flush()'s return and calls fail_fatal_flush instead of dispatching the drain callback on an emptied buffer, and (3) write_or_end no longer appends a chunk to buffered_data_for_node_net when write_maybe_corked returned a fatal negative errno (< -1). Tests add fault-injected EPIPE/ECONNRESET cases for both writer sides in net-syscall-fault.test.ts and a real-peer-reset case in node-net-server.test.ts.
Security risks
None specific to this change. It does not parse untrusted input or touch auth/TLS negotiation; it changes how an already-detected kernel send failure is reported to JS. The failure now closes the socket and surfaces 'error' instead of acknowledging bytes the peer never received, which is strictly less permissive than before.
Level of scrutiny
Moderate. The refactor of on_writable is behavior-preserving (same sys::Error::from_code_int(..., Tag::write), same call_error_handler, same is_detached() guard before close(Normal)). The new on_open call site is the real change: it introduces a nested on_close from inside the open dispatch, which the file already handles for the open-error branch (the comment at the has_exception() check in on_close was updated accordingly). The &self borrow across JS re-entry follows the file's established pattern (internal_flush, write_or_end already do this; the R-2 note documents that all mutated fields are interior-mutable). The wrote >= -1 gate is scoped to the node:net buffered path only (buffer_unwritten_data is true only from write_or_end_buffered), and write_buffered already hands the negative errno to JS which schedules failWrite, so dropping the bytes is consistent with what JS reports. endBuffered is also affected but has no JS caller in src/js/node.
Other factors
No debug build was available in this environment, so the tests were not executed here; the PR itself defers platform-specific test proof to CI. Test structure follows the existing helpers in the same files (fd-scoped fault rules, once-based awaits, combined toEqual objects, await using for the spawned client). The busy-wait in the server test blocks the event loop deliberately (the whole point is to stay inside the listener) and is bounded with an explicit sawReset assertion. No CODEOWNERS entry covers the changed files. My prior inline note about the pre-existing double-'error' on fd-adopted sockets was replied to by the author and is tracked separately per the description; it is pre-existing and not introduced by this PR.
|
Independent check of this PR. I worked on the same report separately and did not see this PR until my diff was done. I am not opening a second PR. These are the results that apply here.
One possible addition: the new tests check the write callback error and the Probe for the third point (reset before accept, no fault injection)const net = require("node:net");
const { spawn } = require("node:child_process");
if (process.argv[2] === "client") {
const s = net.connect({ port: Number(process.argv[3]), host: "127.0.0.1" });
s.on("connect", () => s.resetAndDestroy());
s.on("error", () => {});
} else {
const events = [];
const server = net.createServer(sock => {
sock.on("error", e => events.push(`error ${e.code} ${e.syscall} | ${e.message}`));
sock.on("close", () => {
events.push("close");
console.log(JSON.stringify(events));
server.close();
});
sock.write(Buffer.alloc(1024, 0x41), err => events.push(`write cb ${err ? err.code + " | " + err.message : "ok"}`));
});
server.listen(0, "127.0.0.1", () => {
spawn(process.execPath, [__filename, "client", String(server.address().port)], { stdio: "inherit" });
// Block this loop turn so that the reset arrives while the connection is still in the accept backlog.
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 3000);
});
} |
This branch has not been deployed
2 participants
Problem
node:netwrite made inside a'connection'or'connect'listener can be reported as delivered when it was not. The write callback succeeds and'finish'fires, with no'error'. Repro: the listener writes 16 MB and the peer resets before the listener returns. Node fails the write withECONNRESET. Bun reports success with 2.6 MB sent.on_open(src/runtime/socket/socket_body.rs:1623):let _ = this.internal_flush();. On a fatalsend()errno,internal_flushdropsbuffered_data_for_node_netand returns the errno. The tail discarded it, saw the buffer empty, and dispatched the drain callback.Fix
on_writable(node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488) intofail_fatal_flush. Call it from the tail ofon_opentoo. It calls the error handler with awriteerrno, then closes the socket.write_or_endno longer queues the bytes of a write whose first send was fatal. JS fails that write, but the tail flush sent its bytes again.on_writabledocuments.test/js/node/net/net-syscall-fault.test.ts(6 new cases),test/js/node/net/node-net-server.test.ts(real peer reset, no injector), and the Node suite'stest-net-*,test-http-*,test-tls-*. Self-reviewed: 3 concerns raised, 3 addressed.Background
buffered_data_for_node_netholds the unsent rest of a shortsend(). JS parks the write callback until the native drain callback arrives.internal_flushsends that buffer again. It returns 0, or the errno of a fatal send (EPIPE,ECONNRESET).on_opencall. Its JSopenhandler emits'connection'or'connect'. A write made there is flushed when the handler returns.Notes
Real-kernel repro (no fault injection, no timers). The server's
'connection'listener writes 16 MB (a real short send), callsend(), then stays in the listener until the peer has reset the connection. The peer signals the reset through a marker file.bun mainis release1.4.3-canary.1+c6b7fcb5band a debug build of630e921db0. Both give the same result.Schedule matrix (no injection, the listener does not block). The writer queues 16 MB inside its
'connection'listener or'connect'callback. A node v26 peer checks every byte and resets at a set point: in its callback, after 0 to 40 ms, after 1 B to 12 MB read, N ms after its first data, or a close with unread data. A run fails when the peer got less than everything and the writer saw no error.The peer always received a prefix of the stream. A trace of a failing run on release main shows that the failing
send()is the second one, in the same loop iteration as the accept, with noepoll_pwaitin between. That send is the flush at the tail ofon_open. About 10 ms pass between the two sends while the 14 MB rest is copied into the buffer, so a reset that arrives in that window is enough. The listener does not have to block.Syscall-level repro (LD_PRELOAD shim on
send, accepted fd only).net.createServer(c => { 16 x c.write(4 KiB); c.end(); }), plan: send #2 short (1 byte), send #3EPIPE.Why the second change is needed. When the first send of a write is fatal,
write_maybe_corkedreturns-errnoand JS schedulesfailWriteon the next tick.write_or_endstill appended the whole chunk tobuffered_data_for_node_net, so the tail ofon_opensent it a second time. Two effects:ECONNRESET, the second send succeeds. The peer receives 200 bytes of a write that JS was told failed. The new testis not sent againcovers this.EPIPE, andfail_fatal_flushreports it beforefailWriteruns.'error'then carriesEPIPEwhile Node and main reportECONNRESET. Real-kernel check, reset before accept, two writes in the listener:Only
wrote < -1(a fatal send) skips the queue.-1is the legacy closed or rejected-TLS result, which JS treats as would-block. Its behavior does not change.Other callers of
internal_flushthat discard the errno.end_bufferedandendcall it only whenwrote == total, so the buffer is empty. The publicflush()has no socket-handle caller insrc/js. None of them dispatches drain.History. #33803 made
ENOBUFS/ENOMEMtransient. #32488 madeon_writablefail the write on POSIX. #33506 covered the same ground as #32488 and was closed as stale. The tail ofon_openwas the remaining caller.Overlap. #41785 changes the same
if buffer_unwritten_dataline towrote >= 0for a different bug (Bun.connectend(data)). The second PR to land needs a one-line conflict resolution.Not part of this change.
SocketHandlers.errorinnet.ts(the handler table fornet.connect({ fd }), which includes each connection a cluster worker accepts) fails the parked write callback and then also emits'error'directly. That gives two'error'events for one failed flush. It reproduces on main throughon_writable, so this PR does not change it. Tracked in #43030.Suites run on the debug build.
net-syscall-fault.test.ts(27 pass),node-net-server.test.ts(27 pass),socket-syscall-fault.test.ts,tls-syscall-fault.test.ts,node-net.test.ts, and the Node suite filestest-net-*.js(139),test-http-*.js(386),test-tls-*.js(186). Every failure there also fails on a debug build of main without this change (test-http-agent-keepalive,test-http-client-timeout-option,test-tls-client-allow-partial-trust-chain, two 1-byte-send timeouts innode-http-syscall-fault.test.ts, tenlocalhostcases innode-net.test.ts).cargo check --workspace --target x86_64-pc-windows-msvcpasses.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/node-net-server.test.ts, test/js/node/net/net-syscall-fault.test.ts