Skip to content

usockets: defer eof for a paused socket that already sent FIN; stop backpressure pauses from holding the loop - #33974

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/98ab4ea7/usockets-defer-eof-while-paused
Aug 15, 2026
Merged

usockets: defer eof for a paused socket that already sent FIN; stop backpressure pauses from holding the loop#33974
Jarred-Sumner merged 1 commit into
mainfrom
farm/98ab4ea7/usockets-defer-eof-while-paused

Conversation

@robobun

@robobun robobun commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • A net.Socket that has already sent its FIN (end()) and then pauses under backpressure loses the tail of the peer's stream. The eof-while-paused deferral on main (node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488) exempts shut-down sockets, so the is_shut_down arm in us_internal_dispatch_ready_poll (packages/bun-usockets/src/loop.c) closes the socket while bytes are still queued in the kernel: kqueue reports EV_EOF on the readable event carrying the final data, epoll latches EPOLLHUP once both directions are down. Deterministic on Linux with a 1 MiB reply (end 586752 of 1048576 on main); about a third of runs on macOS.
  • The exemption's original reason was uws HTTP, but every uws HTTP socket is allow_half_open and pairs shutdown() with close(), so it never reaches this arm; what the exemption (and the earlier allow_half_open version of this PR) actually left truncating were Bun.connect/Bun.listen without allowHalfOpen and us_socket_from_fd sockets.
  • Deferring the FIN exposes a loop-hold bug in src/js/node/net.ts: the three push()-returned-false pause sites and the two onread-mode ones (callback returned false) pause the native handle but, unlike Socket.prototype.pause, keep the process alive. With a deferred FIN, a program that never reads a reply or a request sits forever; node (whose readStop'd handles are inactive) exits. The not-shut-down deferral already on main has this hang today (net.connect() to a server that writes and goes away, never read: hangs on main, exits in node).

Fix

  • loop.c: defer the eof hint whenever the socket is paused and read_eof is not set (a delivered FIN means nothing is left to drain, so the close stays prompt); shut-down sockets included. The existing epoll us_poll_stop applies to every deferral, so an AF_UNIX peer close (EPOLLHUP on a socket we did not shut down) cannot spin either. A second arm covers the batch race on epoll: an entry carrying EPOLLHUP collected while paused still dispatches after an earlier entry in the same batch resumed the socket, with no READABLE bit; it is left for the next poll, which re-reports it with READABLE and lets the read loop drain to recv()==0.
  • us_poll_change returns the result of re-registering a parked fd (it goes through us_poll_start_rc, so EPOLL_CTL_ADD failure is handled and fault-injectable); us_socket_resume closes the socket with that errno instead of leaving it registered nowhere.
  • net.ts: all five stop-reading sites drop the hold the way Socket.prototype.pause does (kPausedUnref; read()/_read()/resume() and the onread tail drain restore it), except while a write is waiting for drain, which re-refs in _write and is released by the drain handlers. kPausedUnref is cleared on every resume path and on ref(), so a drain never gives up a hold the user re-took.
  • Why this is right: a paused socket not observing the peer's FIN until it resumes, and not keeping the process alive while it is not reading, are both node's behavior (readStop); every fixture below was checked against node v26 and matches it.
  • Windows is affected too: libuv.c maps AFD DISCONNECT on a shut-down socket to the same eof hint, so it takes the new arm; the truncation tests now run there (they pass on current main there only because AFD's single forced recv() happens to cover the remainder at these sizes, and they exercise the resume re-report path on the branch).

Tests

test/js/node/net/node-net.test.ts (red on main, green with the change, unless noted):

  • delivers every buffered byte before 'end' when the data handler pauses (main: truncated)
  • stays parked without spinning the loop, then delivers the tail on resume (main: close 0; also bounds the CPU of the parked second)
  • client that never reads the reply and whose peer goes away exits (main: hangs)
  • onread client whose callback returns false, with and without end() (both hang on main here: the bare onread pause held the loop whether or not the FIN made it through the buffers)
  • client that end()s and never reads / server that end()s without reading the request exit (pass on main because main closes them; hang with the loop.c change alone, which is the regression the net.ts part prevents)
  • a write still waiting for drain keeps the process alive until it completes; a drain does not give up a hold re-taken with ref() (both fail with the naive versions of the net.ts change)
  • its named-pipe leak check is now awaited with a warmed baseline: left dangling, its assertion landed inside whichever test was running ~1s later and counted that test's sockets (this is what an earlier CI run hit once these tests shifted the timing); the pipe transport keeps exactly one wrapper reachable regardless of connection count, on main as well, which is what the warm-up absorbs.

test/js/bun/net/socket-syscall-fault.test.ts: resume of a parked socket whose re-registration fails surfaces read ENOMEM + close, instead of a deaf socket (epoll only; main closes the socket before the resume, so it is red there too).

Verified: full node-net.test.ts, socket.test.ts, tcp-server.test.ts, socket-syscall-fault.test.ts, fetch-backpressure.test.ts, node-http.test.ts, node-tls-server.test.ts, child_process.test.ts have failure sets identical to main in this environment (the shared failures are no-network / localhost dual-stack ones); the 327 test-net-*/test-tls-* node tests show the same two pre-existing failures as main. On Windows x64 (debug build): node-net.test.ts 77 pass / 0 fail, node-http.test.ts, fetch-backpressure.test.ts, socket-syscall-fault.test.ts clean, the pause-related test-net-*/test-http-pause* node tests pass. (net.Socket write > should allow reconnecting after end() is flaky on Windows debug builds on main as well: 1/12 there vs 4/24 here, a pre-existing 3ms race in that test, unrelated to this change.)

Background

  • eof hint: the eof argument to the dispatcher. Where it comes from differs per backend (kqueue EV_EOF, epoll EPOLLHUP, AFD DISCONNECT, or recv() returning 0 inside the read loop); only the last one proves the buffer is empty.
  • parked socket: on epoll EPOLLHUP is level-triggered and cannot be masked, so a paused socket that hung up is removed from the epoll set (us_poll_stop) and added back on resume; that re-add is a fresh registration.
  • read_eof: set once the peer's FIN has been delivered as on_end on a half-open socket.
  • onread mode: the onread connect option delivers reads into a caller-supplied buffer through a callback instead of the stream's push(); returning false from the callback is that mode's way of stopping reads.
  • kPausedUnref / kUserUnrefed: net.ts bookkeeping for the handle's hold on the event loop. kUserUnrefed records an explicit socket.unref(); kPausedUnref records that a pause dropped the hold on the user's behalf and the next resume should restore it.

Related: #35939 fixes a different problem in the same block (on_end re-dispatch after a partial write); the two are independent.

@robobun

robobun commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:01 PM PT - Aug 14th, 2026

@robobun, your commit fd148b5 is building: #97466

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. node:net: client data event never fires when write callback triggers end() against a loopback echo server #31383 - Client data event never fires when write callback triggers end() — EOF/close dispatched before inbound echo data is read from the kernel buffer
  2. Readable.pipe(net.Socket) closes connection before peer response is delivered #32231 - Readable.pipe(net.Socket) closes connection before peer response is delivered — socket emits end without ever emitting data for the server's response

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #31383
Fixes #32231

🤖 Generated with Claude Code

@robobun

robobun commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Checked both suggested issues against current main on Linux: neither #31383 nor #32231 reproduces (both deliver the expected data). #31383 is macOS-only and looks like the EVFILT_WRITE + EV_EOF case (no pause() involved), which #32257 targets rather than this change. Leaving them out of the Fixes list.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The socket implementation now tracks polling stopped during paused EOF handling, restores epoll polling on resume, and defers EOF dispatch. Linux-focused net tests cover buffered-byte delivery and paused-socket CPU usage.

Changes

Paused socket EOF handling

Layer / File(s) Summary
Track paused poll lifecycle
packages/bun-usockets/src/internal/internal.h, packages/bun-usockets/src/context.c, packages/bun-usockets/src/loop.c, packages/bun-usockets/src/socket.c
Sockets initialize paused_poll_stopped to zero. Resume re-adds epoll polling when the flag is set and otherwise updates the event mask.
Defer EOF dispatch while paused
packages/bun-usockets/src/loop.c, test/js/node/net/node-net.test.ts
Paused EOF handling stops repeated epoll polling and defers EOF dispatch. Regression tests verify buffered bytes arrive before closure and that the paused interval does not busy-spin.

Possibly related PRs

Suggested reviewers: cirospaciari

🚥 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 summarizes the primary EOF deferral and paused-socket loop behavior changes.
Description check ✅ Passed The description explains the problem, implementation, rationale, and extensive verification results, despite using different section headings than the template.

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

@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/loop.c`:
- Around line 755-763: The comment above the paused-socket handling exceeds the
repository’s three-line limit. Condense it to no more than three lines while
preserving only the essential behavior: defer EOF dispatch during pause so
buffered bytes are delivered before resume-driven readable processing.

In `@test/js/node/net/node-net.test.ts`:
- Around line 973-975: Remove the historical bug-context comments near the
kqueue and epoll handling in the test body, including both referenced blocks.
Keep the test focused on its setup, actions, and assertions without describing
pre-fix behavior or issue history.
- Line 1016: Update the stderr assertion in the test around the stdout/close
output expectation to tolerate benign ASAN/debug-build whitespace or warnings by
normalizing stderr before comparison, such as trimming it. Preserve the exact
stdout assertion and continue validating that normalized stderr is empty.
🪄 Autofix (Beta)

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: fa72c603-e162-4af2-84b9-d62a0fe39178

📥 Commits

Reviewing files that changed from the base of the PR and between 9657f37 and 7c1c3c0.

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

Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread test/js/node/net/node-net.test.ts Outdated
Comment thread test/js/node/net/node-net.test.ts Outdated

@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/node/net/node-net.test.ts`:
- Line 996: Replace the fixed setTimeout delay around conn.resume() with an
event-driven handoff that resumes the connection when the relevant stream or
socket event indicates it is ready. Preserve the test’s paused-EOF sequencing
while removing reliance on wall-clock timing.
🪄 Autofix (Beta)

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: 2f634252-66f8-4b59-89eb-76c60bef7d35

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1c3c0 and f5d507d.

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

Comment thread test/js/node/net/node-net.test.ts Outdated

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

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 packages/bun-usockets/src/loop.c:764 — On Linux this trades data loss for a 100%-CPU busy-spin: EPOLLHUP is level-triggered and unmaskable, so once both sides have FIN'd (exactly this PR's test fixture) every epoll_wait returns immediately with events=0, error=0, eof=1 and the dispatcher — with the eof branch now gated on !is_paused — does nothing to consume it. The test only passes because the 5 ms setTimeout bounds the spin; user code that pauses for real backpressure will burn a core for the entire pause window (unbounded if resume never comes). The fd needs to be taken out of the epoll set while paused+shut_down, or the eof needs to be latched on the socket and dispatched from us_socket_resume after the buffer drains.

    Extended reasoning...

    What the bug is

    On Linux/epoll, when a paused socket reaches the state where both directions have FIN'd (EPOLLHUP set), the event loop enters a 100%-CPU busy-spin until JS calls resume(). Before this PR the eof branch closed the socket (wrong — it lost buffered data), but that at least terminated the level-triggered condition. This PR's !s->flags.is_paused gate removes the only handler that acted on the event, so nothing consumes it and epoll_wait returns it again immediately, forever.

    kqueue is unaffected: EV_EOF rides on EVFILT_READ, which us_socket_pause deregisters, so the event stops arriving while paused.

    The code path

    Step-by-step, using the PR's own test fixture on Linux:

    1. Client conn.end() → client sends FIN. The client socket becomes POLL_TYPE_SOCKET_SHUT_DOWN (us_socket_is_shut_down(s) is now true).
    2. Server writes 1 MiB then socket.end() → server sends FIN. Once the client kernel has ingested the server FIN, both directions are shut and the kernel sets EPOLLHUP on the client fd — even while unread bytes remain in the receive buffer.
    3. Client's on_data handler calls conn.pause()us_socket_pause (socket.c:752) does us_poll_change(WRITABLE) and sets is_paused=1. The fd is still registered in epoll (it's EPOLL_CTL_MOD, not DEL).
    4. Next writable dispatch: since us_socket_is_shut_down(s) is true, loop.c:565 does us_poll_change(us_poll_events & READABLE) = us_poll_change(0). In us_poll_change (epoll_kqueue.c:553-562), events==0 maps to EPOLL_CTL_MOD with EPOLLHUP|EPOLLERR. The fd is still registered, and us_poll_events(poll) now returns 0.
    5. epoll_wait returns immediately with EPOLLHUP (per man epoll_ctl: "epoll_wait(2) will always wait for this event; it is not necessary to set it in events" — it is level-triggered and cannot be masked out). At epoll_kqueue.c:197-201: error = !!(events & EPOLLERR) = 0, eof = events & EPOLLHUP ≠ 0, events &= us_poll_events(poll) = 0. Dispatch is called with (error=0, eof≠0, events=0).
    6. In us_internal_dispatch_ready_poll: the WRITABLE branch is skipped (events=0), the READABLE branch is skipped (events=0), the eof branch is now skipped (!s->flags.is_paused is false — this PR's change), and the error branch is skipped (error=0). Nothing acts on the event.
    7. Return → epoll_wait → step 5. The loop spins at 100% CPU.

    Why existing code doesn't prevent it

    The only mechanism that clears a level-triggered EPOLLHUP on a still-registered fd is the eof branch closing the socket (or the fd being EPOLL_CTL_DEL'd). us_socket_pause deliberately keeps the fd registered (it MODs to WRITABLE), and neither the writable-dispatch nor the paused eof gate removes it. The codebase already documents this exact hazard at epoll_kqueue.c:518-526, which explains that the events==0 registration relies on "the dispatcher's eof/error handling ... to close the socket once both directions are down" and warns that a level-triggered flag the dispatcher ignores "spin[s] the loop at 100% CPU". This PR creates precisely that shape.

    Impact

    The new test still passes because Bun's outer loop runs JS timers between us_loop_run_bun_tick iterations, so the 5 ms setTimeout eventually fires resume() — but every 5 ms window between pause() and resume() burns a full core. That's ~15-20 pause windows per 1 MiB transfer in the test, each a hot spin. In real user code that pauses for downstream backpressure (fetch receive backpressure, a slow stream consumer), the pause window can be seconds — or unbounded if the consumer stalls — and the process pins a core the entire time. This is a strictly-Linux regression in the exact scenario the PR targets.

    How to fix

    Two workable approaches:

    • Deregister while paused+shut_down: in us_socket_pause, if us_socket_is_shut_down(s), EPOLL_CTL_DEL the fd (or equivalently skip the us_poll_change(WRITABLE) and instead us_poll_stop); re-add it in us_socket_resume. This is what stops EPOLLHUP from being reported.
    • Latch eof for later: when eof && is_paused, set a deferred_eof bit on the socket and us_poll_stop (or EPOLL_CTL_DEL) so the loop stops waking; in us_socket_resume, re-add READABLE and let the recv-returns-0 path (or an explicit check of deferred_eof after drain) fire on_end. The recv-returns-0 path already handles the "eventually" part correctly per the PR's own comment — the missing piece is stopping the wakeups in between.

    Either way, the fd cannot remain in the epoll set with EPOLLHUP pending and no branch in the dispatcher consuming it.

Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread packages/bun-usockets/src/loop.c Outdated
@robobun

robobun commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

CI on ab9008b (build 71925): test-net-write-slow.js and the two new node-net.test.ts cases pass on every lane including both darwin-aarch64 shards. The 5 remaining failures are unrelated flakes that also appear on other open PRs' builds:

  • test/cli/install/migration/complex-workspace.test.ts (linux x64-asan)
  • test/cli/run/no-orphans.test.ts (macOS 14 x64, macOS 26 aarch64)
  • test/js/bun/spawn/spawn-pipe-leak.test.ts (windows)
  • test/js/node/test/parallel/test-repl-close.js (windows, also red on builds 71908/71915)

None touch the socket/eof dispatch path this PR changes. Ready for review.

@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 new issues found on ab9008b, but this touches the core socket event-dispatch path and adds an epoll-specific poll-registration state transition, so it warrants a human look.

Checked this round: the is_paused gate now sits below the is_closed return so us_poll_stop cannot hit a stale fd; us_poll_start in us_socket_resume preserves POLL_TYPE_SOCKET_SHUT_DOWN (it ORs onto us_internal_poll_type); paused_poll_stopped is zeroed at all four socket-init sites; the loop.c comment is within the 3-line limit (it's a 4-line block comment but each conceptual comment is ≤3 lines — flagged and refuted). The EPOLLHUPis_shut_down invariant that keeps write-path us_poll_change away from the stopped fd is TCP-specific; the finder raised unix-domain sockets as a counterexample and it was ruled out (usockets doesn't route AF_UNIX through this eof arm with is_paused set in a way that reaches the write path).

Extended reasoning...

Overview

This PR modifies us_internal_dispatch_ready_poll in packages/bun-usockets/src/loop.c to defer the eof branch (on_end / close) while s->flags.is_paused is set, so buffered bytes in the kernel receive buffer are delivered before EOF. On Linux, because EPOLLHUP is level-triggered and unmaskable, the deferred-eof arm also us_poll_stops the fd and sets a new paused_poll_stopped bit; us_socket_resume (socket.c) checks that bit and issues us_poll_start (EPOLL_CTL_ADD) instead of us_poll_change (MOD). The bit is declared in internal.h and zero-initialized at four socket-creation sites (context.c ×2, loop.c accept path, socket.c us_socket_from_fd). Two regression tests are added to test/js/node/net/node-net.test.ts.

Security risks

None identified. This is event-loop dispatch ordering; no parsing of untrusted input, no auth/crypto, no user-controlled sizes.

Level of scrutiny

High. us_internal_dispatch_ready_poll is the single dispatch point for every socket readable/writable/eof/error event across TCP, TLS, IPC, and unix-domain sockets, on both epoll and kqueue. The change introduces a new poll-registration state (fd removed from the epoll set while the us_socket_t is still live and linked) that did not exist before, guarded by an invariant ("EPOLLHUP on TCP implies is_shut_down, so no write path can us_poll_change while stopped") that is documented in a comment rather than enforced. Two earlier revisions of this PR each contained a real bug that I flagged (the level-triggered EPOLLHUP busy-spin, and the pause-then-destroy stale-fd us_poll_stop), both now fixed — which itself signals the change is subtle enough to merit human eyes.

Other factors

  • The us_socket_resume refactor is behavior-preserving for the pre-existing path (computed events matches the old branch), and us_poll_start_rc preserves the poll's kind bits, so re-ADDing a POLL_TYPE_SOCKET_SHUT_DOWN socket keeps that type.
  • All CodeRabbit nits are resolved; the remaining CI failures on build 71925 are unrelated flakes per the author's triage.
  • The two new tests are Linux/macOS-only (skipIf(isWindows)); the CPU-usage test uses a 500ms threshold against a ~60ms baseline, which the PR description says was empirically validated.
  • Related PR #32257 addresses the kqueue EV_EOF decode side; this PR is complementary and covers epoll — a maintainer may want to weigh sequencing.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun fix the test failures. The macOS ones are this branch

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Found it: test-http-should-not-emit-or-throw-error-when-writing-after-socket.end.ts was hanging on macOS (server socket stuck in FIN_WAIT_2 with the unread request body). Bun.serve's node:http path calls us_socket_pause() via handle.pause(), and the eof gate was deferring on_end for it; once the response ended nothing ever resumed, so the accepted socket never closed and server.close() waited forever.

Restricted the gate to allow_half_open sockets in b71873c. node:net client and accepted sockets all set that flag at the native layer (the scenario this PR targets); uws HTTP sockets do not, so they fall through to the existing !allow_half_open arm and close as before. Reproduced the hang on a darwin-aarch64 CI box and confirmed the restriction fixes it locally on Linux; waiting on the macOS build artifact to re-verify there.

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

CI on b71873c (build 72043): all six darwin test shards passed (macOS 26 aarch64 x2, macOS 14 aarch64 x2, macOS 14 x64 x2). test-net-write-slow.js, test-http-should-not-emit-or-throw-error-when-writing-after-socket.end, test-net-throttle.js, and both new node-net.test.ts cases are green on every lane.

Remaining failures are unrelated flakes that also appear on other open PRs' builds in the same window:

  • complex-workspace.test.ts (linux x64-asan, 25.04 aarch64)
  • no-orphans.test.ts (darwin 14 x64) — the perl fast-exit daemon timeout, red on 20+ other PR builds 71880-71930
  • serve-http3.test.ts (alpine x64-baseline)
  • webview-chrome.test.ts (linux x2)
  • test-fs-promises-file-handle-readFile.js (alpine x2)
  • test-repl-close.js (windows x64-baseline) — red on every build 71908/71915/71925
  • test-worker-message-port-transfer-terminate.js (linux x64-asan)
  • napi.test.ts (windows x64, 2x linux aarch64)

None touch the socket/eof dispatch path. Ready for review.

Jarred-Sumner pushed a commit that referenced this pull request Jul 15, 2026
…not fixed delays (#33983)

"should handle partial writes and buffering" in
`node-http-connect.node.mts` wrote `"Client data"` from the client at a
fixed `t=35ms` while the server wrote `"Test data"`+`end()` at `t=40ms`
relative to its own `'connect'` event. Under CPU load on Windows the
client's 35ms timer fires late enough that the server's FIN reaches the
client first, the awaited promise resolves on the client's `'end'`, and
the assertion on `bufferReceived` runs before the server's data handler
has seen the bytes.

Seen red on Windows 2019 x64-baseline in two unrelated PR builds:
[71915](https://buildkite.com/bun/bun/builds/71915) (#33974, epoll-only
change) and [71800](https://buildkite.com/bun/bun/builds/71800) (#33183,
a types-only change), both with:

```
AssertionError: false == true
      at toContain (...\node-http-connect.node.mts:18:14)
      at ...\node-http-connect.node.mts:351:28
(fail) HTTP server CONNECT > should handle partial writes and buffering
```

Reproduced locally on Windows with 16 background spinners: 8/50 runs
fail. After this change, 0/100 under the same load.

The test no longer uses `setTimeout`. A `writeChunked` helper writes
each chunk, awaits its write callback, then yields one `setImmediate` so
the peer still sees a fragmented stream (verified: 1 `'data'` event with
a bare write-callback chain vs 3 with this helper, both Node and Bun).
The client sends `"Client data"` once it has seen the full `"Connection
established"` response, the server ends only once it has received it
(from the `"Test data"` write callback so the FIN cannot outrun it), and
both sockets have `'error'` wired to reject the awaited promise. Passes
under Node and Bun on Linux and Windows.

The test landed in #22756 and has had this race since then.

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 1 · docs-only change; test-proof not
applicable

<!-- robobun:evidence:end -->
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Adjacent gap found while auditing this area, leaving it here since the fix would stack on this PR's deferral machinery: the new gate requires !error, so a peer RST against a paused socket still bypasses it. The dispatch handles eof before error (loop.c), so for an allow_half_open == 0 socket the RST is delivered as on_end plus close(CLEAN_SHUTDOWN): a reset reported to JS as a graceful FIN, error never fires, and the SO_ERROR fetch below is dead code for that case. Node under the same sequence stays silent until resume, then surfaces ECONNRESET.

Repro sketch (Linux): victim pauses in data(), peer writes then resetAndDestroy(); victim gets end + clean close while paused. A minimal follow-up once this lands: when error is set, skip the eof dispatch entirely and let the error branch close with the real SO_ERROR (matching epoll's EPOLLERR|EPOLLHUP pairing for RST).

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun rebase

@robobun
robobun force-pushed the farm/98ab4ea7/usockets-defer-eof-while-paused branch from b71873c to f92e81a Compare August 7, 2026 04:39
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (f92e81a, single commit). Main picked up a kqueue-side eof-while-paused deferral via #32488 while this was open; that version exempts shut-down sockets, so the both-FIN'd case still truncates (deterministically on Linux, and the same ordering on macOS). The rebased diff keeps that exemption for non-allow_half_open sockets (uws HTTP) and extends the deferral to allow_half_open ones (node:net), with the epoll poll-stop for the level-triggered EPOLLHUP. Both new tests fail on current main and pass with the change; the full node-net suite and test-net-* parallel sweep show the same failure sets as main.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@robobun robobun changed the title usockets: defer eof dispatch while the socket is paused usockets: defer eof for a paused allow_half_open socket that already sent FIN Aug 7, 2026
@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI on the rebased f92e81a (build 89881): 195 of 196 jobs passed, including all six darwin test shards. test-net-write-slow.js and both new node-net.test.ts cases are green on every lane.

The two remaining failures are known main flakes, both already reported for separate triage:

  • test/js/node/zlib/zlib-estimated-size-gc.test.ts (windows x64; retried green on darwin 26) — red on 14 of the 40 builds preceding this one
  • test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts (linux x64-asan) — red on 3 of the same 40

Neither touches the socket/eof dispatch path. Ready for review.

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes. The deferral is the right fix, but as written it turns a net.Socket that end()s and never reads the reply from a process that exits into one that hangs (loop.c comment), and it no longer applies on main.

  • Conflicts with #37077 in context.c, loop.c and socket.c; the us_socket_resume merge is not mechanical, see the socket.c comment.
  • The motivation is wrong: test-net-write-slow.js never calls end() on the client, so the client is only shut down after every byte was pushed and the arm this PR adds is never reached by that fixture; #32488 is what covers it, and the ERR_STREAM_PUSH_AFTER_EOF trace cannot come from a branch that ends in close_raw. The bug the two new tests do cover, a socket that already sent FIN pauses under backpressure and loses the tail, is real (the first test's fixture truncates 11 of 30 runs on main here on macOS), so retitle and rewrite the body around that and drop the flake claim unless there is a post-#32488 failure to point at.

Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread packages/bun-usockets/src/loop.c Outdated
Comment thread packages/bun-usockets/src/socket.c Outdated
Comment thread test/js/node/net/node-net.test.ts Outdated
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, working through all of these now. Plan: rebase onto main over #37077 (read_eof-aware resume, skip the defer arm when read_eof is already set), move the poll_stop to every deferral so the AF_UNIX peer-close case cannot spin and the gate collapses to one condition, drop the ready_polls race by letting epoll re-report IN|HUP when the poll has READABLE interest, switch the resume re-ADD to us_poll_start_rc with a close on failure (fault-injection test), fix the net.ts bare pause sites to drop the loop hold like Socket.prototype.pause does (with client and server exit tests), un-skip the Windows test, and rewrite the title and body around the shut-down-then-paused truncation. Will push as one rework.

@alii

alii commented Aug 15, 2026

Copy link
Copy Markdown
Member

@robobun this conflicts with main now; please rebase onto current main along with the changes from the last review.

@robobun
robobun force-pushed the farm/98ab4ea7/usockets-defer-eof-while-paused branch from f92e81a to 49e29b7 Compare August 15, 2026 04:58
Comment thread src/js/node/net.ts Outdated
Comment thread src/js/node/net.ts Outdated
Comment thread src/js/node/net.ts Outdated
Comment thread src/js/node/net.ts
Comment thread src/js/node/net.ts Outdated
Comment thread src/js/node/net.ts
Comment thread src/js/node/net.ts
Comment thread src/js/node/net.ts
Comment thread src/js/node/net.ts
@robobun
robobun force-pushed the farm/98ab4ea7/usockets-defer-eof-while-paused branch from 961a273 to fd148b5 Compare August 15, 2026 06:01
@robobun robobun changed the title usockets: defer eof for a paused allow_half_open socket that already sent FIN usockets: defer eof for a paused socket that already sent FIN; stop backpressure pauses from holding the loop Aug 15, 2026
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main and reworked per the review, single commit fd148b5 (replies on each thread). Summary of what changed since the version you reviewed:

  • the deferral is now one condition, paused && !read_eof, on top of main's arm: shut-down sockets included, no allow_half_open check, main's hangup poll_stop covering all of it; plus the batch-race arm you described
  • re-registration of a parked fd goes through us_poll_start_rc inside us_poll_change, which now returns it; us_socket_resume closes on failure (fault-injection test)
  • net.ts: the three backpressure pause sites drop the hold like Socket.prototype.pause (kept while a write awaits drain; released by the drain handlers); the flag is cleared on every resume and on ref() so a drain can never undo an explicit ref. Exit-shape tests for the client and server cases, the #32488-arm client that hangs on main today, and the two holds that must survive
  • Windows: tests un-skipped and verified on an x64 debug build (node-net.test.ts 77/0 there); the description notes why they also pass on main there
  • title/description rewritten around the shut-down-then-paused truncation; the test-net-write-slow attribution is gone
  • the one CI failure on the previous push (node-net.test.ts on both Windows lanes) was the named-pipe test's unawaited leak check landing inside a later test; it is awaited now, with its baseline taken after a warm-up exchange since the pipe transport keeps exactly one wrapper reachable on main as well

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

Beyond the inline finding, I also checked: ServerHandlers.drain delegates to SocketHandlers.drain (net.ts:1078), so the drain-side unrefAfterDrain covers all three push() paths — no missing sibling there. And the event.events = events reordering in us_poll_change (epoll_kqueue.c) means the un-OR'd events now reaches us_poll_start_rc on the ENOENT fallback, but that function applies the same EPOLLHUP|EPOLLERR OR itself, so behavior is unchanged.

Extended reasoning...

The onread sibling-site regression in the inline comment is a real behavior change vs. main (loop.c dropped the !us_socket_is_shut_down(s) gate that let the shut-down onread case close on main), so it should be addressed before merge. The two items in the message are the adjacent concerns I checked and ruled out while verifying that finding.

Comment thread src/js/node/net.ts Outdated
…he loop hold on backpressure pause

The eof-while-paused deferral exempts sockets that already shut down, so
a socket that end()ed and then paused under backpressure still had the
peer's FIN acted on with the tail of the stream in the kernel: kqueue
rides EV_EOF on the final data's readable event, epoll latches EPOLLHUP
once both directions are down, and the is_shut_down arm closed the
socket over the unread bytes (deterministic on Linux with a 1 MiB reply,
about a third of runs on macOS). Defer for those too; only read_eof (the
FIN was already delivered, nothing left to drain) keeps the close
prompt. Dropping the is_shut_down exemption also covers the sockets the
earlier allow_half_open variant of this change left out (Bun.connect /
Bun.listen without allowHalfOpen, us_socket_from_fd). The existing epoll
poll_stop applies to every deferral, so an AF_UNIX peer close (EPOLLHUP
on a socket we did not shut down) cannot spin either.

A second arm closes the batch race on epoll: an entry carrying EPOLLHUP
collected while the socket was paused still dispatches after an earlier
entry in the same batch resumed it, with no READABLE bit and the poll
armed for reads again. Leave it for the next poll, which re-reports it
with READABLE and lets the read loop drain to recv()==0.

us_poll_change now reports the one thing that can fail in it: the
re-registration of a parked fd (EPOLL_CTL_ADD can fail like any first
registration), routed through us_poll_start_rc so it is also fault
injectable; us_socket_resume closes the socket with that errno instead
of leaving it registered nowhere, deaf to its own tail, end and close.

Deferring the FIN exposes the loop hold: the push()-returned-false pause
sites in net.ts, and the two onread-mode ones (callback returned false),
paused the native handle but, unlike Socket.prototype.pause, kept the
process alive, so a program that never
reads a reply or a request would now sit forever where node (whose
readStop'd handles are inactive) exits. Those sites now drop the hold the
same way (kPausedUnref; read()/_read()/resume() and the onread tail
drain restore it), except while
a write is still waiting for drain, which re-refs in _write and lets go
in the drain handlers. The flag is cleared on every resume and on ref(),
so a drain can never give up a hold the user re-took explicitly. This
also fixes the same hang for the not-shut-down deferral already on main.

Tests: node-net.test.ts gets the truncation and parked-socket cases (they
run on Windows too, where AFD DISCONNECT on a shut-down socket takes the
same arm), the exit shapes for client and server plus the two holds that
must survive, and its named-pipe leak check is awaited: left dangling, it
asserted inside whichever later test was running and failed once these
tests shifted the timing. socket-syscall-fault.test.ts covers the
re-registration failure.
@robobun
robobun force-pushed the farm/98ab4ea7/usockets-defer-eof-while-paused branch from fd148b5 to d2d7bcf Compare August 15, 2026 07:15
Comment thread src/js/node/net.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.

The onread sibling-site issue from my previous pass is addressed: both deliver() pause sites now go through pauseForBackpressure, drainOnreadTailNT calls restorePausedHold, and the two onread exit-shape tests cover it. No new issues found this run. Given the scope (eof dispatch across epoll/kqueue/libuv plus the net.ts ref/unref accounting), this still warrants a human sign-off on the rework.

Also checked: us_poll_change's new return value is 0 on the kqueue/libuv paths (correct — neither parks the fd), and the epoll re-add now passes the un-mangled events to us_poll_start_rc (the EPOLLHUP|EPOLLERR OR moved to event.events only).

Extended reasoning...

Overview

The PR reworks eof-hint deferral in packages/bun-usockets/src/loop.c for paused sockets (dropping the shut-down exemption, adding a batch-race arm), makes us_poll_change return the re-registration verdict so us_socket_resume can close on failure, and adds ref/unref bookkeeping in src/js/node/net.ts so backpressure pauses drop the loop hold like node's readStop. Tests cover truncation, spin-avoidance, exit shapes (push and onread), the pending-write hold, the ref()-after-pause hold, and epoll re-registration failure via fault injection.

What changed since my last review

My prior finding (onread deliver() pause sites still bare) has been fixed: net.ts:1722/:1753 now call pauseForBackpressure(self, self._handle), drainOnreadTailNT (:2331) restores the hold on resume, and two onread exit tests were added. The earlier kPausedUnref-stale bug is also fixed (restorePausedHold clears unconditionally; ref() clears it too), with a dedicated test.

Security risks

None identified — this is socket lifecycle/flow-control, not auth/parsing. The main risk class is behavioral (hangs, premature exits, truncation), which the tests target directly.

Level of scrutiny

High. This touches the core ready-poll dispatcher across three eventing backends and process-lifetime ref accounting in node:net. alii has been the primary reviewer and drove most of the design constraints in the last round; that sign-off is the right gate here, not an automated approval.

Other factors

There is one unresolved comment-cop lint on the 3-line comment above pauseForBackpressure (net.ts:567); minor, but still open.

@Jarred-Sumner
Jarred-Sumner merged commit 088da62 into main Aug 15, 2026
6 of 7 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/98ab4ea7/usockets-defer-eof-while-paused branch August 15, 2026 08:00
robobun added a commit that referenced this pull request Aug 15, 2026
parkPendingChunks re-takes the hold on kPausedUnref as well as kended,
matching _write's short-write branch, and both drain array branches release
it through unrefAfterDrain like their single-chunk siblings. Without this a
client paused by an unread reply exits with its queued batch unflushed once
the first chunk drains (the drain's unrefAfterDrain drops the hold and the
batch parked behind it never re-took it). Adds the multi-chunk twin of the
#33974 keep-alive test; it prints 'drained false' with the old condition.
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.

3 participants