Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Updated 10:39 PM PT - Jul 15th, 2026
❌ @robobun, your commit 4a4e248 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33591That installs a local version of the PR into your bun-33591 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Not a duplicate of #31837. That PR hardens the existing singleton (roots the bare |
There was a problem hiding this comment.
Thanks for addressing the earlier feedback — the teardown path and Strong-root lifecycle look right now. Given this reworks per-instance heap allocation, GC rooting, and intrusive timer-heap cleanup across the Rust/C++ FFI boundary, it's worth a human pass before merge.
Extended reasoning...
Overview
This PR changes monitorEventLoopDelay() from returning a module-level singleton to returning a fresh histogram per call, matching Node.js semantics. It touches: the JS builtin (src/js/internal/perf_hooks/monitorEventLoopDelay.ts), the C++ histogram class (adds m_eventLoopDelayMonitor pointer field and a new _clearMonitor extern), the Rust timer subsystem (EventLoopDelayMonitor becomes a per-instance Box with a bun_jsc::strong::Optional root instead of a by-value field of All with a bare JSValue), and cancel_all_timeout_objects (new arm to reclaim enabled monitors at VM teardown). Four new tests cover independence, cross-monitor disable/reset isolation, and clean exit with monitors still enabled.
Prior review
I flagged three items on the first revision: a leak of the new Box + Strong handle at VM teardown (no EventLoopDelayMonitor arm in cancel_all_timeout_objects), a stale section-header comment, and a bun_core::heap convention nit. All three were addressed in 527771d and the threads are resolved. The follow-up bug scan found nothing further.
Security risks
None identified. No untrusted input parsing, auth, or network surface is touched — this is internal timer/histogram plumbing.
Level of scrutiny
High. The change restructures ownership across an FFI boundary: a heap-allocated Rust struct whose raw pointer is stored on a C++ JSCell, rooted back via a Strong handle, linked into an intrusive timer heap, with three release paths (userland disable(), VM teardown walk, and the histogram's m_eventLoopDelayMonitor being nulled). The teardown arm now recovers the container via from_timer_ptr, calls back into C++ to null the owning field, deinits the Strong, unlinks from the heap, and destroys the box — correct ordering matters here for both use-after-free and double-free. This is exactly the class of change CLAUDE.md calls out as most-blocked ("every allocation has exactly one named owner, released exactly once"), and while I believe the current shape is sound, it merits a maintainer's eyes rather than bot approval.
Other factors
- The new subprocess test asserts
expect(stderr).toBe(""), which CLAUDE.md advises against (ASAN/debug builds may emit benign warnings) — minor, not blocking. - PR description notes a known conflict with #33587 (prototype-chain shape) and overlap with #31837 (GC rooting of the same field via a different mechanism); a human should confirm the intended landing order.
- Test coverage for the fix itself is good (independence, disable/reset isolation, teardown).
|
CI on 4a4e248 (rebased): 282/286 jobs passed. The four red lanes are known systemic issues already being fixed separately, none touching perf_hooks or the timer code this PR changes:
The new |
…ograms Each call now allocates its own native monitor (heap-allocated EventLoopDelayMonitor with its own timer, resolution, and enabled state) instead of sharing a single module-level histogram. One consumer's disable()/reset() no longer affects any other consumer. The histogram is now rooted with a Strong handle while monitoring is enabled, replacing the bare JSValue that previously relied on the module-level cache for GC safety. enable() no longer resets the histogram, matching Node.js.
Adds an EventLoopDelayMonitor arm to cancel_all_timeout_objects so a monitor left enabled when a worker or VM shuts down has its timer node unlinked, its Strong root released, and its heap allocation freed. The owning histogram's m_eventLoopDelayMonitor pointer is nulled first so a late disable() call cannot double-free. Also: use bun_core::heap helpers for the Box/raw round-trip, and update the stale section header above DateHeaderTimer/EventLoopDelayMonitor.
527771d to
4a4e248
Compare
… retires its realm (#39871) ### Problem - A file that leaves `monitorEventLoopDelay().enable()` on keeps the monitor firing after `bun test --isolate` (or `--parallel`) retired its global. Each fire is a write-after-free in `hdr_record_value` under `EventLoopDelayMonitor::on_fire` (`JSNodePerformanceHooksHistogram_recordDelay`). Sentry BUN-41KQ, Windows, 23 events. - `All.event_loop_delay` (`src/runtime/timer/mod.rs:429`) is per thread and held the histogram as a bare `JSValue` that only the retired realm kept alive. The swap did not touch the monitor, and `enable()` returned early while enabled, so the next file could not replace the dead histogram. ### Fix - The monitor holds the histogram in a passive `bun_jsc::Weak`, and `on_fire` stops once the cell is gone. A `Strong` would pin the retired realm instead. - `stop_active_handles` disables the monitor, next to the fake-timers reset. It runs at every file swap and in the teardown stop phase, so the `Weak` goes while the heap is alive and the next file finds the monitor free. - `enable()` replaces whatever is registered. `disable()` unlinks the timer only when it is `ACTIVE`, since `on_fire` calls it with the node popped. - Verified: two new cases in `test/cli/test/isolation.test.ts`. The first fails on stock bun and on an unfixed ASAN build. Also ran the whole file and the perf_hooks suites. ### Background - `--isolate` runs each file in a fresh global on one thread. Between files, `stop_active_handles` closes what the file leaked, then the swap replaces the global. The per-thread `timer::All` survives. Leaked fake timers (fb03d39) were the same problem. - `EventLoopDelayMonitor` is one `EventLoopTimer` embedded in `All`. Every `resolution` ms it records the delay into the histogram and re-arms itself. - `bun_jsc::Weak` wraps a `JSC::Weak`. It reads empty once the GC reaps the cell, before the sweep runs `~HistogramData` (`hdr_close`). The handle lives in the JSC heap, and `All` is dropped after `~VM`, so the stop phase has to release it. <details><summary>Notes</summary> - Repro on release 1.4.0 (Linux, 5/5): file A enables a 1 ms monitor in a test. File B runs `Bun.gc(true)`, calls `createHistogram()`, and idles 150 ms. `bun test --isolate a b` prints `count = 139..140` for B's histogram: the freed cell is reused by B's histogram and the stale `record()` calls land in it (`totalCount` lives in the cell). Without `--isolate` the count stays 0. Windows decommits freed pages instead, hence the Windows-only Sentry events (23 since 1.3.14, 2 on 1.4.0). - The first new test asserts two things because the failure mode depends on the allocator. On release bun, a decoy histogram allocated after `Bun.gc(true)` receives the stale records (837 in my run). On the ASAN debug build the cell is not reused, and the failure is that file B's own `enable()` is a no-op, so its histogram stays empty. Each build fails on one of the two assertions. - The second new case is in the "collects globals pinned by leaked handles" block. It passes before and after this change (a bare `JSValue` does not pin either). It is there to keep a future `Strong` out of this field. - `JSNodePerformanceHooksHistogram_recordDelay` keeps its `uncheckedDowncast`. The value now comes out of a live `Weak` and was type checked by `jsFunction_enableEventLoopDelay`. A checked cast cannot detect a freed cell anyway. - Probed on the ASAN build with the monitor left enabled: three Worker terminations and the main thread under `BUN_DESTRUCT_VM_ON_EXIT=1`. No report. A monitor inside a Worker still records. - A ShadowRealm that enables the monitor and is then dropped did not reproduce in a short probe. The `Weak` covers that shape too if such a realm is collected. - Suites run against the debug build: all of `test/cli/test/isolation.test.ts` (28 pass), `test/js/node/perf_hooks/perf_hooks.test.ts`, `test/js/node/test/sequential/test-performance-eventloopdelay.js`. - Related open PRs take other routes and do not handle the file swap. #31837 roots the histogram with a Strong, which would pin one retired realm per file. #33591 makes each `monitorEventLoopDelay()` call independent (a Strong per monitor, no isolation or teardown step). This PR is the small crash fix on the current design. Either of those needs the `stop_active_handles` step if it lands later. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/test/isolation.test.ts bun test v1.4.0 (6e906e4) test/cli/test/isolation.test.ts: (pass) bun test --isolate > without --isolate, leaked global is visible to next file [366.34ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [329.40ms] (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [447.74ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [458.95ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [350.43ms] (pass) bun test --isolate > with --isolate, a file's process.chdir() is undone before the next file [2045.84ms] (pass) bun test --isolate > with --isolate, leaked outbound socket is closed before next file [1793.79ms] (pass) bun test --isolate > with --isolate, leaked fs.watch is closed before next file [1802.39ms] (pass) bun test --isolate > with --isolate, leaked vi.useFakeTimers() is deactivated before next file [2504.91ms] (pass) bun test --isolate > leaked subprocesses are ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (4653fc9) test/cli/test/isolation.test.ts: (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [30.06ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [37.98ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [41.89ms] (pass) bun test --isolate > without --isolate, leaked global is visible to next file [44.36ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [41.27ms] (pass) --isolate: cached module_info handles `import * as ns; export { ns }` as a Namespace export [24.88ms] (pass) --isolate: delete require.cache evicts the SourceProvider cache [37.93ms] (pass) --isolate: SourceProvider cache covers CommonJS modules [34.53ms] (pass) --isolate: SourceProvider cache covers node_modules .mjs and type:commonjs packages [32.54ms] (pass) bun test --isolate > leaked subprocesses are killed for every isolated file, not just the first [47.75ms] (pass) bun test --isolate > module-scope subprocesses are killed for every isolated file, not just the first (--isolate) [53.23ms] (pass) bun test --isolate > with -- ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/test/isolation.test.ts bun test v1.4.0 (6e906e4) test/cli/test/isolation.test.ts: (pass) bun test --isolate > without --isolate, leaked global is visible to next file [444.93ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [480.41ms] (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [654.97ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [722.74ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [498.11ms] (pass) bun test --isolate > with --isolate, a file's process.chdir() is undone before the next file [2523.27ms] (pass) bun test --isolate > with --isolate, leaked outbound socket is closed before next file [2256.11ms] (pass) bun test --isolate > with --isolate, leaked fs.watch is closed before next file [2835.55ms] (pass) bun test --isolate > with --isolate, a leaked monitorEventLoopDelay() is disabled before next file [2686.55ms] (pass) bun test --isolate > with --isolate, lea ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 951ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/23] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 240 extern-C blocks audited [2/23] gen cpp.rs (cppbind) [3/23] gen JS modules (bundle-modules) Preprocess modules (11794ms) Bundle modules (84ms) Postprocesss modules (46ms) Bundle Functions (900ms) Generate Code (41ms) [12.89s] Bundled "src/js" for production 2625 kb 198 internal modules 13 native modules 92 internal functions across 17 files [3/9] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92 ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../JSNodePerformanceHooksHistogramPrototype.cpp | 4 +- src/runtime/jsc_hooks.rs | 5 ++ src/runtime/timer/EventLoopDelayMonitor.rs | 6 +- src/runtime/timer/mod.rs | 39 ++++++----- test/cli/test/isolation.test.ts | 79 ++++++++++++++++++++++ 5 files changed, 110 insertions(+), 23 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests …c/bindings/JSNodePerformanceHooksHistogramPrototype.cpp 2 2 0 src/runtime/jsc_hooks.rs 4 5 0 src/runtime/timer/EventLoopDelayMonitor.rs 2 2 0 src/runtime/timer/mod.rs 7 8 0 test/cli/test/isolation.test.ts 1 4 0 ``` </details> <!-- robobun:evidence:end -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Repro
Two independent consumers in one process (e.g. prom-client's
nodejs_eventloop_lag_*collector plus a health check like@nestjs/terminusEventLoopDelayHealthIndicator) share one histogram and one enabled flag: whichever callsdisable()first silently stops the other's monitoring; the second consumer'senable()returnsfalse; onereset()wipes the other's samples.Cause
src/js/internal/perf_hooks/monitorEventLoopDelay.tskept module-levellet eventLoopDelayHistogram / enabled / resolutionand returned the cached histogram on every call. On the native side,All.event_loop_delaywas a singleEventLoopDelayMonitorembedded by value with one timer and one bare-JSValuehistogram reference (kept alive only by the JS module cache).Fix
monitorEventLoopDelay()call creates a fresh native histogram.enable()heap-allocates aBox<EventLoopDelayMonitor>(own timer, own resolution, ownlast_fire_ns), inserts its intrusive timer node into the heap, and stores the pointer on the C++ histogram (m_eventLoopDelayMonitor); it returnsfalseif one is already present.disable()removes that node and frees the box; returnsfalseif none.bun_jsc::strong::Optional, so an enabled histogram stays rooted even if userland drops its reference (resolves the existing TODO about bare-JSValuerooting).enable()no longer callshistogram->reset(); Node preserves samples across disable/enable.__bun_fire_timeris already container-of based, so theEventLoopDelayMonitorarm works unchanged for any heap-allocated instance. The unusedAll.event_loop_delaysingleton field is removed.Verification
All pass. The three new
monitorEventLoopDelaytests fail on released Bun 1.4.0:Note: #33587 touches the same JS file for the prototype-chain shape and will conflict; both fixes are orthogonal.
[review] gate passed · iteration 2 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 2
evidence per changed file