Bun.listen / Bun.connect: report a write error when a close loses the queued tail of end(data) - #44313
Conversation
|
Status: ready for review. Stacked on #41785 (the base of this PR is that PR's branch, at fe18122). How it was reproduced (linux-x64, release builds, 20 runs for each set of handlers):
The regression tests are in A connection that a read error closes (a reset that the loop reads) still gives that error to Not run by me: macOS and Windows. |
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 the re-entrancy ordering in on_close: loses_end_tail reads flags, the queue and the group mark before any JS entry, and handlers.on_close() is re-read after the error handler runs, so a reload() from that handler is respected. The closing_all byte lands in existing tail padding of us_socket_group_t, so the Rust size assertion and ABI stay intact, and the save/restore in us_socket_group_close_all_ex keeps the mark across re-entrant calls.
Extended reasoning...
The change touches usockets (us_socket_group_t gains a closing_all byte set during close_all), its Rust mirror, NewSocket::on_close/fail_write in socket_body.rs, the .d.ts/docs contract, and adds 33 tests. No security-sensitive surface (no auth, crypto, or parsing); it is a behavior change in close/error reporting for Bun.listen/Bun.connect. A confirmed inline finding (TLS ciphertext spill tail still unreported) plus the behavior change and stacked-PR base mean a human should weigh the contract.
us_socket_group_close_all_ex sets closing_all on the group for the time of the call. An on_close handler can then tell a close that the owner of the group started from a close that the loop or the peer started. The field takes one byte of the padding of us_socket_group_t.
end(data) queues what one send does not take and returns the size of the chunk. When the connection closed with the clean code while that tail was still queued, nothing reported it. NewSocket::on_close now reports that close as a write error, EPIPE with syscall write. The report uses the rule that fail_write already has for a failed send: the error handler gets it, and without an error handler the close handler gets it. report_write_error holds that rule for both. A close that the application starts reports nothing: close_and_detach and detach_for_reconnect empty the queue, close() uses another close code, and close_all marks its group. A failed TLS handshake stays with the handshake handler. Named pipes and upgraded duplexes are left out: their close has code 0 for every origin.
…te error The rows lose the tail by a shutdown() after end(data), by a TLS record that cannot be read, and by a peer that resets. They check the error handler first, the close handler without one, and an error handler without a close handler. terminate(), close(), listener.stop(true), a failed handshake, a slow TLS peer and a node:tls socket report nothing new. Each of those rows also checks that a lost tail is reported, so it fails on a build that never reports.
tcp.mdx and the SocketHandler comments in bun.d.ts say which handler gets the error, which calls discard the queued part, and what the report does not cover.
|
A note on how this PR meets #44300 (
The two merge with a text conflict on that line. The right order is: compute One more effect: with #44300, |
…r a queued tail The sentences said that every close over a queued end(data) tail gives a write error to the error handler. A connection that a read error closes (a reset that the loop reads) gives that error to the close handler and calls no error handler. State both cases in tcp.mdx and in the JSDoc of end(), close and error.
… TLS record The control of the rows that expect no report, the row with an error handler and no close handler, and the two reload() rows lost the tail by shutdown() after end(data). They now lose it by a TLS record that the peer makes unreadable, which no call of the socket under test starts. Only the eight shutdown() rows and the reconnect row depend on what shutdown() does with a queued tail.
015d112 to
f17532a
Compare
On macOS the rows that lose the tail by an unreadable TLS record closed with no error in some runs: over TCP the kernel took the whole queued tail before the record arrived, so the close had nothing queued. An AF_UNIX socket has small kernel buffers that do not grow, so the tail is still queued at the close. Windows keeps TCP.
|
Updated 4:15 PM PT - Sep 30th, 2026
✅ @robobun, your commit 938bd74ec6ec6ddb922357ca56a1918b8969e894 passed in 🧪 To try this PR locally: bunx bun-pr 44313That installs a local version of the PR into your bun-44313 --bun |
|
The one open item is the tail that only the TLS ciphertext spill holds. It stays a named limit of this PR, and the thread has the reason. Changes since that review:
|
Stacked on #41785.
Problem
socket.end(data)queues what one send does not take. If the connection closes with no read error, nothing reports the loss. TLS on a unix socket:end()returned 8388608, the peer got less,closegot no error.NewSocket::on_close(src/runtime/socket/socket_body.rs) reports only a read error or a failed send. A hangup gives the clean code, and TLS hides the errno of a failed send.Fix
on_closereports a clean close over a queued tail as a write error:EPIPE, syscallwrite.errorhandler gets it, orclosewithout one: the rule of Bun.listen / Bun.connect: send the whole end(data) chunk before the socket closes #41785 for a failed send.terminate(),close(), andclose_all(listener.stop(true)), which marks its group.test/js/bun/net/socket.test.ts, all fail on the base. Self-reviewed: 17 concerns raised, 14 addressed.Background
on_closea code: 0 (clean), 1 (reset), 2 (fast shutdown), or a read errno. On Linux a failedsend()takes the pending error, so the later hangup gives 0.Downsides
errororclosegets an error whereclosegotundefined, also forshutdown()afterend(data).closehandler,unref()with a queued tail, a tail that only TLS holds, Windows named pipes..text58146549 B,NewSocket504 B,us_socket_group_t88 B and syscalls per connection are unchanged.Notes
Order and scope
end()on main queues nothing, so this diff has no effect there.ECONNRESETon TCP), and this report stays for the closes where no send returned an errno. No test here depends on the code of a flush over TCP, so the rows do not change with tls: report a rejected send() as a write error instead of a clean close #42336.shutdown(), see below), usockets: ask the kernel before a stalled send after the peer's FIN closes the connection #43504 (us_socket_stalled_write_means_peer_goneon epoll and kqueue), socket: drain node:net's pending-write queue with a cursor #44219 (the queue type), node:tls: complete write(cb) and end(cb) over a Duplex when the stream completes the ciphertext #43877 (write completion over a Duplex), usockets: close a TLS socket whose send() keeps failing instead of spinning the writable dispatch #34510 (a stored send errno, where a maintainer asked whether to store it at all).A close with a read error
closegetsECONNRESETwith syscallread, anderroris not called, with or without a queued tail. This PR does not change that.end(16 MiB). Witherrorandclosehandlers:close ECONNRESET read, 15 of 15. Withcloseonly: the same, 15 of 15. Witherroronly: no handler runs, 15 of 15. The base gives the same 45 results.fail_write(Bun.listen / Bun.connect: send the whole end(data) chunk before the socket closes #41785) givesECONNRESETwith syscallwritetoerror: 25 of 25 on this branch with a peer that reads 64 KiB and then resets.errorhandler learns of the loss in the second shape only.tcp.mdxand the JSDoc ofend(),closeanderrorinbun.d.tsstate both cases.closehandler, give theerrorhandler the error of a read-error close over a queued tail. It is one more clause inon_close, and it changes what a read error does, so it needs a maintainer's decision.shutdown()afterend(data), and #44300shutdown()sends the FIN ahead of the queued tail. The tail is never sent, the peer answers the FIN, and the loop closes with the clean code. The base reports nothing. This PR reports the write error.shutdown()send the FIN after the queued bytes. That close then has no tail, andon_closereports nothing. The two rules do not conflict insrc/:loses_end_tailreads the queue at the close. socket: shutdown() sends the FIN after queued bytes; a close frees the queue and keeps bytesWritten #44300 also frees the queue inon_close, so that free has to come after the read.shutdown()does with a queued tail: the eightshutdown() after end(data)rows and the reconnect row. The other 24 lose the tail by a TLS record that cannot be read or by a peer reset. They hold under both rules.fail_write, and a reset that the loop reads is a read error. The report then applies to TLS.unref()end(16 MiB), thenunref(), with nothing else alive: the process exits with the tail queued, and the peer sees a clean close. No handler runs. Release builds, 40 runs each: the base lost data in 31 runs, this branch in 33. In the other runs the sends finished before the exit.node:netin Bun loses data the same way (3 of 3 on both builds).Not in this PR
eventing/libuv.c,crypto/openssl.c,HttpContext.h). No Windows machine was available, and a comment in the code keeps that path: "Until that detection is verified on Windows, keep the legacy contract there". So Windows keeps the base behaviour: a flush that gets a fatal errno drops the tail without a report.us_socket_raw_writefolds a failed send of TLS ciphertext to 0, so the wrapper never sees its errno. The report here saysEPIPE, also where the kernel saidECONNRESET(the twoflush()rows of the table).on_closesees nothing.us_socket_ssl_spill_pendingat close is one way to cover it. The JSDoc oferrorandclosenames this limit.upgradeTLS()afterend(data): the wrapper that is retired keeps its queue and gets no close.$write, never callend(data), and fail a queued write themselves (ERR_SOCKET_CLOSEDon a clean close, where Node reportswrite EPIPE).Repro (linux-x64, release builds)
end(8 MiB)from aBun.listensocket witherrorandclosehandlers. The peer reads one chunk and callsterminate(). Events afterend(). The table is from the first revision of this PR (base 6927ed3):closeerror EPIPE write,closeflush()after the resetcloseerror EPIPE write,closeclose ECONNRESET readclose ECONNRESET readflush()after the reseterror ECONNRESET write,closeerror EPIPE write,closeend(250000)closeclose(the limit above)The first row again on this revision (base fe18122), 20 runs for each set of handlers.
errorandclose: the base givesclose, this PR giveserror EPIPE write,close.closeonly:close, andclose EPIPE write.erroronly: no handler runs, anderror EPIPE write.When
on_closereportsThe close has code 0, the socket is a usockets socket,
end()was called, the handshake did not fail, the queue is not empty, and the group is not inclose_all.loop.c:916shutdown()afterend(data), or a TLS socket after a failed sendloop.c:947openssl.c:2413,:2538,:2547openssl.c:2528openssl.c:2162,:2305,:2316,:1914handshakehandler reports itopenssl.c:2157,:2358,:2470,:2771context.cclose_allwalk and low-priority drainlistener.stop(true), listener finalize, test isolation, VM teardownCloses that the socket starts:
terminate(), a rejected handshake and the end ofend()go throughclose_and_detach, which empties the queue.close()uses code 2. A reconnect goes throughdetach_for_reconnect.fail_writecloses after the flush emptied the queue.Tests
describeblock. With thesrc/andpackages/of the base: 33 fail (one of them by its timeout: it waits for anerrorevent that the base never sends). With this branch: 33 pass, in 9 runs of the block on the debug build and 5 on the release build.errorhandler, anerrorhandler without aclosehandler, and the tworeload()rows. This is also the control of every row that expects no report. These rows run over AF_UNIX on POSIX and over TCP on Windows. Over TCP on macOS they closed with no error in some CI runs: the kernel took the whole queued tail before the record arrived. The kernel buffers of an AF_UNIX socket are small and do not grow.shutdown()afterend(data), the same on every backend: TCP and TLS, both sides, with and without anerrorhandler, and the reconnect row.close ECONNRESET read), which the base gives too, so those rows do not tell this change from the base there.terminate(),close(),listener.stop(true), a failed handshake in five configurations, a slow TLS peer that gets every byte, a node:tls socket. Each of these rows also checks the control, so it fails on the base.codeclause, the TCPclose()row fails. NoEND_AFTER_FLUSHclause, the node:tls row fails. NoREJECTEDclause, three of the five handshake rows getEPIPE. No group mark, the TCPstop(true)row fails (the TLS one ends with code 1). Nois_usockets_backedclause, no row fails: it decides for named pipes and upgraded duplexes, whose close has code 0 for every origin.should not call drain before handshakeneeds the public internet.end(data) without an end handler keeps the process alive until the queued tail is sentis a test of Bun.listen / Bun.connect: send the whole end(data) chunk before the socket closes #41785 that timed out at 5 s. Both fail on a build of the base too. In the full run on the base build, two more tests of the base timed out at that load.end(data) without an end handler keeps the process alive until the queued tail is sentprinted itscloseline before itsfin readline. On macOS,end(data) whose tail is still queued when the peer's FIN is read > tls 1.2 > Bun.connect socket > allowHalfOpen: false > end(data) in data() with an end handlergotend()returning -2 with no request read.test/js/bun/net/{tcp-server,socket-syscall-fault}.test.ts,test/js/node/net/{node-net-allowHalfOpen.test.js,node-net-server.test.ts,node-net.test.ts},test/js/node/tls/{node-tls-socket-allow-half-open-option,node-tls-raw-end,node-tls-server,node-tls-connect,tls-syscall-fault}.test.ts,test/js/node/http/{node-http-server-socket-end-drain,node-http-backpressure}.test.ts,test/js/node/http2/node-http2.test.js, the ninetest-net-half-open-peer-reset-*.mjs, and three node parallel half-open scripts.node-net.test.tsfails the same 11 tests on a build of the base. On this revision:test/integration/bun-types/bun-types.test.ts, 21 pass.cargo check -p bun_runtimeforx86_64-pc-windows-msvc(this revision) andaarch64-apple-darwin(the first revision).Measurements (release builds, base = fe18122)
size -A: every section has the same size on both builds (.text58146549 B,.rodata19804908 B,.data59648 B,.bss1810040 B).nm -Son the unstripped binaries): the sum grows by 268 B over 23 symbols, while.textkeeps its size.on_close+279 B (TCP) and +327 B (TLS). The newreport_write_erroris 477 B.fail_writeis 461 B smaller for each of TCP and TLS, because it callsreport_write_error.us_socket_group_close_all_ex+2 B. The other 17 symbols change by 1 to 13 B.size_offrom the debug info:NewSocket<false>andNewSocket<true>504 B,SocketGroup88 B, before and after.SocketGroupis the Rust mirror ofus_socket_group_t, and a compile-time check holds the two to one size. The new byte takes padding.LD_PRELOADshim on the libc calls, (count at N=300 - count at N=100) / 200, sequential connections from a node peer. Nothing queued, TCP:send1,recv1,close1,setsockopt2,accept42,epoll_ctlADD 1, MOD 0, DEL 1, on both builds, three runs each. Nothing queued, TLS:send3,recv3, the rest as TCP, on both builds, three runs each.send2.13 to 2.65 on the base and 2.15 to 2.90 on this branch, four runs each.recv1,close1,epoll_ctlADD 1, MOD 1, DEL 1 on both. The difference insendis inside the spread between runs.loses_end_tail, three inon_close, and a second read of theclosecallback. No allocation and no syscall. Perclose_all: one byte load and two byte stores.perf,valgrind,bloatyorstraceon the machine, so there are no instruction counts.Self-review
errorfirst, shared withfail_write), anerrorhandler without aclosehandler, a handshake that reported by role (EPIPEin three of five configurations), the clauses of the guard that no test held, rows that are the same on every backend, the platform table in the test (Android is epoll), the limits inbun.d.tsandtcp.mdx, the named-pipe exclusion, and the five findings on the Windows draft (removed).shutdown()and a close after a TLS read failure report the tail. The data was not sent and the application did not close the socket.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/bun/net/socket.test.ts