usockets(kqueue): close half-open sockets whose write side hit EV_EOF instead of spinning - #37076
Closed
cirospaciari wants to merge 9 commits into
Closed
cirospaciari wants to merge 9 commits into
cirospaciari wants to merge 9 commits into
Conversation
… instead of spinning On kqueue EVFILT_WRITE is one-shot and the half-open EOF branch re-adds it unconditionally. Once the peer is fully gone the filter fires again immediately with EV_EOF, the writable dispatch lets the owner send() -> EPIPE which us_socket_write masked as backpressure and re-armed, and the loop spun a core forever with no error surfaced. Treat EV_EOF on EVFILT_WRITE for a socket we did not shut down and are no longer reading as an error, taking the same SO_ERROR close path epoll's EPOLLERR|EPOLLHUP already takes, and stop re-arming writable from the legacy write entry points on peer-gone errnos (as libuv's uv__write does).
cirospaciari
marked this pull request as ready for review
August 6, 2026 23:20
Contributor
WalkthroughThe native socket layer now stops writable polling after terminal peer errors and handles unhandled kqueue writable EOF events as socket errors. HTTP upgrade regression coverage validates closure after queued writes, peer FIN, and connection reset. Rust formatting and lint changes do not alter behavior. ChangesSocket peer-reset handling
Rust cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/node/http/node-http-server-socket-end-drain.test.ts`:
- Around line 65-67: Update the test around bunRun in “upgrade socket with
queued writes…” to retain and assert the captured stderr alongside stdout and
exitCode. Assert the combined subprocess result so unexpected fixture errors
cannot pass silently, while preserving the existing expected stdout and
successful exit status.
🪄 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: d84960d7-dc5e-4093-a6df-d5f51afbf986
📒 Files selected for processing (4)
packages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.ctest/js/node/http/node-http-server-socket-end-drain.test.tstest/js/node/http/node-http-upgrade-halfopen-reset-fixture.ts
PendingAnchor is a #[must_use] token deliberately consumed by binding, so by-value is the design; clippy (which only runs on PRs, not main pushes) flags it on every PR since #37055 landed. No-Verification-Needed: lint attribute only, no runtime behavior change
swallow() is a deliberate polymorphic sink over () and Result<(), E> consumer methods; routing the unit-returning ones through it is the module's design. This lint was masked in CI until bun_parsers compiled again (clippy only runs on PRs, and a failing dep crate hides its dependents' lints). No-Verification-Needed: lint attribute only, no runtime behavior change
ig-ant
reviewed
Aug 6, 2026
us_poll_change sets absolute flags, and rearm_writable forced READABLE back on for any non-paused socket. For a half-open socket whose read side already hit EOF that re-arms the dead read filter, which re-fires and delivers on_end a second time. Read interest is only ever off because something dropped it deliberately (pause, half-open EOF, low-prio parking), so preserve it instead of forcing it on.
Collaborator
|
Updated 5:10 PM PT - Aug 6th, 2026
@cirospaciari, your commit 3319b55 is building: |
Nine parallel tests (Bun.listen/Bun.serve plain+TLS, fetch upload, node:http/https response+upgrade) driving a backpressured socket whose peer stops reading, FINs, then RSTs: the victim must close, with 'end' delivered at most once. From #37077.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Skipping the rearm for every peer-gone failure left a race window where a half-open socket ended up with no kqueue filter registered at all: a writable event without EV_EOF, an RST landing before the flush send(), and the epilogue no-ops with zero events - nothing ever dispatches again. Re-arming the one-shot EVFILT_WRITE instead makes the next dispatch carry EV_EOF and take the loop.c error-close. Paused sockets keep the skip: the close branch deliberately excludes them, so their re-arm would spin.
…ation from #37077 Replaces the dispatch-layer EV_EOF branch with #37077's design: kqueue's EV_EOF on EVFILT_WRITE (SS_CANTSENDMORE without our own shutdown) is translated to the poll error at the event layer, matching epoll's EPOLLERR, and a read_eof latch guarantees on_end fires at most once - readable interest is never re-added by rearm or resume after the peer's FIN was delivered. Covers two cases the previous mechanism missed: resume() after 'end' re-firing on_end, and a paused socket never observing a peer reset behind pending writes.
Member
Author
|
Closing in favor of #37077 — the uSockets fix and the peer-reset test matrix there are identical to this branch (verified byte-for-byte). Two extras live on this branch if wanted: a timer-free node:http upgrade half-open test also validated against node v26.3.0 (test/js/node/http/node-http-server-socket-end-drain.test.ts + fixture), and two one-line clippy |
This was referenced Aug 7, 2026
ig-ant
added a commit
to anthropics/sandbox-runtime
that referenced
this pull request
Aug 11, 2026
…sion (#461) * fix(proxy): close CONNECT clients that abandon a slow permission decision The host allow/deny decision for a CONNECT can take tens of seconds (a model-classifier call) or arbitrarily long (an interactive permission prompt), while sandboxed clients time out much sooner. When the client gave up mid-decision, the proxy had no EOF handling in that window: the descriptor stayed half-open for the life of the decision and the verdict (403/200) was then written to the dead socket. On runtimes whose EPIPE handling retries dead-peer writes (Bun pre oven-sh/bun#37076) that fed an idle-CPU busy-loop; on every runtime it leaked a descriptor per abandoned decision. Now EOF during the decision window destroys the socket immediately, and every decision-phase status write (407/400/403/502/500) checks liveness first — a verdict for a dead client is dropped (the decision still completes, so host-side caches serve the retry). Write errors release the descriptor instead of only logging. Because 'end' never fires on a paused socket, the decision window captures data (bounded at 64KB; overflow is treated as abandonment) to keep the stream flowing so EOF notification is reliable across runtimes, folding captured bytes back into the tunnel head at handoff; the ClientHello peek resumes explicitly since a 'data' listener does not resume an explicitly paused stream. Established tunnels are exempt (the handling disarms at handoff) and now set allowHalfOpen so a client FIN after its request bytes still gets the upstream's reply on runtimes that default http-server sockets to auto-close. Plain-HTTP denies are dropped only for a destroyed socket — legal HTTP half-close still gets its verdict. The catch-all 500 now respects an already-written 200 like the 502 path (no status line injected into tunnel payload). New regression suite drives a real proxy over TCP: EOF-mid-decision and the write-error backstop both fail against the unpatched proxy and pass patched; live-client deny/tunnel, half-closed-tunnel FIN forwarding, and two plain-HTTP variants pin the unchanged behaviors. * review: keep the terminating path's stream states untouched The decision-window flowing capture broke the entire TLS-terminating path (every credential-mask-sigv4 test): its disarm-time pause() injects a pause/resume cycle on a CONNECT-upgraded socket, the exact hazard relayPaused documents under Bun and was built pull-mode to avoid. Bisected: pause() alone red, capture without pause green. Scope the capture to the opaque path (no mitmCA): there the only consumer handoff is pipe(), and the capture listener now stays attached as a discarding sink — 'data' broadcasts to all listeners, so nothing is dropped in the awaited-dial gap and no pause is ever needed. On the mitmCA path the peek/relay machinery keeps sole ownership of the socket's flow; decision-window EOF there is best-effort ('close' fires on full close — the incident case — and the peek's own reads surface 'end'), with the write-time guards and error-handler backstop unchanged. The peek resume() is reverted with its reason (an explicitly paused caller no longer exists).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Before: on macOS, a natively half-open uSockets socket (node:http server / Upgrade tunnel sockets, node:net sockets,
Bun.listen({allowHalfOpen})) whose peer FIN'd and then went away while the owner still had bytes queued would never close. The process sat at ~100% of a core doingsend()→ twokevent64changelist calls → immediate wake, forever, with ~no JS on the stack and no'error'/'close'ever reaching JS.After: the socket is closed with
SO_ERROR(typicallyECONNRESET), exactly like the epoll build already does viaEPOLLERR|EPOLLHUP, and the loop goes idle. This matches Node.js: the same scenarios run on node v26.3.0 emit'close'(with'end'exactly once) and exit idle.Why it spun.
EVFILT_WRITEis registeredEV_ONESHOT, and kqueue reports a peer reset behind pending writes asEV_EOFon that write filter (SS_CANTSENDMORE). That was treated as a read EOF: the half-open EOF branch dispatchedon_end(again) and re-armed the one-shot write filter, which fired again immediately, forever. kqueue never setsEV_ERRORfor this state, so the error-close path was unreachable; epoll reports the same state asEPOLLERR|EPOLLHUPand closes.The change (same design as #37077, kept in sync with it):
eventing/epoll_kqueue.c:EV_EOFonEVFILT_WRITEis split out assend_eofand translated to the poll error forPOLL_TYPE_SOCKET/POLL_TYPE_SEMI_SOCKET— the write side is dead without our ownshutdown()(own-shutdown sockets have poll typePOLL_TYPE_SOCKET_SHUT_DOWNand are excluded), so it takes the sameus_socket_get_error→ close path epoll takes.eofis now derived fromEVFILT_READonly, mirroring libuv's kqueue.c.read_eoflatch on the socket: once the peer's FIN is dispatched ason_endon anallow_half_opensocket, readable interest is never re-added (us_internal_rearm_writable,us_socket_resume) andon_endnever re-fires.Reference: libuv only derives EOF from
EVFILT_READ, delivers it once (UV_HANDLE_READ_EOF), and fails writes on a dead write side instead of re-polling it as backpressure (uv__try_write).How did you verify your code works?
test/js/node/http/node-http-server-socket-end-drain.test.ts+node-http-upgrade-halfopen-reset-fixture.ts: fully event-driven (no timers) — an Upgrade socket queues 32 MB, the client never reads, FINs once the writes are queued, RSTs once the server saw the FIN; natural process exit is the loop-went-idle proof. Fails on current main (spins, ~727 ms CPU per 500 ms window, test times out); passes with this change; node v26.3.0 passes the identical fixture.test-net-half-open-peer-reset-*parallel tests (Bun.listen/Bun.serve plain+TLS, fetch upload, node:http/https response+upgrade) asserting close with'end'at most once — all pass on macOS with this change, all spin/time out on current main.'end'exactly once, clean close, same as this PR's behavior.test/js/bun/net+test/js/node/net+test/js/web/websocket+test/js/node/httpsuites locally: 0 failures.Also carries two one-line clippy
#[allow]s (yaml.rsneedless_pass_by_value,uws_handlers.rsunit_arg) for pre-existing lints on main that red every PR's clippy job (clippy doesn't run on main pushes), plus rustfmt fixes pushed by autofix-ci.