usockets(win): deliver UV_DISCONNECT on libuv's slow poll path - #37104
usockets(win): deliver UV_DISCONNECT on libuv's slow poll path#37104robobun wants to merge 11 commits into
Conversation
The select()-based slow poll path (taken when a winsock LSP with a non-IFS protocol chain owns the socket) never reported UV_DISCONNECT, and a poll subscribed to only UV_DISCONNECT handed select() three empty fd sets, which fails WSAEINVAL immediately and stopped the watcher with a bogus error. usockets arms UV_DISCONNECT on every poll and parks paused and half-closed sockets in exactly that state, so on LSP machines healthy paused sockets were error-closed with ECONNRESET within one loop iteration, and peer FIN/RST for quiesced sockets was never delivered. Synthesize UV_DISCONNECT on the slow path from select() readability plus MSG_PEEK discrimination (0 = FIN, error other than WSAEWOULDBLOCK or WSAENOTCONN = dead connection), pace the one undeliverable state (data pending on a DISCONNECT-only poll) with a 1s re-peek instead of a hot respin, and add the UV_FORCE_SLOW_POLL=1 hook so tests reach the path without installing an LSP.
git apply resolves git-style (diff --git) paths against the enclosing repo root when run from vendor/<dep> inside this repo, and silently skips them with exit 0, so the patch would never actually apply. Traditional --- a/ +++ b/ headers are resolved relative to the cwd and apply correctly.
Track the close callback's error argument and delay the client's answering FIN behind allowHalfOpen, so a build whose slow path error-closes the parked socket fails the half-close test instead of racing a same-tick clean teardown past it.
WalkthroughChangesThe Windows libuv slow-poll path now detects disconnects, preserves buffered data, limits polling wakeups, and supports forced testing. The dependency patch list and socket tests cover paused reads, half-closes, resets, backpressure, and poll-slot contention. Windows slow-poll socket handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
A 1-byte MSG_PEEK on a UDP socket with a datagram wider than the buffer fails with WSAEMSGSIZE; a stream socket can never produce it, so treating it as a dead connection misreported every multi-byte datagram on a DISCONNECT-armed UDP poll.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…wanted data A poll subscribed to UV_WRITABLE | UV_DISCONNECT (a receive-paused socket writing under backpressure) with unread data pending made select() return instantly via the read set, and the fall-through parked it with Sleep(1000): writable wakes were throttled to one per second instead of firing the instant the send buffer drained. Re-select() with the read set suppressed for that lap instead, capped at one second so the disconnect re-peek keeps running behind the pending data.
…r park The reset test captured the error/close discriminators but never asserted them; an RST behind buffered data must surface as a dirty close. The new drain test parks a receive-paused writer under backpressure with unread data pending: a slow path that sleeps through that state cannot move 8MB inside the timeout.
|
Updated 12:41 AM PT - Aug 7th, 2026
✅ @robobun, your commit 70f9574c9e9a415c0700c311a1f7d9456028fcae passed in 🧪 To try this PR locally: bunx bun-pr 37104That installs a local version of the PR into your bun-37104 --bun |
|
Addressed the review: the Sleep(1000) park no longer throttles a backpressured writer. When the parked poll also wants UV_WRITABLE, the thread now re-selects with the read set suppressed for that lap (capped at 1s so the disconnect re-peek keeps running), so the send-buffer drain wakes it instantly. A new test parks a receive-paused writer under backpressure with unread data pending: the old code could not move 8MB inside 20s, the fix drains it in ~240ms. Also tightened the RST test to assert the close surfaces dirty. Verified on Windows Server 2019: all 4 slow-poll tests plus the full socket.test.ts file pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/net/socket.test.ts`:
- Around line 3891-3898: Update pump to detect a negative result from s.write
before adding it to sent, and fail immediately with an error that identifies the
closed or errored socket condition. Preserve the existing backpressure return
for nonnegative writes where n is less than want, and ensure the failure reaches
the surrounding test flow instead of waiting for drained.promise.
🪄 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: abae043a-59fc-44b1-9aa0-b28ab3c20dba
📒 Files selected for processing (3)
patches/libuv/win-slow-poll-disconnect.patchscripts/build/deps/libuv.tstest/js/bun/net/socket.test.ts
Self-review findings, merged with the writability-park refinement: - The park makes slow poll requests long-lived, so a third subscription generation (a backpressured write while a parked DISCONNECT-only request holds one slot and a resumed request holds the other) reached uv__slow_poll_submit_poll_req's both-slots assert(0): an abort in debug builds, and in release the new subscription stayed unarmed until a request completed, up to the full select() timeout. Cap every select() lap at one second and re-read handle->events each lap so live requests converge onto subscription changes within a tick, and make the both-busy branch a graceful return like the fast path's. - Replace the bare static lazy init of the force hook with uv_once (the file's own idiom; uv_poll_init_socket runs on every loop's thread), and rename the variable to BUN_FEATURE_FLAG_UV_FORCE_SLOW_POLL: Bun should not mint names in the UV_ namespace it does not own, and BUN_FEATURE_FLAG_FORCE_WAITER_THREAD is the established convention for a release-live env hook that forces a slower production path. - New test covering the slot exhaustion sequence.
A partial first chunk satisfied the old got-some assertion even when the re-armed WRITABLE subscription stalled; pump the payload through drain and assert every byte arrives.
… tests A socket death in either drain test left the child waiting on the drained promise until the parent timeout with nothing on stdout; route every unexpected error/close straight to a FAIL line and exit.
|
Status: self-review follow-ups are in.
Full matrix re-verified on Windows Server 2019: 5/5 slow-poll tests pass on the fixed build; hook-only build fails the paused-socket tests (instant dirty close), the half-close test, and the both-slots test (write returned -10054 mid-pump). Whole socket.test.ts passes with and without the hook forced; udp_socket.test.ts passes forced. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/net/socket.test.ts`:
- Around line 4042-4049: Replace the fixed 80 ms delay after gotData.promise
with Bun.sleep(0) to wait only for the next settled loop turn. For the 400 ms
delay before paused.resume(), either document why the unavoidable timing wait is
required and immediately assert after pump(paused) that sent is less than TOTAL,
or replace it with an observable condition/deadline poll proving the paused poll
is parked; ensure the test cannot silently pass without exercising the
both-slots-busy path.
🪄 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: 46c91550-e173-4333-8c3f-8c3fca1150a8
📒 Files selected for processing (3)
patches/libuv/win-slow-poll-disconnect.patchscripts/build/deps/libuv.tstest/js/bun/net/socket.test.ts
| // Let the paused server poll park with the byte pending (slot 1 | ||
| // checked out, sleeping between peeks). | ||
| await Bun.sleep(400); | ||
| paused.resume(); | ||
| await gotData.promise; | ||
| // One settled loop turn so resume's request cycles back to a | ||
| // parked readable wait (slot 2 checked out too). | ||
| await Bun.sleep(80); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the wall-clock setup sleeps so the test reliably reaches the both-slots-busy state.
Lines 4044 and 4049 use fixed wall-clock delays to establish the precondition under test. If either window is mistimed on a loaded CI agent, the second poll request is not yet parked when pump(paused) runs. The test then still drains 4 MiB and passes, so a regression in the both-slots-busy branch would go undetected. The failure mode is silent under-coverage, not flakiness.
Line 4049 waits only for a settled loop turn, so Bun.sleep(0) expresses that intent deterministically and satisfies the no-arbitrary-sleep rule. Line 4044 needs the paused poll to park; there is no JS-observable signal for that, so document why the delay is unavoidable, or add an assertion that proves the write actually hit backpressure (for example, assert sent < TOTAL immediately after pump(paused)), so the test fails when the intended path is not exercised.
As per coding guidelines: "Do not use setTimeout or await sleep(N) to wait for conditions; await the event or poll with a deadline." and "Tests must prove they fail for the intended reason and must exercise the actual production guards, constants, environment knobs, and preconditions."
♻️ Proposed change
await opened.promise;
client.write("x");
// Let the paused server poll park with the byte pending (slot 1
- // checked out, sleeping between peeks).
+ // checked out, sleeping between peeks). No JS-observable signal
+ // exists for "the slow-poll worker has parked", so this delay is
+ // the only way to reach the state under test.
await Bun.sleep(400);
paused.resume();
await gotData.promise;
// One settled loop turn so resume's request cycles back to a
// parked readable wait (slot 2 checked out too).
- await Bun.sleep(80);
+ await Bun.sleep(0);
// The first partial write re-arms WRITABLE with both slots held:
// the submission that used to have no slot to land in.
pump(paused);
+ // Prove the write actually backpressured; otherwise this test would
+ // pass without exercising the both-slots-busy branch.
+ if (sent >= TOTAL) {
+ console.log("FAIL no backpressure: sent " + sent + " without blocking");
+ process.exit(1);
+ }
client.resume();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Let the paused server poll park with the byte pending (slot 1 | |
| // checked out, sleeping between peeks). | |
| await Bun.sleep(400); | |
| paused.resume(); | |
| await gotData.promise; | |
| // One settled loop turn so resume's request cycles back to a | |
| // parked readable wait (slot 2 checked out too). | |
| await Bun.sleep(80); | |
| await opened.promise; | |
| client.write("x"); | |
| // Let the paused server poll park with the byte pending (slot 1 | |
| // checked out, sleeping between peeks). No JS-observable signal | |
| // exists for "the slow-poll worker has parked", so this delay is | |
| // the only way to reach the state under test. | |
| await Bun.sleep(400); | |
| paused.resume(); | |
| await gotData.promise; | |
| // One settled loop turn so resume's request cycles back to a | |
| // parked readable wait (slot 2 checked out too). | |
| await Bun.sleep(0); | |
| // The first partial write re-arms WRITABLE with both slots held: | |
| // the submission that used to have no slot to land in. | |
| pump(paused); | |
| // Prove the write actually backpressured; otherwise this test would | |
| // pass without exercising the both-slots-busy branch. | |
| if (sent >= TOTAL) { | |
| console.log("FAIL no backpressure: sent " + sent + " without blocking"); | |
| process.exit(1); | |
| } | |
| client.resume(); |
🤖 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 `@test/js/bun/net/socket.test.ts` around lines 4042 - 4049, Replace the fixed
80 ms delay after gotData.promise with Bun.sleep(0) to wait only for the next
settled loop turn. For the 400 ms delay before paused.resume(), either document
why the unavoidable timing wait is required and immediately assert after
pump(paused) that sent is less than TOTAL, or replace it with an observable
condition/deadline poll proving the paused poll is parked; ensure the test
cannot silently pass without exercising the both-slots-busy path.
Source: Coding guidelines
| + /* Refreshed every lap, and each select() below waits at most one | ||
| + * second: uv_poll_start/uv_poll_stop on the loop thread change the | ||
| + * subscription mid-flight, and when both requests are checked out | ||
| + * (see uv__slow_poll_submit_poll_req) this lap refresh is the only | ||
| + * thing that converges a parked request onto the new subscription. */ | ||
| + events = handle->events; | ||
| + |
There was a problem hiding this comment.
🟡 The per-lap events = handle->events; re-read is now load-bearing cross-thread communication — the both-slots-checked-out branch in uv__slow_poll_submit_poll_req returns early and relies on the worker thread eventually observing the loop thread's later write to handle->events ("re-reads handle->events every lap … so the live requests converge"). That's a plain read concurrent with a plain write with no atomic/volatile/barrier, and REVIEW.md's threading rule is explicit: "Atomics for every shared counter … benign same-value races are still UB." In practice it will always work (the intervening select()/recv()/Sleep()/GetTickCount64() are opaque calls the compiler can't hoist across, and aligned single-byte access is hardware-atomic on every Windows target), so this is not blocking — but a (int) ReadNoFence8((CHAR volatile*)&handle->events) or a volatile-qualified read would make the intended cross-thread visibility well-defined at zero cost.
Extended reasoning...
What the bug is
The rewritten uv__slow_poll_thread_proc reads handle->events at the top of every lap:
for (;;) {
/* Refreshed every lap … when both requests are checked out
* (see uv__slow_poll_submit_poll_req) this lap refresh is the only
* thing that converges a parked request onto the new subscription. */
events = handle->events;and the new both-slots-busy branch in uv__slow_poll_submit_poll_req explicitly depends on that read observing a value the loop thread writes after the worker was queued:
} else {
/* Both requests are checked out. … That loop re-reads handle->events
* every lap (at most one second), so the live requests converge on
* the new subscription … */
return;
}handle->events is a plain unsigned char field (uv/win.h) with no atomic qualifier, no volatile, and no barrier around the read or the write in uv_poll_start. Under the C memory model, a read on one thread concurrent with a write on another with no synchronization is a data race — undefined behavior. REVIEW.md's thread-affinity rule names this directly: "Atomics for every shared counter (even metrics); a mutex only counts if EVERY accessor takes it; benign same-value races are still UB."
What changed vs upstream
Upstream's single handle->events read at function entry was published-before by QueueUserWorkItem: the loop thread writes handle->events, then queues the work item, then the worker reads it — a clean happens-before chain. (Upstream does already have a milder form of this race: a second uv_poll_start can write handle->events while slot 1's worker is still reading it, and either value is acceptable there.) What's new in this PR is that the correctness of a recovery path — the both-slots-checked-out convergence that test 5 exercises — now depends on the worker observing a post-queue write, rather than merely tolerating either value.
Why existing code doesn't prevent it
There is no volatile qualifier, no Interlocked*/ReadNoFence* on the read, and no release store on the loop-thread write in uv_poll_start. The visibility guarantee rests entirely on implementation behavior of the toolchain and hardware, not on any language-level primitive.
Impact
In practice, none observable — which is why this is a nit, not a blocker:
- The load cannot be hoisted. Every lap contains opaque external calls (
GetTickCount64(),select(),recv(),Sleep()) that the compiler must assume can write through any escaped pointer, andhandleescaped viaQueueUserWorkItem. So the compiler cannot cachehandle->eventsin a register across laps. - Single-byte access is hardware-atomic on every Windows target Bun ships (x86-64, aarch64), so no torn reads.
- Visibility is bounded. x86-64 TSO plus the intervening kernel calls make the write visible well within the 1s lap; on aarch64 the syscalls provide the fence.
- There is a fallback. Even if the worker never observed the update, it eventually completes on its own (socket activity or the 3-minute deadline), and the loop-thread processing step then resubmits with the correct
handle->events— just slower than the intended ≤1s convergence.
So the worst case is one extra lap of staleness, not corruption, a hang, or a crash.
Step-by-step proof (the race that's now load-bearing)
- Loop thread:
uv_poll_start(handle, UV_DISCONNECT)→ writeshandle->events = UV_DISCONNECT, queues worker A on slot 1. Worker A parks in theSleep(1000)re-peek loop with an unread byte pending. - Loop thread:
uv_poll_start(handle, UV_READABLE|UV_DISCONNECT)→ writeshandle->events, queues worker B on slot 2. Worker B blocks inselect(). - Loop thread:
uv_poll_start(handle, UV_WRITABLE|UV_DISCONNECT)→ writeshandle->events = 6. Both slot flags are set →uv__slow_poll_submit_poll_reqhits the newelsebranch and simply returns, relying on step 4. - Worker thread A: at the top of its next lap, executes
events = handle->events;. This read (worker thread) is concurrent with the write in step 3 (loop thread) with no synchronization primitive between them. The patch's design requires this read to eventually return6so worker A armswfdsand deliversUV_WRITABLE.
That is precisely the shape REVIEW.md forbids: cross-thread communication through a plain field.
How to fix it
A one-liner. On the read side:
events = (int) ReadNoFence8((CHAR const volatile*)&handle->events);or equivalently qualify the read with volatile (events = *(volatile unsigned char*)&handle->events;). If you want strict-C well-definedness rather than just MSVC well-definedness, pair it with a release store on the loop-thread side — but since libuv's own uv_poll_start already writes this field non-atomically and the field is a single byte, the volatile read alone brings the patch to parity with how upstream already treats it, at zero runtime cost.
1 participant
Problem
On Windows, when a winsock LSP (layered service provider) with a non-IFS protocol chain owns a socket (some antivirus/firewall/VPN products install these), libuv cannot AFD-poll it and falls back to its slow poll path: a
select()on a worker thread (uv__slow_poll_thread_procinsrc/win/poll.c). That path had two defects for the way usockets usesuv_poll:It never reported
UV_DISCONNECT. usockets armsUV_DISCONNECTon every poll and depends on it for connection teardown: it is how a half-closed socket that stopped reading learns of the peer's FIN, and how a paused socket discriminates FIN from abort (poll_cbinpackages/bun-usockets/src/eventing/libuv.c). On LSP machines all of that silently degraded: hung teardown (e.g.server.close()waiting forever on a half-closed connection), paused sockets never learning their peer died.Worse: a poll subscribed to only
UV_DISCONNECT, which is the normal parked state for a paused or half-closed socket (pause drops READABLE, the writable dispatch drops WRITABLE once flushed), handedselect()three empty fd sets. Winsockselect()fails that withWSAEINVALimmediately, which stopped the watcher and reported a bogus error, so usockets error-closed the healthy connection. On an LSP machine, every paused socket dies within one loop iteration.Winsock semantics verified with a raw C probe on Windows Server 2019:
probe output
Fix
patches/libuv/win-slow-poll-disconnect.patch, touching only the slow path (the AFD fast path, which reportsAFD_POLL_DISCONNECTnatively, is untouched):UV_DISCONNECTis requested, so the fd sets are never empty and a FIN or RST (which make the socket select()-readable) wakes the poll.UV_DISCONNECTrequested, discriminate withrecv(MSG_PEEK): 0 means FIN, an error other thanWSAEWOULDBLOCK/WSAENOTCONN/WSAEMSGSIZEmeans the connection is gone; both reportUV_DISCONNECT.WSAENOTCONNexempts listening sockets (readable on a pending accept, no peer to lose);WSAEMSGSIZEexempts datagrams wider than the 1-byte peek, which a stream socket can never produce.select()lap is capped at one second and re-readshandle->events, anduv__slow_poll_submit_poll_req's both-slots-busy branch becomes a graceful return (mirroring the fast path's) instead ofassert(0). Slow requests are now long-lived by design (a parked DISCONNECT-only request can hold a slot for minutes), so a third subscription generation, e.g. a backpressured write while a parked request holds one slot and a resumed request holds the other, must land somewhere: the live requests converge onto the new subscription within a lap and the processing step resubmits with correct accounting. Previously that sequence hit the assert (or, with asserts compiled out, left the new subscription unarmed until a request completed, up to the full select() timeout).poll_req_1even when the failing request waspoll_req_2.BUN_FEATURE_FLAG_UV_FORCE_SLOW_POLL=1hook (read once per process underuv_once, the file's idiom, sinceuv_poll_init_socketruns on every loop's thread) that routesuv_poll_init_socketto the slow path, so the path can be exercised without installing an LSP. The hook only selects between two production paths; the slow path is otherwise unreachable from any CI environment, and this is patched vendored C, out of reach ofbun:internal-for-testing. The name follows theBUN_FEATURE_FLAG_FORCE_WAITER_THREADprecedent for release-live env hooks that force a slower production path.When no
UV_DISCONNECTis subscribed, the rewritten thread proc behaves like upstream (same fd set arming, same masking at the processing step, same 3-minute completion cadence; the select just wakes to re-check its subscription once a second), so non-LSP machines see zero behavior change. Upstreamable to libuv/libuv minus the hook.The patch file deliberately uses traditional
--- a//+++ b/headers: the dep fetcher runsgit applyfromvendor/libuvinside this repo, and git resolvesdiff --git-style paths against the enclosing repo root and silently skips them with exit 0 (tracked separately; the sibling abort patch is affected by that today).Tests
Five Windows-only tests in
test/js/bun/net/socket.test.ts, each spawning a child withBUN_FEATURE_FLAG_UV_FORCE_SLOW_POLL=1:WSAEINVALinsta-close),shutdown()) paused socket sees the peer's answering FIN and closes cleanly, without an error (catches the hung-teardown case),Verification on Windows Server 2019 x64, debug build:
socket.test.tsfile (85 tests) passes both with and without the hook in the environment, andnode-http-connect.test.ts,udp_socket.test.ts(207 tests) anddgram.test.tspass with it forced (general traffic at parity on the slow path: TLS, backpressure, CONNECT tunnels, UDP);FAIL early-death error=ESHUTDOWN closed=true), the half-close test fails with a dirty close instead of a clean FIN-answered teardown, and the both-slots test fails withFAIL write returned -10054 after 131072 bytes.POSIX backends are unaffected: they do not use libuv, and kqueue/epoll deliver EOF/HUP natively. There is no way to reproduce the LSP condition itself in CI, which is what the force hook is for; the two-build matrix above stands in for the fail-before the gate cannot run on a Windows-only test.