Skip to content

node:http2: report a peer reset that a client write sees first - #42182

Open
robobun wants to merge 22 commits into
mainfrom
robobun/c91a08d7/http2-write-side-reset
Open

robobun wants to merge 22 commits into
mainfrom
robobun/c91a08d7/http2-write-side-reset

Conversation

@robobun

@robobun robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A node:http2 client whose write is the first operation to see a peer RST gets a clean close (rstCode 8, no 'error'). With only the socket alive, the process exits 0 silently.
  • note_transport_write_fatal (src/runtime/api/bun/h2_frame_parser.rs:3027) latches a rejected send() and drops the errno. The deferred tick then closes the transport with CloseCode::Normal.

Fix

  • A client session keeps the errno and closes the socket with it, like the loop after a failed recv(). Server sessions keep the plain close, as in Node.
  • Only an errno a read would return is reported, so EPIPE stays a plain close (macOS: the socket's own error).
  • Verified: nine new tests (node-http2.test.js, node-http2-syscall-fault.test.ts) and test/js/node/http2/.

Background

  • H2FrameParser is the native engine of an Http2Session. A deferred tick flushes its cork buffer and closes a dead transport.
  • A close code above 2 is the error that ended the connection. Rejected: an error dispatch before the close (the error arrives twice).

Downsides

  • ECONNRESET where Node closes cleanly, in two cases that need a maintainer's yes: a request made inside a read callback, and a reset behind an unread GOAWAY, which this close does not read (as on main).
  • Still hit: a write that overflows the cork buffer or is above one DATA frame keeps main's path, where the reset reads as a clean end. So does a peer that closes, then resets (EPIPE on linux). TLS has its own path (tls: consolidate the open TLS fixes (both engines, node:tls, node:https, WebSocket, SQL) #44618).
  • Cost: 4 bytes per session. A write that does not fail does no new work.
Notes

Where this differs from Node

Node reports a reset only when its read side sees it. It drops the status of a failed write: Http2Session::ClearOutgoing calls Done(0) under TODO(addaleax): Pass status instead of 0, so that we actually error out with the error from the write to the underlying protocol (src/node_http2.cc, v26.3.0). On linux the failed send() also consumes the socket error, so the read() that follows returns a clean EOF. Which side sees the reset first depends on where the request is made: Node defers a write made from a timer until after the next poll, and writes at once from inside a read callback. bun writes at once everywhere.

Since #40048 the connect flush sends the preface and then the queued HEADERS, so a reset at the preface hits the first request of a connection (the first row).

linux-x64, node v26.3.0, same fixtures as the tests, the socket is the only handle:

where the write that meets the reset is made node main @6b394bfe this branch
first request, peer resets at the preface ECONNRESET, rstCode 2 (3/3) no event, exit 0 (12/12) ECONNRESET, rstCode 2
idle session, loop busy, request() from a timer (the original report) ECONNRESET, rstCode 2 (8/8, one clean close in an earlier 3) no event, exit 0 ECONNRESET, rstCode 2
the same from a timer, with a body that overflows the cork buffer (16360 bytes) or is above one DATA frame (20000 bytes) ECONNRESET, rstCode 2 (12/13 for 20000 bytes, one clean close) clean close, rstCode 8 (16360 bytes). No event, exit 0 (20000 bytes) the same as main: not covered
idle session, loop busy, request() inside 'remoteSettings' clean close, rstCode 8 (3/3) clean close, rstCode 8 ECONNRESET, rstCode 2
server, response write fails silent, rstCode 0 silent, rstCode 0 silent, rstCode 0
first request, the peer closes at once (FIN, then RST for the client's data) clean close, no 'error' no event, exit 0 no event, exit 0. Clean close when a timer holds the loop

The 'remoteSettings' row is the divergence: bun reports a reset that Node, by its own TODO, means to report and does not. The request did fail, and a client that is told nothing cannot retry it. I did not measure Node on darwin or Windows. The injected-errno tests pin what bun reports there.

The last row is the next section. The third row is "Two changes that were taken out". The other rows were measured at earlier heads, and the tests pin them at this one.

A peer that closes first and then resets

A server that accepts a connection and closes it at once (net.createServer(s => s.destroy())) sends its FIN before the client's preface arrives. The kernel then answers the client's data with an RST. Measured on linux 6.1 with a raw socket: the second send() fails with EPIPE, SO_ERROR is empty after it, and recv() returns EOF. When recv() is the first call after the RST it also returns EOF, and the EPIPE stays in SO_ERROR. No read of that socket reports an error, so Node reports a clean close.

Earlier heads of this PR reported every EPIPE as ECONNRESET. The fold was there for darwin, where a send on a reset socket fails with EPIPE. On linux it reported an error that Node and the read side of the kernel do not have, and it moved the cells where main already equals Node (any other live handle) away from Node. Now EPIPE is a plain close on linux. macOS and FreeBSD fail every send on a disconnected socket with EPIPE and keep the cause as the socket error (sosendcheck returns EPIPE before it looks at so_error), and a read returns that error. There close_after_fatal_send reads SO_ERROR and reports it as it is. linux does not read it: after a send that failed with EPIPE, SO_ERROR can only hold a soft error (sk_err_soft), which a read does not return. I could not run macOS. Its CI lanes run the three real-reset tests, which fail if this reading is wrong. They pass there.

The client makes one request with 'error', 'end' and 'close' listeners and holds nothing but the socket. linux-x64, the client and the peer run the same binary:

build what the client is told
node v26.3.0 request end, request close, session close
1.4.3 canary 367d939 nothing at all, exit 0 (3/3)
this branch with the fold (merged with main @620b50f6) session error ECONNRESET, session close, request error ECONNRESET, request close (3/3)
this branch nothing at all, exit 0 (3/3)
this branch, a 1 s timer holds the loop request end, request close, session close (3/3)

The pipeline and for await forms of the same request agree with these rows. With the loop held open the branch prints Node's words in Node's order. The plain close of this case is made inside the deferred task queue, so its events are lost when the socket is the last handle. That is the delivery bug of "The silent exit" below, and #42485 owns it. This PR does not fix this case and does not change it.

Server sessions

The first version of this PR closed server sessions with the errno too. A raw client that sends one request and resets when the server's 'stream' handler starts made an h2c server with no 'error' listener exit with an uncaught ECONNRESET, 4/4, with no loop blocking: the response write fails, the session is destroyed with the error, and the still-open stream emits it. Node and main close quietly (stream, stream.close(rstCode=0), session.close). close_transport_after_fatal_write now passes the errno for !is_server only, and a server session whose response write fails closes quietly pins it (it fails on the earlier head with uncaught:ECONNRESET). A server with a stream still open dies with an uncaught ERR_SOCKET_CLOSED on main and on this branch alike. That is #42197's bug, and this PR does not change it.

From Sep 21 to head 690be7d the server seat moved a second time, through uncork(): see "Two changes that were taken out".

Two changes that were taken out

Two more changes were in this PR from Sep 21 until head 690be7d. Both are removed at b6a8fb0.

The latch on a failed writev(). A request body above one DATA frame leaves with its corked HEADERS in one writev() (flush_batch_vectored). On main us_socket_raw_writev returns 0 for a failed writev(), so the parser sees backpressure and no failure. The change reported the errno and latched it.

uncork() kept the deferred tick. A fatal send made while the cork buffer fills latches while the cork holds the auto-flush registration. The next flush() calls uncork(), which unregisters, so nothing closes the transport in the deferred tick. The change kept the registration while the latch was set.

Both made the close run in the deferred tick, before the loop's next read, in shapes where main leaves the socket to the loop. On main the loop then reads what is queued, and the close comes after it. A maintainer's test run against 690be7d found what that cost:

  • A response or a GOAWAY that was queued ahead of the peer's reset was dropped when the request body was above one DATA frame. Main, 1.4.2 and Node hear it.
  • The guard in uncork() did not ask whether the session is a client. A client that sent a POST and left ended an h2c server that calls server.close() in its handler, with an uncaught ERR_SOCKET_CLOSED. Main closes quietly there.

My runs of those two cases at b6a8fb0 (linux-x64, debug build): the queued response or GOAWAY is heard in 4 of 4 cases, with main's words (response 200, session error EBADF, ... and goaway 0, request end, request close, rstCode 8, session close), and 0 of 3 servers are ended.

So a write that overflows the cork buffer, or a body above one DATA frame, keeps main's path in this PR. From a timer with nothing queued: bodies of 0, 100 and 16340 bytes report ECONNRESET, rstCode 2. Bodies of 16360 and 16374 bytes end in a clean close. Bodies of 16375, 20000 and 70000 bytes print no event. To report the reset in those cases the close has to read the queue first and then carry the errno: see "Frames queued ahead of the reset are not read".

One face this PR does not cover: a small write on a stream that is already open

The stream is opened inside 'remoteSettings', so its HEADERS leave at once. After the loop was busy while the reset arrived, a timer calls req.end(Buffer.alloc(100)). The transport closes with the errno (us_socket_close(errno 104) in the trace), but the socket emits 'close' before 'error', so the session tears down cleanly first: stream.close(rstCode=8), session.close, no 'error'. The cause is the deferred task queue, not this change. The close runs inside that queue, net.ts emits 'error' from process.nextTick and 'close' from setImmediate, and no checkpoint follows the queue, so here the immediate runs first. A 20000-byte body takes main's path (see "Two changes that were taken out").

req.end(body) from a timer on an open stream node v26.3.0 canary 367d939 this branch this branch + #42485
100 bytes clean close 2/3, ECONNRESET 1/3 no event, exit 0 (3/3) clean close, rstCode 8 (3/3) ECONNRESET, rstCode 2 (at 53b7b94c)
20000 bytes clean close 2/3, ECONNRESET 1/3 clean close, rstCode 8 (3/3) clean close, rstCode 8 (3/3) not run at this head

Node is not deterministic here, so this is not a parity break. #42485 moves the close out of the deferred queue into an event loop task, and that fixes the order for the 100-byte case. I merged #42485 at 366003a into an earlier head of this branch (53b7b94c, which still had the two changes that are now removed): the merge was clean by text, and the tests of both PRs passed together. That pair was not built again at this head.

Frames queued ahead of the reset are not read

The loop reads everything that is queued before it closes a socket for an error (#39860). The close in the deferred tick does not: not the plain close on main, and not close_after_fatal_send. Measured on linux 6.1 with a raw socket: the peer writes 17 bytes and resets, the client's send() fails with ECONNRESET, and recv() then returns the 17 bytes and after them EOF. So where the deferred tick closes (a send that fits the cork), the frames that arrived ahead of the reset are dropped, on main and here. What this PR changes in that case is the report: ECONNRESET where main closes quietly, and an unread GOAWAY does not make it quiet through isEconnresetAfterGoaway in http2.ts. Node reads the frames first.

I tried to keep the plain close whenever bytes are queued (us_socket_queued_input). That is wrong the other way: in the main case of this PR a SETTINGS ACK is usually queued ahead of the reset, and the tests then report nothing. The right step is to read the queue and then close with the errno. That read cannot run where the close runs today: the deferred task queue also runs under on_native_read, where the loop's receive buffer and the parser are in use. #42485 moves the close to an event loop task, which has a clean stack, so the read belongs on top of it. With that in place the latch can cover the cork overflow and the writev() again.

Since this PR was opened, main gave the TLS engine that shape (#44618, #45124). A rejected send ends the writes only. The socket stays open while input is queued, and the close after the read carries the error (ssl_close_after_rejected_send in openssl.c). The parser no longer latches a TLS write: NativeSocketWrite for &TLSSocket clamps the result. Plain TCP could take the same shape, with the writable event as the clean stack in place of #42485's task. I have not built that, and this PR does not do it.

The silent exit

Traced with the H2FrameParser, uws and Socket debug scopes on main: the deferred close does run. us_socket_close(s, normal) dispatches on_close, the net.ts close handler takes the clean-EOF branch (finishSocketEnd: push(null) and unref()), and the only JS work that leaves behind is a process.nextTick. drain_microtasks_with_global (src/jsc/event_loop.rs) drains microtasks first and runs the deferred task queue after, so that tick is still queued when the loop finds nothing alive and exits. Closing with the errno takes destroy(err) instead, which registers a setImmediate for 'close' and holds the loop. So a close that carries an errno is delivered, as a side effect and not at the layer that owns the bug. A plain close made inside the deferred queue still waits for the next loop activity: a server session, and a client whose send failed with EPIPE on a socket that holds no error. The layer fix (one more checkpoint after the deferred queue, or running the close as an event loop task the way FileSink::run_pending_later does) is independent of this PR and is tracked separately. Control: a peer that sends a real FIN at the preface delivers every event on main.

How the failure reaches JS

Nothing new decides it. NewSocket::close_after_fatal_send closes with the errno (us_socket_close now takes the code as an int in Rust, as it already does in C), on_close turns a code above the CloseCode range into the error, the net.ts close handlers shape it like Node's read ECONNRESET and keep their teardown-noise rules, and the session takes its existing transport-error path. dead_transport_close_code gives a close code only for an errno that a read of the socket would also return: ECONNRESET, ECONNABORTED, ENOTCONN, ETIMEDOUT, ENETDOWN, ENETUNREACH, EHOSTUNREACH. That is the peer-gone set of us_socket_write_check_error without EPIPE, and each keeps its identity. On Windows the code is a WSA code, and read_error_from_close_code reports WSAECONNABORTED as ECONNRESET, as it does for a failed read. EPIPE is not in the set: it is a plain close on linux, and on macOS and FreeBSD the error the socket holds is reported in its place, so a reset, a timeout or an unreachable host keeps its code there. Every other send errno, for example EPROTOTYPE after the bounded retry window, keeps the plain close it has on main (sustained errno in test/js/bun/net/socket-syscall-fault.test.ts pins that). An earlier head closed with every latched errno. A client then reported read EPROTOTYPE, and whether that test saw it depended on the order of the socket's 'error' and 'close' events. The node:net write path is untouched. An error dispatch followed by a close was the other option: it delivers the socket error twice and skips the teardownNoise rule in SocketEmitEndNT.

The session 'error' is emitted before the stream's, where Node emits the stream's first. That order is not specific to this path: a read-side RST with an open stream already reports session.error, session.close, stream.error, stream.close on 1.4.3.

Self-review

The push of 382ca9d (merge with main, the EPIPE rule): 8 concerns raised, 5 survived. Three were fixed: the socket error behind an EPIPE is read only on macOS and FreeBSD and is reported as it is, the comments that state the close-code contract name the new producer, and ECONNABORTED has a test. One is a stated limit of a test: the injected EPIPE case proves that no close with an errno was made and does not prove delivery. One was the missing read of the queue. It was filed as a Downside, and one day later a maintainer's test run showed that it was a regression against main for large bodies. That is why the two changes are out.

The push of 48d3419 (the two changes that are now removed): two concerns were raised and neither survived. Neither review asked what main does with the queue in those shapes.

Tests

node-http2.test.js, describe "a client session reports a peer reset that one of its writes sees first": a busy idle session with request() from a timer, the same with request() inside 'remoteSettings', and a reset at the preface. The client blocks in fs.readSync(0) and the parent writes that byte only after resetAndDestroy() returns, so the RST is always there before the request is written.

node-http2-syscall-fault.test.ts, describe "node:http2 transport write errors": ECONNRESET, ECONNABORTED and ETIMEDOUT injected on a later request (each keeps its identity), ECONNRESET on the second send of the connect flush, an injected EPIPE, and the quiet server. The EPIPE case checks that no 'error' is reported. It proves that no close with an errno was made, and it does not prove delivery: in that child no event arrives before the process exits, which is the gap #42485 closes. The faulted request is made from a zero timer and not inside 'remoteSettings': a plain close made under a read reports EBADF on main (4 of 4 runs of the EPIPE case), which is also #42485's.

Seven client cases fail on main's src/ and pass here. The EPIPE case and the server case pass both ways: they pin what must not be reported. With the EPIPE fold restored the EPIPE case reports ECONNRESET. No client starts a timer to wait and each reports at process.on('exit'), so every case that expects an error also checks that the events arrive before the process exits.

Related open PRs

Suites

linux-x64, debug build, head 341384f, merged with main @c69a636b. test/js/node/http2/: 625 pass, 0 fail, 6 skip, 2 todo. test/js/bun/net/socket-syscall-fault.test.ts and test/js/node/tls/tls-syscall-fault.test.ts: 58 pass. All with --timeout 60000: a debug child needs about 3 s to load node:http2 in my container, so the subprocess tests of that directory exceed the default 5 s there (the existing ones and the nine new ones). bun check reports no type error in the two test files. Not run at this head: Node's test-http2-* files (261 passed at 53b7b94c), Windows and macOS, and rust:check-all (it passed for Windows, macOS and FreeBSD at 382ca9d, and the code that differs per platform is unchanged since).

Cost: the parser gets one Cell<i32>. socket.c and libusockets.h are main's. The send path has no new branch: the errno is stored where the latch is set, and it is read where the transport is closed.


no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-syscall-fault.test.ts

The h2 frame parser latched a flag on a fatal send() and dropped the
errno, then closed the transport with CloseCode::Normal on the next
deferred tick. That close reaches JS as a clean EOF, and it runs before
the loop can poll the read side, so a request whose HEADERS write hit
the peer's RST ended with no 'error' and no 'response'.

Keep the errno and close the socket with it, which is how the uSockets
loop closes a socket whose recv() failed. The close handlers in net.ts
then shape it like Node's read ECONNRESET, and the session takes its
existing transport-error path: ECONNRESET on the session and on the
stream, with rstCode INTERNAL_ERROR.
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The change preserves fatal transport write errors through HTTP/2 handling, normalizes peer-reset send errors to ECONNRESET, closes established sockets with the error code, and adds regression coverage for connection and request flushing.

Changes

HTTP/2 transport error handling

Layer / File(s) Summary
Socket error-code closure
src/runtime/socket/socket_body.rs, src/uws_sys/socket.rs, src/uws_sys/us_socket_t.rs
Socket closure APIs now accept error codes. Fatal send errors are normalized on POSIX and reported for connected sockets.
Parser fatal-write tracking
src/runtime/api/bun/h2_frame_parser.rs
The parser stores fatal write results, passes them to deferred socket closure, and clears the stored errno after recovery.
Transport error regression coverage
test/js/node/http2/node-http2-syscall-fault.test.ts, test/js/node/http2/node-http2.test.js
Tests cover injected send failures and peer resets during connection and request writes. They verify ECONNRESET, destruction, reset codes, clean stderr, and successful exit.

Suggested reviewers: jarred-sumner, cirospaciari

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 39fb7

The HTTP/2 behavior change is covered, but the subprocess fixture should follow the repository’s import convention before merge.

Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: reporting peer resets detected first during an HTTP/2 client write.
Description check Passed The description explains the problem, fix, scope, limitations, and verification results. It does not use the exact template headings, but it provides the required information and is substantially comp…

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@robobun

robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Two faces of one bug, both reproduced on main and both fixed by this branch for client sessions. Two more cases are listed under "Not covered": this branch leaves them as they are on main.

An idle session whose loop was busy while the reset arrived (1.4.3 and main, 3/3 on linux-x64 and on Windows x64). The peer writes the HTTP/2 frames by hand, sends an RST while the client's event loop is blocked, and the client then calls session.request() from a timer:

bun:  request() returned | stream.end | stream.close(rstCode=8) | session.close
node: request() returned | stream.error:ECONNRESET | stream.close(rstCode=2) | session.error:ECONNRESET | session.close

The first request of a connection that the peer resets at the preface (main, which has #40048). The connect flush sends the preface and then the queued HEADERS, and the RST lands between the two sends. With the socket as the only handle, 12/12 on a debug build:

bun:  exit=0 session.connect || at exit: session.destroyed=false stream.destroyed=false
node: exit=0 session.connect | stream.error:ECONNRESET | stream.close(rstCode=2) | session.error:ECONNRESET | session.close

In both, a send() is the first operation to see the reset, and no FIN came before it. It returns -ECONNRESET, the h2 parser drops the errno and closes the transport with the clean close code, and that close runs before the loop can poll the read side.

With this change a client session and its request report ECONNRESET, the stream's rstCode is 2, and both are destroyed before the process exits. Server sessions are unchanged.

Two things a maintainer should know before merging, both in the PR notes:

  • An earlier head closed server sessions with the errno too. A client that resets after its request then ended an h2c server that has no 'error' listener, 4/4. The errno close is now for client sessions only, and a test pins the quiet server.

  • In one ordering (a request() made inside 'remoteSettings', which Node emits from a read callback) Node reports a clean close and this branch reports ECONNRESET. Node drops the status of a failed write under a TODO in ClearOutgoing. The notes have the table for each ordering.

  • Fix: node:http2: report a peer reset that a client write sees first #42182

  • Not covered, taken out again at b6a8fb0: a write that overflows the 16 KiB cork buffer, and a body above one DATA frame. Two changes covered them from Sep 21 to head 690be7d (a latch on a failed writev(), and uncork() keeping the deferred tick). Both made the close run before the loop's next read where main lets the loop read first. A maintainer's test run found that a response or a GOAWAY queued ahead of the reset was then dropped for large bodies, and that an h2c server which calls server.close() in its handler was ended by a client that had left. At b6a8fb0 those two cases print main's lines again (my runs: the queued answer is heard in 4 of 4 cases, 0 of 3 servers ended). From a timer with nothing queued, bodies of 0, 100 and 16340 bytes report ECONNRESET. 16360 and 16374 bytes end in a clean close and 16375 bytes and up print no event, as on main.

  • Not covered, on purpose: a server that accepts and closes at once (net.createServer(s => s.destroy())). Its FIN comes first, so on linux the client's send fails with EPIPE, a read of that socket is a clean EOF, and Node reports a clean close (request end, request close, session close). An earlier head reported ECONNRESET there. The branch now keeps the plain close, as Node does: it prints nothing when the socket is the last handle (as main does, 3 of 3) and Node's exact words when a timer holds the loop. The delivery of that close is node:http2: close the transport after a fatal send from an event loop task #42485's change. With node:http2: close the transport after a fatal send from an event loop task #42485 at 366003a merged into this branch the case prints Node's three lines in 3 of 3 runs, and the reset cases above still report ECONNRESET.

  • Not covered: frames that are queued ahead of the reset (a GOAWAY, the end of a response). The close after a failed send does not read them, as on main, so an unread GOAWAY does not make the reset quiet. The read needs a clean stack, which node:http2: close the transport after a fatal send from an event loop task #42485's task gives. Main now does this read for TLS inside the TLS engine (tls: consolidate the open TLS fixes (both engines, node:tls, node:https, WebSocket, SQL) #44618, node:tls: keep the peer's answer when a pending write is rejected #45124), and plain TCP could take the same shape. The PR notes have the details.

  • Not covered: a small req.end(body) on a stream that is already open still ends in a clean close. The transport closes with the errno, but the close runs inside the deferred task queue, and there the socket emits 'close' before 'error'. Node is not deterministic in that case either (clean close 2 of 3). node:http2: close the transport after a fatal send from an event loop task #42485 fixes the order. Merged locally on top of this branch, every cell reports ECONNRESET and both PRs' tests pass. The PR notes have the table.

  • Tests: test/js/node/http2/node-http2.test.js, the three cases under "a client session reports a peer reset that one of its writes sees first". test/js/node/http2/node-http2-syscall-fault.test.ts, the six cases under "node:http2 transport write errors". Seven client cases fail on main's src/ and pass on this branch. Two pass both ways and pin what must not be reported: an injected EPIPE, and the quiet server. In the real-reset cases the client blocks on stdin until the parent has reset the connection, so the order does not depend on timing.

Comment thread test/js/node/http2/node-http2.test.js Outdated
One peer reset gives a different send errno per platform: linux reports
ECONNRESET, darwin reports EPIPE. The read side reports ECONNRESET on
every platform, and that is the code node surfaces for a peer that
vanished, so close the dead transport with ECONNRESET for the peer-gone
errnos that mean the same thing. Every other errno keeps its identity.

The new test injects each errno through the socket fault harness, so the
code the session and the stream report is pinned on every platform.
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
@robobun

robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:10 PM PT - Oct 13th, 2026

✅ @robobun, your commit 341384fa3d6d7df2c84d7b9707f20c2659cb7d33 passed in Build #125267! 🎉


🧪   To try this PR locally:

bunx bun-pr 42182

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

bun-42182 --bun

Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/uws_sys/socket.rs Outdated
Comment thread src/uws_sys/us_socket_t.rs Outdated
Comment thread src/uws_sys/us_socket_t.rs Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/uws_sys/socket.rs
Comment thread src/uws_sys/us_socket_t.rs
@robobun

robobun commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Update after the darwin CI lane.

macOS returns EPIPE, not ECONNRESET, from the first send() after a peer RST (XNU checks SS_CANTSENDMORE before so_error), so the first version of this change reported EPIPE on the session and the stream there. Node reports ECONNRESET for this failure on every platform, because it learns of the reset from the read side, which reports one code everywhere.

So a dead transport now closes with ECONNRESET for the peer-gone errnos that mean the same thing (EPIPE, ECONNABORTED). Any other errno (ETIMEDOUT, EHOSTUNREACH, ...) keeps its identity, and the node:net write path is untouched, so a failed socket.write() still reports the platform's own errno.

Both errnos are pinned on every platform now: test/js/node/http2/node-http2-syscall-fault.test.ts injects each one through the socket fault harness and asserts the session and stream report ECONNRESET with rstCode 2.

Also shortened the doc comments the comment linter flagged.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Since the client preface waits for 'connect', the connect flush sends the
preface and then the queued request's HEADERS back to back. A peer that
resets at the preface can land its RST between the two sends, so the
first request of a fresh connection takes the fatal-send path. Before
this branch that closed the transport as a clean EOF from the deferred
tick. The only JS work that leaves behind is a process.nextTick, which
the deferred task queue runs after the microtask drain, so with the
socket as the only handle the process exited with no event at all.

Add that case with a real reset and with an injected errno on the second
send. The clients start no timer and report when the process exits on
its own, so every case also checks that the events arrive before exit.

Drop a sentence from a comment: errno 1 never reaches
close_after_fatal_send, because a -1 write result is not latched.
…te that fills the cork

us_socket_raw_writev returned 0 for a failed writev(), so a request body
above one DATA frame never reached note_transport_write_fatal. It now
reports the errno like us_socket_write_check_error. Both use one
classifier, us_internal_classify_failed_send.

A fatal send made while the cork buffer fills latches while the cork
holds the auto-flush registration. uncork() then cancelled the deferred
tick that closes the transport, and the later failing send did not
register again. uncork() now keeps the registration while a fatal write
is latched.

Tests: the busy client blocks on stdin until the parent has reset the
connection, in place of a fixed wait. New cases: a request body above
one frame, an injected errno on a body that fills the cork, and an
injected errno on the writev().
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs Outdated
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/uws_sys/socket.rs Outdated
Comment thread src/uws_sys/us_socket_t.rs Outdated
@robobun

robobun commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

About the vectored write, the finding outside the diff in the last review: 48d3419 addresses it. I measured the previous head again after I posted this comment, and the first version of it was wrong about what that head did. This is the corrected account.

  • At 1e4763c a real reset on the vectored path did not end in a clean close. us_socket_raw_writev returned 0, the next flush() sent the buffered bytes with send(), that failed EPIPE and latched, and the close reported ECONNRESET (the EPIPE fold). What that path lost was the errno of the writev() itself, for example ETIMEDOUT. us_socket_raw_writev now reports it through a fatal_write_error out-parameter, like us_socket_write_check_error, and flush_batch_vectored latches it. Both C functions use one classifier, us_internal_classify_failed_send (the inline block, moved and not copied).
  • The clean close was in a different place. A fatal send made while the cork buffer fills latches while the cork still holds the auto-flush registration. The next flush() calls uncork(), which unregistered. The send after it failed too, but the latch was already set, so nothing registered again and nothing closed the transport. That is a clean close on linux, and a hang when only the write side sees the failure. At 1e4763c this hit a request body that overflows the 16 KiB cork in one frame: 16340 bytes reported ECONNRESET, 16360 and 16374 a clean close, 16375 and up ECONNRESET again. Any burst of writes that overflows the cork in one tick takes the same path. uncork() now keeps the registration while a fatal write is latched.
  • At this head, request bodies of 0, 100, 16374, 16375, 20000 and 70000 bytes from a timer all report ECONNRESET, rstCode 2.
  • New cases: a real reset with a 20000-byte body, an injected ECONNRESET on a 16374-byte body, and an injected ETIMEDOUT on the writev() with every later send() failing EPIPE, as the kernel does. Without the uncork() change the two injected cases time out and the real 20000-byte case reports a clean close. Without the latch in flush_batch_vectored the writev case reports ECONNRESET where the injected errno is ETIMEDOUT.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/node/http2/node-http2-syscall-fault.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

…2-write-side-reset

Conflicts: main added us_socket_writev, a caller of us_socket_raw_writev,
and this branch added the fatal_write_error parameter to the latter.
us_socket_writev passes NULL. A caller that passes NULL keeps main's
behaviour: the failed writev is not classified and writable is re-armed.
socket_body.rs: both sides added a function at the same place.
EPIPE names no cause. On linux a send fails with it when the peer sent
its FIN first and then reset the data that followed. The failed send
takes the socket error, and a read of that socket is a clean EOF. Node
reports a clean close there. The earlier fold reported ECONNRESET.

macOS and FreeBSD fail every send on a disconnected socket with EPIPE
and keep the cause as the socket error, which is what a read returns.
There the socket error is read and reported as it is, so a reset is
still reported. linux does not read it: after such a send SO_ERROR can
only hold a soft error, which a read does not return.

ECONNABORTED keeps its identity on POSIX, as a read reports it.

The comments that state the close-code contract name
close_after_fatal_send, and its doc says that it does not read what is
queued ahead of the error.

Tests: ECONNABORTED is reported as itself, and an injected EPIPE
reports no error. The subprocess cases take a longer timeout on a
debug build, where each child needs several seconds to load
node:http2.
Comment thread src/js/node/net.ts Outdated
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/socket/socket_body.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/js/node/http2/node-http2-syscall-fault.test.ts Outdated
Comment thread src/runtime/api/bun/h2_frame_parser.rs
Comment thread src/runtime/api/bun/h2_frame_parser.rs
test/CLAUDE.md asks for the runner's default. The cases need about one
second on a release build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Both made the close after a fatal send run before the loop's next read
in shapes where main lets the loop read first. A response or a GOAWAY
that was queued ahead of the peer's reset was then dropped when the
request body was above one DATA frame. An h2c server that calls
server.close() in its handler ended with an uncaught ERR_SOCKET_CLOSED
when the client had already left.

us_socket_raw_writev, flush_batch_vectored and uncork() are main's
again. A client session still closes with the errno where main already
closes in the deferred tick. The other cases need the close to read the
queue first.

Tests: the cases for a body that fills the cork buffer, for a body
above one frame and for an injected writev error go with the code they
pinned.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/socket/socket_body.rs
…2-write-side-reset

# Conflicts:
#	src/uws_sys/socket.rs
#	test/js/node/http2/node-http2.test.js
Comment thread src/js/node/net.ts
Comment thread src/runtime/socket/socket_body.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant