usockets: stop pause() from arming writable interest it never had - #37099
usockets: stop pause() from arming writable interest it never had#37099robobun wants to merge 9 commits into
Conversation
us_socket_pause forced the poll to WRITABLE. Two consequences: - The always-writable socket immediately dispatched a writable event, so every pause() with nothing buffered fired a bogus JS drain. - On a socket whose write side we already shut down, the fresh kqueue EVFILT_WRITE one-shot reported our own SS_CANTSENDMORE as EV_EOF instantly, and the eof dispatch closed the half-closed socket within milliseconds even though the peer was alive and silent (verified on macOS; Linux kept it open, so the platforms diverged). libuv's uv_read_stop only ever removes read interest. pause() now only keeps pre-existing writable interest (a backpressured write stays armed), and kqueue_change's 0-event fallback (the one-shot write filter armed to catch peer teardown) skips sockets we shut down ourselves: for them any write filter completes instantly with our own EV_EOF and can never distinguish anything.
|
Warning Review limit reached
Next review available in: 33 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 (3)
WalkthroughThe change adds explicit writable-event scheduling for queued socket and HTTP work. It updates kqueue shutdown and pause/resume filter handling to preserve peer teardown detection and adds regression tests for idle, paused, and half-closed sockets. ChangesSocket polling behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… teardown watch for shut-down sockets CI caught node:http pipelined flood prevention deadlocking: the park/ replay machinery (HTTP_NODE_READS_PAUSED -> onWritable tail -> Bun__NodeHTTP__onReadsResumable) relied on the writable event pause() used to force-arm. The queued pipelined responses live in the JS pipeline queue (or the AsyncSocket buffer) without any kernel send having been attempted, so no write failure ever arms the poll and the replay never ran: requests parked forever. New us_socket_mark_writable_pending() arms writable interest for bytes held outside the socket's own write path; the three sites that park requests behind queued responses (onReadsPaused and both HttpContext backpressure branches) now ask for their flush/replay wakeup explicitly instead of riding a side effect of pause(). Review fix: the pause change left a paused shut-down kqueue socket with zero filters, so the peer's FIN/RST was never delivered (epoll keeps the implicit EPOLLHUP|EPOLLERR). kqueue_change now arms a read-side teardown watch (EV_ADD|EV_CLEAR: the read filter's EV_EOF is the peer's FIN/RST, never our own SS_CANTSENDMORE echo) for 0-event polls on shut-down sockets, deleting any leftover write one-shot, and us_internal_socket_raw_shutdown arms it directly when the poll was already at 0 events (the diff would no-op). New test pins the flip side: shutdown+pause still closes when the peer terminates. Also from review: the silent-peer window is 250ms with rationale, and both tests release their sockets before asserting.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
CI status: the node:http pipelined flood-prevention deadlock from the previous run is fixed (that suite is green on 199dbf3). The remaining red lane is a filesystem_router.test.ts segfault on debian 13 aarch64 with no socket involvement; it passes repeatedly with this diff locally and has been reported for main-break triage. The two Windows failures passed on retry. |
…h; resume() keeps rather than manufactures writable Review catches, both the same class as the PR: - pause() before shutdown() armed the 0-event fallback write one-shot while own_shutdown was still false, and the teardown transition's conditional delete read the caller's old_events (a literal 0 from the raw_shutdown direct call), so the phantom one-shot survived and echoed our own SS_CANTSENDMORE as EV_EOF: the sibling ordering still closed a paused half-closed socket prematurely on kqueue. The teardown watch now deletes EVFILT_WRITE unconditionally (ENOENT receipt is harmless when none exists). New test pins the pause-then-shutdown ordering. - us_socket_resume manufactured WRITABLE the same way pause() used to, firing one bogus drain per pause/resume round trip; it now re-adds readable and only keeps pre-existing writable interest (backpressure during the pause already re-armed it). The drain test now covers the full round trip.
The libuv backend has no event for a reset against a paused 0-event poll (AFD only reports subscribed events and the DISCONNECT was consumed), so the test strands on Windows. That gap predates this change and is tracked separately; the test pins the POSIX contract this PR fixes.
The comments described the pre-change pause/resume interest steps (W -> R|W -> R and the undeleted write one-shot); the cycle macOS 26 needs is preserved but now runs through the shut-down teardown transition.
… every shutdown; share the writable-pending helper macOS 26 only delivers a peer's close on a read filter registered after SHUT_WR: the teardown watch armed before the shutdown syscall made node-http-halfclose-midupload time out at connection-closed on the darwin 26 lanes (the End path's previous delete-then-re-add cycle re-added after). The watch now arms after shutdown(2); EV_EOF is level state, so a FIN landing in the gap is still reported by the fresh registration. Review catches, same class: - pause() then resume() then shutdown() left the 0-event fallback's phantom write one-shot armed across SHUT_WR (resume no longer records writable, so neither diff saw it). The still-reading shutdown path now scrubs EVFILT_WRITE explicitly; new silent-peer test pins the third ordering. - us_socket_sendfile_needs_more was the pre-existing sibling of the new helper with the old force-READABLE semantics; it now routes through us_socket_mark_writable_pending (respects pause and shut-down).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/net/socket.test.ts`:
- Around line 365-398: Replace duration-based sleeps in the socket tests with
observable checkpoints and bounded polling. In test/js/bun/net/socket.test.ts
ranges 365-398, await deterministic connection/event-loop checkpoints before
asserting no drain; in ranges 401-439, 441-477, and 479-516, replace each 250 ms
wait with the peer protocol checkpoint proving the socket remains live, using
the same observable half-closed lifecycle checkpoint for the latter two ranges.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3c2db7d-bfac-4341-83f6-97dbfdc6431d
📒 Files selected for processing (9)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/socket.cpackages/bun-uws/src/HttpContext.hsrc/jsc/bindings/node/JSNodeHTTPServerSocket.cppsrc/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cppsrc/uws_sys/libuwsockets.cpptest/js/bun/net/socket.test.ts
…ints An echo round trip through an independent pair on the same loop cannot complete before already-ready events for other sockets have dispatched, so N round trips prove N full poll cycles ran: the buggy drain/close would have fired inside the first. Deterministic and faster than the fixed windows. Also trims the sendfile wrapper comment.
…; fix a stale ordering comment us_socket_mark_needs_more_not_ssl was the remaining open-coded copy of the pre-fix pattern (forces READABLE, no shut-down guard) with a live caller in the file-response stream; it now delegates to us_socket_mark_writable_pending like its sendfile twin. The End-path comment also still said the teardown watch arms before SHUT_WR after 6206cde moved it after; the ordering claim now defers to socket.c.
There was a problem hiding this comment.
All prior inline findings have been addressed (8d7a63e routes us_socket_mark_needs_more_not_ssl through the shared helper and fixes the stale ordering comment). I didn't find anything further this round, but this is core usockets eventing-layer work — pause/resume interest semantics, kqueue teardown-watch arming, and a macOS 26 ordering workaround — that a human familiar with the loop should sign off on.
What was reviewed:
- The three pause/shutdown orderings and the phantom
EVFILT_WRITEone-shot scrub — all now covered by the still-reading branch's unconditional delete. - Remaining unconditional-R|W siblings in libuwsockets.cpp — both
sendfile_needs_moreandmark_needs_more_not_sslnow delegate to the pause-respecting helper. kqueue_changeteardown_watch:change_list[2]is not overrun (the two teardown entries are mutually exclusive with the readable-diff and 0-event-fallback branches).- The
loopCyclescheckpoint helper for the negative assertions — same-loop echo round trips replace the earlier sleeps.
Extended reasoning...
Overview
This PR changes the semantics of us_socket_pause/us_socket_resume in usockets so they no longer manufacture writable interest, adds a kqueue read-side teardown watch for shut-down 0-event polls (so peer FIN/RST still delivers), scrubs the unrecorded 0-event-fallback EVFILT_WRITE one-shot at every shutdown transition, and introduces us_socket_mark_writable_pending() for callers (node:http flood-prevention park sites, the two Bun.serve "needs more" helpers) that hold bytes outside the socket's write path and previously relied on pause() arming writable for them. Nine files across usockets, uWS, the node:http C++ bindings, and tests.
Security risks
None identified. This is I/O eventing / poll-interest management; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
High. epoll_kqueue.c and socket.c are the lowest layer of Bun's networking — every TCP socket, HTTP server, and WebSocket runs through this dispatch. The change alters kqueue filter arming with platform-divergent behavior (epoll's implicit EPOLLHUP vs kqueue needing an explicit filter), threads a macOS 26 kernel-behavior workaround (filter must be registered after SHUT_WR) through us_internal_socket_raw_shutdown, and rewires the wakeup for node:http's pipelined-request replay. A subtle mistake here manifests as hangs, spurious closes, or busy-loops that only reproduce on one platform under load.
Other factors
The PR has been through several review rounds; each round surfaced same-class siblings (sendfile_needs_more, mark_needs_more_not_ssl, the pause→resume→shutdown ordering) that were all taken. The four new tests use a same-loop echo checkpoint instead of sleeps and cover both directions of the shutdown/pause interaction plus the peer-terminates flip side. CI covers the macOS lanes this is primarily aimed at (the Linux environment cannot exercise the kqueue paths). Given the depth, platform sensitivity, and the number of iterations it took to converge, a maintainer who owns this layer should confirm the final shape — particularly the kqueue_change teardown_watch branch and the AFTER-shutdown(2) ordering claim for macOS 26.
2 participants
What
Two related
us_socket_pausebugs, both from pause() forcing the poll to WRITABLE instead of keeping whatever writable interest already existed:pause()with nothing buffered fired a bogus JSdrainevent (the always-writable socket dispatches the freshly armed writable immediately). Node never emitsdrainwithout a preceding failed write.shutdown()thenpause()closed the socket within ~2ms even though the peer was alive and silent. The fresh kqueueEVFILT_WRITEone-shot reports our ownSS_CANTSENDMOREasEV_EOFinstantly, and the eof dispatch treats a shut-down socket's eof as the connection being over. Verified on macOS hardware (closed after 2ms; Linux correctly stayed open), so the platforms diverged.Fix
us_socket_pausenow drops readable interest and only keeps pre-existing writable interest, matching libuv'suv_read_stopwhich never manufactures write interest. A backpressured write keeps its re-arm.HTTP_NODE_READS_PAUSED, replay from theonWritabletail) was woken by that spurious event, and the queued pipelined responses live in the JS pipeline queue or the AsyncSocket buffer without any kernel send having been attempted, so nothing else ever armed the poll. CI caught this as a deadlock in the pipelined-responses test (parked requests never replayed). Newus_socket_mark_writable_pending()arms writable interest for bytes held outside the socket's own write path, and the three park sites (onNodeHttpReadsPausedplus bothHttpContextbackpressure branches) now request their flush/replay wakeup explicitly.EPOLLHUP|EPOLLERR).kqueue_changenow arms a read-side teardown watch (EV_ADD|EV_CLEAR: the read filter'sEV_EOFis the peer's FIN/RST, never our ownSS_CANTSENDMOREecho) for 0-event polls on shut-down sockets, deletes any leftover write one-shot, andus_internal_socket_raw_shutdownarms the watch directly when the poll was already at 0 events (the interest diff would no-op).Verification
Three tests in
test/js/bun/net/socket.test.ts:closed after 2ms (PREMATURE).node-http.test.tsincluding the pipelined flood-prevention test,socket.test.ts, andnode-net.test.tsshow the same failure set as clean main in this environment.Related: #37077 iterates the adjacent three-state rule for write-side
EV_EOFon shut-down sockets; this PR removes the pause-path arming that made those echoes fire in the first place, and the two compose. #33974 owns the paused-eof deferral forallow_half_opensockets; this PR is about interest arming, not eof dispatch.no test proof · iteration 8 · 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