Have condition-gated drive loops ref the event loop so unref'd timers fire without spinning - #32014
Have condition-gated drive loops ref the event loop so unref'd timers fire without spinning#32014robobun wants to merge 3 commits into
Conversation
|
Updated 10:36 PM PT - Jul 15th, 2026
❌ @robobun, your commit 9db2a20 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32014That installs a local version of the PR into your bun-32014 --bun |
|
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:
WalkthroughIntegrates timer-deadline parking into the idle event loop, converts timer entry points to unsafe raw-pointer receivers to avoid aliased borrows during re-entrant timer fires, updates Windows libuv handling, and adds tests ensuring unref() timers fire without busy-spinning. ChangesUnref'd timer event loop handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/runtime/jsc_hooks.rs`:
- Around line 1003-1013: The manual pairing of ref_()/unref() around
tick_with_timeout should be replaced with an RAII guard to ensure unref() always
runs on scope exit; create a small guard type (e.g., LoopRefGuard) whose
constructor calls unsafe { (*loop_).ref_() } and whose Drop implementation calls
unsafe { (*loop_).unref() }, then in the branch where needs_virtual_poll is true
instantiate the guard (let _guard = LoopRefGuard::new(loop_);) before calling
unsafe { (*loop_).tick_with_timeout(Some(×pec)) }; remove the explicit
unref() call and any manual ref_()/unref() pairing so the release is tied to the
acquisition site even if the branch later gains early returns.
In `@test/js/web/timers/timers-unref-idle-loop.test.ts`:
- Around line 45-68: In the "waiting on an unref'd timer parks the event loop
instead of spinning" test, add an unconditional assertion that inspects `stderr`
(the `stderr` variable returned from Promise.all on `proc`) before asserting
`exitCode` so test diagnostics include subprocess error output; locate the
assertion block that currently checks `{ fired, exitCode }` and `cpuMs` and
insert a check such as expecting `stderr` to be empty or matching the allowed
pattern immediately after parsing `stdout` and before the `exitCode` equality
assertion.
- Around line 9-68: Wrap these four independent tests in concurrency by either
replacing each it(...) with it.concurrent(...) (targets: the four test functions
currently using setTimeout/setInterval and Bun.spawn) or grouping them inside a
describe.concurrent(...) block; ensure test semantics unchanged (keep awaits,
promises, Bun.spawn usage and expects) and run the suite to verify no flakiness
from concurrent execution.
- Around line 27-43: Update the test "unref'd setTimeout fires while top-level
await keeps the process alive" to assert the subprocess stderr before asserting
exitCode: after awaiting proc.stdout.text(), proc.stderr.text(), and
proc.exited, first check that stderr is the expected value (or at least empty
string) using the existing stderr variable, then assert stdout and exitCode;
locate the test by the it(...) block or variables proc/ stdout/ stderr/ exitCode
and reorder/add the stderr assertion so diagnostics are printed when the
subprocess fails.
🪄 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: fac42a7f-325c-4214-9064-2529b85d935d
📒 Files selected for processing (3)
src/runtime/jsc_hooks.rssrc/runtime/timer/mod.rstest/js/web/timers/timers-unref-idle-loop.test.ts
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/jsc_hooks.rs (2)
925-928:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
get_timeoutinvariant comment.This comment still says
get_timeoutmust only run insideloop.isActive(), but the idle branch below now intentionally calls it to discover the next unref'd timer deadline. Leaving that invariant in place makes the new behavior look accidental.As per coding guidelines, "Sweep the same PR for everything describing the old state" and "A comment contradicting the code is a correctness bug."
🤖 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 `@src/runtime/jsc_hooks.rs` around lines 925 - 928, Update the invariant comment around ctx.timer.getTimeout/get_timeout to reflect the new intentional behavior: note that while get_timeout has side effects (pops + fires due to WTFTimer heap entries) it is normally guarded by loop.isActive(), but it may also be invoked in the idle branch to discover the next unref'd timer deadline; remove the absolute prohibition and clearly document the exceptional idle-case, why it is safe there, and any expectations (e.g., that callers handle the side effects or only rely on deadline discovery) so the comment matches get_timeout, loop.isActive(), and the idle-branch semantics.Source: Coding guidelines
988-995:⚠️ Potential issue | 🟠 MajorFix aliased-
&mutUB intimer::All::get_timeoutcall sites (and update stale comment).
src/runtime/jsc_hooks.rscallstimer::All::get_timeout(&mut (*state).timer, ...)from multipleauto_tickbranches (active-path and the parked/unix path).src/runtime/timer/mod.rsalready notes thatget_timeout(&mut self, ...)can re-enter JS viaWTFTimer::firewhile the outer&mut Allis still live (aliased-&mutUB), and calls out a TODO to fix this by switching the receiver to a raw pointer (this: *mut Self) and updating thejsc_hooks.rscall sites together. Change the signature and update all sibling call sites in the same PR.- The comment near the idle parking logic states
get_timeoutmust only run insideif (loop.isActive()), but the unix parked branch still invokestimer::All::get_timeoutwhenloop.is_active()is false. Update the comment to reflect the actual control flow (or adjust the guard to match the stated intent).🤖 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 `@src/runtime/jsc_hooks.rs` around lines 988 - 995, Change timer::All::get_timeout to take a raw receiver (this: *mut Self) instead of &mut self to avoid aliased-&mut UB from re-entrancy via WTFTimer::fire, then update all call sites that pass &mut (*state).timer (notably the calls in src/runtime/jsc_hooks.rs inside the auto_tick active and parked/unix branches) to pass a raw pointer (e.g., &mut (*state).timer as *mut _ or state.timer.as_mut_ptr) and adjust any unsafe blocks accordingly; also update the idle-parking comment near the unix parked branch to reflect that get_timeout may be invoked even when loop.is_active() is false (or alternatively tighten the guard to only call when loop.is_active()), ensuring the comment and control flow are consistent.Source: Coding guidelines
♻️ Duplicate comments (2)
test/js/web/timers/timers-unref-idle-loop.test.ts (2)
9-82: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider making tests concurrent (duplicate of previous review).
The past review suggestion to make these tests concurrent remains valid. All four tests are independent (no shared state or ordering dependencies), and per guidelines, tests that spawn processes should prefer
test.concurrentordescribe.concurrentto improve suite execution time.🤖 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/web/timers/timers-unref-idle-loop.test.ts` around lines 9 - 82, These four independent tests (the "unref'd ..." test cases in timers-unref-idle-loop.test.ts) should be run concurrently to speed the suite; change each it(...) to test.concurrent(...) (or wrap the group in describe.concurrent and keep the individual it blocks) so the tests that spawn processes (the blocks creating Bun.spawn and the top-level setTimeout/Interval promises) run in parallel without changing test logic or assertions.
71-81: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSame stderr pattern issue as the previous test.
This test has the same
stderr: expect.any(String)pattern that contradicts the documented behavior ofbunEnv(see comment on lines 42-48). The conventional pattern would assertexpect(stderr).toBe("")before the output 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/web/timers/timers-unref-idle-loop.test.ts` around lines 71 - 81, The test currently asserts stderr with expect.any(String) which conflicts with the documented bunEnv behavior; change the test to assert expect(stderr).toBe("") first (before parsing stdout), and then update the subsequent equality check to expect stderr to be "" (or remove stderr from the expected object) so the check uses the explicit empty-stderr expectation; refer to the variables stdout, stderr, output and the existing expect({ output, exitCode, stderr }) block when making the change.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.
Inline comments:
In `@test/js/web/timers/timers-unref-idle-loop.test.ts`:
- Around line 42-48: Update the two subprocess assertions in
timers-unref-idle-loop.test.ts to assert stderr is empty instead of using
expect.any(String): replace the object expectation that contains stderr:
expect.any(String) with stderr: "" (or filter known benign noise before
asserting) so bunEnv uses strict stderr checks; also mark the two independent
subprocess tests as test.concurrent to allow them to run in parallel. Ensure you
update the expectation object in the block containing expect({ stdout:
stdout.trim(), exitCode, stderr }).toEqual(...) and adjust the test declarations
for the two subprocess tests to test.concurrent(...).
---
Outside diff comments:
In `@src/runtime/jsc_hooks.rs`:
- Around line 925-928: Update the invariant comment around
ctx.timer.getTimeout/get_timeout to reflect the new intentional behavior: note
that while get_timeout has side effects (pops + fires due to WTFTimer heap
entries) it is normally guarded by loop.isActive(), but it may also be invoked
in the idle branch to discover the next unref'd timer deadline; remove the
absolute prohibition and clearly document the exceptional idle-case, why it is
safe there, and any expectations (e.g., that callers handle the side effects or
only rely on deadline discovery) so the comment matches get_timeout,
loop.isActive(), and the idle-branch semantics.
- Around line 988-995: Change timer::All::get_timeout to take a raw receiver
(this: *mut Self) instead of &mut self to avoid aliased-&mut UB from re-entrancy
via WTFTimer::fire, then update all call sites that pass &mut (*state).timer
(notably the calls in src/runtime/jsc_hooks.rs inside the auto_tick active and
parked/unix branches) to pass a raw pointer (e.g., &mut (*state).timer as *mut _
or state.timer.as_mut_ptr) and adjust any unsafe blocks accordingly; also update
the idle-parking comment near the unix parked branch to reflect that get_timeout
may be invoked even when loop.is_active() is false (or alternatively tighten the
guard to only call when loop.is_active()), ensuring the comment and control flow
are consistent.
---
Duplicate comments:
In `@test/js/web/timers/timers-unref-idle-loop.test.ts`:
- Around line 9-82: These four independent tests (the "unref'd ..." test cases
in timers-unref-idle-loop.test.ts) should be run concurrently to speed the
suite; change each it(...) to test.concurrent(...) (or wrap the group in
describe.concurrent and keep the individual it blocks) so the tests that spawn
processes (the blocks creating Bun.spawn and the top-level setTimeout/Interval
promises) run in parallel without changing test logic or assertions.
- Around line 71-81: The test currently asserts stderr with expect.any(String)
which conflicts with the documented bunEnv behavior; change the test to assert
expect(stderr).toBe("") first (before parsing stdout), and then update the
subsequent equality check to expect stderr to be "" (or remove stderr from the
expected object) so the check uses the explicit empty-stderr expectation; refer
to the variables stdout, stderr, output and the existing expect({ output,
exitCode, stderr }) block when making the change.
🪄 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: e60f6e18-615b-4060-9ae3-24c99c4ecef3
📒 Files selected for processing (2)
src/runtime/jsc_hooks.rstest/js/web/timers/timers-unref-idle-loop.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 (2)
src/runtime/timer/mod.rs (2)
923-999:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFinish the raw-pointer re-entrancy fix in
drain_timerstoo.This change removes the call-frame
&mut Allfromget_timeout, butdrain_timersstill documents the identical aliasing hazard at Lines 1061-1076 and still reachesEventLoopTimer::fire(...)with the caller-side&mut selfalive. A fired timer can re-enterupdate/removeand mint a second&mut All, so the same UB class remains on the main timer-drain path. Please switchdrain_timersand its call site to the samethis: *mut Selfpattern in this PR. As per coding guidelines, "Fix the whole class in the same PR."🤖 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 `@src/runtime/timer/mod.rs` around lines 923 - 999, The drain_timers path still holds a caller-side &mut self across EventLoopTimer::fire, leaving the same aliased-&mut UB; change drain_timers to accept and operate on a raw this: *mut All (mirror get_timeout) and update its caller(s) to pass the erased pointer (instead of &mut self) so you short-livedly borrow &mut *this only for peek()/delete_min() and drop before calling unsafe { EventLoopTimer::fire(min, &el_now, vm) }; update function signature(s) named drain_timers and its callers, and adjust local borrows and safety comments to match the raw-pointer re-entrancy pattern used in get_timeout.Source: Coding guidelines
809-835:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check that a timer was actually armed before forcing
run().
timers.peek()andensure_uv_timer()are separated by an unlocked window, and this type already allows cross-thread heap mutation underself.lock. If the last timer disappears in that gap,ensure_uv_timer()can return without starting the current minimum, but this code stillref_()suv_timerand blocks inrun(). That can park on stale timer state instead of falling back to the non-blocking pump. Haveensure_uv_timer()report whether it armed a live deadline, or re-check/stop the handle here before theref_()/run()pair.🤖 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 `@src/runtime/timer/mod.rs` around lines 809 - 835, The code may call uv_timer.ref_() and uws_loop.run() even if no timer was actually armed between timers.peek() and ensure_uv_timer(); update tick_uv_loop_with_timer_deadline to verify a live deadline before blocking: either change ensure_uv_timer() to return a bool indicating it armed a deadline and use that result to skip ref_/run when false, or after calling ensure_uv_timer() re-check the timers (e.g., peek() or an uv_timer.is_active()/is_started()-style query) and only call (*this).uv_timer.ref_() and (*uws_loop).run() if a live timer exists; still keep the existing unref() logic using active_timer_count as before.
🤖 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 `@src/runtime/timer/mod.rs`:
- Around line 923-999: The drain_timers path still holds a caller-side &mut self
across EventLoopTimer::fire, leaving the same aliased-&mut UB; change
drain_timers to accept and operate on a raw this: *mut All (mirror get_timeout)
and update its caller(s) to pass the erased pointer (instead of &mut self) so
you short-livedly borrow &mut *this only for peek()/delete_min() and drop before
calling unsafe { EventLoopTimer::fire(min, &el_now, vm) }; update function
signature(s) named drain_timers and its callers, and adjust local borrows and
safety comments to match the raw-pointer re-entrancy pattern used in
get_timeout.
- Around line 809-835: The code may call uv_timer.ref_() and uws_loop.run() even
if no timer was actually armed between timers.peek() and ensure_uv_timer();
update tick_uv_loop_with_timer_deadline to verify a live deadline before
blocking: either change ensure_uv_timer() to return a bool indicating it armed a
deadline and use that result to skip ref_/run when false, or after calling
ensure_uv_timer() re-check the timers (e.g., peek() or an
uv_timer.is_active()/is_started()-style query) and only call
(*this).uv_timer.ref_() and (*uws_loop).run() if a live timer exists; still keep
the existing unref() logic using active_timer_count as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f324e00-0e52-4104-8078-c035d0176c9e
📒 Files selected for processing (3)
src/runtime/jsc_hooks.rssrc/runtime/timer/mod.rstest/js/web/timers/timers-unref-idle-loop.test.ts
80367ec to
7bf3a2c
Compare
7bf3a2c to
85157df
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the setImmediate-park regression with the immediate_fired guard and the new test — that resolves my earlier comment. I didn't find any further issues, but since this changes blocking semantics in auto_tick (the core event-loop tick path) on both POSIX and Windows, it's worth a human pass before merge.
Extended reasoning...
Overview
This PR modifies auto_tick in src/runtime/jsc_hooks.rs so the !is_active() branch parks on the soonest timer-heap deadline instead of doing a non-blocking pump (fixing a busy-spin on POSIX and a never-fires on Windows for unref'd timers when only the runtime is driving the loop). It also converts All::get_timeout/All::drain_timers to raw-pointer receivers to close an aliased-&mut re-entrancy hole, adds a Windows tick_uv_loop_with_timer_deadline helper that ref's the uv_timer for one UV_RUN_ONCE, and adds a new test file with five regression tests including a CPU-usage check.
Since my previous review the author pushed a fix (commit c68e55b) for the issue I flagged: immediate_fired is now snapshotted before tick_immediate_tasks, and an else if immediate_fired branch falls through to the non-blocking tick_without_idle() so a just-fired setImmediate that satisfied the driver's wait condition doesn't get parked on an unrelated unref'd-timer deadline. A dedicated test ("awaiting setImmediate exits promptly when an unref'd timer is pending") now covers exactly that repro.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input handling is involved. The unsafe Rust is bookkeeping around already-unsafe FFI (uws/libuv loop pointers, the per-thread All), and the &mut self → *mut Self conversions tighten the existing aliasing story rather than loosen it.
Level of scrutiny
High. auto_tick is the heart of Bun's event-loop scheduling — it decides whether the runtime blocks, spins, or returns to the driver on every tick. A subtle mistake here can hang processes, busy-spin CPUs, or change exit semantics across every Bun program. The fix touches blocking behavior on two distinct backends (uSockets/epoll/kqueue and libuv), and the Windows path in particular adds new ref/unref bracketing around uv_run(UV_RUN_ONCE). This is well outside the "simple/mechanical" bar for bot approval.
Other factors
All CodeRabbit threads are resolved, the PR description is thorough, CI was previously green on all relevant lanes per the author's notes, and the bug-hunting system found nothing on the latest revision. The new immediate_fired guard plus its regression test directly implement the fix I suggested. I'm deferring purely on blast-radius grounds, not because of any open concern with the current diff.
c68e55b to
4fd2f80
Compare
robobun
changed the title
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
The mechanism here is right, and I want to say that clearly up front because I went looking for reasons it wasn't: a scoped ref over the existing uws::Loop::ref_() counter is the correct shape (KeepAlive would be strictly worse — it's a stored-state field with manual unref(), no RAII, bottoming out on the same primitive), putting the fix inside auto_tick's idle branch instead would be worse (is_active() is not a single-consumer bit), and I could not find a condition-gated drive loop you missed (e.g. --inspect-wait already goes through the liveness-gated path). The get_timeout/drain_timers raw-receiver conversion is also a real soundness fix that closes a documented TODO. Nice work.
Requesting changes for one behavior regression plus test/API-shape issues — none of which needs the approach rethought:
- (must fix) Every guarded loop went from non-blocking poll of its exit flag to park in the OS until woken, so every exit condition now needs a paired wakeup — and the REPL's SIGINT doesn't have one. With the ref held and an empty timer heap (the normal state while
awaiting inbun repl),auto_tickparks inkevent64/epoll_pwait2with a NULL timeout (get_timeoutreturns false), both backends retry onEINTR, and the REPL's SIGINT handler only storesexecution_forbidden— a bare flag flip with no loop wakeup. Pre-PR the non-blocking pump observed that flag within microseconds; post-PR, Ctrl+C on a hungawaitin the REPL parks forever. I audited the other exit-condition setters reachable from the refed loops; this is the only broken one, but it's an interactive one. Inline comment has the fix (wake the loop from the handler — async-signal-safe — plus the doc contract). - Tests 1–2 (the headline in-process ones) can't produce an attributable failure on any platform: on Windows a regression becomes the whole file hanging for the 3-minute CI timeout (bun:test's own per-test timeout lives in the same timer heap that stops draining), and on POSIX they pass on an unfixed build. Make them subprocess tests.
- 4 of the 6 live refed sites (
--hotentry,bun test --watch,--hot --preload— that one added by this PR's own last commit with no test — and the Worker wait) have no coverage at all, and a 5th refed site,VirtualMachine::wait_for, is dead code with zero callers. Delete it or say why it stays. ref_loop_scoped()should return a named#[must_use]guard (the repo'sEnteredEventLoopidiom), not the codebase's only-> impl Drop: rustc emits no unused warning for a bareev.ref_loop_scoped();, which silently reintroduces the exact bug at a site with (per 3) no test to catch it.- The ref makes
is_event_loop_alive*()true for the guarded scope, with one JS-observable consequence: an unref'dsetImmediateinside a test / preload / sync-required ESM that used to be dropped now runs. Probably the right behavior — but it's an unlabeled, untested behavior change. Name it and test it. - Both the PR body and the shipped test comment point the deliberate Node-TLA divergence at #14951, which is closed — so the divergence ships with no live tracker and a green test the future parity PR must knowingly break. (Also, "node --test does the same" is a Node >= 24 behavior produced by
node:testdeliberately ref'ing the loop — which actually strengthens the bun:test half of this change; worth stating accurately.)
With 1 fixed and the tests real, I'd approve this.
|
@robobun please address the review feedback above (#32014 (review)). Suggested order: the REPL SIGINT wakeup first (it's the one behavior regression) plus the |
|
@alii thanks, that review found one real regression and three gaps. All six items are addressed in 61d2d58, in the order you suggested; every thread has a reply with the details and is resolved. The mechanism is unchanged and no test timeout was raised. 1. REPL SIGINT wakeup (the behavior regression). Real, and worse than "latency": with the loop ref'd and no wakeup, Ctrl+C on a hung
2. Subprocess tests. 3. Untested sites + dead code. Added 4. Named guard. 5. Unref'd 6. Node-TLA divergence. Filed #33283 for the unsettled-TLA detection (#14951's close was for the CPU-spin half, which this PR fixes, so a fresh issue rather than a reopen). The divergent assertions are fenced with |
61d2d58 to
c8ae33e
Compare
|
@coderabbitai review The approach changed substantially since your last pass (which reviewed a design that parked inside |
|
✅ Action performedFull review finished. |
520557a to
5f883e5
Compare
|
CI status, for whoever picks this up: the diff is green, and the two red builds are unrelated lanes. Build #68115 ( Build #68108 ( (The earlier I did not take the heap-snapshot one on faith, since the
Interleaved file timings, alternating arms to avoid confounding with machine drift:
The within-arm spread (34s to 48s) swamps any between-arm difference, and the neutered mean is higher. That file is simply very heavy under debug+ASAN, and which of its tests cross the 5s default timeout shuffles between runs. So the SIGKILL is that file being slow on that agent, not this change. Local verification on the pushed tree: Happy to push again if you want a fresh build, but the token here is read-only so I cannot retry a single job, and I would rather not stack empty commits on the branch. Ready to merge or re-run from my side. |
5f883e5 to
171bd9a
Compare
600e0a4 to
589c94a
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
ait_for_promise,+30p:1-10— This file is an accidentally-committed shell artifact — the filenameait_for_promise,+30pis a fragment of ased -n '/wait_for_promise/,+30p'address expression, and the content is a stale 1554-line snapshot ofsrc/jsc/event_loop.rs(itswait_for_promise_with_terminationstill has the pre-600e0a4f_loop_refplacement). It has no.rssuffix and no build-system references; pleasegit rmit before merging.Extended reasoning...
What the bug is
The PR adds a new file at the repository root named
ait_for_promise,+30p(new file mode 100644, 1554 lines). This is not a source file — it's a shell-mishap artifact. The filename is the tail of asedaddress expression: something likesed -n /wait_for_promise/,+30p src/jsc/event_loop.rswhere an unquoted argument was word-split or mis-redirected, leavingait_for_promise,+30pas an output filename. The file's content is a full snapshot ofsrc/jsc/event_loop.rsat some point during this PR's development.Why it's clearly accidental
Several independent signals confirm this is not intentional:
- The filename is not a valid identifier or path in any language — it contains a comma and
+, and matches thesedaddr1,+Nrange syntax with the leading/wchopped off. - The content is stale relative to the PR's own HEAD. Its
wait_for_promise_with_termination(around line 1207 in the artifact) haslet _loop_ref = self.ref_loop_scoped();taken before thewhileloop, above theif !self.vm_ref().is_event_loop_alive() { break; }check. Commit 600e0a4 (this PR's tip) moved that guard inside the loop, after the liveness check, precisely because the outer placement made the check dead code. So the artifact captures a superseded intermediate state. - No
.rsextension and no build-system references. Cargo won't compile it; nothing in the diff or the tree references it. - It's at the repo root, not under
src/ortest/.
Step-by-step proof
- The diff shows:
diff --git a/ait_for_promise,+30p b/ait_for_promise,+30p/new file mode 100644/@@ -0,0 +1,1554 @@. - The file opens with the exact module doc-comment of
src/jsc/event_loop.rs://! \jsc.EventLoop` — the JS-thread event loop.` - Search the artifact for
wait_for_promise_with_termination: thePromiseStatus::Pending =>arm readslet _loop_ref = self.ref_loop_scoped();immediately, then enters thewhileloop, then reachesif !self.vm_ref().is_event_loop_alive() { break; }. Compare to the realsrc/jsc/event_loop.rshunk in this same diff, which placeslet _loop_ref = self.ref_loop_scoped();after that check with the comment "Scoped to auto_tick only, so the check above reads the real ref state". The artifact predates that fix. rg 'ait_for_promise' --files-with-matcheson the PR tree finds only the file itself — no build script, no Cargo.toml, no include.
Impact
There's no runtime impact (the file isn't compiled), but merging it would land 1554 lines of stale duplicated source at the repo root. It would pollute
grep/rgresults for anyone searchingevent_loopsymbols (every hit doubled, half of them pointing at buggy superseded code), confuse future readers, and clutter git history. That's a concrete enough consequence to block merge.Fix
git rm 'ait_for_promise,+30p'and amend/push. No other change needed. - The filename is not a valid identifier or path in any language — it contains a comma and
|
Re: the |
|
CI on
|
…mers fire without spinning `wait_for_promise`, `wait_for_promise_with_termination`, the entry-point and preload loader loops, and bun:test's drive loop all tick `auto_tick` until a JS-visible condition is satisfied, whether or not JS refs the loop. In that state `auto_tick`'s `!is_active()` branch is a non-blocking pump: the driver busy-spins a core on POSIX, and on Windows `uv_run(UV_RUN_ONCE)` returns without running due timers because `uv__loop_alive()` is false, so an unref'd timer never fires. Each of those drivers now holds a scoped uSockets-loop ref via `EventLoop::ref_loop_scoped()`, a named `#[must_use] LoopRefGuard`. With the ref held `auto_tick` takes its existing active branch and parks on the next timer-heap deadline on both platforms; `auto_tick` itself is unchanged. Liveness-gated drivers (`auto_tick_active`, `wait_for_tasks`) deliberately do not take it, so exit semantics are unchanged. A guarded loop parks in epoll/kqueue rather than polling, so every exit condition now needs a paired loop wakeup. The REPL's SIGINT handler only set `execution_forbidden`, which was observed only when JSC happened to wake the loop for its own reasons; it now wakes the loop through `uws::Loop::wakeup_raw`, a raw-pointer entry point so a handler interrupting a thread that holds `&mut Loop` cannot mint an aliasing `&mut`. The contract is documented on `ref_loop_scoped`. The ref also makes `is_event_loop_alive*()` true for the guarded scope, so an unref'd `setImmediate` scheduled inside a driver now runs instead of being cleared. This matches Node, whose `node:test` runner refs the loop for the same reason. Also converts `All::get_timeout` and `All::drain_timers` to raw-pointer receivers so the call-site `&mut All` no longer aliases the re-entrant `&mut All` that fired timer callbacks mint via `runtime_state()`, and deletes `VirtualMachine::wait_for`, which had no callers.
The fixture widened its RSS threshold only when the binary is named bun-asan, so a `bun bd` debug build (ASAN-instrumented, named bun-debug) measured ASAN's ~136 MB of quarantine retention against the 10 MB release threshold and reported a leak in all three modes. Probe ASAN_ENABLED the way test/harness.ts does, keeping the binary-name check as a fallback.
…ill fires The rebase onto #31216 picked up an is_event_loop_alive() break inside wait_for_promise_with_termination's loop. ref_loop_scoped bumps the same counter that check reads, so taking the guard before the loop made the break dead code and a Worker whose entry module never settles parked indefinitely instead of exiting (Node exits 13 here). Scope the guard to auto_tick only, after the liveness check, so the check reads the real ref state. Replaces the Worker test in timers-unref-idle-loop.test.ts with one that guards this: it previously asserted an unref'd timer fires inside a Worker's entry-module TLA with nothing else ref'd, which Node 26.3 does not do (the Worker exits 13 without waiting). Extends ref_loop_scoped's contract to cover the hybrid case.
589c94a to
9db2a20
Compare
|
CI on
Ready for a maintainer to merge or re-run; happy to rebase again if main moves first. |
|
Heads up: #34478 is a narrow us_loop_pump-level fix for the subprocess side of this (spawn.test.ts was timing out the whole file on Windows lanes). It's compatible with the LoopRefGuard approach here; once this lands the us_loop_pump bump is just a safety net. |
|
Another symptom this fixes, verified on Windows x64 against both head commits:
// hang.test.ts
import { test } from "bun:test";
test("never resolves", async () => { await new Promise(() => {}); });
This is what turned the Win2019 ConPTY race in |
|
This spin was traced again independently (a This branch no longer merges cleanly against main ( |
3 participants
What does this PR do?
Problem
Unref'd timers never fire on Windows when nothing else refs the event loop but a driver is still spinning it waiting on a condition, and on POSIX the same state busy-spins a core until the deadline.
A 2s unref'd-timer wait measures ~2000ms of CPU time (pure spin) on POSIX.
Cause
wait_for_promise,wait_for_promise_with_termination, the entry-point/preload loader loops, andbun:test's drive loop all tickauto_tickuntil a JS-visible condition is satisfied (promise settled, test phase done) regardless of whether JS has anything refing the loop. In that stateauto_tick's!is_active()branch is a non-blockingtick_without_idle(): the driver busy-spins on POSIX (the subsequentdrain_timerseventually fires the timer), and on Windowsuv_run(UV_RUN_ONCE)returns immediately without running any timers whenuv__loop_alive()is false.Fix
Each condition-gated driver holds a scoped uSockets-loop ref for its duration via the new
EventLoop::ref_loop_scoped()(a named#[must_use] LoopRefGuard). With that ref held,auto_ticktakes its existing active branch and parks on the next timer-heap deadline on both POSIX and Windows —auto_tickitself is unchanged. Liveness-gated drivers (the main run loop viaauto_tick_active,wait_for_tasks) deliberately do not take the ref, so exit semantics are unchanged.The guarded drivers:
wait_for_promise, the--hot/--watchentry loader, thebun test --watchloader,load_preloads' watcher branch, andbun:test's drive loop.wait_for_promise_with_termination(Workers) is a hybrid: since #31216 it breaks on!is_event_loop_alive()inside the loop, which reads the same counter the guard bumps, so there the guard is scoped toauto_tickonly, taken after the liveness check — see the Rebase note below.Two consequences the ref brings with it, both handled here:
epoll_pwait2/kevent64(both of which retry onEINTR) rather than polling, so a bare flag store is no longer observable. The REPL's SIGINT handler only setexecution_forbidden, so Ctrl+C on a hungawaitwas observed only when JSC happened to wake the loop for its own reasons — measured 12ms–600ms depending on build and background GC activity, with park deadlines as long as 201 seconds when it didn't. The handler now wakes the loop after setting the flag (uws::Loop::wakeup_raw: a raw-pointer entry point so a handler interrupting a thread holding&mut Loopcan't mint an aliasing&mut; it bottoms out in an atomic add plus awrite()to the wakeup eventfd, so it is async-signal-safe). The contract is documented onref_loop_scoped,wait_for_promise, and exercised by a pty REPL test.is_event_loop_alive*()true for the guarded scope. That is JS-visible: an unref'dsetImmediateis cleared-without-running only when the loop looks dead, so one scheduled inside abun:testtest body, a preload, or a sync-required ESM module now runs instead of being dropped. This matches Node, and is covered by two dedicated tests.Also converts
All::get_timeoutandAll::drain_timersto raw-pointer receivers so the call-site&mut Allauto-ref no longer aliases the re-entrant&mut Allthat fired WTFTimer/timer callbacks mint viaruntime_state()(this was the explicitTODOaboveget_timeouton main; the doc moves to a# Safetysection).VirtualMachine::wait_forhad no callers and is deleted.Rebase notes (three conflicts with main since the original review, all in
get_timeout/drain_timersor the Worker driver):get_timeoutgained anow_out: &mut Option<Timespec>out-parameter. Kept the raw-pointer receiver, dropped the now-deadlet this = self+ the stale receiverTODO, boundmaybe_nowtonow_out. Thejsc_hooks.rsconflicts were comment-only.!is_event_loop_alive()break insidewait_for_promise_with_termination's loop (Node's Worker exit-13 semantics). That check reads the same counterref_loop_scopedbumps, so holding the guard across the whole loop made the break dead code and a Worker whose module never settles parked indefinitely. The guard there is now scoped toauto_tickonly, taken after the liveness check, so the check reads the real ref state. The Worker test is rewritten to guard that (it previously asserted an unref'd timer fires inside a Worker's entry-module TLA with nothing else ref'd, which Node 26.3 does not do; the Worker exits 13 without waiting).drain_due_wtf_timers(this: *mut Self, ...)and rewrote bothget_timeoutanddrain_timersbodies around it. Main's new bodies already use a rawthisinternally; the resolution keeps the raw-pointer receiver and the new body, and drops thelet this = self+ staleTODO+not_unsafe_ptr_arg_derefallow (no longer applies on anunsafe fn).Node 26.3 cross-check
node --testawaiting an unref'd timernode:testdeliberately refs the loop for exactly this reason)node --testwith an unref'dsetImmediateawait new Promise(r=>setImmediate(r))+ long unref'd timerWarning: Detected unsettled top-level await, exit 13Warning: Detected unsettled top-level await, exit 13Row 4 is a deliberate, tracked divergence:
wait_for_promiserefs the loop so the timer fires rather than Bun busy-spinning at 100% CPU. Node's early-exit requires unsettled-TLA detection, tracked in #33283 (split out of #14951, which was closed for the CPU-spin half). The test asserting today's behavior fences the two divergent assertions (stderrandexitCode) behind a comment pointing at #33283, so the parity PR is a one-line flip rather than an argument with a green test.How did you verify your code works?
test/js/web/timers/timers-unref-idle-loop.test.ts— ten subprocess tests, one per guarded driver plus the invariants. Each driver test measures the child'sprocess.cpuUsage()across an await of an unref'd timer, so a missing ref fails on POSIX (spin ≈ 2000ms CPU) as well as Windows (hang), rather than hanging the parent's test file for the CI timeout. Withref_loop_scopedneutered, 8 of the 10 fail; with the fix, all 10 pass (cpuMs≈ 12).setTimeout/setIntervalwait_for_promisevia top-level await--hotentry loader (byte-identical to thebun test --watchloop)--hot --preloadloadersetImmediateruns inside a bun:test body and inside a preloadawait setImmediatewith a 60s unref'd timer pending exits promptlybeforeExitstill firestest/js/bun/repl/repl.test.ts— pty test: evaluate a never-settlingawait, send SIGINT, assert the prompt returns and the REPL is usable.Also verified:
setTimeout.test.js+test/js/node/timers/(50 pass, 0 fail) and the full REPL suite (118 pass).One unrelated-looking test change, for an honest local run:
setTimeout-clear-in-callback-leak-fixture.jswidened its RSS threshold only when the binary is namedbun-asan, so abun bddebug build (ASAN-instrumented, namedbun-debug) measured ASAN's ~136 MB of quarantine retention against the 10 MB release threshold and reported a leak in all three modes. It now probesASAN_ENABLEDthe waytest/harness.tsalready does, keeping the name check as a fallback. With the quarantine disabled the delta is 0.0 MB, so there is no leak to begin with; the release threshold is unchanged.Related issues
kevent64zero-timeout busy-poll in those stacks is the spin this PR removes.no test proof · iteration 20 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/repl/repl.test.ts