Repository navigation
Conversation
The eight "parallel requests, single origin" cases ran as one concurrent group. All of their origins and proxies share the event loop of the test process, so the cases added to each other's TLS handshake queue. On a slow host with a debug build, the oldest handshakes waited longer than the 10 s that Bun.serve gives a connection to send its first request. The origin reset them and fetch() rejected with ECONNRESET. test.serial() keeps the 32 parallel requests of each case and removes the overlap between cases. The block takes the same time, because the servers of all cases run on one thread.
|
Updated 3:05 AM PT - Oct 21st, 2026
❌ @robobun, your commit 98769c0 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46115That installs a local version of the PR into your bun-46115 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughLow-priority TLS sockets now advance timeout stamps while queued and receive timeout processing at the queue limit or on spill-close. A testing API sweeps socket timers. New regression fixtures cover parked sockets, TLS bursts, and deferred accepts. ChangesTLS handshake queue timeouts
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: preventing timeout processing for sockets waiting in the TLS handshake queue. |
| Description check | |
The description provides a detailed problem statement, implementation summary, tradeoffs, scope, limitations, and extensive verification results. It does not use the exact template headings, but it co… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides a detailed problem statement, implementation summary, tradeoffs, scope, limitations, and extensive verification results. It does not use the exact template headings, but it covers both required areas: what the PR does and how it was verified.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Status Reproduced on Linux x64 with the official release binaries:
With this PR (#46115): 0 failed requests, 0 lost clients, and the unchanged stress test passes 4 of 4 runs. |
…andshake queue The loop reads at most 5 handshaking TLS sockets per iteration and parks the others with their reads off. Since #36403 the timeout sweep also walks the parked sockets and runs their owner's timeout. So a server that could not read a ClientHello within the 10 s that a connection has to send its request closed the socket over the unread bytes, and a client that had waited for the server got ECONNRESET. The sweep now moves the stamp of a parked socket on with each tick. The socket keeps the time it had left and the timeout comes due after the queue releases it. The accept path no longer parks a socket on the readable dispatch it synthesizes for a deferred accept: a peer that sends nothing stays linked, where the sweep closes it as before. The stress block that showed this runs its cases concurrently again, with half the requests per case on a debug build.
A client that the server neither answers nor closes now shows up in the summary as "no answer" and does not hang the fixture.
…it has input The accept path skipped the readable dispatch for every TLS accept over the handshake budget. A socket that arrived with its ClientHello then stayed linked and unread for one more loop iteration, with its timeout running, and was parked one iteration later than on main. The accept path now looks at the receive queue (one recv with MSG_PEEK per accept over the budget). A socket with input is parked at once, as on main, and its timeout stops there. A socket with no input stays linked, where the sweep closes it. Also states the rules of the queue in one comment.
Without a limit, the only things that take a socket out of the TLS handshake queue are the drain and a reset. A FIN is not seen there. So under a steady flood of new TLS connections the queue keeps the sockets of clients that left, and their number grows for as long as the flood lasts. The sweep now counts the ticks a socket spends in the queue. After 30 ticks (120 s, the handshakeTimeout of node) the socket's timeouts run again and its owner closes it.
With the fix in usockets the block needs no change to pass.
The Bun.listen fixture runs the sweep by hand: no sleep, and it also covers the wrap of the wheel, a socket with no timeout armed and the 120 s limit. The Bun.serve fixture stays on the real clock, takes its certificate and marker path from the environment and reports how many requests the server had answered when the last tick came due.
… at once Linux only: a connection without a deferred accept has no input yet when the accept path runs.
The three tests spawn up to two Bun processes each. In socket-syscall-fault.test.ts and bun-serve-ssl.test.ts they shared the CPU of a debug build with tests that rely on the default 5 s timeout. serve.test.ts, socket-syscall-fault.test.ts and bun-serve-ssl.test.ts are as on main again.
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline findings, two things were checked and ruled out: dropping the s->ssl ? ... : us_dispatch_timeout branch in the parked-socket walk is safe because the only park site (loop.c:680) is gated on s->ssl, so a non-SSL socket never enters low_prio_head; and deferring long_timeout only when global_tick % 15 == 0 matches the group walk's long_timestamp = (global_tick / 15) % 240 at loop.c:261, since the group's tick is advanced before the parked walk runs.
Extended reasoning...
The change touches the uSockets timer sweep and deferred-accept path in packages/bun-usockets/src/loop.c, adds a byte field to us_socket_t, and exposes us_internal_timer_sweep to JS through bun:internal-for-testing; it touches no auth, crypto or input-parsing surface. Four verified findings are posted inline and the hunt ended at its bug cap, so approval is not appropriate; this note only records the SSL-only park gate and the long-timeout cadence as checked and consistent.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| loop->data.low_prio_head = s; | ||
|
|
||
| flags->low_prio_state = 1; | ||
| s->low_prio_ticks = 0; |
There was a problem hiding this comment.
🔴 Under sustained TLS handshake overload, a client that trickles its handshake can hold a Bun.serve connection open past the 10 s idle timeout without bound, which the base branch closes. loop.c:716 zeroes low_prio_ticks on every park, so a socket the queue released, read once and parked again gets a fresh 120 s hold each time. MAX_LOW_PRIO_TICKS at loop.c:323 therefore bounds one park spell, not the socket. Fix: make the cap cumulative per socket, i.e. initialize low_prio_ticks at the socket init sites that set timeout = 255 (loop.c:565, socket.c:444, context.c:313) and stop resetting it at loop.c:716, so the owner's timeout runs after 30 held ticks in total.
Why this was flagged
A TLS server loop is over the 5-per-iteration handshake budget (loop.c:683) and iterates slowly enough that sweep ticks pass while sockets sit in the queue, the condition the PR itself targets. A client sends its handshake one record piece at a time. Its server socket is parked at loop.c:698-716 with low_prio_ticks = 0; each tick the sweep at loop.c:323-328 holds its stamps back instead of firing. When us_internal_handle_low_priority_sockets (loop.c:361-385) releases it, the next readable dispatch sets low_prio_state 0 at loop.c:682, reads one piece, and on the following piece the budget is spent again, so loop.c:716 parks it with low_prio_ticks reset to 0. The 30-tick cap at loop.c:323 restarts every cycle. On the base branch the parked walk fired the owner's timeout at its stamp, so HttpContext closed the connection at 10 s. The comment at loop.c:245-246 says the cap exists because past 120 s the peer has given up; it does not hold for a peer that keeps sending a byte per park spell.
Verification: loop.c:716 s->low_prio_ticks = 0; runs on every park, and the sweep at lines 323-328 pushes s->timeout forward one slot per tick, so each re-park restarts the 30-tick allowance. On the base branch an unfinished handshake was closed at ~10 s; after this change a client that trickles handshake pieces keeps its fd open for as long as the overload lasts.
| for (let tick = 0; tick < TICKS; tick++) { | ||
| Bun.sleepSync(TICK_MS); | ||
| await oneIteration(); | ||
| } |
There was a problem hiding this comment.
🟡 nit (optional): this fixture waits on the real clock for over 12 s, which CLAUDE.md's ~1 s per test budget and "never wait for time to pass in tests" rule reject. tls-serve-parked-burst-fixture.ts:74 calls Bun.sleepSync(TICK_MS) three times at 4100 ms so that the 4 s sweep fires between iterations. Fix: drive the ticks with sweepSocketTimers between the hand-run iterations, as tls-serve-deferred-accept-fixture.ts:82 already does for Bun.serve, so the test asserts the same answered: 40 without sleeping; if the real clock is load-bearing, say in one line what the hook cannot reproduce. [also at: test/js/bun/net/tls-handshake-queue-timeout.test.ts:73 - nit: explicit per-test timeouts, which test/CLAUDE.md forbids ("Do not set a timeout on tests"), sweep:^\s+(30|60)_000,$.]
Why this was flagged
test/js/bun/net/tls-handshake-queue-timeout.test.ts:46-57 runs tls-serve-parked-burst-fixture.ts, whose server loop at lines 73-76 blocks in Bun.sleepSync(TICK_MS) for TICKS = 3 iterations with TICK_MS = 4100, about 12.3 s of wall time plus the 40 TLS clients, and the PR text reports 14 s release and 18 s debug for the file. test/CLAUDE.md:21 says never wait for time to pass in tests and the root CLAUDE.md budget is about 1 s per test and 10 s per file; the repo review rules say a correct but slow test still gets changes-requested and that the workload should shrink rather than timeouts grow. The sweep is already exposed as sweepSocketTimers (src/js/internal-for-testing.ts:744) and the sibling fixture tls-serve-deferred-accept-fixture.ts:82 drives Bun.serve ticks with it, so no observable signal is missing. The 60_000 timeout at the test site exists only to cover this wait.
Verification: tls-serve-parked-burst-fixture.ts:73-76 is for (let tick = 0; tick < TICKS; tick++) { Bun.sleepSync(TICK_MS); await oneIteration(); } with TICK_MS = 4100 (line 28) and TICKS = 3 (line 29), i.e. ~12.3 s of blocking wall-clock sleep in the server process on every run. This violates test/CLAUDE.md:21 ("never wait for time to pass in tests"). Consequence is bounded: ~12 s of CI wall time per run, not a wrong result.
| answer = Promise.withResolvers<number>(); | ||
| socket.write("?"); | ||
| return answer.promise; | ||
| }; |
There was a problem hiding this comment.
🟡 nit (optional): if the client process dies or the control connection closes, this fixture hangs until the test runner kills it instead of printing an error. ask() at tls-serve-deferred-accept-fixture.ts:59-63 awaits answer.promise with no deadline and nothing rejects it on child exit or socket close, although the header at line 26 says a stalled step ends in an error. Fix: reject the pending answer from the control socket's close/error handlers and from child.exited, or race it against STEP_DEADLINE_MS like the loop at lines 76-80.
Why this was flagged
server() at tls-serve-deferred-accept-fixture.ts:86 and :88 awaits ask(), which writes "?" on the control socket and returns answer.promise (lines 59-63). The only resolver is the control socket's data handler at line 55-57. The control listener at lines 51-66 defines no close or error handler, and child.exited (line 68) is not wired to the promise. If the spawned client exits early or the control socket drops, the promise never settles and the fixture waits until the 30_000 timeout at tls-handshake-queue-timeout.test.ts:73 kills it, with no JSON summary and no error naming the stalled step. The comment at line 26 claims every stalled step ends in an error; the file-wait loop at lines 76-80 has a deadline but ask() does not. The repo rules require every failure event to reject the awaited promise.
Verification: nit. The spawned client process exits or the control connection drops before it answers a "?". In test/js/bun/net/tls-serve-deferred-accept-fixture.ts, ask (lines 59-63) returns answer.promise; the only settle site is the data handler (lines 55-57). The Bun.listen handlers (lines 54-65) define only data and open, so the test only ends via the 30_000 ms per-test timeout.
| s->long_timeout = 255; | ||
| us_dispatch_long_timeout(s); | ||
| if (loop_data->low_prio_iterator != s) continue; | ||
| if (!s->ssl_close_after_spill && s->low_prio_ticks < MAX_LOW_PRIO_TICKS) { |
There was a problem hiding this comment.
🟡 (optional) Operators of a TLS Bun.serve under sustained handshake overload now keep sockets whose peers already hung up for up to ~130 s instead of ~13 s, so the parked set grows about 10x before anything reaps it. loop.c:323 defers a parked socket's timeout for 30 ticks without checking that the peer is still there, and a parked socket has READABLE off (loop.c:687), so a client FIN raises no event. Fix: stop deferring once the peer is gone, e.g. peek with us_socket_queued_input at each sweep tick and take the dispatch branch when the result is not LIBUS_QUEUED_INPUT_DATA, so the 120 s grace covers only peers still waiting. The PR notes ~1200 fds at 12 clients/s; at ab -c 1000 rates the same ratio reaches fd limits.
Why this was flagged
A TLS Bun.serve loop stays over the 5-per-iteration handshake budget (loop.c:683) for a long stretch, as in the PR's own flood run. Each over-budget readable dispatch parks the socket at loop.c:686-716 with READABLE masked off, and the LIFO queue starves the oldest entries. Clients that give up close their socket and send a FIN; with reads off, epoll reports only EPOLLHUP|EPOLLERR for that fd (epoll_kqueue.c:742-746), and a half-close is neither, so the server never learns the peer left. On the base branch the sweep fires the owner's 10 s stamp for a parked socket on exact match, so HttpContext closes it at the next tick and the parked set is bounded by about 13 s of arrivals. With the change, loop.c:323-328 pushes the stamp forward on every tick for MAX_LOW_PRIO_TICKS = 30 ticks, so the same socket, its SSL object and its kernel buffers survive until roughly tick 33 (~130 s); the parked set is bounded by ~130 s of arrivals, ten times more.
Verification: Parking turns reads off (loop.c:687), and with neither R nor W the epoll mask is only EPOLLHUP | EPOLLERR (epoll_kqueue.c:742-746); a peer FIN alone raises no event. After this change loop.c:323-328 unconditionally defers the stamp for MAX_LOW_PRIO_TICKS, with no check of whether the peer is still there. A dead-peer parked socket therefore holds its fd and SSL state for up to ~130 s instead of ~13 s.
Merge before #45614, #45757 and #45758 (see the notes).
Problem
Bun.serveresets clients that wait for their handshake. Underab -r -n 40000 -c 1000, Bun 1.4.3 fails 1280 to 1605 requests. Bun 1.4.2 fails 0 to 19.packages/bun-usockets/src/loop.c:315). The 10 s request timeout closes a socket whose ClientHello nobody read.Fix
test/js/bun/net/tls-handshake-queue-timeout.test.ts(3 tests, each fails without the fix). Self-reviewed: 16 concerns raised, 13 addressed (notes).Background
HttpContext::onTimeout: a syscall per due socket, HTTP server only.Downsides
socket.timeout(n)on a parkedBun.listenTLS socket comes late by the parked time.recv(MSG_PEEK)per TLS accept over the budget, +4 instructions per deferred accept, +96 bytes.Notes
The rule. A socket in the TLS handshake queue has input that this loop did not read. Nothing acts on the socket over that input. A FIN does not end it (#44618, the EOF arm of
us_internal_dispatch_ready_poll). With this PR no timeout of its owner runs either, for at most 120 s. Only a socket with input is parked. The comment aboveMAX_LOW_PRIO_SOCKETS_PER_LOOP_ITERATIONstates the rule once.Merge order. #45614 keeps the timeout dispatch in the walk over parked sockets. It also stores the stamp in a different way, and it tests that the handler of a parked socket runs. #45757 and #45758 edit the same walk and the comment of the #36403 fixture. This PR changes the rule that they build on, so each needs a rebase after it.
sweepSocketTimersis commit 9dda2dc of #45614, cherry-picked.How it was found.
proxy-stress-concurrent.test.tsfails on a debug build on a slow host:32× parallel http-proxy → https-origin keepalive=falserejects withECONNRESET: The socket connection was closed unexpectedly. On an idle 12-vCPU VM 2 of 4 runs of the file fail. In a failing run the sweep closes 54 parked sockets, each with 1500 unread bytes (the ClientHello). With this PR the test is unchanged and passes. No user reported the resets, and CI did not show them (146 builds of main, 119566 to 128670). An earlier version of this PR made those casestest.serial. That change is gone.Reproduce. Linux x64. Official builds: the release binaries of 1.4.2 and 1.4.3, and Node 26.3.0. Built here: a release and a debug build of this PR, and a debug build of main
91e7e3b140.ab -r -n 40000 -c 1000, handler returns"ok": failed requestsnode:httpsECONNRESET30The clients of the second to fifth row are Node
tls.connectsockets in a second process. A lost client gotECONNRESETor a close with no answer, after about 13 s.Not covered.
node:netandnode:tlskeep their timeouts in JS timers (socket.timeout(0)insrc/js/node/net.ts). A socket timeout or thehandshakeTimeoutof atls.Serverstill runs while the socket is parked.Per backend. The accept-path change applies to listeners with a deferred accept:
Bun.serveandnode:http(s)over TCP on Linux and FreeBSD. macOS, Windows, unix sockets,Bun.listen,node:tlsand client sockets get the fix from the sweep alone. I ran everything on Linux x64 only.Cost. clang 23 with the release flags and no LTO,
loop.cof main against this PR. Syscalls are counted with anLD_PRELOADshim on debug builds.recv(MSG_PEEK).loop.o.text: 6200 to 6296 bytes.sizeof(struct us_socket_t)stays 80: the tick counter is in padding.epoll_ctl(MOD)465 to 469 becomes 0,recv5 to 6 becomes 20 to 40,recv(MSG_PEEK)0 becomes 260 to 280.Tests. All three are in
test/js/bun/net/tls-handshake-queue-timeout.test.ts: 14 s on a release build, 18 s on a debug build.answered: 10, ECONNRESET: 30. It is not driven by the hook: the sweep must find the sockets where a slow loop leaves them, between iterations that give the clients time to answer.Bun.listentest and the deferred-accept test run the sweep by hand. main has nosweepSocketTimers, so there they fail at the import. With main'sloop.cand the hook the summaries areoneTick: ["20,0"], parkedTooLong: [20, 0]andclosedWhileParked: 12.255guard givesunarmed: [0, 15]. No% 240adds"5,0"tooneTick. No 120 s limit givesparkedTooLong: [5, 0]. A stamp that is dropped and not moved givesoneTick: ["5,0"], which is also what 1.4.2 does. An accept path that skips every accept over the budget givesclosedWhileParked: 12.proxy-stress-concurrent.test.tson the debug build: 4 of 4 runs pass on the idle host (slowest case 10.1 to 12.7 s). Pinned to one core with two busy loops: main fails 5 of 5 runs withECONNRESET. This PR passes 5 of 5, and the slowest case takes 24.2 to 28.7 s of its 30 s limit.serve,bun-server,bun-serve-ssl,tls-keepalive,socket,socket-syscall-fault,tcp-server,tls-fatal-error-closes,tls-reject-before-client-cert,node-tls-server,node-tls-connect,tls-syscall-fault,renegotiation,node-tls-upgrade,node-tls-raw-end,serve-http2-lifecycle,proxyand the 7proxy-stressfiles. The failures are the ones that main's binary has on this machine: tests that need the internet or a non-root user, and tests that are flaky on a debug build, most by their 5 s limit.Self-review: not done.
global_tick % 15). On main a long-wheel timeout of aBun.listensocket does not reach its handler (usockets: store the socket timeout as a 32-bit due tick, sosocket.timeout()above 240 s fires #45614).fetchuses that wheel, but it runs on another thread's loop.ssl_close_after_spill). It does what main does. To reach it a test needs send faults. A comment incontext.csays that this state was never seen on a parked socket. It belongs to other code and is not changed.Alternatives.
HttpContext::onTimeout: see Background.Questions for a maintainer.
handshakeTimeoutof Node. A lower limit closes waiting clients sooner and holds fewer fds in a flood. The limit is commit e43a0ac, separate from the rest.socket.timeout(n)of aBun.listenTLS socket does not run while the socket is parked?