Skip to content

usockets(kqueue): deliver a peer reset to sockets polling for no events - #37101

Open
robobun wants to merge 8 commits into
mainfrom
farm/b6fcd001/kqueue-halfopen-deaf-socket
Open

usockets(kqueue): deliver a peer reset to sockets polling for no events#37101
robobun wants to merge 8 commits into
mainfrom
farm/b6fcd001/kqueue-halfopen-deaf-socket

Conversation

@robobun

@robobun robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

What

On macOS, an allowHalfOpen socket that consumed the peer's FIN and drained its writes became permanently deaf: a later RST from the peer was never delivered, close/error never fired, and the socket leaked until process exit (raw Bun.listen/Bun.connect sockets have no default timeout). Linux is unaffected.

Repro shape (macOS):

const server = Bun.listen({
  hostname: "127.0.0.1", port: 0, allowHalfOpen: true,
  socket: { data() {}, end() {}, close() { console.log("closed"); }, error() {} },
});
const client = net.connect({ port: server.port, allowHalfOpen: true });
client.once("connect", () => client.end()); // FIN; server side stays half-open
// two turns later:
client.resetAndDestroy(); // RST
// server's close/error never fires on macOS; on Linux it fires with ECONNRESET

Why

A half-open socket past on_end whose writes drained polls for no events at all. The two backends treat that state differently:

  • epoll keeps the fd registered with zero interest, and the kernel reports EPOLLHUP/EPOLLERR regardless, so the RST closes the socket through the error path (us_poll_start_rc documents this).
  • kqueue has no implicit reporting, only filters. kqueue_change armed an EV_ONESHOT EVFILT_WRITE as the detector for this state, but a connected socket is immediately writable, so the very next kevent wakeup consumed the oneshot. The dispatch then cleared POLL_TYPE_POLLING_OUT (loop.c) and the post-dispatch us_poll_change(0) was an old == new no-op. Net result: zero filters on the fd, and nothing can ever wake that socket again.

Fix

