usockets: re-arm readable in raw_shutdown after read_eof so the close is delivered on Windows - #34487
usockets: re-arm readable in raw_shutdown after read_eof so the close is delivered on Windows#34487robobun wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesHalf-open socket shutdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/http/node-http-connect.test.ts:2— TheisWindowsimport added here is never used anywhere in the file — likely a leftover from an earlier draft that gated the new test on platform. Drop it from the import list.Extended reasoning...
What the bug is
The diff changes the harness import at line 2 of
test/js/node/http/node-http-connect.test.tsfrom:import { bunEnv, bunExe, nodeExe } from "harness";
to:
import { bunEnv, bunExe, isWindows, nodeExe } from "harness";
but
isWindowsis never referenced anywhere in the file body. Verified with grep against the full modified file — the only occurrence of the tokenisWindowsis on line 2 (the import itself).Step-by-step proof
- Before this PR, the import was
{ bunEnv, bunExe, nodeExe }. - This PR adds
isWindowsto that destructuring list. - The new test
"half-open CONNECT socket's poll settles after peer FIN (no drain/end storm)"runs unconditionally on all platforms — there is notest.skipIf(isWindows), noif (isWindows)branch, and no other reference toisWindowsanywhere in the ~780-line file. - Therefore the identifier is bound at module scope and never read — dead code introduced by this PR.
The PR description notes the fix is Windows-specific and the fail-before proof is Windows-only, so it's plausible an earlier draft gated the test with
test.skipIf(!isWindows)or similar, and the import survived when that gate was removed (the test was made cross-platform sinceendCount == 1is a valid assertion on POSIX too).Why existing code doesn't prevent it
TypeScript's
noUnusedLocalsdoes not flag unused named imports (they're module bindings, not locals), and Bun's test runner doesn't lint imports at runtime — so nothing in the toolchain currently catches this.Impact
None at runtime —
harnessis already imported forbunEnv/bunExe/nodeExe, so there's no extra module load, and an unused binding has no observable effect. This is purely a code-cleanliness issue.REVIEW.md is explicit on the standard: "Every line you add must be demonstrably live" and "Delete dead code in the same PR that makes it dead." An import added by the PR that nothing consumes fails the first check.
How to fix
Remove
isWindowsfrom the import list:import { bunEnv, bunExe, nodeExe } from "harness";
(i.e., revert line 2 to what it was before this PR).
- Before this PR, the import was
|
Pushed 61c11e3: |
![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/node/http/node-http-connect.test.ts`:
- Around line 630-632: Update the write fixture around socket.write in the
parent and corresponding child-process paths to capture the boolean backpressure
result from the 4 MiB write, assert that it is false, and thereby verify the
short-write/re-arm path is exercised. Preserve the existing write payload and
test flow while applying the same assertion to all referenced write sites.
🪄 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: 5253f74d-b28b-4c65-8cd4-d89b130a93c0
📒 Files selected for processing (3)
packages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.ctest/js/node/http/node-http-connect.test.ts
|
Updated 8:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit ff1251a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34487That installs a local version of the PR into your bun-34487 --bun |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http/node-http-connect.test.ts`:
- Around line 631-635: Update the parent result handling around the
backpressured fixture to destructure the recorded backpressured value alongside
endCount, and add a non-restrictive assertion that it is a boolean (for example,
using the existing assertion style with expect.any(Boolean)). Preserve endCount
=== 1 as the behavioral invariant.
🪄 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: 2f45eb5d-f2ab-444d-9e38-bbcf1f8998f9
📒 Files selected for processing (1)
test/js/node/http/node-http-connect.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/http/node-http-connect.test.ts (1)
607-610: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the expected drain path occurs
The test currently passes with
drainCount === 0, so it would not detect an implementation that suppresses all writable events instead of only preventing repeated events. Assert at least one drain while retaining the upper bound.Proposed fix
expect(drainCount).toBeLessThanOrEqual(3); + expect(drainCount).toBeGreaterThanOrEqual(1);🤖 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/node/http/node-http-connect.test.ts` around lines 607 - 610, Update the drainCount assertion in the half-open writable-event test to require at least one drain while preserving the existing upper bound of three, ensuring the expected drain path occurs without allowing repeated events.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/http/node-http-connect.test.ts`:
- Around line 607-610: Update the drainCount assertion in the half-open
writable-event test to require at least one drain while preserving the existing
upper bound of three, ensuring the expected drain path occurs without allowing
repeated events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef89e1ab-4f45-48fa-bec4-281a885e343c
📒 Files selected for processing (1)
test/js/node/http/node-http-connect.test.ts
|
Re |
There was a problem hiding this comment.
404ab6f addresses both prior 🔴 threads: raw_shutdown now re-arms READABLE and lets the SHUT_DOWN branch close on the next poll (so onWritable sees a live socket after shutdown() returns — the synchronous-close_raw UAF path is gone), and the readable_ended DISCONNECT branch now probes for RST. The remaining inline comment is a follow-up on the latter: the probe only runs while poll_cb is still firing, so a post-settle RST still has no delivery path.
Extended reasoning...
404ab6f responded to both of my 15:26 comments. I re-traced us_internal_socket_raw_shutdown: with readable_ended it now does us_poll_change(READABLE) + bsd_shutdown_socket() and returns without dispatching on_close; the SHUT_DOWN → eof → close_raw path fires on the next iteration on all three backends (libuv via UV_DISCONNECT/readable, epoll via EPOLLHUP/recv=0, kqueue via EVFILT_READ recv=0). That restores the "shutdown never synchronously closes" contract the removed TODO documented, so the HttpContext::onWritable use-after-dtor concern no longer applies. The libuv.c change matches the probe I suggested; the new inline finding is the residual gap in that same area, not a new class.
|
af206e7: hands |
There was a problem hiding this comment.
Beyond the inline findings on af206e7 × 404ab6f, I also checked whether us_socket_resume clearing fin_deferred for a readable_ended socket drops sweep RST coverage — it doesn't: resume re-arms WRITABLE, which ORs in UV_DISCONNECT, so a later RST reaches poll_cb's readable_ended probe directly.
Extended reasoning...
af206e7 addressed the previous round's post-settle-RST comment by handing settled readable_ended sockets to the fin_deferred sweep. This run examined the ownership-handoff sites for that new state and found raw_shutdown (inline finding). The adjacent us_socket_resume site was also examined — it already clears fin_deferred, and although a readable_ended socket then loses sweep coverage, resume's us_poll_change(WRITABLE) re-adds UV_DISCONNECT, so poll_cb fires again on a later RST and the in-branch probe catches it; no gap there.
|
a00469c is the final iteration for this PR. All sibling sites that interact with the new CI: four consecutive builds (74617, 74623, 74631, 74640) with Ready for review. |
There was a problem hiding this comment.
a00469c addresses the last nit (the fin_deferred clear in us_socket_resume now runs after the !is_paused early-return, so a settled readable_ended socket keeps sweep ownership). Nothing further from this pass; deferring final sign-off to a human given the subsystem.
Checked: the paused path that now proceeds past the early-return still self-heals — us_poll_change(WRITABLE) re-ORs UV_DISCONNECT, so poll_cb's readable_ended branch re-enrolls the socket in the sweep within ~2 iterations; and raw_shutdown (92e79f6) already clears fin_deferred for the SHUT_DOWN case, so no stale ownership there.
Extended reasoning...
Overview
This PR adds a readable_ended bit to us_socket_t and uses it to stop the Windows/libuv poll_cb from re-deriving EOF on every tick once a half-open socket has already dispatched on_end — the level-triggered AFD UV_DISCONNECT was re-arming WRITABLE|DISCONNECT and firing on_writable/on_end per uv_run iteration. It touches packages/bun-usockets/src/{context.c,loop.c,socket.c,eventing/libuv.c,internal/internal.h} plus a regression test in test/js/node/http/node-http-connect.test.ts. Over five follow-up commits it also: probes for RST when the settled poll would otherwise miss it, hands settled half-open sockets to the fin_deferred sweep, defers close to next iteration in raw_shutdown to avoid a synchronous ~HttpResponseData() UAF, and clears fin_deferred at the two ownership-handoff points (raw_shutdown, resume).
Security risks
None identified. This is event-loop poll-mask/lifecycle bookkeeping; no user-controlled input parsing, no auth/crypto surface. The main risk class was memory safety (the synchronous-close UAF found and fixed in 404ab6f) and lifecycle correctness (sweep ownership vs. WSAESHUTDOWN misclassification, fixed in 92e79f6).
Level of scrutiny
High. This is core usockets eventing across three backends (epoll/kqueue/libuv-AFD) with subtle differences in level-triggered vs. one-shot vs. unmaskable semantics, and it interacts with the fin_deferred sweep, half-open close sequencing, and uWS onWritable re-entrancy. The PR went through five substantive review iterations, each of which found a real issue that required a fix commit. That history alone argues for a human maintainer's final look, and the load-bearing behavior is Windows-only so only the Windows CI lanes exercise it.
Other factors
- a00469c implements exactly the fix suggested in my last nit (move the clear after the early-return) and updates the comment. I traced the remaining paused+readable_ended interaction: it self-heals via the
WRITABLEre-arm re-ORingUV_DISCONNECT, and the SHUT_DOWN case can't carry a stalefin_deferredbecause 92e79f6 clears it inraw_shutdown. - The bug-hunting pass on this head found nothing new.
- The one open item deliberately left out of scope (SSL sibling of the
raw_shutdownfix inus_internal_ssl_shutdown) is pre-existing on POSIX and the author has filed it separately; I agree that's reasonable scoping. - The new test asserts
stderr === "", which REVIEW.md discourages for ASAN/debug noise, but the file already spawns withbunEnvand other tests in the suite do similar; not raising it. - Not approving: this is not a simple/mechanical change, it's Windows-AFD-specific socket lifecycle logic in a vendored C library with cross-platform reach, and it took multiple rounds to converge. A maintainer should sign off.
… a half-open socket (#34498) ### What does this PR do? #### Problem `us_internal_ssl_shutdown` only takes the `SSL_shutdown()` path when `SSL_RECEIVED_SHUTDOWN` is already set (the `!SSL_in_init && !RECEIVED_SHUTDOWN` branch at the top handles the common case with a raw TCP half-close instead). That precondition is met for an `allow_half_open` TLS server socket whose peer has already half-closed: `ssl_on_end` sets `SSL_RECEIVED_SHUTDOWN` before dispatching `'end'`, and the ZERO_RETURN path sets it by receiving the peer's close_notify. With `RECEIVED_SHUTDOWN` already set, `SSL_shutdown()` sends our close_notify and returns 1. The function then returned without calling `us_internal_socket_raw_shutdown`, so: - no TCP FIN was ever sent to the peer (only the close_notify TLS record); - the poll type stayed `SOCKET`, not `SHUT_DOWN`; - on epoll, the half-open poll had already been changed to `WRITABLE` when the peer's FIN was handled, the writable dispatch drops it to 0 once drained, and `EPOLLHUP` (the only thing reported at events=0) needs both halves FIN'd, so the loop's `is_shut_down` close path never ran. Every other exit from `us_internal_ssl_shutdown` already calls `us_internal_socket_raw_shutdown`; only the `ret >= 0` path did not. #### Reachable path The only sockets that reach the `SSL_shutdown()` path after a peer half-close are `UWS_HTTP_TLS` sockets with `allow_half_open` set: `node:https` server CONNECT/Upgrade tunnels (`upgradeToTunnelMode` sets `allow_half_open`), where the JS `'end'` handler's `socket.end()` routes through `handle.end()` → `us_socket_shutdown`. `net.Socket`'s Duplex `autoDestroy: true` normally follows with `handle.close()`, which hid this; with `autoDestroy` disabled the socket never closes. #### Fix Call `us_internal_socket_raw_shutdown(s)` after the `SSL_shutdown` branch regardless of `ret`, matching the other exit paths. This sends the TCP FIN and moves the poll type to `SHUT_DOWN`. #### Relation to #34487 Once the FIN is sent, epoll reports `EPOLLHUP` (both halves down) and the socket closes via the `is_shut_down` eof branch. kqueue and libuv still need the `readable_ended` re-arm from #34487 for the poll to re-derive eof at events=0; this PR is the TLS-side prerequisite that gets the FIN on the wire at all. ### How did you verify your code works? New test in `test/js/node/http/node-http-connect.test.ts` spawns an https CONNECT server with `autoDestroy` disabled, a TLS client that half-closes first (raw FIN, no close_notify first), and waits for the server socket's `'close'`: ``` # before (fail) … https CONNECT socket.end() after peer FIN half-closes TCP so the socket can close ^ this test timed out after 5000ms. # after (pass) … https CONNECT socket.end() after peer FIN half-closes TCP so the socket can close ``` Gated to Linux because the close is observed via `EPOLLHUP`; kqueue/libuv coverage comes with #34487. Also ran with no new failures vs main: `test/js/node/tls/` (159/161), `test/js/bun/net/socket.test.ts` + `test/js/node/net/` (249/268), and `test/js/node/test/parallel/test-{tls,https}-*.js` (207/210). <!-- robobun:evidence:begin --> --- **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/node/http/node-http-connect.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
@robobun rebase |
a00469c to
00d841d
Compare
|
Rebased onto main (78f3a46, picks up #34498). Conflict was in CI build 74979 finished 284/286. |
There was a problem hiding this comment.
All five prior rounds of findings are addressed and this pass found nothing new, but this is subtle enough (Windows AFD level-triggered DISCONNECT, fin_deferred sweep ownership across settle/shutdown/resume, and the raw_shutdown re-arm now affecting kqueue too) that a maintainer should sign off.
Checked this pass: the reordered !is_paused early-return in us_socket_resume — a paused+readable_ended socket that resumes self-heals via us_poll_change(WRITABLE) re-ORing UV_DISCONNECT and re-enrolling in the sweep on the next settle; the paused+readable_ended+shut_down kqueue case is effectively unreachable (readable_ended is only set while not paused, and raw_shutdown's READABLE arm delivers the close before a same-tick pause/resume could intervene). Also confirmed readable_ended/fin_deferred are zeroed at every socket creation site (accept, connect, listen, from_fd, start_connections) and that the new bit sits in the existing pad so us_socket_t doesn't grow.
Extended reasoning...
Overview
Fixes a Windows-only busy-spin where an allow_half_open usockets socket's uv_poll_t re-arms WRITABLE|UV_DISCONNECT on every tick after peer FIN, flooding on_writable/'drain'. Adds a readable_ended bit to us_socket_t, guards the half-open EOF re-entry in loop.c, and teaches libuv.c poll_cb to stop mapping level-triggered UV_DISCONNECT back to a readable dispatch once on_end has fired. Follow-on commits (all in this PR) address interactions the initial fix exposed: RST-after-FIN detection via probe + fin_deferred sweep enrollment, raw_shutdown arming READABLE instead of closing synchronously (avoids UAF in HttpContext::onWritable), clearing fin_deferred in raw_shutdown so the sweep's send-probe doesn't misread WSAESHUTDOWN as a reset, and reordering us_socket_resume's fin_deferred clear after the !is_paused early-return.
Files: internal.h (struct bit), context.c/loop.c/socket.c (init sites + guards), eventing/libuv.c (poll_cb readable_ended branch), and a new spawned-fixture test in node-http-connect.test.ts.
Security risks
None. No user-controlled input parsing, no auth/crypto changes. The change alters when poll events re-arm and when the fin_deferred sweep owns a socket; the risk class is lifetime/ordering (UAF, missed close, spurious RST), not security.
Level of scrutiny
High. packages/bun-usockets is the foundation of every TCP socket in Bun; a mistake here affects all HTTP/net/TLS on the affected platform. The fix reasons about Windows AFD polling semantics (level-triggered DISCONNECT, AFD_POLL_ABORT subscription rules from Bun's libuv patches), kqueue's lack of unmaskable HUP, and epoll's unmaskable EPOLLERR/EPOLLHUP — three different eventing models that must converge on the same observable behavior. The PR went through five review iterations, each finding a real interaction bug (synchronous-close UAF, post-settle RST gap, WSAESHUTDOWN misread, resume() ordering), which is itself a signal that the state machine is delicate.
Other factors
- CI green on all lanes including Windows across four consecutive builds; the new test passed on every Windows lane and the fail-before was verified locally on windows-x64 (
drainCount1707→1). - The SSL sibling (
us_internal_ssl_shutdownreturning withoutraw_shutdownwhenSSL_RECEIVED_SHUTDOWNis set) was explicitly deferred as pre-existing on epoll/kqueue and out of scope; that's reasonable but worth a maintainer's nod. - The 30s per-test timeout on "tests should run on bun" is justified (child
bun-debugspawns seven subtests; ~2s startup under ASAN) — it's not masking a hang in the code under test. - Jarred is already engaged (requested the rebase), so human review is already in flight.
|
Heads up on overlap with #34478: that PR's |
|
This fix also covers a POSIX-reproducible path that the PR body does not mention: a partial write issued from inside the Linux x64, release bun on main: const server = Bun.listen({
hostname: "127.0.0.1", port: 0, allowHalfOpen: true,
socket: { open(s) { s.shutdown(); }, data(){}, end(s){ s.end(); server.stop(); }, close(){} },
});
let endCount = 0;
await Bun.connect({
hostname: "127.0.0.1", port: server.port, allowHalfOpen: true,
socket: {
data(){}, drain(){}, close(){},
end(s) {
endCount++;
if (endCount === 1) s.write(Buffer.alloc(4 * 1024 * 1024));
},
},
});
setTimeout(() => { console.log("end fired", endCount, "times"); process.exit(endCount === 1 ? 0 : 1); }, 500);Prints test("allowHalfOpen: end fires once when the handler backpressures a write", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", `
const server = Bun.listen({
hostname: "127.0.0.1", port: 0, allowHalfOpen: true,
socket: { open(s){ s.shutdown(); }, data(){}, end(s){ s.end(); server.stop(); }, close(){} },
});
let endCount = 0;
const { promise, resolve } = Promise.withResolvers();
await Bun.connect({
hostname: "127.0.0.1", port: server.port, allowHalfOpen: true,
socket: {
data(){}, drain(){},
end(s){ if (++endCount === 1) s.write(Buffer.alloc(4 * 1024 * 1024)); },
close(){ resolve(); },
},
});
await promise;
console.log(JSON.stringify({ endCount }));
`],
env: bunEnv, stdout: "pipe", stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(JSON.parse(stdout.trim())).toEqual({ endCount: 1 });
expect(exitCode).toBe(0);
}); |
|
@robobun fix conflicts adopt and fix comments |
… is delivered After the peer's FIN on an allow_half_open socket (read_eof), the poll drops readable interest and settles with no events once writes drain. A later shutdown() then masked against those zero events, so nothing was armed to deliver the now fully-closed state and the socket stranded on the libuv backend (epoll reports HUP unmasked; kqueue already arms a sentinel here). Arm READABLE explicitly in that state so the SHUT_DOWN eof branch closes the socket on the next poll.
00d841d to
d3f367f
Compare
robobun
changed the title
|
Rebased onto main and reduced, per review. While resolving conflicts it turned out #37077 had landed the bulk of this PR on main: the What main still lacks is the Review items: long comments removed or cut to one line, the inline fixture and |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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. |
|
Status: ready for review. The diff is d3f367f (fix) + 9dfa9ee (test) + ff1251a (comment); all review threads are resolved. CI on 93619: final result: 145 jobs passed, including Local verification for the record: on windows-x64 the test times out 3/3 against main and passes 3/3 with the fix; |
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 3867-3868: Replace the fixed five-iteration setImmediate loop in
the socket shutdown test with an awaited observable condition that confirms
readable interest has been removed and the poll has reached its required idle
state. Invoke shutdown only after that callback or state signal is received,
preserving the existing test flow without arbitrary delays.
🪄 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: 07beab16-6380-48ce-971f-4150128f2391
📒 Files selected for processing (2)
packages/bun-usockets/src/socket.ctest/js/bun/net/socket.test.ts
…mises on error Wait for the post-EOF drain event (the last thing the socket emits) plus the two loop turns needed for the poll phase to consume the FIN's final re-report, instead of an unexplained number of turns. Awaiting drain alone is too early: shutdown() then still finds a wakeup armed and the test passes without the fix. Wire error handlers to reject and drop the dead peer.end().
3 participants
Problem
allowHalfOpensocket whose peer has sent FIN strands forever on Windows if we callshutdown()after the poll has gone idle. Repro:Bun.listen({allowHalfOpen: true}), peershutdown()s, victim getsend, victim callsshutdown()a few ticks later, victim'sclosenever fires.read_eof, the half-open EOF path drops readable interest and, once pending writes drain, the poll settles with no events armed.us_internal_socket_raw_shutdown(packages/bun-usockets/src/socket.c) then masks the poll withevents & READABLE, which is0 & READABLE: a no-opus_poll_change, so nothing is armed to report that both halves are now closed.Fix
raw_shutdown, whenread_eofis set, armREADABLEexplicitly instead of masking. The next poll delivers the EOF against aSHUT_DOWNsocket and the existing branch closes it.read_eofa readable wakeup can only ever report EOF (there is no more data), and theSHUT_DOWNeof branch is exactly the "both halves closed" close path every backend already uses; this just guarantees it has an event to run on.test/js/bun/net/socket.test.ts"closes when shutdown() runs after the poll has settled". On windows-x64 it times out against main and passes with this change; on Linux it passes both ways (HUP), as expected.socket.test.ts,tcp-server.test.ts,node-net*.test.*,node-http-connect.test.ts, and the vendoredtest-net-*half*,test-http-*connect*,test-http-*upgrade*,test-net-half-open-peer-reset-*suites. No changes versus main.Background
read_eofand stops polling for reads so the EOF is not re-reported.READABLE/WRITABLE) registered with the OS for a socket. On libuv this maps touv_poll_start; registering zero events means the OS will not wake us for that socket at all.raw_shutdownmarks the socketSHUT_DOWNand sends our FIN. The loop closes aSHUT_DOWNsocket when it next observes EOF on it; that observation needs a readable wakeup.History: this PR originally also fixed the Windows
drainstorm on these sockets; #37077 landed that part on main, so this is rebased onto it and reduced to the remainingraw_shutdowngap.no test proof · iteration 19 · 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