Skip to content

usockets: keep the kernel poll registration in sync across us_poll_resize - #37098

Open
robobun wants to merge 8 commits into
mainfrom
farm/22cf8486/fix-poll-resize-rearm
Open

usockets: keep the kernel poll registration in sync across us_poll_resize#37098
robobun wants to merge 8 commits into
mainfrom
farm/22cf8486/fix-poll-resize-rearm

Conversation

@robobun

@robobun robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

us_poll_resize (the grow path of socket adoption, called from us_socket_adopt) re-registers the kernel poll under the new poll pointer. Both branches get it wrong:

kqueue force-arms both filters regardless of the poll's actual interest:

kqueue_change(loop->fd, new_p->state.fd, 0, LIBUS_SOCKET_WRITABLE | LIBUS_SOCKET_READABLE, new_p);

while the memcpy'd poll state keeps the old polling bits. For a socket not watching both directions (paused, or half-open after on_end), the kernel ends up with a level-triggered EVFILT_READ the poll does not know about. The dispatcher masks it (events &= us_poll_events(poll)) but never deletes it, and us_poll_change can never diff it away because the poll state says READ was never armed. A masked level-triggered filter with data or a FIN pending re-fires on every kevent call: a 100% CPU spin.

epoll goes through us_poll_change(new_p, loop, events), whose old != new diff skips the epoll_ctl entirely when events == 0. Zero-event polls are a real steady state (a half-open socket after on_end once its write buffer drained, relying on the implicit EPOLLHUP/EPOLLERR, see the comment in us_poll_start_rc). Skipping the MOD leaves the kernel epitem's data.ptr pointing at the old poll, which us_socket_adopt retires and frees. A later EPOLLHUP/EPOLLERR (reported even at zero interest) dispatches the freed pointer:

==ERROR: AddressSanitizer: heap-use-after-free ... thread T0
    #0 us_poll_events packages/bun-usockets/src/eventing/epoll_kqueue.c:94:23
    #1 us_internal_dispatch_ready_polls packages/bun-usockets/src/eventing/epoll_kqueue.c:277:23
    #2 us_loop_run_bun_tick packages/bun-usockets/src/eventing/epoll_kqueue.c:508:5