All in packages/bun-usockets/src/eventing/epoll_kqueue.c:

  • kqueue_change: arm the zero-event detector as EV_ADD | EV_CLEAR instead of EV_ONESHOT, and unconditionally, so a WRITABLE -> 0 transition converts a still-armed oneshot. EV_CLEAR fires once on arming (the dispatcher masks it out) and then only on write-side state changes, so an idle half-open or paused socket does not wake the loop, while a dead connection re-activates the knote.

  • us_poll_change: a 0 -> 0 change on a socket poll re-arms the detector instead of no-opping, because the writable dispatch just consumed the oneshot behind the tracked state.

  • dispatcher: EV_EOF on the detector (any write-filter event while WRITABLE is not armed, since the knote also survives a later 0 -> READABLE transition) means SS_CANTSENDMORE, and its routing depends on what that proves:

    • fflags carries a pending socket error (a TCP reset always does): the poll error, routed exactly like epoll's implicit EPOLLERR, closing with SO_ERROR.
    • a shut-down socket at zero events: the read knote is gone, so the echo is its only close signal; it dispatches as eof and the shut-down branch in loop.c clean-closes. For the half-open path (peer's FIN consumed, then we shut down) that is exact epoll parity with EPOLLHUP. For a paused socket it closes on our own shutdown's echo, which is what kqueue did before this PR and what node:http's mid-upload half-close depends on (the socket with the unread request body is never resumed); epoll instead waits for the peer's FIN to complete SHUTDOWN_MASK, a pre-existing platform difference this PR keeps rather than widens.
    • otherwise silent: a resumed reader hears the peer through the read filter (a stale echo must not clean-close it early), and a unix peer's graceful close() (SS_CANTSENDMORE with so_error 0) on a not-shut-down socket must not be torn down with a fabricated ECONNRESET for what epoll reports as a mere EPOLLHUP; a paused not-shut-down socket keeps its queued data readable for resume().

    Polls with WRITABLE armed dispatch exactly as before.

  • us_poll_stop: delete both filters explicitly. The detector knote is invisible to the event diff, and detach paths (us_socket_detach) stop the poll while keeping the fd open; a leftover knote would keep the freed poll as udata.

The same gap was noted in the test comment of #37098 ("kqueue keeps no kernel filter on such a socket, so the reset goes unseen there"). It is distinct from #37077, which handles EV_EOF arriving on a still-armed write filter behind pending writes; here the problem is that no filter exists to deliver anything. The two are compatible.

Tests

test/js/bun/net/socket.test.ts, all running on Linux and macOS (skipped on Windows, whose libuv backend tracks this state separately):

  • "allowHalfOpen socket sees the peer reset after end + drain": FIN, wait for on_end plus the drain dispatch, then RST, require close/error to fire. Times out on an unfixed macOS build; on Linux it passes before and after (locking in the epoll behavior the fix mirrors).
  • "paused socket sees the peer reset": pause() reaches the same zero-event state, and a peer reset must still close the socket (Linux already does via EPOLLERR). Also times out on an unfixed macOS build.
  • "paused unix socket keeps data from a peer that closed gracefully": the guard for the fflags gate. A graceful unix disconnect must not close the paused socket or discard its queued bytes; resume() delivers the data and then the clean end-of-stream.
  • "allowHalfOpen socket closes cleanly after both sides shut down": the end handler replies with socket.shutdown() (the native half-close node:net's end() maps to; the raw JS end() closes the socket itself after flushing), and the shutdown echo must deliver the clean close.
  • "half-closed paused socket still hears the peer after resume": shutdown() while paused then resume(); the surviving detector's stale echo must not clean-close the socket ahead of the peer's two-chunk reply and FIN.

This machine is Linux, where the bug does not reproduce, so the failing-before runs are CI's to show on the macOS lanes; locally bun bd test test/js/bun/net/socket.test.ts passes with the only failures being the file's pre-existing ones (identical list on main, network-restricted environment).


no test proof · iteration 1 · 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

…reset is seen

A socket polling for no events (an allowHalfOpen socket past on_end whose
writes drained, or a paused socket) kept no kqueue filter at all: the
FIN-detector EVFILT_WRITE was EV_ONESHOT, so the first (masked) writable
wakeup consumed it, and the post-dispatch us_poll_change(0) was a no-op.
A later RST was never delivered, close/error never fired, and the socket
leaked until process exit. epoll is immune because a zero-event
registration still reports EPOLLHUP/EPOLLERR.

- kqueue_change: arm the zero-event detector as EV_ADD | EV_CLEAR so it
  survives delivery, and arm it unconditionally so a WRITABLE -> 0
  transition converts a still-armed oneshot.
- us_poll_change: treat 0 -> 0 as a re-arm for socket polls, since the
  writable dispatch consumed the oneshot behind the tracked state.
- dispatcher: EV_EOF on the write filter of a zero-event socket poll is
  SS_CANTSENDMORE, i.e. the connection died; route it to the error path
  (like epoll's implicit EPOLLERR) instead of eof, which would re-run
  on_end on a half-open socket. After our own shutdown it only counts
  with a pending socket error in fflags.
- us_poll_stop: delete both filters explicitly; the detector knote is
  invisible to the event diff and must not outlive the poll on detach
  paths that keep the fd open.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The kqueue event path separates read EOF from write-side EOF and preserves termination errors for zero-interest sockets. Persistent write detectors are re-armed and explicitly removed. Non-Windows tests cover half-open, paused, reset, and shutdown socket cases.

Changes

Kqueue socket termination

Layer / File(s) Summary
EOF coalescing and dispatch
packages/bun-usockets/src/eventing/epoll_kqueue.c
Kqueue events track write-side EOF and write-side EOF errors separately. Dispatch routes zero-interest termination to the error path when required.
Zero-interest detector lifecycle
packages/bun-usockets/src/eventing/epoll_kqueue.c
Zero-interest registrations use persistent write detection. Poll changes re-arm the detector, and poll stopping deletes both filters with EINTR retries.
Socket termination regression coverage
test/js/bun/net/socket.test.ts
Non-Windows tests cover TCP resets, paused Unix-socket closure, paused TCP resets, bilateral shutdown, and resumed half-closed sockets.

Possibly related PRs

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the kqueue fix that delivers peer resets to sockets polling for no events.
Description check ✅ Passed The description explains the issue, cause, fix, affected platforms, tests, and verification results, despite using different section headings.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Aug 7, 2026
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:38 AM PT - Aug 7th, 2026

@robobun, your commit 6fa9975a4ad4a1d8148ac114bccddf94813c8800 passed in Build #89976! 🎉


🧪   To try this PR locally:

bunx bun-pr 37101

That installs a local version of the PR into your bun-37101 executable, so you can run:

bun-37101 --bun

Comment thread packages/bun-usockets/src/eventing/epoll_kqueue.c Outdated
…s and shutdown echoes

EV_EOF on the detector knote is SS_CANTSENDMORE, not a read EOF, and it
is not always a dead peer: a unix peer's graceful close() sets it with
so_error == 0, and our own shutdown() echoes it too. Route it to the
error path only when fflags carries a socket error, or when a socket we
neither shut down nor paused loses its write side. A paused socket
without a pending error stays quiet so us_socket_resume can re-arm
EVFILT_READ and drain any queued data before the proper end-of-stream,
matching epoll's deferred EPOLLHUP handling. The write-side EV_EOF is
never surfaced as eof from the detector state.
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Both review findings were correct, fixed in d156602:

  • A unix peer's graceful close() sets SS_CANTSENDMORE with so_error == 0, so the plain-socket arm can no longer treat every write-side EV_EOF as a dead connection. The error route now requires a pending socket error in fflags, or a socket that is neither shut down nor paused. A paused socket without a pending error stays quiet, so resume() re-arms EVFILT_READ and drains any queued data before the normal end-of-stream, matching epoll's deferred EPOLLHUP handling instead of closing with a spurious ECONNRESET.
  • The shut-down arm previously let the own-shutdown echo fall through and dispatch as eof, which clean-closed a paused socket at loop.c:849 before resume() could read pending data. The write-side EV_EOF is now never surfaced as eof from the detector state, so the echo is a no-dispatch; a reset after our shutdown still closes through the error path via fflags.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@packages/bun-usockets/src/eventing/epoll_kqueue.c`:
- Around line 716-728: The kqueue filter operations currently ignore failures
and may leave stale poll state or free p while its udata is still referenced.
Update the kqueue_change calls around lines 711 and 725 in
packages/bun-usockets/src/eventing/epoll_kqueue.c to propagate specific errors,
and update us_poll_stop at lines 741-754 to handle each filter error and syscall
failure while ignoring only ENOENT for deletion. Ensure us_poll_free does not
release p until all possible kqueue references have been removed.

In `@test/js/bun/net/socket.test.ts`:
- Around line 688-704: Update the connect wait around client.once("connect",
resolve) to also reject on the client’s "error" event instead of swallowing
connection failures. Wrap the remaining test flow in try/finally and call
client.destroy() in finally so the socket is released when any await or
assertion fails.
🪄 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: 411b9bf4-df41-4112-a993-9c0e5e4310f6

📥 Commits

Reviewing files that changed from the base of the PR and between 45eda51 and d156602.

📒 Files selected for processing (2)
  • packages/bun-usockets/src/eventing/epoll_kqueue.c
  • test/js/bun/net/socket.test.ts

Comment thread packages/bun-usockets/src/eventing/epoll_kqueue.c
Comment thread test/js/bun/net/socket.test.ts
robobun added 2 commits August 7, 2026 05:51
…ending socket error

A unix peer's graceful close() sets SS_CANTSENDMORE with so_error == 0,
the same write-side EV_EOF shape as our own shutdown echo, so treating
any not-paused socket's write EOF as a dead connection fabricated
ECONNRESET for a graceful disconnect that epoll reports as a mere
EPOLLHUP. A TCP reset always leaves the error in fflags, so gating the
error route on it keeps the deaf-socket fix while a graceful disconnect
leaves the socket reachable through resume(), a write's EPIPE, or a
later reset.

Tests: a paused socket must still close on a peer reset (EPOLLERR
parity), and a paused unix socket must survive a graceful peer close
with its queued data intact through resume().
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Self-review turned up one more hole in the b825723 predecessor: the zero-event detector treated any write-side EV_EOF on a not-paused socket as a dead connection, but a unix peer's graceful close() produces exactly that shape (SS_CANTSENDMORE with so_error == 0), so a half-open or paused unix socket would have been closed with a fabricated ECONNRESET where Linux sees a mere EPOLLHUP. b825723 tightens the rule to what epoll actually distinguishes: the detector routes to the error path only when fflags carries a pending socket error, which a TCP reset always does, and stays silent otherwise (own-shutdown echo, graceful unix disconnect, FIN after our shutdown), leaving the socket reachable through resume(), a write's EPIPE, or a later reset.

Two tests added alongside the existing one: a paused TCP socket must still close on a peer reset (it reaches the same zero-event state; Linux closes via EPOLLERR, and this also timed out on unfixed macOS), and a paused unix socket must survive a graceful peer close with its queued bytes intact through resume(). A note on coverage: Bun.listen({unix}) does not currently honor allowHalfOpen (#33498 is the open PR plumbing it), so the unix variant uses pause() to reach the zero-event state; the dispatcher rule covers both the same way.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/bun-usockets/src/eventing/epoll_kqueue.c (1)

359-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Route active write-filter resets to the error path.

Line 360 adds every send_eof to eof. Line 386 checks send_eof_err only when wanted == 0.

If an active socket receives EVFILT_WRITE | EV_EOF with nonzero fflags, this code dispatches error = 0 and eof = 1. The dispatcher then receives a read-EOF signal instead of the reset error signal.

Apply send_eof_err before the zero-interest condition. Keep an error-bearing write EOF out of eof. Add coverage for a reset while the socket still has armed events.

🤖 Prompt for 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.

In `@packages/bun-usockets/src/eventing/epoll_kqueue.c` around lines 359 - 390,
Update the kqueue event handling around send_eof so send_eof_err is applied
regardless of wanted being zero, routing error-bearing write EOFs through error
and excluding them from eof. Preserve the existing zero-interest handling for
non-error send EOFs, and add coverage for a reset on a socket with armed events.
🤖 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 723-748: Update the paused Unix socket test’s promise handling so
early teardown rejects gotData before onClosed is resolved. Wire the close() and
error() callbacks to reject the data promise when data() has not yet delivered,
while preserving successful resolution through onData and preventing later
success after failure.

---

Outside diff comments:
In `@packages/bun-usockets/src/eventing/epoll_kqueue.c`:
- Around line 359-390: Update the kqueue event handling around send_eof so
send_eof_err is applied regardless of wanted being zero, routing error-bearing
write EOFs through error and excluding them from eof. Preserve the existing
zero-interest handling for non-error send EOFs, and add coverage for a reset on
a socket with armed events.
🪄 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: de034d2c-2456-446a-a7ca-5dca75887b93

📥 Commits

Reviewing files that changed from the base of the PR and between d156602 and cfd67dc.

📒 Files selected for processing (2)
  • packages/bun-usockets/src/eventing/epoll_kqueue.c
  • test/js/bun/net/socket.test.ts

Comment thread test/js/bun/net/socket.test.ts Outdated
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

On the outside-diff suggestion to route an armed socket's write-filter EV_EOF with nonzero fflags to the error path: declining here, deliberately. For polls with events armed this PR keeps dispatch byte-identical to main; changing how an active socket's write EV_EOF is routed is exactly what #37077 is about (a reset behind pending writes re-fired on_end and spun the loop), and it needs the nuances that PR carries (distinguishing backpressure from a dead peer, the shut-down deaf state). Scoping this PR to polls at zero requested events means every event it touches is one main either never receives or mishandles into an on_end refire, which keeps the regression surface at the detector state its tests cover. If #37077 lands first I will rebase; the two changes are compatible.

Comment thread packages/bun-usockets/src/eventing/epoll_kqueue.c
Comment thread packages/bun-usockets/src/eventing/epoll_kqueue.c Outdated
…and only at zero events

Two corrections to the detector routing:

A shut-down, not-paused socket at zero events got there through the
half-open on_end path, which consumed the peer's FIN and deleted the
read knote, so the write-side shutdown echo is its only close signal.
Suppressing it leaked the socket; deliver it as eof so the shut-down
branch clean-closes, exactly like epoll's EPOLLHUP for the same
both-directions-shut state.

The detector knote also survives a 0 -> READABLE transition (the diff
has no reason to touch the write filter), so a stale echo can arrive
while the socket legitimately reads, where it must not clean-close
ahead of the peer's data and FIN; the read filter is the close signal
there. Gate the detector routing on WRITABLE-not-armed rather than
zero events, and scope the clean close to zero events.

Tests cover both directions with socket.shutdown() (the native
half-close node:net's end() uses; the raw end() closes at the JS
layer before the kernel signal matters).
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Both follow-up findings were correct, fixed in 1e748ff:

  • The shut-down, not-paused socket at zero events only gets there through the half-open on_end path, which already consumed the peer's FIN and deleted the read knote, so the write-side shutdown echo is its only close signal and suppressing it leaked the socket. It now dispatches as eof and the shut-down branch in loop.c clean-closes, matching epoll's EPOLLHUP for the both-directions-shut state.
  • The detector knote does survive a 0 -> READABLE transition, so the gate is now WRITABLE-not-armed instead of zero events: a stale echo arriving on a resumed reader is suppressed (the read filter is the close signal there), while the clean-close case above additionally requires zero events, which is what distinguishes "both sides done" from "still reading".

Tests for both: a server whose end handler replies with socket.shutdown() must see close (times out on kqueue without the first fix), and a paused socket that shutdowns then resumes must still receive the peer's two-chunk reply and FIN (loses data with the echo mis-delivered). Both use socket.shutdown(), the native half-close that node:net's end() maps to; the raw JS end() closes the socket itself after flushing, so it never depends on the kernel's close signal.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both findings from my previous pass are addressed in 1e748ff — the !(wanted & WRITABLE) gate and the shut-down/not-paused/zero-events clean-close arm trace correctly through the five test scenarios, and the WRITABLE-armed path is byte-identical to before. I'm deferring to a human here: this rewrites the kqueue socket-dispatch state machine (EV_ONESHOT→EV_CLEAR detector, 0→0 re-arm, unconditional filter deletion in us_poll_stop), the reasoning leans on XNU-specific filt_sowrite/SS_CANTSENDMORE semantics, and three review rounds each found a real state-transition bug — worth a maintainer's eyes and the macOS CI lanes before merging.

Checked this pass: the clean-close arm's invariant ("only route to wanted==0 without pause already consumed the peer's FIN") holds for the allow_half_open path at loop.c:865; the SSL low-prio parking path (loop.c:637) can also reach it but pre-PR leaked there too, so not a regression. us_poll_stop's unconditional dual EV_DELETE is safe for UDP/listen polls (ENOENT ignored), and the epoll-only us_internal_async_close caller doesn't reach the kqueue branch.

Extended reasoning...

Overview

The PR fixes a macOS-only socket leak: an allowHalfOpen or paused socket that reaches zero requested poll events becomes permanently deaf to a peer RST because the EV_ONESHOT write filter used as a detector was consumed on the first (trivially-writable) wakeup, leaving zero knotes on the fd. The fix in packages/bun-usockets/src/eventing/epoll_kqueue.c:

  • Switches the zero-event detector from EV_ONESHOT to EV_ADD | EV_CLEAR so it persists across the initial writable firing.
  • Adds a 0→0 branch in us_poll_change that re-arms the detector for socket polls (previously a no-op that left zero knotes).
  • Splits write-filter EV_EOF (send_eof/send_eof_err) from read-side eof in the kevent coalescer, and routes it in the dispatcher based on (kind, is_paused, wanted, fflags): socket error → poll error path; shut-down + not-paused + wanted==0 → clean-close via loop.c:849; everything else → suppressed.
  • Rewrites us_poll_stop to unconditionally EV_DELETE both filters, since the detector knote is invisible to the tracked-events diff and would otherwise survive detach paths with the freed poll as udata.

Five new tests in test/js/bun/net/socket.test.ts cover: half-open + RST, paused + RST, paused unix + graceful close (data preserved), half-open + mutual shutdown (clean close), and pause→shutdown→resume (stale echo suppressed).

Security risks

None identified. This is event-delivery plumbing; no parsing of untrusted input, no auth/crypto surface. The us_socket_t cast is guarded by poll-kind check.

Level of scrutiny

High. This is the core kqueue socket dispatch path — every macOS TCP/unix socket in Bun runs through it. The state space is (poll kind × is_paused × wanted × filter armed × EV_EOF × fflags), and three prior review passes on this PR each surfaced a real state-transition bug (fabricated ECONNRESET on unix graceful close; shut-down socket leak at wanted==0; stale echo after 0→READABLE resume). The author could not verify locally (Linux machine), so correctness rests on CI's macOS lanes and reviewer reasoning about XNU kqueue semantics.

Other factors

  • The current revision (1e748ff) correctly addresses both of my 06:29 findings; I re-traced all five test scenarios plus the WRITABLE-armed baseline through the new dispatcher and each reaches the intended loop.c branch.
  • The us_poll_stop change affects all kqueue us_poll_stop callers (socket close, detach, listen-socket close, UDP close); deleting an absent filter reports ENOENT via KEVENT_FLAG_ERROR_EVENTS and is harmless.
  • The clean-close arm's stated invariant depends on loop.c:865 being the only non-pause route to a not-shut-down socket at wanted==0; the SSL low-prio queue at loop.c:637 is another route, but a socket shut down from that state leaked pre-PR too (zero knotes), so closing it is not a regression.
  • Tests follow harness conventions (using/tempDir, port:0, error→reject wiring, try/finally cleanup) and each maps to a specific dispatcher branch. All are Windows-skipped; the macOS behavior is the only thing being fixed and Linux serves as the reference.

Given the criticality of this code path, the subtlety of the XNU semantics involved, and the iteration count, this should have maintainer sign-off and green macOS CI rather than bot approval.

macOS CI's node-http-halfclose-midupload test showed the paused
exclusion broke node:http's mid-upload half-close: the response side
ends a socket whose unread request body keeps it paused, nothing ever
resumes it, and the suppressed echo left connection close undelivered.
A shut-down socket with no read knote has no other signal, and closing
on the echo is what kqueue did before this PR; epoll instead waits for
the peer's FIN to complete SHUTDOWN_MASK, a pre-existing platform
difference this change keeps rather than widens. The resumed-reader
suppression stands: wanted == 0 is what distinguishes the two.
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI on the macOS lanes caught an over-correction: excluding paused sockets from the shut-down clean close broke node-http-halfclose-midupload there. node:http half-closes the response side while the unread request body keeps the socket paused, nothing ever resumes it, and the suppressed echo left the connection's close undelivered. 6fa9975 restores the close on the echo for any shut-down socket at zero events, which is what kqueue did before this PR; the resumed-reader suppression stands because wanted == 0 (read knote gone, nothing else can signal) is exactly what separates the two cases. epoll waits for the peer's FIN to complete SHUTDOWN_MASK before its EPOLLHUP, so Linux and macOS keep the same pre-existing difference in that corner, no wider than main's.

For the earlier review note about a paused socket closing on its own shutdown echo before resume() can drain queued data: that remains main's macOS behavior, deliberately unchanged here after CI showed real consumers depend on the close. Deferring it like epoll does would need a read-side FIN detector for shut-down sockets, which is beyond this fix.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 920-928: Replace the two fixed setImmediate waits in the socket
shutdown/resume test around socket.shutdown() and socket.resume() with an
observable condition confirming the poll has reached zero events and the
detector is armed; otherwise document and validate why the existing timing is
deterministic. Ensure the test demonstrably exercises the zero-interest detector
path and fails on the pre-fix implementation.
🪄 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: 44b55eae-27ec-4ab6-95d0-4b71a099b57a

📥 Commits

Reviewing files that changed from the base of the PR and between cfd67dc and 6fa9975.

📒 Files selected for processing (2)
  • packages/bun-usockets/src/eventing/epoll_kqueue.c
  • test/js/bun/net/socket.test.ts

Comment thread test/js/bun/net/socket.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of my earlier findings were addressed in 1e748ff, and 6fa9975's CI-driven adjustment (dropping the is_paused gate on the shut-down clean-close arm) checks out against the widened !(wanted & WRITABLE) gate — the resumed-reader case is still protected because wanted == READABLE falls through to the silent branch. I didn't find anything new this pass.

That said, this rewires kqueue's zero-event detector and the socket-poll dispatch path that every macOS connection flows through, and it took three correction rounds (two from review, one from CI on the macOS lanes) to converge. A human should sign off on the final state-machine, particularly the acknowledged epoll/kqueue divergence for paused+shut-down sockets and the us_poll_stop unconditional-delete change.

What was reviewed

  • The send_eof routing table against each of the five new tests and the node:http half-close-mid-upload case that drove 6fa9975.
  • us_poll_stop's explicit dual-filter delete vs. detach paths that keep the fd open — the ENOENT-on-absent-filter reasoning holds.
  • The 0→0 re-arm scoped to POLL_TYPE_SOCKET/SHUT_DOWN only, so callback/listen polls are untouched.
Extended reasoning...

Overview

This PR fixes a macOS-only socket leak: an allowHalfOpen or paused socket that reached zero requested poll events lost its last kqueue filter (the EV_ONESHOT write knote was consumed by the first trivially-writable wakeup), leaving the fd with no kernel presence at all — a later peer RST was never delivered. The fix replaces the oneshot with an EV_ADD | EV_CLEAR detector, adds a 0→0 re-arm in us_poll_change, teaches the dispatcher to route the detector's EV_EOF by fflags/kind/wanted-events, and makes us_poll_stop unconditionally delete both filters so detach paths don't leave a stale udata pointing at freed memory. Five new tests in socket.test.ts cover the reset, graceful-unix-close, both-sides-shutdown, and stale-echo-after-resume cases.

Security risks

None. This is event-delivery correctness in the C socket layer; no user input parsing, auth, or crypto paths are touched. The us_poll_stop change actually closes a pre-existing use-after-free window (a leftover knote holding a freed us_poll_t* as udata on the us_socket_detach path).

Level of scrutiny

High. us_internal_dispatch_ready_polls, kqueue_change, us_poll_change, and us_poll_stop are the four functions every macOS socket's lifecycle passes through. A wrong branch here means loop spins, silent data loss, or leaked fds across the entire runtime — HTTP server, fetch, node:net, node:http. The PR itself demonstrates this: an intermediate revision (1e748ff's is_paused exclusion) broke node-http-halfclose-midupload on the macOS CI lanes and had to be reverted in 6fa9975. The author is on Linux and explicitly deferred macOS verification to CI.

Prior review findings

My previous run flagged two regressions in the intermediate d156602 state — a shut-down not-paused socket leaking (echo over-suppressed), and the wanted == 0 gate missing the 0→READABLE detector-survival case. Both were fixed in 1e748ff with tests. The subsequent 6fa9975 change removes the is_paused gate from the shut-down clean-close arm, which I re-traced: it restores pre-PR behavior for paused+shut-down sockets at zero events (they close on the echo, as node:http depends on), while the resumed-reader protection survives because wanted == READABLE still routes to the silent branch. The author explicitly documents the remaining epoll/kqueue divergence (a paused+shut-down socket closes on its own echo on kqueue but waits for the peer's FIN on epoll) as pre-existing and out of scope.

Other factors

  • Three correction rounds to converge, one caught only by platform-specific CI, is a strong signal that the state space here is subtle enough to warrant a maintainer's read of the final routing table.
  • The dispatcher change is scoped to the !(wanted & WRITABLE) case for socket-kind polls; polls with WRITABLE armed dispatch byte-identically to main (the author explicitly deferred that to #37077).
  • Tests are well-constructed (failure events wired to reject, resources released via using/finally, no external network), but the load-bearing macOS behavior can only be verified in CI, and build #89976 for the final commit was still running when I checked.
  • The us_poll_stop change from diff-based to unconditional-delete-both is a small but real semantic widening that a maintainer should confirm is safe for every us_poll_stop caller (listen sockets, callback polls, UDP, connecting sockets), not just the socket-detach path the comment names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant