usockets(win): surface a dead connection behind a paused socket's deferred empty-buffer FIN - #37097
usockets(win): surface a dead connection behind a paused socket's deferred empty-buffer FIN#37097robobun wants to merge 3 commits into
Conversation
…erred empty-buffer FIN Two defects in the libuv backend's fin_deferred sweep machinery: 1. fin_deferred was never initialized at socket creation. It shares a byte with unclassified_send_failures:7 and every init site set only the 7-bit field, so the flag started as malloc garbage. Garbage 1s made the existing peeked>0 latch skip its fin_deferred_count increment while close/resume decremented a count that was never incremented, driving it negative - and the sweep escalation is gated on fin_deferred_count > 0, so it never ran at all. Initialize the flag at all four socket birth sites (listen, connect, accept, from_fd). 2. poll_cb's paused-socket probe latched fin_deferred only in the peeked > 0 branch (FIN behind buffered data). A clean FIN arriving with an empty receive buffer (peeked == 0) deferred the eof without latching, so the consumed one-shot DISCONNECT left the poll with no subscription and the sweep never probed the socket: when the connection later died there was no event left to ride, and the socket stranded with no error/close until JS resumed it or an idle timeout fired - forever for timeout-less net sockets. Latch in the peeked == 0 branch too. Verified on Windows: the new socket.test.ts case strands under the released build (victim never closes after the peer is gone) and closes within one sweep period (~2.7s) with the fix. The clean FIN alone still stays deferred across a sweep (pause contract), and end does not fire for the dead socket.
|
Warning Review limit reached
Next review available in: 25 minutes 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 for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (5)
Comment |
|
Updated 12:17 AM PT - Aug 7th, 2026
✅ @robobun, your commit 1ee4c37c759e69f1767d5c4b9f8e468122900638 passed in 🧪 To try this PR locally: bunx bun-pr 37097That installs a local version of the PR into your bun-37097 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not duplicates, but both are in the same FIN-then-reset family and worth cross-referencing:
One point from this PR that affects how the other two are judged: without the init fix, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches core usockets eventing on Windows and — by fixing the fin_deferred init — activates the sweep escalation loop in sweep_timer_cb that was effectively dead before, so the blast radius is wider than the one symptom described. Worth a human look.
Checked: the hoisted us_internal_poll_cb_adopted_socket call is safe (probeable already dereferences it); the new peeked == 0 latch is balanced by the existing decrements in us_socket_resume / us_internal_socket_close_raw; the four init sites cover every us_socket_t birth (accept, connect, listen, from_fd); the added stores compile on non-libuv paths (fin_deferred is unconditionally declared in internal.h).
Extended reasoning...
Overview
Two fixes to the Windows (libuv) backend's paused-socket FIN-deferral machinery in packages/bun-usockets/:
- Initialize the
fin_deferred:1bitfield to 0 at all four socket birth sites (context.c×2,loop.caccept path,socket.cfrom_fd). Previously only the adjacentunclassified_send_failures:7in the same byte was set, leavingfin_deferredas malloc garbage. - In
libuv.cpoll_cb, latchfin_deferredin thepeeked == 0branch (clean FIN, empty rx buffer) the same way the existingpeeked > 0branch does, and hoist thesocklookup above the branch chain. - New Windows-only concurrent test in
socket.test.ts(~17s of structural sleeps, 40s timeout) that pauses a server socket, has the peer FIN, waits a full 4s sweep to assert the FIN alone stays deferred, then kills the peer and writes into the dead connection to provoke the RST the sweep detects.
Security risks
None. No parsing of untrusted input, no auth/crypto. The zero-byte send probe and the sweep loop are pre-existing; this PR only feeds one more state into them.
Level of scrutiny
High. This is low-level C socket lifecycle code where a miscounted fin_deferred_count or a mis-latched flag can hang or prematurely close connections. Critically, the init fix means sweep_timer_cb's escalation loop — which per the PR description never ran before because garbage fin_deferred bits drove the count negative — will now actually execute on Windows for both the pre-existing peeked > 0 case and the new peeked == 0 case. That's a behavioral activation with wider reach than the single symptom, on a platform I can't exercise here.
Other factors
- The reasoning in the PR description and code comments is thorough and internally consistent; the count balancing checks out (latch guarded by
!sock->fin_deferred; resume/close_raw each decrement once and clear; sweep clears before calling close_raw so no double-decrement). - The test's sleeps are justified in comments as structural (fixed 4s sweep cadence, absence-of-close assertion), which is the documented exception in REVIEW.md, but it's still a ~17s Windows-only timing test — CI will be the real verification.
us_socket_detachdoes not clearfin_deferred(pre-existing, not introduced here), but detach is a niche path and the field is Windows-only; noting for completeness, not as a blocker.- No prior human or bot review on this PR; CI build was still in progress at review time.
A detached socket leaves usockets' management without going through us_internal_socket_close_raw, so a latched fin_deferred would leak the count and keep the sweep walking the group list every tick forever.
|
Good catch on |
There was a problem hiding this comment.
No issues found by the bug hunt, but this touches core usockets socket-lifecycle C code (every accept/connect init, the Windows poll_cb dispatch, the sweep counter invariant) and overlaps in design with two other open PRs (#34487, #37077) on the same fin_deferred/UV_DISCONNECT machinery — a human should decide which approach lands and in what order.
What was reviewed:
fin_deferredcount balance: latch sites (bothpoll_cbbranches) vs. decrement sites (close_raw,resume,detach, sweep) — balanced;us_socket_adopt's memcpy carries the flag to the new allocation and the old goes to the closed list without a spurious decrement.- All four socket birth sites now init the bitfield;
us_socket_from_fdis Windows-unreachable (returns 0) so its store is inert. - Test resource cleanup on mid-test assertion failure — ruled out (the
peerconnect is awaited andterminate()d on the happy path; theusing servercovers the listener).
Extended reasoning...
Overview
The PR fixes a Windows-specific socket stranding bug in the usockets libuv backend: a paused socket that receives a clean FIN on an empty receive buffer, and whose connection later dies, never gets a close callback. The fix has two parts: (1) initialize the fin_deferred bitfield at all four us_socket_t birth sites (it shares a byte with unclassified_send_failures:7 and was previously malloc garbage, which drove fin_deferred_count negative and disabled the sweep entirely), and (2) latch fin_deferred in poll_cb's peeked == 0 branch so the 4s sweep probes the socket after the one-shot UV_DISCONNECT has been consumed. A follow-up commit adds the same guarded clear to us_socket_detach so a detached socket doesn't leak the count. A Windows-gated test in socket.test.ts reproduces the strand.
Security risks
None identified. This is event-loop plumbing on already-accepted sockets; no parsing of untrusted input, no auth/crypto, no user-controlled sizes. The new stores are to a 1-bit field on a struct we own.
Level of scrutiny
High. This is core C in the networking hot path: every accepted socket, every connect socket, and every Windows poll dispatch runs through the touched code. The fin_deferred_count invariant spans five files and multiple lifecycle exits (close, resume, detach, adopt, sweep), and getting it wrong either strands sockets or makes the sweep walk every group every 4s forever. The change is small and mechanically consistent with the existing peeked > 0 latch, but the counter bookkeeping is exactly the kind of cross-file invariant that benefits from a maintainer's eyes.
Other factors
- Competing open PRs: #34487 and #37077 both add the identical four init lines and touch the same
poll_cbUV_DISCONNECTblock with different (and in #37077's case, structurally competing) approaches. The author has explained how they compose, but which design wins for the non-paused quiesce case is a maintainer call, and merge order matters. - Test timing: the new test is ~17s of wall clock on Windows (structural 4.6s + 12s bounded race against a fixed 4s sweep cadence). The sleeps are commented per REVIEW.md's requirement, but a maintainer may want to weigh the CI cost.
- CI: build #89902 is still running per the timeline; Windows results not yet visible.
… buffered-FIN and resume paths us_internal_libuv_peer_reset_probe treated any send() error other than WSAEWOULDBLOCK/WSAESHUTDOWN as peer-gone. WSAENOBUFS is documented transient-on-healthy (bsd_send_is_transient_error), and a false positive reset-closes a healthy paused connection, so mirror us_internal_send_errno_is_peer_gone's allowlist instead. The asymmetry favors this: a missed detection self-heals at the next 4s sweep, a false positive is unrecoverable. Tests: add the buffered-data sibling (the pre-existing peeked > 0 latch that the init fix first makes reachable) asserting the reset wins over the deferred data and end, and a resume-delivery case pinning that a resumed socket gets its deferred data + end with a clean close.
|
Self-review follow-ups, pushed in 1ee4c37:
All three tests pass on Windows x64 with the fix; the empty-buffer one remains the deterministic fail-before proof on the released build. |
There was a problem hiding this comment.
No bugs found, but this touches Windows socket-lifecycle C code (fin_deferred sweep counter balance, close/detach/resume paths, and a denylist→allowlist change in the peer-reset probe) and overlaps with two open PRs (#34487, #37077) on the same machinery — worth a human look to confirm the design and sequencing.
What was reviewed:
- Counter balance: latch sites (both
peekedbranches) vs. clear sites (close_raw,detach,resume,sweep_timer_cb) — no double-decrement (sweep clears the flag before callingclose_raw);us_socket_adopt's memcpy preserves the flag on the relocated socket so the count stays consistent. - The four init sites cover every
us_socket_tbirth;fin_deferredis an unconditional struct field so the un-#ifdef'd inits compile on non-libuv backends. - Allowlist mirrors
us_internal_send_errno_is_peer_goneminusEPIPE(no WSA equivalent for send-side);WSAESHUTDOWNcorrectly falls todefault: return 0.
Extended reasoning...
Overview
This PR fixes a Windows-only socket strand: a paused socket whose peer sends a clean FIN on an empty receive buffer, then later dies, never gets a close callback. It touches four C files in packages/bun-usockets/ (the libuv backend's fin_deferred sweep) plus three Windows-gated tests in test/js/bun/net/socket.test.ts. The changes are: (1) initialize s->fin_deferred = 0 at all four socket birth sites, (2) latch fin_deferred in the peeked == 0 branch of poll_cb (previously only peeked > 0 latched), (3) clear the flag in us_socket_detach since detach bypasses close_raw, and (4) tighten the peer-reset probe from a denylist to an allowlist of WSA peer-gone codes.
Security risks
None identified. This is internal socket lifecycle bookkeeping with no user-controlled input reaching the changed code paths; the probe's allowlist change is strictly more conservative (fewer conditions treated as peer-gone).
Level of scrutiny
High. This is low-level C networking code on a platform-specific event path with a manually balanced counter invariant (fin_deferred_count) whose correctness depends on every latch site being paired with exactly one clear site across close/detach/resume/adopt. A miscount either disables the sweep (negative) or makes it walk every group every 4s forever (leaked positive). The PR description is thorough and the counter balance checks out on inspection, but the interaction with us_socket_adopt (memcpy of the bitfield onto the relocated socket while the old one is closed-listed without going through close_raw) and with the two overlapping open PRs (#34487 latches from the readable_ended state, #37077 proposes a competing UV_PRIORITIZED design) is subtle enough that a maintainer familiar with the AFD/DISCONNECT machinery should confirm.
Other factors
The three new tests are Windows-only and use structural sleeps (a 4.6s wait to span one sweep period, 12s stranded-detection races) — the comments justify why no observable signal exists, but they add ~20s to the Windows lane. The duplicate-PR bot flagged design overlap with #34487 and #37077 on the same poll_cb UV_DISCONNECT block; the author's response argues they compose, but a maintainer should decide the landing order. Given all of this, deferring to human review.
1 participant
Symptom
On Windows, a paused socket whose peer sends a clean FIN while the socket's receive buffer is empty, and whose connection later dies, never gets an error/close callback. The socket strands until JS happens to resume it or an idle timeout reaps it, forever for timeout-less
netsockets (receive backpressure inBun.servebody streams andnet.Socket#pauseare the common ways to be paused). On Linux the same sequence closes the socket via the unmaskableEPOLLERR.Cause
Two defects in the libuv backend's
fin_deferredsweep machinery (the 4s sweep that probes paused sockets whose FIN report was already consumed):fin_deferredwas never initialized. It shares a byte withunclassified_send_failures:7inus_socket_t, and every socket init site set only the 7-bit field, so the flag started as malloc garbage. Garbage 1s made the existingpeeked > 0latch inpoll_cbskip itsfin_deferred_countincrement, while close/resume decremented a count that was never incremented, driving it negative. The sweep escalation is gated onfin_deferred_count > 0, so it never ran at all; the machinery was effectively dead.The
peeked == 0path never latched.poll_cb's paused-socket probe (packages/bun-usockets/src/eventing/libuv.c) marked a socket for the sweep only when the FIN arrived behind buffered data. A clean FIN on an empty buffer deferred the eof (pause contract) without latching, and the one-shotUV_DISCONNECTreport was consumed by that dispatch: the poll re-arms withoutUV_DISCONNECT, a paused socket's events have converged to none, so nothing is subscribed that could ever report the connection dying later, and the sweep did not know to probe it.Fix
s->fin_deferred = 0at all four socket birth sites (listen-socket init, connect-socket init, the accept path,us_socket_from_fd), making the count sane and the existing sweep machinery actually run.us_socket_detachnow clears a latched flag too, since a detached fd leaves usockets' management without passing throughus_internal_socket_close_raw.fin_deferredin thepeeked == 0branch too, so the sweep probes the socket and escalates once the connection is dead, closing it withLIBUS_SOCKET_CLOSE_CODE_CONNECTION_RESETexactly like the buffered-data case.us_internal_libuv_peer_reset_probefrom a denylist (anything butWSAEWOULDBLOCK/WSAESHUTDOWNcounted as peer-gone) to the WSA mirror ofus_internal_send_errno_is_peer_gone's allowlist. This matters more now that the sweep actually runs:WSAENOBUFSis documented transient-on-healthy in this codebase (bsd_send_is_transient_error), and a false positive reset-closes a healthy paused connection, while a missed detection self-heals at the next 4s sweep.A healthy half-closed peer keeps the socket deferred: the sweep's probe succeeds and the eof still waits for
resume().One Windows TCP finding shaped the test: Windows does not emit an RST when a socket that already sent its FIN is aborted (verified at the winsock level: after abort from
FIN_WAIT_2, the victim's zero-byte send succeeds,SO_ERRORis 0, and recv reports the graceful EOF), so a fully idle loopback victim has no TCP-level signal of the peer's death. The tests have the victim write one byte into the dead connection; the RST reply to that segment resets the victim's TCB the same way a remote peer's RST segment would, which is the state the sweep probe detects.Platform status
EPOLLERR/EPOLLHUPcloses the paused victim milliseconds after the reset (verified on a debug build of this branch). The C changes here are inert on the epoll path.EVFILT_WRITEarmed.Verification
On Windows x64, three Windows-gated tests in
test/js/bun/net/socket.test.ts:endnever fires, and the clean FIN alone stays deferred across a full sweep).peeked > 0latch that the init fix first makes reachable, asserting the reset wins and neither the buffereddatanorendis delivered (node parity for a paused socket whose peer died). On the released build this path depends on malloc garbage, so it strands nondeterministically; the deterministic fail-before proof is the empty-buffer test.enddelivered, clean close) and the count hand-back from the sweep.test/js/bun/net/socket.test.tson Linux: same pass/fail set as main (the new tests are Windows-only).Adjacent: #37077 fixed the unpaused flavor of this FIN-then-RST family for kqueue; #34487/#35939 cover the delivered-end (
readable_ended) flavor. This PR is the paused case plus the initialization defect that kept the whole sweep off.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