windows: make us_loop_pump poll IOCP for unref'd handles (POSIX parity) - #34478
Conversation
|
Updated 1:14 AM PT - Jul 23rd, 2026
✅ @robobun, your commit f426d9a59b4b2019f748d77b68dd8777826a61ff passed in 🧪 To try this PR locally: bunx bun-pr 34478That installs a local version of the PR into your bun-34478 --bun |
WalkthroughChangesThis PR updates libuv socket polling and loop pumping, documents Windows subprocess unref behavior, and adds regression tests for subprocess exits, CONNECT tunnel shutdown, and Event-loop and subprocess lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Both earlier nits are addressed in c53a81d and I found nothing further, but I'd like a human to sign off: the two-line active_handles++/-- bracket sits in us_loop_pump, the Windows per-tick drive path, and pokes a libuv-internal counter directly rather than going through a public API. It also overlaps with the broader approach in #32014, which is a strategy call a maintainer should make.
What was reviewed:
us_loop_pumpchange:UV_RUN_NOWAITkeeps the poll timeout at 0, so the forced iteration cannot block; the increment/decrement is balanced on the only path through the function.- Exit semantics:
is_event_loop_alive()is read by callers beforetick_without_idle()/us_loop_pump, so an unref'd-and-not-awaited script still exits immediately (verified in the PR's own table). process.rschange is comment-only; the un-gatedproc.unref()calls in the existing tests now match the new dedicated regression test.
Extended reasoning...
Overview
Three files: packages/bun-usockets/src/eventing/libuv.c adds a two-line loop->uv_loop->active_handles++ / -- bracket around the existing uv_run(UV_RUN_NOWAIT) in us_loop_pump (Windows-only, gated by LIBUS_USE_LIBUV); src/spawn/process.rs replaces a stale "this is disabled on Windows" comment with one describing the new mechanism (no code change); test/js/bun/spawn/spawn.test.ts adds a subprocess regression test and drops three if (!isWindows) proc.unref() guards plus a stale comment (both requested in my earlier review and applied in c53a81d).
Security risks
None identified. No user-controlled input reaches the changed code; it's an internal event-loop drive helper. No auth, crypto, parsing, or filesystem surface.
Level of scrutiny
High. us_loop_pump is called from tick_without_idle() on every Windows event-loop tick — from wait_for_promise, the bun:test runner, and other condition-gated drivers — so a mistake here affects the entire Windows runtime. The change also writes directly to uv_loop_t->active_handles, a libuv-internal field, rather than going through uv_ref/uv_unref on a real handle. The reasoning is sound (with UV_RUN_NOWAIT the poll timeout stays 0, and uv__loop_alive() becoming nonzero only causes the while-body to execute once instead of zero times), but reaching into libuv internals on the hottest path is exactly the kind of change a maintainer familiar with the Windows loop should confirm.
Other factors
- The PR description explicitly positions this as a minimal stopgap compatible with #32014 (the comprehensive fix for condition-gated drive loops). Whether to land this narrow fix now vs. fold it into #32014 is a maintainer call.
- Both of my earlier inline comments (drop the
!isWindowsunref guards; use a combined{stdout, stderr, exitCode}assertion) were addressed in c53a81d and the threads are resolved. - The bug hunter found nothing on this revision. I checked that the increment/decrement is balanced (single straight-line path, no early return between them), that
us_loop_pumpis only reachable on Windows (#[cfg(windows)]on the FFI import insrc/uws_sys/Loop.rs), and that the new test drains stdout/stderr/exited concurrently and assertssignalCode === nullper the hang-guard convention.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/api/bun/subprocess.rs:1348-1360— On Windows, once.exitedis read on a still-running child,EXITED_PROMISE_PENDINGis set and never cleared, sojs_unrefpermanently skipsuv_unref()on the process handle —p.exited.then(log); p.unref()now keeps the parent alive until the child exits, whereas on POSIX (and Windows before this PR) it exits immediately. A getter access can't distinguishawait p.exitedfromp.exited.catch(noop), so this silently changesunref()semantics on one platform only; consider narrowing the re-ref (e.g. only re-ref if the handle was actually unref'd, and clear the flag / re-unref inon_process_exit) or at least clearing the flag soref()/unref()still work after.exited.Extended reasoning...
What changed
In the Running arm of
get_exited(subprocess.rs:1348-1360), Windows now unconditionally does two things:#[cfg(windows)] { self.update_flags(|f| f.insert(Flags::EXITED_PROMISE_PENDING)); self.process_mut().enable_keeping_event_loop_alive(); // → uv_ref() }
and
js_unref(subprocess.rs:550-559) now gates the poller unref on that flag:#[cfg(windows)] let skip_poller = self.flags.get().contains(Flags::EXITED_PROMISE_PENDING); if !skip_poller { self.process_mut().disable_keeping_event_loop_alive(); // → uv_unref() }
EXITED_PROMISE_PENDINGis inserted once and never removed anywhere in the file.uv_ref/uv_unrefare a single idempotent flag on the handle, not a counter. So once.exitedhas been read on a running child, every subsequentproc.unref()on Windows is a no-op for theuv_process_t— including afterproc.ref(); proc.unref(), sincejs_refdoesn't clear the flag either.Why this diverges from POSIX (and from Windows-before-this-PR)
On POSIX, the
#[cfg(windows)]blocks are absent:get_exitednever touches the poller andjs_unrefalways callsdisable_keeping_event_loop_alive(). Sounref()after reading.exitedstill drops the process fromPosixLoop.activeand the script can exit while the child keeps running.On Windows before this PR,
get_exitedalso didn't touch the poller andjs_unrefunconditionally calleduv_unref()(the "This is disabled on Windows" comment was stale — the call has shipped since #8456, per the PR description). So the pre-PR Windows behavior for this pattern matched POSIX.Step-by-step proof
const p = Bun.spawn({ cmd: ["sleep", "999999"], stdio: ["ignore","ignore","ignore"] }); p.exited.then(code => console.log("helper died", code)); // observe, don't await p.unref(); // nothing else ref'd — script should exit now, helper keeps running
POSIX (before & after):
p.exited→ Running arm creates the promise; no#[cfg(windows)]block runs.p.unref()→js_unref→disable_keeping_event_loop_alive()decrementsPosixLoop.active.- Nothing else ref'd → event loop returns → script exits immediately, child keeps running.
Windows before this PR:
p.exited→ Running arm creates the promise; poller untouched.p.unref()→js_unref→PollerWindows::disable_keeping_event_loop_alive→uv_unref().uv__loop_alive()→ 0 →uv_run()returns → script exits immediately.
Windows after this PR:
p.exited→ Running arm setsEXITED_PROMISE_PENDINGand callsuv_ref()(idempotent — handle was already ref'd).p.unref()→js_unrefreadsEXITED_PROMISE_PENDING, setsskip_poller = true, skipsuv_unref(). Stdio wasignoreso nothing else is unref'd either.uv_process_tstays inloop->active_handles→uv__loop_alive()stays nonzero →uv_run()blocks in IOCP → script blocks for 999999s.
The PR's own
does-not-hang.jsfixture never touches.exited, so the "unref semantic is preserved" row in the verification table does not exercise this variant.Why this is a real pattern, not a contrivance
The comment in
get_exitedsays "Accessing .exited while the child is still running is explicit intent to wait" — but a property getter cannot distinguishawait p.exitedfromp.exited.then(cleanup)orp.exited.catch(logSpawnError). The fire-and-forget-with-observer shape is common: spawn a long-lived helper, attach a.thenso you can react if it dies unexpectedly, thenunref()so it doesn't block your own shutdown. On POSIX that works exactly as intended; on Windows after this change the app hangs at shutdown until the helper is killed. There is no escape hatch — the flag is sticky, so evenp.ref(); p.unref()won't undo it.REVIEW.md flags exactly this class under "Fix the whole class … POSIX/Windows branches" and "Never change a Bun-native default" —
Subprocess#unref()is a Bun-native API whose observable contract now differs by platform.Possible narrower fixes
- Only re-ref in
get_exitedwhen the handle is currently unref'd (i.e. the caller already calledunref()before reading.exited— the actual bug being fixed), and don't set a sticky flag at all. A laterunref()after.exitedwould then still work, matching POSIX. - Or keep the flag but clear it (and
uv_unref()if the user had calledunref()) inon_process_exit, so it only pins the handle for the duration of the wait rather than permanently neuteringunref(). - At minimum, clear
EXITED_PROMISE_PENDINGinsidejs_refsop.ref(); p.unref()gives users a way out.
Whatever the resolution, this variant should get a test alongside the two new ones (a subprocess that reads
.exited, callsunref(), and asserts the parent exits without waiting on a long-lived child).
…herwise idle On Windows, proc.unref() calls uv_unref() on the uv_process_t, which drops it from loop->active_handles. With nothing else ref'd, uv__loop_alive() is 0, so uv_run() (any mode) skips its while-body entirely and never calls uv__poll. The wait-thread still posts the exit packet to the kernel IOCP queue, but it is never dequeued and on_exit_uv never fires, so await proc.exited never resolves. Bun's condition-gated drive loops (wait_for_promise, the bun:test runner) keep calling tick_without_idle() -> us_loop_pump() -> uv_run(NOWAIT) in that state, which turns into a hard busy-spin that never makes progress. This is what makes spawn.test.ts's 'should not hang' block time out the whole file on Windows lanes (build 74453 and earlier). Force us_loop_pump() to execute exactly one non-blocking uv_run iteration by bracketing the call with an active_handles bump. uv__poll(0) then dequeues any ready IOCP completions and uv__run_timers runs due timers. UV_RUN_NOWAIT keeps the poll timeout at 0, so this never blocks. Also replaces the stale 'This is disabled on Windows' comment in PollerWindows::disable_keeping_event_loop_alive with what actually happens.
…elivered Also switch the new test to a combined-object assertion per the review convention.
…ited is observed The us_loop_pump bump regressed test-http-server-unconsume-consume.js on Windows: forcing a uv_run iteration on an idle loop also processes unref'd socket polls, and a half-closed socket in that test re-arms on every tick. Scope the fix to the subprocess path instead. Accessing .exited while the child is still running now keeps the uv_process_t ref'd (and records a flag so a subsequent unref() does not undo it). The uv_process_t stays in loop->active_handles, uv_run() polls IOCP, the wait-thread's exit packet is dequeued, and on_exit_uv fires. A script that spawns and unrefs without touching .exited still exits immediately (does-not-hang.js), so the unref semantic is preserved when the caller is not waiting.
… EOF on allow_half_open sockets tick_without_idle on POSIX (us_loop_run_bun_tick) polls epoll/kqueue and dispatches regardless of ref state (only early-outs on num_polls==0). On Windows (us_loop_pump -> uv_run(UV_RUN_NOWAIT)), libuv's uv__loop_alive() guard skips the body when only unref'd handles remain, so IOCP completions for unref'd handles and due timers are never processed. That asymmetry is the underlying bug; spawn.test.ts is one symptom. Bracket uv_run(UV_RUN_NOWAIT) with an active_handles bump so it always executes one non-blocking iteration (POSIX parity). This surfaced a pre-existing allow_half_open socket loop on Windows: after the first EOF, loop.c leaves the poll writable-only, but us_poll_change always re-adds UV_DISCONNECT. AFD keeps signalling DISCONNECT level- triggered, poll_cb maps it to READABLE, recv(0) re-dispatches EOF, and the allow_half_open branch re-arms WRITABLE, so the poll bounces forever and onDrain posts a task every tick (the test-http-server-unconsume-consume.js CONNECT-tunnel socket hits this). Drop repeated DISCONNECT once readable polling has already been stopped. Drops the earlier subprocess-specific get_exited re-ref workaround.
e6d5a38 to
26de6ee
Compare
There was a problem hiding this comment.
The rework to the structural us_loop_pump fix resolves both of my earlier open threads on the previous revision — the .exited-getter re-ref trade-off and the onExit exclusion no longer apply now that subprocess.rs isn't special-casing EXITED_PROMISE_PENDING, and the new onExit then unref() test covers that sibling. Beyond the inline finding on the allow_half_open arm, I checked that the active_handles++/-- bracket can't block (UV_RUN_NOWAIT keeps the poll timeout at 0) and doesn't change exit semantics (Bun's is_event_loop_alive() is read before us_loop_pump).
Extended reasoning...
The PR was reworked in 26de6ee from the subprocess-scoped re-ref approach back to the loop-level fix, which moots my two prior inline comments on src/runtime/api/bun/subprocess.rs and src/spawn/process.rs — those referenced EXITED_PROMISE_PENDING / get_exited code that is no longer in the diff. Noting that here so those threads can be closed. The remaining inline finding (RST-after-FIN dropped by the new allow_half_open arm) is the substantive item for this revision; deferring to human review given this touches the Windows event-loop core and directly manipulates libuv's active_handles.
…n post-EOF arm UV_DISCONNECT also carries AFD_POLL_ABORT (RST) via the win-poll-abort-with-disconnect patch. The empty arm dropped both, so an RST arriving after FIN on a writable-only allow_half_open socket was silently lost (diverging from epoll's unmaskable EPOLLERR). Mirror the sibling paused-socket arm: SO_ERROR + zero-byte-send probe surfaces the reset as an error dispatch, and fin_deferred lets the sweep timer escalate a later reset once DISCONNECT is disarmed.
…open bounce Un-skip abort.ts:113 (timeout works) and add the #33334 repro to abort.test.ts in a subprocess so a regression is an attributable failure. Add a bun:test that runs the test-http-server-unconsume-consume CONNECT pattern in a subprocess and asserts 'end' fires exactly once and the process exits, pinning the poll_cb allow_half_open branch.
There was a problem hiding this comment.
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 `@test/js/bun/spawn/spawn.test.ts`:
- Around line 757-761: Condense the comment above the Windows regression test to
no more than three lines while preserving the active-handle/IOCP invariant:
uv_unref() can remove uv_process_t from active handles, causing uv_run() to skip
dequeueing the exit packet and leave children spinning; us_loop_pump now forces
a non-blocking iteration like POSIX us_loop_run_bun_tick.
- Line 756: Gate the “unref() + .exited with nothing else ref'd (Windows)” suite
with describe.skipIf(!isWindows), using the existing isWindows symbol so the
IOCP regression cases only run on Windows.
In `@test/js/node/http/node-http-connect.test.ts`:
- Around line 784-790: Replace the six-line regression-history comment above the
CONNECT test with only the confirmed issue URL for issue `#33334`. Keep the test
declaration and behavior unchanged.
🪄 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: c52a2fe5-76c4-443e-8853-ba1774f33dee
📒 Files selected for processing (4)
packages/bun-usockets/src/eventing/libuv.csrc/spawn/process.rstest/js/bun/spawn/spawn.test.tstest/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/bun/spawn/spawn.test.ts (1)
782-788: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid asserting empty stderr for the nested Bun subprocess.
This child launches another
bunExe()process, which can emit benign debug/ASAN diagnostics even when the lifecycle behavior succeeds. Keep drainingstderr, but do not require it to equal"".Proposed adjustment
const [stdout, stderr, exitCode] = await Promise.all([child.stdout.text(), child.stderr.text(), child.exited]); -expect({ stdout, stderr, exitCode, signalCode: child.signalCode }).toEqual({ +if (exitCode !== 0) console.error(stderr); +expect({ stdout, exitCode, signalCode: child.signalCode }).toEqual({ stdout: "resolved\n", - stderr: "", exitCode: 0, signalCode: null, });Based on learnings, nested Bun subprocess tests should avoid strict stderr assertions.
🤖 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/spawn/spawn.test.ts` around lines 782 - 788, Update the assertion around the nested Bun subprocess in the child lifecycle test to stop requiring stderr to equal an empty string, while continuing to await and drain child.stderr.text(). Preserve the existing stdout, exitCode, and signalCode expectations.Source: Learnings
🤖 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/bun/spawn/spawn.test.ts`:
- Around line 782-788: Update the assertion around the nested Bun subprocess in
the child lifecycle test to stop requiring stderr to equal an empty string,
while continuing to await and drain child.stderr.text(). Preserve the existing
stdout, exitCode, and signalCode expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 869b23c3-4366-42c9-9acf-51fb45145323
📒 Files selected for processing (3)
test/js/bun/spawn/spawn.test.tstest/js/node/http/node-http-connect.test.tstest/js/web/abort/abort.test.ts
…f_open arm on !is_closed WSAESHUTDOWN from send() after a local shutdown(SD_SEND) is not a peer reset, but the fin_deferred sweep probes sockets after local shutdown and would have reported it as CONNECTION_RESET instead of lingering. Also gate the whole allow_half_open arm body on !is_closed (matching the paused-socket sibling) so a completion racing close cannot re-set fin_deferred on an already-unlinked socket and leak fin_deferred_count.
Bun's libuv integration on Windows: what is structurally wrong and what the correct fix isTL;DRThe 1. libuv's documented contractFrom
2. Bun's model on POSIX: two countersPOSIX ( void us_loop_run_bun_tick(struct us_loop_t *loop, const struct timespec* timeout, ...) {
if (loop->num_polls == 0) return; // DISPATCH gate: ref-agnostic
...
epoll_wait(fd, ..., timeout); // timeout comes from the caller (BLOCK gate)
us_internal_dispatch_ready_polls(loop);
...
}Two independent counters:
So on POSIX, 3. Bun's model on Windows: one counter, conflatedOn Windows (
Every usockets This means there is no ref-agnostic dispatch gate on Windows. When 4. Direct
|
| Site | What it does |
|---|---|
src/libuv_sys/libuv.rs:461-496 |
Loop::{add_active, sub_active, inc, dec, ref_, unref, unref_count} all write uv_loop_t.active_handles directly |
packages/bun-usockets/src/context.c:628,721 |
active_handles++/-- around listen-socket lifecycle |
packages/bun-usockets/src/socket.c:221,237 |
active_handles-- on close paths |
src/event_loop/MiniEventLoop.rs:372-374 |
Exact same bracket as #34478: loop.inc(); loop.tick(); loop.dec(); |
src/io/keep_alive.rs |
KeepAlive ref/unref → loop_add_active/loop_sub_active |
src/jsc/event_loop.rs:1105-1122 (hold_forever_poll) |
creates a ref'd 4-minute uv_timer_t when !loop.is_active() to force uv_run to iterate — same purpose, different mechanism |
MiniEventLoop::tick_once (used by bun install, bun x, shell, etc.) has shipped the inc/tick/dec bracket since the Rust port. tick_possibly_forever has shipped hold_forever_poll (a ref'd dummy timer) since before that. #34478 applies the same compensation to the one site that was missing it (us_loop_pump).
5. Why Node.js does not hit this
Node's main loop is uv_run(loop, UV_RUN_DEFAULT). When uv__loop_alive()==0, uv_run returns and the process exits. Node has no outer condition-gated drive loop on top of libuv; child.unref() with nothing else ref'd means the process exits without the exit event firing, which is the documented unref() semantic.
Bun's wait_for_promise (src/jsc/event_loop.rs:963-977) and the bun:test driver (src/runtime/cli/test_command.rs:3232-3247) are outer loops with their own predicate (promise pending? / phase != Done?). They call auto_tick repeatedly regardless of libuv's opinion. That is the structural difference.
6. The fix options, evaluated
| Option | Mechanism | API surface | Regression surface | Matches existing code |
|---|---|---|---|---|
(a) active_handles++/-- in us_loop_pump (this PR) |
Force one NOWAIT iteration | Direct field write (same as libuv.rs:461-496) |
Runs prepare/check/endgames/timers/poll when they previously did not. One real bug surfaced (allow_half_open bounce, fixed in this PR); CI shows all 24 Windows lanes green. | Identical to MiniEventLoop::tick_once |
(b) uv_ref/uv_unref an existing handle |
Same as (a), via public API | uv_ref((uv_handle_t*)loop->uv_pre) |
Identical to (a) | Cosmetically cleaner; semantically identical |
| (c) Driver holds a LoopRefGuard (#32014) | Each condition-gated driver refs the loop for its duration | uws::Loop::ref_() (= active_handles++) |
Every guarded driver goes from "non-blocking poll of its exit condition" to "park until woken": each exit condition needs a paired wakeup(). #32014's review found REPL SIGINT (Ctrl+C on a hung await observed only when JSC happened to wake the loop). Open since June with changes-requested. |
Same primitive (active_handles++), different layer |
| (d) Driver exits on dead loop (#33286) | wait_for_promise breaks when !is_event_loop_alive() |
None | Changes TLA semantics (exit 13 on unsettled TLA, matching Node). Does not cover bun:test. |
Matches wait_for_promise_with_termination (workers) which already does this |
| (e) Dedicated pump handle | One uv_prepare_t created at loop init; us_loop_pump refs it around the call |
uv_ref/uv_unref on a purpose-built handle |
Identical to (a)/(b) | Same as hold_forever_poll but per-pump instead of per-driver |
Options (a), (b), (e) are semantically identical (they all make uv__loop_alive() briefly nonzero so uv_run(NOWAIT) does one non-blocking poll). The difference is cosmetic: (a) writes the field, (b)/(e) go through uv_ref. Bun already writes the field directly everywhere else, so (a) is consistent; (b) is a two-line change if the public API is preferred.
Option (c) is architecturally cleaner (the driver's "I'm waiting" intent is expressed as a ref, and uv_run(ONCE) parks properly instead of busy-spinning) but has a larger blast radius: every guarded driver's exit path must wake the loop, and #32014's review history shows that is easy to get wrong.
Option (d) is correct Node-parity for TLA but is a user-visible behavior change and does not solve bun:test.
7. Recommendation
The us_loop_pump bracket is the minimal fix at the layer where the two-counter/one-counter asymmetry actually lives, and it matches MiniEventLoop::tick_once exactly. If the direct field write is the objection, switching to uv_ref((uv_handle_t *)loop->uv_pre) / uv_unref(...) is a two-line change with identical semantics.
The allow_half_open EOF-bounce fix in poll_cb is orthogonal: it is a pre-existing bug in usockets' Windows AFD handling that was masked only because uv_run never processed unref'd polls. It would also surface under #32014's approach (which keeps the loop ref'd and therefore polling) and is covered by #34487 as a standalone fix.
If the preference is to pursue (c) (#32014) as the long-term shape, this PR's us_loop_pump change is a strict subset and can land as a stopgap that #32014 makes redundant (but does not conflict with).
|
This also fixes an IPC-specific hang that came up while un-skipping // test/js/bun/spawn/ipc-unref.test.ts
import { beforeAll, describe, it } from "bun:test";
import { bunEnv, bunExe } from "harness";
describe("unref vs ipc", () => {
beforeAll(async () => {
const defer = Promise.withResolvers<string>();
const child = Bun.spawn(
[bunExe(), "-e", "Bun.serve({port:0,fetch:()=>new Response()}); process.send('up')"],
{ env: bunEnv, stdout: "inherit", stderr: "inherit", stdin: "ignore", ipc: m => defer.resolve(m) },
);
child.unref();
await defer.promise; // hangs forever on Windows; beforeAll timeout never fires either
child.kill();
}, 10_000);
it("dummy", () => {});
});On Windows the parent-side IPC (The |
|
@robobun fix conflicts? |
Conflicts: packages/bun-usockets/src/eventing/libuv.c: main (cc40d78) landed its own poll_cb half-open arm; take main's arm, keep this PR's us_loop_pump active_handles bracket, the early-return guard, and the WSAESHUTDOWN probe fix. test/js/web/abort/abort.test.ts: both sides added a test at the same insertion point; keep both.
|
Merged main at 439ed5a. Resolution notes:
Verified on Windows aarch64 debug (439ed5a) against the repro from this thread and build 78321: Linux x64 debug+ASAN: same set passes. Dropped the half-open clause from the title since that part is now on main. |
|
Conflicts resolved (439ed5a): #32630 landed its own |
|
@robobun can you fix appcontainer test failures? |
…nchInContainer Reusing the module-load-time attribute list across later event-loop turns proved fragile once us_loop_pump runs a uv_run iteration between module load and the test body: CreateProcessW started returning ERROR_INVALID_PARAMETER for the second call even though the backing buffers and SID bytes were byte-identical when inspected (inspection itself perturbed the failure away). Re-initializing the list into the same backing buffers on each launch is cheap and avoids whatever interaction that is.
|
appcontainer.test.ts fixed in f426d9a: the test was reusing a module-load-time I could not pin down the exact mechanism (a single |
There was a problem hiding this comment.
No new issues found after the merge with main, but this touches the core Windows event-loop drive path (us_loop_pump, poll_cb) and the appcontainer test fix has an admitted unexplained mechanism, so leaving this for human sign-off.
What was reviewed:
us_loop_pumpactive_handles++/--bracket — matches the existingMiniEventLoop::tick_oncepattern;UV_RUN_NOWAITkeeps poll timeout at 0 so it never blocks.poll_cbearly-return guard and theWSAESHUTDOWNexclusion inpeer_reset_probe— both narrow, checked against the paused-socket sibling arm.- The RST-discriminating half-open arm my earlier comments flagged was dropped in the merge (main's version from #32630 is used instead), so those concerns no longer apply here.
- appcontainer.test.ts: rebuilding the attribute list per-call is safe, but the root cause of the
ERROR_INVALID_PARAMETERis unpinned per the author's note.
Extended reasoning...
Overview
After the merge with main (439ed5a) and the appcontainer fix (f426d9a), the diff is now: (1) the active_handles++/-- bracket around uv_run(UV_RUN_NOWAIT) in us_loop_pump, (2) a WSAESHUTDOWN exclusion in us_internal_libuv_peer_reset_probe, (3) an early-return guard in poll_cb when no error/eof/R/W event remains, (4) a comment update in PollerWindows::disable_keeping_event_loop_alive, (5) new regression tests in spawn/abort/node-http-connect and un-skipping existing Windows guards, and (6) rebuilding the PROC_THREAD_ATTRIBUTE_LIST per-call in appcontainer.test.ts.
Security risks
None identified. The change is Windows event-loop dispatch mechanics; no auth, crypto, untrusted-input parsing, or permission surface.
Level of scrutiny
High. us_loop_pump is called on every non-blocking tick of Bun's Windows event loop from tick_without_idle(), wait_for_promise, and the test runner. Forcing uv_run to always execute one iteration changes what runs on every tick when the loop would otherwise be considered dead — the appcontainer test breakage is one example of previously-masked behavior surfacing. The author's own analysis (the long comment on 2026-07-20) frames this as aligning with Bun's established direct-active_handles model and points to MiniEventLoop::tick_once as precedent, which checks out, but the blast radius (every Windows tick) and the fact that it already surfaced two latent issues (the CONNECT-tunnel bounce, now on main; the appcontainer attr-list fragility) argue for maintainer review.
Other factors
- All my prior inline findings on this PR are resolved and the code they flagged (the RST-discriminating half-open arm,
fin_deferredhandling) was dropped in the merge conflict resolution — main's #32630 arm is used instead. - The appcontainer.test.ts change works around a failure the
us_loop_pumpchange surfaced without a pinned root cause ("I could not pin down the exact mechanism"). Verifier agents examined whether this is routing around a runtime bug and concluded it is a test-harness FFI lifetime issue rather than a Bun regression, but per REVIEW.md's "Never edit a test to route around a runtime bug it exposed" this is worth a human glance. - A maintainer is already actively engaged (asked for conflict resolution and the appcontainer fix), so this is not blocking on review attention.
|
Root cause of the
Why
|
|
CI on f426d9a (build 78394): all 16 Windows test lanes passed; |
… Node parity The drain-aware wait means the entry promise can now transition Pending -> Rejected inside on_before_exit() (e.g. a beforeExit listener rejects the awaited promise). That rejection was falling through the Pending-only check and the process exited 0 with the error swallowed. Add the missing Rejected arm (same shape as the initial-load check and web_worker.rs) so the error is routed through uncaught_exception. spawn.test.ts's "unref() + .exited with nothing else ref'd (Windows)" block asserted the old busy-spin side effect (exit 0, "resolved"). Node exits 13 for all three shapes; update the expectations. The underlying Windows IOCP-unref'd-handle fix (#34478) is still covered by the non-TLA "should not hang" block above it.
Fixes
test/js/bun/spawn/spawn.test.tstiming out on Windows lanes (4/4 retries on:windows: 11 aarch64in build 74453, and intermittently on 2019 x64 / x64-baseline going back months).Fixes #33334
Repro
Hangs 20/20 on Windows aarch64 debug and release. In
spawn.test.tstheshould not hangblock exercises this via the["unref", "exited"]order: when a batch lands with only unref'd children outstanding, the whole file times out. The same mechanism is why #33334 (AbortSignal.timeout()underbun test) hangs the whole file on Windows.Cause
tick_without_idle()is a platform asymmetry. On POSIX it isus_loop_run_bun_tick(ts=0), which polls epoll/kqueue and dispatches regardless of ref state (it only early-outs onnum_polls == 0). On Windows it isus_loop_pump()→uv_run(UV_RUN_NOWAIT), and libuv'suv_run()skips its while-body whenuv__loop_alive()is 0. With only unref'd handles remaining,uv__pollnever runs, so IOCP completions posted for those handles (a subprocess exit packet from the wait-thread, a socket event) are never dequeued; anduv__run_timersnever runs, so due timers never fire. Bun's condition-gated drive loops (wait_for_promise, thebun:testrunner) keep callingtick_without_idle()in that state, which turns into a busy-spin that makes no progress.The
PollerWindowscomment "This is disabled on Windows" has been stale since #8456 (theuv_unref()call shipped alongside it).Fix
Bracket the
uv_run(UV_RUN_NOWAIT)inus_loop_pumpwith anactive_handlesbump so it always executes exactly one non-blocking iteration.uv__poll(0)dequeues any ready IOCP completions anduv__run_timersruns due timers;UV_RUN_NOWAITkeeps the poll timeout at 0, so this never blocks.is_event_loop_alive()is read beforeus_loop_pumpruns, so exit semantics are unchanged: a script that spawns, unref's, and does not await still exits immediately.This surfaced a pre-existing
allow_half_opensocket loop on Windows (test-http-server-unconsume-consume.js, a CONNECT-tunnel socket): after the first EOF,loop.cleaves the poll writable-only, butus_poll_changealways re-addsUV_DISCONNECT; AFD keeps signalling DISCONNECT level-triggered;poll_cbmaps it to READABLE;recv(0)re-dispatches EOF; and theallow_half_openbranch re-arms WRITABLE, so the poll bounces between 0 and WRITABLE forever andonDrainposts a task every tick. That loop was only masked becauseuv_run()never processed the unref'd poll.poll_cbnow drops the repeated FIN once anallow_half_opensocket has already stopped readable polling, discriminating it fromAFD_POLL_ABORT(RST) via the sameSO_ERROR+ zero-byte-send probe the paused-socket arm uses, and settingfin_deferredso the sweep timer owns a later reset once DISCONNECT is disarmed.Also drops the three
if (!isWindows) proc.unref()guards in the existingspawn unref and kill should not hangtests so they exerciseunref()on Windows, and un-skipstest/js/web/abort/abort.ts:113(timeout works).Verification
Windows aarch64 debug, rebased on main:
unref(); await .exited/.exited; unref(); await/onExit; unref(); awaitspawn.test.ts -t "should not hang"(10 runs)spawn.test.tsspawn; unref; /* no await */exits immediatelyabort.test.ts#33334 repronode-http-connect.test.tsCONNECT-tunnel exitendfires oncetest-http-server-unconsume-consume.jsLinux x64 debug+ASAN:
spawn.test.ts,abort.test.ts,node-http-connect.test.tsnew tests all pass (Windows-only bug, cross-platform coverage).Node.js does not hit this because its main loop is
uv_run(UV_RUN_DEFAULT), which simply returns (and the process exits) whenuv__loop_alive()is 0; Bun's condition-gated drive loops sit on top of libuv and supply their own keep-going predicate.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn.test.ts test/js/bun/windows/appcontainer.test.ts test/js/node/http/node-http-connect.test.ts