The fix:

  • epoll: issue the EPOLL_CTL_MOD unconditionally, since the MOD is what moves data.ptr to the new poll.
  • kqueue: arm exactly the poll's current interest, with EV_ADD updating udata in place on retained filters and EV_DELETE removing filters the poll does not want (so a stale FIN-detector oneshot cannot outlive the old poll). At zero events the FIN-detector EVFILT_WRITE oneshot is re-added so a pending knote's udata moves off the old poll. The adds are ordered before the one possible delete: on FreeBSD the kevent64 shim passes no eventlist, so the first error (the delete's benign ENOENT) aborts the rest of the changelist.
  • Both branches panic if the re-registration fails: the adoption cannot be unwound at this point, and a failed re-registration leaves the kernel referencing memory the caller frees, so a loud crash at the faulting call beats a use-after-free later (same policy as the eventfd failure in us_internal_create_async). The trailing delete's ENOENT stays tolerated.

Reachability and how this is tested

No in-tree adopter grows its ext today, so the grow path cannot execute on its own: upgradeTLS, the Postgres/MySQL TLS upgrades, and WebSocket client adoption pass equal pointer-sized ext sizes, and the uWS server WebSocket upgrade shrinks (sizeof(WebSocketData) + sizeof(void *) = 168 vs sizeof(HttpResponseData<SSL>) = 232 under libstdc++; 160 vs 224 under libc++). Verified empirically with a probe: every adoption reports a non-growing resize and takes the early return.

The margin is one struct change, though: WebSocketData gained fields recently for node:http compat, and 64 more bytes flip the WebSocket upgrade back into the grow path, at which point the failure modes above appear far from their cause.

To make the path testable for real, this PR adds a US_FAULT_ADOPT_GROW point to the existing socket fault-injection framework (the framework exists precisely for failure paths that are unreachable without injection, like US_FAULT_POLL_START and US_FAULT_SSL_LOOP_BUFFER). Armed with socketFaultInjection.set({ syscall: "adopt_grow", action: "short", bytes: N }), us_socket_adopt inflates the new ext size by N, so us_poll_resize genuinely reallocates, re-registers the kernel poll, and retires the old socket. Over-allocating is memory-safe (us_calloc zeroes the tail; the copy uses the old size).

Two tests in test/js/bun/net/socket.test.ts drive it:

  • "upgradeTLS after the peer half-closed survives a subsequent reset" (Linux, the zero-event epoll state): client half-closes, the server's end fires and the poll drains to zero events, upgradeTLS adopts with forced growth, the loop retires the old socket, then the client resets. On the previous resize code this is the deterministic ASAN heap-use-after-free quoted above; with the fix the reset dispatches to the live poll and tears down exactly once.
  • "upgradeTLS survives a forced poll reallocation (handshake + echo)": a full TLS handshake and echo through a forcibly relocated socket that is actively polling readable, covering the grow path's normal-interest re-registration on every backend.

Both run wherever fault injection is compiled in (default for ASAN builds, i.e. the linux-x64-asan CI lane; release lanes skip them). On a tree without this PR the first test fails at the setter (adopt_grow is rejected as unknown), which is the missing-hook failure, not the crash; the crash proof is the revert experiment above (hook and test present, resize fix reverted to main's version). macOS CI lanes are release-only, so the kqueue branch executes under injection only on local darwin debug builds; it is additionally compile-checked against the real headers by the macOS lanes.

These tests also give first executed coverage to the socket-retirement machinery from #25361 (adopted/prev redirection, closed-list deferral), which was equally unreachable before.

How did you verify your code works?

  • The revert experiment: with the injection hook and tests in place and only us_poll_resize reverted to main's version, the half-closed test crashes with the ASAN heap-use-after-free above on every run; with the fix it passes.
  • test/js/bun/net/socket.test.ts (including both new tests), tcp-server.test.ts, websocket-server*.test.ts, and node-net.test.ts under the ASAN debug build show failure sets identical to pristine main and to the released build (the deltas are this sandbox's known environmental failures: no public DNS, load-dependent timeouts on the publish suite).
  • The kqueue branch is syntax-checked against a header shim locally and compiles for real on the macOS CI lanes.

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

…_poll_resize

The grow path re-registered the kernel poll under the new pointer with
two defects:

- kqueue: it armed EVFILT_READ and EVFILT_WRITE unconditionally while
  the memcpy'd poll state kept the old polling bits. For a socket not
  watching both directions, the dispatcher masks the extra
  level-triggered filter but can never delete it (us_poll_change diffs
  against poll state that says it was never armed), so pending data or
  a FIN would re-fire it on every kevent call.
- epoll: it went through us_poll_change, whose old==new diff skips the
  EPOLL_CTL_MOD entirely at zero events (a real steady state for a
  half-open socket after on_end), leaving the kernel epitem's data.ptr
  aimed at the old poll that us_socket_adopt frees.

Arm exactly the poll's current events on kqueue, re-adding the
zero-event FIN-detector oneshot so its udata follows the new poll and
deleting filters the poll does not want, and issue the EPOLL_CTL_MOD
unconditionally on epoll.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 23 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ce2643f8-705b-4fc2-ac98-c2f7805af3f8

📥 Commits

Reviewing files that changed from the base of the PR and between 85c50d0 and 662fd72.

📒 Files selected for processing (3)
  • packages/bun-usockets/src/context.c
  • src/runtime/socket/socket_body.rs
  • test/js/bun/net/socket.test.ts

Walkthrough

Changes

The poll resize path now rebinds epoll and kqueue registrations. Socket fault injection can force adoption growth. TLS tests cover teardown after half-close and continued echo after poll reallocation.

Poll resize and TLS handling

Layer / File(s) Summary
Adoption growth fault injection
packages/bun-usockets/src/internal/fault_inject.h, packages/bun-usockets/src/context.c, src/js/internal-for-testing.ts, src/runtime/socket/socket_body.rs, src/uws_sys/lib.rs
Adds the adopt_grow fault hook, validates its actions, documents its byte parameter, and increases adopted socket extension size before poll resizing.
Kernel poll registration rebinding
packages/bun-usockets/src/eventing/epoll_kqueue.c
us_poll_resize updates epoll and kqueue registrations for the resized poll, preserves zero-event handling, retries interrupted operations, and panics on other registration failures.
TLS resize and teardown tests
test/js/bun/net/socket.test.ts
Adds Linux-only half-close/reset coverage and cross-platform TLS handshake and echo coverage after forced poll reallocation.

Possibly related PRs

  • oven-sh/bun#37077: Both modify epoll/kqueue event registration and cover socket teardown after half-close or reset.

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 and concisely describes the main change: synchronizing kernel poll registrations during us_poll_resize.
Description check ✅ Passed The description includes both required sections and provides detailed scope, rationale, testing, and verification information.

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 12:25 AM PT - Aug 7th, 2026

@robobun, your commit 662fd72 has 1 failures in Build #89931 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37098

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

bun-37098 --bun

Comment thread packages/bun-usockets/src/eventing/epoll_kqueue.c
…V_DELETE

On FreeBSD the kevent64 shim passes no eventlist, so the first failing
change aborts the rest of the changelist. With the EVFILT_READ EV_DELETE
first, a poll at zero events or WRITABLE-only would ENOENT before the
EV_ADD that moves the write knote's udata to the new poll. Emit the adds
first; at most one delete remains and it is always last.

@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 640-643: Update the event-registration error handling around the
kevent64 call and the corresponding epoll branch to check ret for syscall
failure and inspect returned error events in change_list. Detect failures for
EV_ADD operations and propagate them so callers do not free polls still
referenced by kernel state, while continuing to tolerate the expected ENOENT
from trailing EV_DELETE operations by filtering each event’s filter and flags.

In `@test/js/bun/net/socket.test.ts`:
- Around line 692-694: Make the precondition around the two setImmediate calls
observable instead of relying on a fixed hop count: verify that the poll has
reached zero watched events before invoking upgradeTLS(). Preserve the intended
scheduling behavior, but fail the test explicitly if the required state is not
reached, ensuring the regression case exercises the production guard rather than
passing silently.
🪄 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: a4c2e046-8957-436b-850f-a13aa3b74fcc

📥 Commits

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

📒 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 Outdated
A registration failure here leaves the kernel referencing the old poll,
which the caller frees; an invariant break or kernel OOM at this point
cannot be unwound mid-adoption, so follow the us_internal_create_async
precedent and panic instead of leaving a use-after-free behind. The
trailing EV_DELETE's ENOENT stays tolerated.

Also widen the test's event-loop yield into a slack loop with a comment
explaining why it is not a timing condition.
Comment thread test/js/bun/net/socket.test.ts Outdated
robobun added 2 commits August 7, 2026 05:26
A connect failure or an early server-socket death now rejects with a
diagnosable error instead of hanging the test to its timeout.
…stable

Every in-tree adopter passes an equal or smaller ext size, so
us_poll_resize's grow path (reallocate, re-register the kernel poll
under the new pointer, retire the old socket) never executes on its
own. US_FAULT_ADOPT_GROW (action "short", bytes = N) inflates the
adopted ext size in us_socket_adopt so tests can drive that path for
real.

The half-closed upgradeTLS test arms it when available and now yields
past the old socket's retirement before the peer resets: on the
previous resize code this is a deterministic ASAN heap-use-after-free
(the kernel's epitem still points at the freed poll); with the fix the
reset dispatches cleanly. A second test drives a full TLS handshake
and echo through a forcibly relocated socket.

Also tightens two comments in us_poll_resize: state the live
registration invariant instead of narrating the replaced code, and
describe the 0-event FIN-detector re-add as moving or re-arming a
possibly-pending oneshot rather than matching a maintained invariant.
Comment thread src/js/internal-for-testing.ts
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/uws_sys/lib.rs
Comment thread src/runtime/socket/socket_body.rs

@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: 3

🤖 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/context.c`:
- Around line 316-321: Update the adopt-grow handling around US_FAULT_CHECK to
use 0 instead of INT_MAX as the fault_grow_bytes sentinel, since valid rules
require bytes > 0. Before adding fault_grow_bytes to ext_size, validate it does
not exceed INT_MAX - ext_size; only perform the addition and subsequent resize
when the check passes.

In `@src/runtime/socket/socket_body.rs`:
- Around line 5147-5152: Update the validation error in the adopt_grow action
check to identify both accepted actions, “short” and “none,” as the valid
remedies. Keep the existing ACTION_SHORT/ACTION_NONE validation logic unchanged
and revise only the message returned by the surrounding syscall validation.

In `@test/js/bun/net/socket.test.ts`:
- Around line 776-799: Update the TLS server and client close handlers in the
echo test to call onEchoFail when the echo promise is not yet settled, while
allowing close events after successful data handling. Preserve the existing
client data flow and ensure both close paths reject early instead of waiting for
the outer timeout.
🪄 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: 5f3b3b89-0a70-4620-8b7f-656a335b00ae

📥 Commits

Reviewing files that changed from the base of the PR and between 563e318 and 85c50d0.

📒 Files selected for processing (7)
  • packages/bun-usockets/src/context.c
  • packages/bun-usockets/src/eventing/epoll_kqueue.c
  • packages/bun-usockets/src/internal/fault_inject.h
  • src/js/internal-for-testing.ts
  • src/runtime/socket/socket_body.rs
  • src/uws_sys/lib.rs
  • test/js/bun/net/socket.test.ts

Comment thread packages/bun-usockets/src/context.c
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread test/js/bun/net/socket.test.ts Outdated
…n error message, reject early closes in the echo test
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for 662fd72: the only red lane failure is test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (SIGABRT on the x64-asan lane), which also fails identically on earlier builds of this branch before the related commits existed and is untouched by this diff; it has been reported separately. The remaining entries passed on retry. The new socket tests passed on the ASAN lane, which is the lane that exercises the forced-growth path for real.

@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.

No bugs found this pass, but this is low-level event-loop C touching kernel poll registration across epoll/kqueue/FreeBSD-shim with a new fail-fast policy — worth a human look.

What was reviewed:

  • kqueue change_list[2] bounds: enumerated all four events states, each writes exactly 2 entries (adds before the one delete, per the FreeBSD-shim ordering constraint).
  • EVFILT_WRITE re-registration uses EV_ADD | EV_ONESHOT in all cases — matches kqueue_change's existing convention for the WRITABLE filter.
  • epoll path no longer strips/restores poll_type: fine, new_p is memcpy'd so its polling bits already match events.
  • Fault-injection hook: INT_MAX sentinel + overflow guard, action/syscall validation, and test rejection wiring all look consistent.
Extended reasoning...

Overview

The PR rewrites us_poll_resize's grow path in packages/bun-usockets/src/eventing/epoll_kqueue.c to correctly re-register the kernel poll under the new pointer on both epoll (unconditional EPOLL_CTL_MOD so data.ptr moves even at zero events) and kqueue (arm exactly the poll's current interest, adds-before-deletes so the FreeBSD kevent64 shim's abort-on-first-error can only abort on the benign trailing ENOENT). Both branches now BUN_PANIC on registration failure. To make the currently-unreachable grow path testable, it adds a US_FAULT_ADOPT_GROW fault-injection point (context.c, fault_inject.h, uws_sys/lib.rs, socket_body.rs, internal-for-testing.ts) and two tests in socket.test.ts.

Security risks

None identified. No user-facing input reaches the changed paths; the fault-injection hook is compiled in only under LIBUS_SOCKET_FAULT_INJECTION (ASAN builds) and is exposed through bun:internal-for-testing.

Level of scrutiny

High. This is core event-loop C dealing with kernel poll registration semantics across three backends, and the failure mode being fixed is a use-after-free. The kqueue reasoning (oneshot FIN-detector knote survival, EV_ADD updating udata in place, FreeBSD shim changelist-abort semantics, ENOENT tolerance) is intricate and the PR itself notes the kqueue branch under injection only executes on local darwin debug builds — macOS CI lanes are release-only and only compile-check it. The new BUN_PANIC fail-fast policy is a design decision.

Other factors

Prior review rounds already addressed: my earlier FreeBSD changelist-ordering finding (EV_ADDs now precede the one possible EV_DELETE), test promise-rejection wiring, the CodeRabbit overflow-guard/error-message/close-rejection nits, and the comment-cop trims — all threads are resolved. The bug hunting system found nothing this run. CI is reported green on the ASAN lane where the new tests actually run. Still, given the cross-platform kernel-API subtlety and the currently-unreachable-in-production nature of the path, a human maintainer should sign off on the kqueue logic and the panic-on-failure choice.

Jarred-Sumner pushed a commit that referenced this pull request Aug 12, 2026
…live socket (#37661)

### What does this PR do?

When `us_socket_adopt` has to grow a socket's ext it goes through
`us_poll_resize`, which allocates a new `us_socket_t`, copies the old
one over, and retires the old block: `is_closed = 1`, `flags.adopted =
1`, `prev = <replacement>`, pushed onto `loop->data.closed_head`
(context.c, `us_socket_adopt`). The dispatcher in `loop.c` is usually
holding the old pointer while this happens (the adoption runs inside
`on_open`, `on_writable` or `on_data`), so after each of those callbacks
it re-derives the live socket from that bookkeeping. Until now every one
of those sites followed exactly one link:

```c
if (s && s->flags.adopted && s->prev) {
    s = s->prev;
}
```

That is only right if the socket was relocated once per callback. Every
relocation retires its source block the same way, so a callback that
adopts twice before returning (`old -> mid -> new`, with `mid` also
flagged and pointing at `new`) leaves one hop parked on `mid`, which is
itself retired. The `is_closed` checks that follow then give up on the
rest of the event for a socket that is still live: the readable half of
a combined readable+writable event, the rest of the recv drain loop and
the eof/error handling behind it (loop.c), the deferred-accept readable
dispatch after `on_open`, and on Windows the paused-socket probe and
`fin_deferred` bookkeeping in `poll_cb` (libuv.c). The kernel re-reports
these level-triggered conditions on the next tick, so the result is a
dropped event tail rather than a crash, but the bookkeeping is only
correct because there happens to be a single link.

The fix replaces the five copies of the one-hop `if` with one helper in
`internal/internal.h`, `us_internal_socket_follow_adopted`, which walks
to the end of the chain. `loop.c` uses it at the four dispatch sites,
and the libuv.c `us_internal_poll_cb_adopted_socket` helper that
`poll_cb` already funnels through now calls it too. Nothing else reads
`flags.adopted`, so these are all the readers.

Why walking the chain is sound:

- Every block on the chain is still allocated while a dispatch holds it.
Retired blocks sit on `closed_head` and are only freed by
`us_internal_free_closed_sockets`, which runs in the outermost tick's
`us_internal_loop_post`, never from inside a dispatch (see the
`tick_depth` check in loop.c). For the same reason a retired block's
memory cannot be handed out as a new socket within the tick.
- The walk terminates on the live block. `us_poll_resize` copies the
source before `us_socket_adopt` flags it, so the copy, and therefore the
live tail, always has `adopted` clear. `adopted` is set in exactly one
place and always together with `prev`, so every link the walk follows is
a forwarding link.

When `adopted` is clear, which is every socket today, the `while`
evaluates its condition once, which is the same work the `if` did. The
NULL tolerance of the old checks is kept because `on_writable` and
`on_data` may return NULL.

### Reachability and testing

This PR has no test because the code it changes cannot currently
execute. No in-tree adopter grows its ext: `upgradeTLS`, the Postgres
and MySQL TLS upgrades (`adopt_tls`) and the WebSocket client
(`adopt_group`) pass equal pointer-sized old/new ext sizes, and the uWS
server WebSocket upgrade shrinks (checked against this tree's headers:
`sizeof(WebSocketData) + sizeof(void *)` is 168 vs
`sizeof(HttpResponseData<SSL>)` at 232), so `us_poll_resize` always
takes its early return and `flags.adopted` is never set. The redirection
itself dates from #25361; #37098 (open, same grow path) reaches the same
conclusion and adds a fault-injection hook so that a single relocation
can be exercised. A two-link chain additionally needs two adoptions
inside one callback, which nothing JS-reachable does even with that
hook. There is therefore no input on which the old and new code behave
differently, and any test added here would pass with or without the
change, so none is included. The change adds no state and touches no
data structures.

It also does not touch `context.c` or `epoll_kqueue.c`, so it is
independent of #37098; if that lands first, its tests run the helper's
one-link case for real.

### How did you verify your code works?

- The debug (ASAN) build compiles, including the C++ translation units
that include `internal.h` (`AsyncSocket.h`, `libuwsockets.cpp`,
`bindings.cpp`). libuv.c is Windows-only and its change is a one-line
body replacement behind the same signature; the Windows CI lanes compile
it.
- `bun bd test test/js/bun/net/socket.test.ts` and
`test/js/bun/websocket/websocket-server.test.ts` cover the in-tree
adopters (`upgradeTLS`, server WebSocket upgrade). The only failures are
the ones this sandbox also produces with the released binary:
`localhost` resolving to `::1` makes a handful of `Bun.connect` tests
fail with ECONNREFUSED, one test needs public DNS, and the 300k-message
`(benchmark)` case exceeds its 30s budget under ASAN. Everything else
passes.
@robobun

robobun commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up: #39610 changes the kqueue rule this PR's us_poll_resize branch assumes. A socket poll now keeps its EVFILT_READ knote (EV_CLEAR) while it is not reading, so the EV_DELETE of the read filter here would remove it. With the new rule the existing unconditional re-add is a udata move that keeps each knote's mode, which is what the spin this PR describes needs.

Jarred-Sumner pushed a commit that referenced this pull request Aug 19, 2026
### Problem
- `test/js/node/tls/node-tls-server.test.ts` is red on every darwin lane
since #39600: "reports the reset that arrives while the socket is paused
as ECONNRESET, not 'end'" hangs to the timeout. Linux passes.
- Cause: on kqueue a paused socket has no filter. `us_socket_pause`
(`socket.c:864`) deletes the read filter, and the one-shot write filter
it adds is consumed at once. The peer's RST is reported only after
`resume()`.
- Also found: `EV_ADD` on an existing knote keeps its flags (macOS 14
and 26). The read sentinel from #37077 stayed edge-triggered after the
re-add on resume.

### Fix
- `kqueue_change` (`epoll_kqueue.c`) keeps the read knote of a socket
poll in both modes: level-triggered while the socket reads, `EV_CLEAR`
while it does not. A mode switch deletes the knote and adds a new one.
`us_poll_stop` deletes it in either mode.
- The three hand-armed sentinel sites and the write filter added at zero
events are removed. A delete of an unregistered filter is not a failure.
- Correct because the dispatcher already handles these events: it masks
the readable bit, defers a FIN while paused, and closes on a reset.
After resume the knote is level-triggered again, so a partial drain
cannot stall.
- Verified: a six-scenario harness of the real `kqueue_change` on both
macs (notes), the related suites on Linux, and new `Bun.listen` tcp/tls
tests in `test/js/bun/net/socket.test.ts`. Linux behavior does not
change, so this PR's darwin lanes prove the red test.

### Background
- epoll reports HUP and ERR for a socket that polls nothing. libuv
probes for a reset. kqueue reports nothing without a knote, and its
write filter is one-shot since #25475.
- An `EV_CLEAR` knote fires once per activation (data, FIN, RST). The
dispatcher sees a FIN as eof and a reset as error (`fflags ==
ECONNRESET`).
- `us_poll_resize` re-adds both filters only to move the udata. `EV_ADD`
keeps the mode of an existing knote, so that still works.

<details><summary>Notes</summary>

Culprit: #39600 added the tests. The kqueue gap predates it: the write
filter became one-shot in #25475, and the sentinel from #37077 covered
shutdown and half-open but not pause. The rare darwin pass in CI was the
RST landing before the one-shot write filter was consumed.

kqueue probes, run as small C programs on darwin-arm64 (macOS 26.6, xnu
12377) and darwin-x64 (macOS 14.8, xnu 10063), identical results:
- read filter deleted + one-shot write consumed, then RST: no event (the
bug). RST while the one-shot is still armed: `EVFILT_WRITE` with `EV_EOF
fflags=54` (the flaky pass).
- `EV_ADD|EV_CLEAR` knote, then plain `EV_ADD`: still edge-triggered,
udata updated. `EV_ADD|EV_CLEAR` over a level knote: still level.
`EV_DELETE` + `EV_ADD` in one changelist: level again.
- `EV_CLEAR` knote, RST behind 10 unread bytes: one event, `EV_EOF
fflags=ECONNRESET`, `SO_ERROR=ECONNRESET`, no re-fire. FIN behind data:
one event, `fflags=0`, no re-fire. More data while not reading: one
wakeup per arrival.
- `EV_CLEAR` knote registered before our own `SHUT_WR` still reports the
peer's later FIN and RST, fresh or already cleared once. So dropping the
post-shutdown re-arm in `raw_shutdown` is safe.
- `KEVENT_FLAG_ERROR_EVENTS` with a failing delete first: the error
entry keeps `EV_DELETE` in flags and `ENOENT` in data, and the following
add still applies.

The real `kqueue_change` body, compiled into a harness on both machines:
pause then RST (reported, no spin), pause then FIN then resume (FIN
deferred once, level-triggered after resume), stop on a paused socket
(nothing left), resize touch (udata moved, mode kept), plain changes on
a reading socket, EBADF still reported. The changed files also compile
with `-fsyntax-only` in the kqueue configuration.

Linux, debug build: `node-tls-server.test.ts` (73 pass, the SNICallback
failure is pre-existing in this container), `node-net.test.ts`,
`node-net-allowHalfOpen.test.js`, `fetch-backpressure.test.ts`,
`node-http-backpressure.test.ts` (the same 15 pre-existing failures as
unmodified main: localhost resolution and h3), `socket.test.ts` (the
same 9 pre-existing failures as main, the 2 new tests pass), the nine
`test-net-half-open-peer-reset-*.mjs` fixtures,
`node-http-server-socket-end-drain`, `node-http-connect`,
`tls-syscall-fault`, `net-syscall-fault`.

The new `socket.test.ts` variants also fail on a release binary from
before #39600 (the close carried no error), so they pin that contract
for the Bun socket API on Linux as well. On macOS without this diff they
hang like the node ones.

Node itself does not report a reset while a socket is paused. libuv
removes the fd from the poll set, and node v26.3.0 reports `ECONNRESET`
after the resume (checked with the same server logic). Bun's epoll and
libuv backends have reported it at once for a long time, and #39600 made
that the tested contract. This PR only brings kqueue to the same
contract.

Related open PRs: #37098 rewrites the kqueue branch of `us_poll_resize`
and would delete the read knote of a non-reading socket, which this rule
keeps. #37099 touches the same pause code against an older base.
`JSNodeHTTPServerSocketPrototype.cpp:226` pauses and resumes around a
shutdown to work around the same gap. It still works and is left alone
here.

kqueue on FreeBSD (shimmed, not a CI target) stops a changelist at the
first failing entry. The delete in a mode switch always finds a knote,
because every socket poll starts with a level read filter, so the add
after it is not affected.

Windows lanes on the first CI run: the new tests reached the close with
`syscall: "read"` but no `code`. The libuv backend reports the reset on
a paused socket as intended, but on Windows the close carries the raw
WSA code, which `on_close` stores unmapped (node:net accepts `code ===
undefined` as a reset, which is why the node tests pass there). That is
a pre-existing bug in a different layer and is handed off separately.
The tests check the code on POSIX only until it is fixed.
</details>
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