Repository navigation
usockets: keep a socket paused when the TLS handshake queue drains it - #42351
Conversation
us_socket_pause on a socket parked in the low-priority handshake queue finds the reads already off and only sets is_paused. The queue drain switched the reads back on without looking at the flag, so the handshake ran and the owner got handshake and data callbacks on a socket whose pause() had returned. The drain now relinks a paused socket into its group and leaves its reads off, with low_prio_state 0: it is an ordinary paused socket again. us_socket_resume arms the reads and the readable dispatch gates it like any other handshaking socket.
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 8 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Status
|
![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 uSockets low-priority TLS handshake queue state machine — where low_prio_state, group linkage, and poll events must stay consistent for close/resume/timeout — a human look is still worthwhile.
What was reviewed:
- State after the new paused branch: unlinked from
low_prio_head,low_prio_count--, re-linked to group,low_prio_state=0, reads stay off —us_socket_resumere-arms and the readable dispatch re-gates via the budget. - Budget accounting: paused sockets still consume one drain slot per iteration, matching the closed-socket branch above it; queue is LIFO so nothing starves.
- Sibling re-arm sites (
socket.c:415, the "already parked" arm atloop.c:634) already respectis_paused; this drain was the remaining gap. - Test: polls on
getEventLoopStats().iterationwith a deadline (no sleeps),port: 0, assertswhilePausedis zero and all 32 come back after resume; the exactclientHandshakesWhilePaused: 5depends on shared-loop iteration timing and is not Windows-skipped, so worth a CI check on the libuv backend.
Extended reasoning...
Overview
The PR adds a 7-line guard to us_internal_handle_low_priority_sockets in packages/bun-usockets/src/loop.c: when draining a parked TLS-handshake socket back into its group, it now checks s->flags.is_paused and, if set, clears low_prio_state to 0 and skips the us_poll_change(... | LIBUS_SOCKET_READABLE) call. This prevents the drain from silently re-arming reads on a socket whose pause() had already returned. A new test.concurrent case in test/js/bun/net/socket-syscall-fault.test.ts spawns the new fixture tls-pause-parked-handshake-fixture.ts, which parks 27 of 32 TLS sockets, pauses all 32, spins the loop enough to drain the queue, and asserts zero handshake/data callbacks fired while paused and all 32 recover after resume().
Security risks
None identified. The change is a state-machine correctness fix in the event loop; it does not touch TLS verification, credential handling, or input parsing. The failure mode being fixed (callbacks after pause()) is a correctness/back-pressure issue, not a security boundary. The fixture uses rejectUnauthorized: false against its own local self-signed server, which is standard for the test suite.
Level of scrutiny
High. loop.c is the shared uSockets event-loop core across epoll/kqueue/libuv, and the low-priority queue state machine has several interacting invariants (low_prio_state ∈ {0,1,2}, group vs. low_prio_head linkage sharing prev/next, low_prio_count, poll-event bits, is_paused). I traced the new branch's exit state against every later consumer: us_socket_resume (arms reads, dispatch re-gates via budget since low_prio_state==0), us_socket_close (low_prio_state != 1 so it takes the group-unlink path, which is correct since the socket was re-linked before the check), and the timeout sweep (socket is back in head_sockets). The pattern mirrors the us_socket_is_closed branch two lines above, which also continues after adjusting state. REVIEW.md's cross-platform parity check applies here; the drain code itself is platform-shared, but the fixture's exact clientHandshakesWhilePaused: 5 assertion depends on iteration-level timing that the author verified on one platform (20/20 runs) and is not gated behind isWindows.
Other factors
The test follows repo conventions closely: test.concurrent, port: 0, deadline-bounded polling on an observable counter (no sleep), summary asserted before exit code via a single .toEqual on the whole object, fixture placed next to sibling tls-*-fixture.ts files, and bun:internal-for-testing used rather than adding production surface. The PR description demonstrates a precise mechanistic understanding (budget, LIFO drain, low_prio_state transitions) and lists the suites run on debug+ASAN. No CODEOWNERS entry covers packages/bun-usockets/. The bug hunt exited on dry_streak with no findings. Given the criticality of the code path and the tight timing assertion in the test, deferring to a human is the safer call over auto-approval.
|
Updated 11:41 AM PT - Sep 11th, 2026
✅ @robobun, your commit 82a3ce13dd847b7977f3974ff505defa59a70a40 passed in 🧪 To try this PR locally: bunx bun-pr 42351That installs a local version of the PR into your bun-42351 --bun |
|
The fixture this PR added ( |
Problem
Bun.listen({ tls }):socket.pause()on a connection that waits in the TLS handshake queue does not hold. The queue drain arms the reads again, andhandshakeanddatafire on a socket whosepause()returned. In a burst of 32 connections, all 27 queued sockets get both callbacks (1.4.2, canary, main).us_internal_handle_low_priority_sockets(packages/bun-usockets/src/loop.c:320). It callsus_poll_change(events | LIBUS_SOCKET_READABLE)for each socket it takes out of the queue and does not checks->flags.is_paused.Fix
low_prio_stateto 0. It is an ordinary paused socket again.us_socket_resumearms the reads. The readable dispatch then limits the socket like any other handshaking socket, so a burst ofresume()calls cannot run all the delayed handshakes in one iteration.test/js/bun/net/socket-syscall-fault.test.ts(fixturetls-pause-parked-handshake-fixture.ts). Alsosocket.test.tsand thenode:tlssuites.Background
loop->data.low_prio_head, reads off. Each later iteration starts with a drain that takes 5 sockets out and switches their reads on.low_prio_state: 0 is normal, 1 is parked, 2 is taken out by the drain. State 2 is read on its next readable event with no charge to the budget.us_socket_pauseswitches the readable poll off and setsis_paused. On a parked socket the poll is off already, so only the flag changes.us_socket_resumeundoes both.Notes
Reported from a fuzz run as a two-process script: N TCP connections, then all ClientHellos in one tick, and a control byte in that same tick that makes the server pause every accepted socket. N=40 gave 12 to 30 sockets with callbacks after
pause()on 1.4.2, 23 on canary d316760, 15 to 72 of 200 on main 6b394bf (ASAN). N=5 (nothing queues) gave 0 in every run. With this change the same script gives 0 for N=5, 40 and 200.resume()on a parked socket was handled already: the "already parked" arm in the readable dispatch (loop.c:627). This is thepause()direction.The fixture is deterministic because server and clients share one loop, so the loop iteration counter (
getEventLoopStats().iteration, the counter ofus_internal_loop_pre) is an exact clock. It parks sockets with the recipe oftls-close-all-sibling-fixture.ts: pause every accepted socket inopen, then resume all of them at once, so the next iteration reads 5 ClientHellos and parks the rest. An immediate of that iteration pauses all 32 again. The loop then turns 36 iterations, several times what the queue needs to drain.Fixture output, release 1.4.3-canary.1 and a debug+ASAN build of main without the change:
With the change (20 of 20 runs, 4 at a time):
clientHandshakesWhilePaused: 5is the budget: the 5 ClientHellos read before the pause. It also proves that the other 27 sockets were parked whenpause()ran. The last three numbers are taken afterresume(): every socket that the drain left paused comes back.A paused socket still pays one unit of the iteration's budget when the drain takes it out, the same as a closed socket in that loop. Other handshakes are then parked for one more iteration and go first in the next drain (the queue is LIFO), so nothing starves.
Related open PRs do not touch the drain: #42285 (
App::closeIdlemarks parked sockets), #36403 (timeout sweep for parked sockets), #37099 (pause()arms writable interest).Suites run on a debug+ASAN build:
socket-syscall-fault.test.ts,socket.test.ts,tcp-server.test.ts,node-tls-server.test.ts,node-tls-connect.test.ts,node-net-server.test.ts,fetch.tls.test.ts,tls-keepalive.test.ts, and the 245 vendoredtest-tls-*.jsandtest-https-*.jsfiles. Failures that are the same without the change:socket.test.ts"should not call drain before handshake" (needs DNS forwww.example.com), "an unref'd Bun.listen() ... across GC" (over 5 s on a debug build),node-tls-server.test.ts"SNICallback runs even when..." (localhostresolution in this container), vendoredtest-https-timeout.js(hangs on a debug build of main as well) andtest-tls-client-allow-partial-trust-chain.js(usesdescribeoutside the test runner).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-syscall-fault.test.ts