event loop: run the timers phase before the entry point's queued work - #33509
Open
robobun wants to merge 13 commits into
Open
event loop: run the timers phase before the entry point's queued work#33509robobun wants to merge 13 commits into
robobun wants to merge 13 commits into
Claude / Claude Code Review
completed
Jul 7, 2026 in 12m 38s
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/jsc/VirtualMachine.rs:2456-2464 |
run_entry_point_body lacks the unhandled_error_counter guard the worker path has |
| 🟡 Nit | src/runtime/dispatch.rs:903-912 |
__bun_drain_expired_timers duplicates existing drain_timers_export |
| 🟡 Nit | src/jsc/VirtualMachine.rs:2453 |
In-tree comments reference 'the PR' — non-durable after merge |
Annotations
Check warning on line 2464 in src/jsc/VirtualMachine.rs
claude / Claude Code Review
run_entry_point_body lacks the unhandled_error_counter guard the worker path has
🟡 The worker path's guard at web_worker.rs:1144 checks `vm.unhandled_error_counter == 0` before `drain_expired_timers()`, but `run_entry_point_body` only checks `status_ptr == Rejected` — so identical `Promise.reject(...); setTimeout(..., 1); /*busy-wait*/` code now fires the timer under `bun run`/`bun test` but not in a worker (both matched pre-PR: neither fired). After 57ff06aa switched to `self.tick()`, that tick's trailing `handle_rejected_promises()` has already bumped the counter by the ti
Check warning on line 912 in src/runtime/dispatch.rs
claude / Claude Code Review
__bun_drain_expired_timers duplicates existing drain_timers_export
The body of `__bun_drain_expired_timers` is byte-identical to the pre-existing `drain_timers_export` at `src/runtime/timer/Timer.rs:503-511` (both: `timer_all()` → null-check → `(*all).drain_timers(vm.cast::<()>())`). Since both live in `bun_runtime`, the new `#[no_mangle]` shim could be a one-line `crate::timer::timer::drain_timers_export(vm)` delegation instead of duplicating the null-guard and cast — otherwise a future change to either has to be made in two places.
Check warning on line 2453 in src/jsc/VirtualMachine.rs
claude / Claude Code Review
In-tree comments reference 'the PR' — non-durable after merge
The doc comment here says "(see the Known-exclusion note on the PR)", and node-timers.test.ts:255 says "see the PR's Known-exclusion note" — after merge, "the PR" is unresolvable from source without git-blame. Suggest dropping both parentheticals (the sentences already state the exclusion self-contained, as the sibling web_worker.rs comment in this PR does), or use the numbered form `PR #33509` matching existing precedent in `src/`.
Loading