usockets: report a peer reset behind unread data as ECONNRESET, not 'end' - #39600
Conversation
… an end of stream An error-flagged poll event (EPOLLERR, or EV_EOF with a socket error) on a socket that did not read a FIN went through the orderly end path first. For node sockets that dispatched on_end before the error close; a TLS socket's on_end closes the socket itself with a clean code, so the error close never ran and a reset was reported as 'end'. The read loop also folded a recv() error that followed data into an end of stream. Close with the error in both cases. Only a FIN that recv() actually returned still delivers its end before the error close.
|
Warning Review limit reached
Next review available in: 18 minutes Limit details: You’ve used all 5 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fix pushed, waiting for CI. How it was reproduced: a Tests: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the eof/error dispatch ordering inside us_internal_dispatch_ready_poll — the hot path every socket in the runtime goes through, with distinct epoll/kqueue/AFD semantics — a human look is still worthwhile.
What was reviewed:
- Traced that the FIN-then-RST case the removed
read_anybranch protected is preserved:recv()==0now setsread_fin, soeof && error && !read_finis false and the end→error-close order is kept (test-net-error-twicedependency noted in the description). - Checked
read_finscoping: declared before the WRITABLE block and defaults to 0, so a paused socket (no READABLE inevents) correctly skips the end path on an error event. - Checked the new
eof=0guard sits after theeof_deferrablechecks (which are already!error-gated) and beforeif(eof && s), so thes==NULLcase is still guarded. - Test wires
error/connectErrorto reject, settles on the failing'end'too so it doesn't hang on regression, and gates the bytes-delivered assertion on!isWindows.
Extended reasoning...
Overview
The PR changes us_internal_dispatch_ready_poll in packages/bun-usockets/src/loop.c so that a peer RST arriving behind unread data is reported as a read error (ECONNRESET) instead of an orderly end-of-stream. Two mechanical changes: (1) the read loop no longer folds a recv() error that followed delivered data into EOF — it closes with the errno unconditionally; (2) a new read_fin flag distinguishes a real FIN (recv()==0) from the eof hint (EPOLLHUP / EV_EOF), and an error event whose read loop never saw a FIN skips the on_end path and goes straight to the existing error close. Four new tests in test/js/node/tls/node-tls-server.test.ts cover tls×net × paused×drained.
Security risks
None identified. This is TCP lifecycle-event routing; no input parsing, auth, or crypto is touched. The change makes error reporting stricter (a reset that was silently reported as 'end' now surfaces as ECONNRESET), which is fail-louder rather than fail-open.
Level of scrutiny
High. us_internal_dispatch_ready_poll is the single dispatch point for every non-UDP socket in Bun — net, tls, http, http2, websocket, fetch, valkey, sql clients all flow through it. The eof/error ordering here is exactly the kind of thing that has subtle downstream dependents (the description itself names test-net-error-twice and the teardownNoise logic in net.ts), and the semantics differ across epoll/kqueue/AFD. The removed read_any branch was itself a deliberate fix with a long justifying comment; the PR argues (convincingly, and with the read_fin flag) that the FIN-then-RST case it protected is preserved and only the data-then-RST-without-FIN case changes, but that reasoning deserves a maintainer's sign-off.
Other factors
The PR description is unusually thorough: it names the exact mechanism, explains why TLS lost the error where net kept it (us_internal_ssl_on_end closes with code 0), cites libuv's behavior as the reference, and reports running the ~1000 vendored net/tls/http/http2 tests plus the half-open-peer-reset fixtures on a debug build. The new tests are well-constructed (failure paths reject the awaited promise, the 'end' that must not happen also settles so a regression doesn't time out, Windows's discard-on-reset is branched). I found nothing wrong with the change, but core event-loop dispatch is not something to auto-approve.
|
For the human look the review asks for, this is what the two changed branches do per backend. epoll (Linux): a reset is reported as kqueue (macOS): a reset sets libuv (Windows): Unchanged on purpose: a reset on a paused socket is still reported at once and the unread tail is discarded, as before this change (Node would deliver the tail on resume). The close code still comes from |
…et as an end #39600 fixed that. The stub still runs under node in its own process: it has to read while this loop is blocked, and its report of the peer's close should not come from the socket code under test.
### Problem - `close()` on a `rediss://` client does not close when the peer has stopped reading. It returns with `connected` still true, no `onclose`, and every in-flight command pending. A peer that never reads keeps it that way for good. - Cause: `close()` asks for a TLS fast shutdown. If the socket still holds ciphertext the kernel will not take, usockets parks the shutdown behind that spill with no timer (`packages/bun-usockets/src/crypto/openssl.c`, `us_internal_ssl_close`). ### Fix - `close()` still asks for the fast shutdown. If the socket is still open when that returns, usockets deferred it. `close()` then closes again with the client-detected-failure code, which closes at once and sends an RST. - Correct because it detects the deferral instead of predicting it. usockets first tries to drain the spill, so a `close()` after a stall the peer has recovered from still ends in a FIN. Plain TCP never defers, so `redis://` is unchanged. - Visible change: `close()` on a stuck `rediss://` peer now returns with `connected` false, `onclose` fired, and each pending command rejected with `ERR_REDIS_CONNECTION_CLOSED`. - Verified: `test/js/valkey/reliability/connection-failures.test.ts`, three new tests. The stuck `rediss://` peer test fails on main. ### Background - A TLS fast shutdown closes without waiting for the peer's `close_notify`. usockets defers it only when the close carries no reason pointer. This client passes none. A comment at the call site says so. - The postgres and mysql clients have the same exposure. Not fixed here. - The durable fix is in usockets: bound the deferral with the socket timeout, or add a close code that skips it. Then this check can go. <details><summary>Notes</summary> History: one fix from a post-merge review of #39511, #39513 and #38281. It was stacked on #39546 (the `disconnect()` rewrite), which has merged. This branch is rebased onto main. The `duplicate()` fix has its own PR now, and the TLS context change moved to #39542. An earlier revision asked whether a spill existed and closed with an RST whenever it did. That would have cut short the recovered-peer case, where usockets can still drain. The current check does not. A comment in `node:net`'s `_handle.close()` path described the deferral as waiting only on our own fd. It is corrected. Behaviour there is unchanged. Test mechanics: - Stuck peer over `redis://`, against an in-process stub. The stub stops reading after HELLO. The client writes 256 KB values until two flushes in a row hand nothing to the socket. Then `close()` must settle everything at once, the stub must see `end` (not `ECONNRESET`) once it reads again, and `connect()` must open a second connection. - The two `rediss://` tests use a TLS stub run under Node in its own process, and skip when Node is not installed. It must be a separate process so it can read while the client's loop is blocked. It runs under Node so that its report of the peer's close does not come from the socket code under test. (When this was written, Bun's own sockets reported data followed by a reset as an orderly end. #39600 fixed that and is merged into this branch.) When told to read again, it writes a file once its byte count reaches what the client says it handed over, less one spilled batch, and has stopped growing. When the connection ends it writes one byte, to tell a FIN (the kernel takes it) from an RST (the kernel refuses it). - Stuck peer over `rediss://`: same stall. `close()` must settle everything at once. The stub, reading again afterwards, must find no `end` and a reset. Fails on the base branch: `close()` returns with `connected` still true. - Recovered peer over `rediss://`: same stall, then the stub drains while the client's loop is blocked, so the spill is still held when `close()` runs with no loop turn in between. The stub must see the rest of the data, an `end`, and its write taken. Passes on the base branch, and pins what the earlier revision would have changed. On macOS it cannot tell the two apart: the reset arrives while the stub's receive window is still shut and the kernel discards it. Linux accepts the reset and discards the unread data with it, so there the test should fail against the earlier revision. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/valkey/reliability/connection-failures.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
|
The two "reset that arrives while the socket is paused" tests hang on the darwin lanes: on kqueue a paused socket had no filter left to report the reset. Fix in #39610. |
### Problem - `test/js/node/tls/node-tls-server.test.ts` is red on every darwin lane since #39600: "reports the reset that arrives while the socket is paused as ECONNRESET, not 'end'" hangs to the timeout. Linux passes. - Cause: on kqueue a paused socket has no filter. `us_socket_pause` (`socket.c:864`) deletes the read filter, and the one-shot write filter it adds is consumed at once. The peer's RST is reported only after `resume()`. - Also found: `EV_ADD` on an existing knote keeps its flags (macOS 14 and 26). The read sentinel from #37077 stayed edge-triggered after the re-add on resume. ### Fix - `kqueue_change` (`epoll_kqueue.c`) keeps the read knote of a socket poll in both modes: level-triggered while the socket reads, `EV_CLEAR` while it does not. A mode switch deletes the knote and adds a new one. `us_poll_stop` deletes it in either mode. - The three hand-armed sentinel sites and the write filter added at zero events are removed. A delete of an unregistered filter is not a failure. - Correct because the dispatcher already handles these events: it masks the readable bit, defers a FIN while paused, and closes on a reset. After resume the knote is level-triggered again, so a partial drain cannot stall. - Verified: a six-scenario harness of the real `kqueue_change` on both macs (notes), the related suites on Linux, and new `Bun.listen` tcp/tls tests in `test/js/bun/net/socket.test.ts`. Linux behavior does not change, so this PR's darwin lanes prove the red test. ### Background - epoll reports HUP and ERR for a socket that polls nothing. libuv probes for a reset. kqueue reports nothing without a knote, and its write filter is one-shot since #25475. - An `EV_CLEAR` knote fires once per activation (data, FIN, RST). The dispatcher sees a FIN as eof and a reset as error (`fflags == ECONNRESET`). - `us_poll_resize` re-adds both filters only to move the udata. `EV_ADD` keeps the mode of an existing knote, so that still works. <details><summary>Notes</summary> Culprit: #39600 added the tests. The kqueue gap predates it: the write filter became one-shot in #25475, and the sentinel from #37077 covered shutdown and half-open but not pause. The rare darwin pass in CI was the RST landing before the one-shot write filter was consumed. kqueue probes, run as small C programs on darwin-arm64 (macOS 26.6, xnu 12377) and darwin-x64 (macOS 14.8, xnu 10063), identical results: - read filter deleted + one-shot write consumed, then RST: no event (the bug). RST while the one-shot is still armed: `EVFILT_WRITE` with `EV_EOF fflags=54` (the flaky pass). - `EV_ADD|EV_CLEAR` knote, then plain `EV_ADD`: still edge-triggered, udata updated. `EV_ADD|EV_CLEAR` over a level knote: still level. `EV_DELETE` + `EV_ADD` in one changelist: level again. - `EV_CLEAR` knote, RST behind 10 unread bytes: one event, `EV_EOF fflags=ECONNRESET`, `SO_ERROR=ECONNRESET`, no re-fire. FIN behind data: one event, `fflags=0`, no re-fire. More data while not reading: one wakeup per arrival. - `EV_CLEAR` knote registered before our own `SHUT_WR` still reports the peer's later FIN and RST, fresh or already cleared once. So dropping the post-shutdown re-arm in `raw_shutdown` is safe. - `KEVENT_FLAG_ERROR_EVENTS` with a failing delete first: the error entry keeps `EV_DELETE` in flags and `ENOENT` in data, and the following add still applies. The real `kqueue_change` body, compiled into a harness on both machines: pause then RST (reported, no spin), pause then FIN then resume (FIN deferred once, level-triggered after resume), stop on a paused socket (nothing left), resize touch (udata moved, mode kept), plain changes on a reading socket, EBADF still reported. The changed files also compile with `-fsyntax-only` in the kqueue configuration. Linux, debug build: `node-tls-server.test.ts` (73 pass, the SNICallback failure is pre-existing in this container), `node-net.test.ts`, `node-net-allowHalfOpen.test.js`, `fetch-backpressure.test.ts`, `node-http-backpressure.test.ts` (the same 15 pre-existing failures as unmodified main: localhost resolution and h3), `socket.test.ts` (the same 9 pre-existing failures as main, the 2 new tests pass), the nine `test-net-half-open-peer-reset-*.mjs` fixtures, `node-http-server-socket-end-drain`, `node-http-connect`, `tls-syscall-fault`, `net-syscall-fault`. The new `socket.test.ts` variants also fail on a release binary from before #39600 (the close carried no error), so they pin that contract for the Bun socket API on Linux as well. On macOS without this diff they hang like the node ones. Node itself does not report a reset while a socket is paused. libuv removes the fd from the poll set, and node v26.3.0 reports `ECONNRESET` after the resume (checked with the same server logic). Bun's epoll and libuv backends have reported it at once for a long time, and #39600 made that the tested contract. This PR only brings kqueue to the same contract. Related open PRs: #37098 rewrites the kqueue branch of `us_poll_resize` and would delete the read knote of a non-reading socket, which this rule keeps. #37099 touches the same pause code against an older base. `JSNodeHTTPServerSocketPrototype.cpp:226` pauses and resumes around a shutdown to work around the same gap. It still works and is left alone here. kqueue on FreeBSD (shimmed, not a CI target) stops a changelist at the first failing entry. The delete in a mode switch always finds a knote, because every socket poll starts with a level read filter, so the add after it is not affected. Windows lanes on the first CI run: the new tests reached the close with `syscall: "read"` but no `code`. The libuv backend reports the reset on a paused socket as intended, but on Windows the close carries the raw WSA code, which `on_close` stores unmapped (node:net accepts `code === undefined` as a reset, which is why the node tests pass there). That is a pre-existing bug in a different layer and is handed off separately. The tests check the code on POSIX only until it is fixed. </details>
…osed (#39621) ### Problem - Follow-up to #39600, found in its self-review. When a peer resets the connection behind data and the `data()` handler closes the socket (`terminate()`), a `Bun.connect()` opened from that socket's `close()` handler to a port nothing listens on reports `ECONNRESET` instead of `ECONNREFUSED`. Deterministic on Linux with the fd layout in the new test. Before #39600 it reported `ECONNREFUSED`. - Cause: the poll-error close in `us_internal_dispatch_ready_poll` (`packages/bun-usockets/src/loop.c:925`) runs for a socket that a handler closed earlier in the same dispatch. `us_socket_get_error()` then reads `SO_ERROR` from the closed socket's fd number. The connect opened from `close()` owns that number by then (it takes the lowest free fd), and `SO_ERROR` is cleared by the read. The connect's own dispatch later finds no error and falls back to `ECONNRESET` (`loop.c:495`). - Before #39600 the eof block returned early for a closed socket on this path. #39600 routes error events past the eof block on purpose, so the error block is where the check belongs. The same stale read already happened before #39600 when `on_end` closed the socket during an error event, or on an error event without an eof hint. ### Fix - The poll-error close is skipped when the socket is already closed: `if (error && s && !us_socket_is_closed(s))`. Nothing is left to close for such a socket, and `us_internal_socket_close_raw` was already a no-op for it. The only effect of the block was the stale `SO_ERROR` read. - The closed socket's memory is still valid here: `close_raw` puts it on the loop's closed list, which is freed after the dispatch. The eof block reads `is_closed` the same way. - Verified: `test/js/bun/net/socket.test.ts`, "a socket closed by data() while its peer's reset is being dispatched". It fails on a build of main (stdout `ECONNRESET`), passes with this change (25 of 25 runs), and passes on a build from before #39600. Also run: the rest of `socket.test.ts` (the same 9 failures as main in this container: `localhost` resolution and external network), the 4 reset tests in `node-tls-server.test.ts`, the nine `test-net-half-open-peer-reset-*` fixtures, `test-net-error-twice`, `test-net-socket-reset-send`, `test-net-socket-reset-twice`, `test-net-server-reset`, `test-net-connect-reset`, `test-tls-econnreset`. ### Background - Dispatch of one socket event: the read loop runs `data()` for each `recv()`, then the eof block handles a FIN, then the error block closes the socket with `SO_ERROR` when the event carried the error flag. A handler can close the socket at any point in between. `close()` handlers run synchronously inside that close, so code in them runs in the middle of the dispatch. - `SO_ERROR` is a destructive read: `getsockopt(SO_ERROR)` returns the pending error and clears it. Reading it through a stale fd number acts on whatever socket owns the number now. - fd reuse: a closed fd number is handed to the next `socket()` call if it is the lowest free one. The test reserves a number so that the accepted socket gets a lower number than the peer, because `peer.terminate()` frees the peer's number first. It runs in a child process so that nothing else in the process interferes with the numbering.
### Problem - `test/js/node/tls/node-tls-server.test.ts` "server socket whose peer resets the connection behind unread data" timed out on macOS (all 4 cases). - The test filled both kernel buffers, then called `terminate()`. macOS drops an RST that arrives at a zero receive window. The socket stayed ESTABLISHED. Node's `resetAndDestroy()` hangs the same way there, so that part is kernel behavior. - A smaller write exposed a Bun difference: `terminate()` on a TLS socket sent a close_notify alert before the RST. The server then correctly reported `'end'`. Node sends only the RST. ### Fix - `us_internal_ssl_close` with `LIBUS_SOCKET_CLOSE_CODE_CONNECTION_RESET` skips `SSL_shutdown` and raw-closes. Only the RST goes out, like node. - The test writes one 64 KB chunk so the receive window stays open. ### Verification - macOS debug build: full file 74 pass. `test/js/node/tls`, `test/js/node/net`, `test/js/bun/net`: 668 pass, 0 fail. - `test/js/node/http`, `test/js/bun/http`, `test/js/web/fetch`: remaining failures (valkey-gc worker teardown, two network fetch tests) fail the same on main. - With the `loop.c` hunks of #39600 reverted, the two "paused" cases fail (`["end"]`).
|
#39653 reshapes the four tests added here: macOS does not reliably deliver a reset to a socket whose buffer is full, which is why they stayed red on the darwin lanes. |
3 participants
Problem
tls.createServersocket whose peer resets the connection (RST) while this side still has unread data emits only'end'. Node emits'error'withcode: 'ECONNRESET'(read ECONNRESET) and never emits'end'. WithallowHalfOpen: truethe Bun socket then stays open forever. Found while working on valkey: close at once when a TLS fast shutdown is deferred #39548, whose TLS stub had to move into a Node child process because of this.netserver socket in the same situation emits'end'first. With the defaultallowHalfOpen: falsethe'end'destroys the socket and theECONNRESETis lost too. WithallowHalfOpen: trueit emits'end', then'error'.us_internal_dispatch_ready_poll(packages/bun-usockets/src/loop.c):loop.c:885(eof block): an event that carries the error flag (EPOLLERR next to EPOLLHUP) still takes the orderly end path first. For node sockets (alwaysallow_half_openat this layer) that dispatcheson_end, and only then doesloop.c:929close with the socket error. A TLS socket'son_end(us_internal_ssl_on_end) closes the socket itself with the clean code 0, so the error close atloop.c:929finds a closed socket and the error is gone.loop.c:825(read loop): arecv()error that followed data was folded into an end of stream (if (eof && read_any) break;) and went down the same end path. On Linux the kernel keeps the receive queue after a reset, sorecv()returns the data and thenECONNRESET. This is the normal shape of a reset behind unread data.Fix
recv()errno whether or not it delivered data first. Onlyrecv() == 0sets the newread_finflag.eof && error && !read_fin) skips the end path and goes straight to the existing error close, which reportsSO_ERROR.recv()did return on an error event keeps today's behavior:on_end, then the error close (test-net-error-twiceand theteardownNoiselogic innet.tsdepend on that order).read()returns the data and then the error, Node destroys the socket withread ECONNRESET, and'end'needs a real EOF. A FIN is the only thing that means end of stream. EPOLLHUP (and kqueue's EV_EOF with a nonzerofflags) is also set by a reset, so it cannot stand in for one once the error flag is present. The JS layer already turns a close that carries an errno intoread ECONNRESET(SocketEmitEndNTinsrc/js/node/net.ts), so no JS change is needed.test/js/node/tls/node-tls-server.test.ts, "server socket whose peer resets the connection behind unread data": 4 tests (tls and net, reset while paused and reset drained behind data). All 4 fail on main (they observe["end"]) and pass with this change, with both the release binary and a debug build of main.allowHalfOpen: falsenow also giveserror ECONNRESET, closefor tls and net (script run, not a test: it is the same native path).error ECONNRESET (read ECONNRESET),close hadError=truein all variants.test-net-*,test-tls-*,test-http-*,test-https-*andtest-http2-*files,test/js/node/tls,test/js/node/net,test/js/node/http,test/js/bun/net,test/js/bun/http,test/js/web/fetch, the ninetest-net-half-open-peer-reset-*fixtures andtest/js/valkey/reliability/connection-failures.test.tswere run on a debug build. Every remaining failure also fails on a debug build of main or on the release binary in this container (external network,localhostresolution, debug-build timing).Background
us_internal_dispatch_ready_pollreceives three things per socket event:events(readable, writable),error(EPOLLERR, or kqueue EV_EOF with a socket error infflags) and aneofhint (EPOLLHUP, which Linux sets once both directions are down, or kqueue EV_EOF). The read loop callsrecv()until it returns 0 (a FIN), an error, or EAGAIN. After the loop, the eof block dispatcheson_endand handles half-open, and the error block closes the socket withSO_ERROR.recv()returns it and then fails withECONNRESET. The poll reports readable, error and hangup at the same time. Windows discards the queue, sorecv()fails at once there. The test only asserts that data was delivered on POSIX.allow_half_openin usockets.net.tsimplements the JSallowHalfOpenoption on top. That is why a reset on a node socket went through the half-open branch of the eof block, which is the branch that dispatcheson_endand then falls through to the error close.on_end: for node TLS sockets,us_internal_ssl_on_enddoes not dispatch an end event. It closes the socket with code 0 andnet.tssynthesizes'end'from that clean close. That is why the lost error was visible on TLS sockets first: for plain socketson_endonly pushes EOF, and the error close that followed still reached JS in theallowHalfOpen: truecase.on_closereceives either a usockets close code (0 clean, 1 reset, 2 fast shutdown) or, for closes driven by the event loop, an errno.net.tsdestroys the socket withread ECONNRESETwhen the close carries an errno and an'error'listener exists.