Conversation
Adds a nested_dispatch_ticks counter to the POSIX event loop, incremented when us_loop_run_bun_tick begins while an outer ready-poll dispatch is still mid-batch - i.e. a poll callback synchronously waited on the event loop. Exposed as getEventLoopStats().nestedDispatchTicks in bun:internal-for-testing (always 0 on Windows, whose readiness is driven by libuv). This makes synchronous event-loop re-entry from inside a dispatch callback observable so it can be asserted on, and eventually forbidden (#33261). No behavior change: one well-predicted integer compare and increment at tick entry.
|
Updated 12:58 PM PT - Jul 2nd, 2026
❌ @alii, your commit ef3022f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33267That installs a local version of the PR into your bun-33267 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a ChangesNested Dispatch Tick Tracking
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this touches the core event-loop tick path and the C↔Rust InternalLoopData FFI struct layout, so a quick human sanity-check on the field placement and predicate would be worthwhile.
Extended reasoning...
Overview
Adds a diagnostic nested_dispatch_ticks counter to us_internal_loop_data_t, incremented at the top of us_loop_run_bun_tick when current_ready_poll < num_ready_polls (i.e. re-entered mid-dispatch). The field is appended to the end of the C struct (loop_data.h) and its Rust mirror (InternalLoopData.rs), read in get_active_tasks (event_loop.rs), and surfaced as getEventLoopStats().nestedDispatchTicks in bun:internal-for-testing. A new test drives it via HTMLRewriter inside a Bun.serve handler.
Security risks
None identified. The counter is a passive read-only stat exposed through an internal testing module; no user-controlled input reaches it and it doesn't gate any behavior.
Level of scrutiny
Medium. The logic is trivial (one compare + increment, calloc-zeroed on all platforms including the libuv path), and exposure is testing-only. However, the increment sits in us_loop_run_bun_tick — the hottest path in the runtime — and the change modifies an FFI struct whose layout must stay byte-identical between C and Rust. The field is correctly appended after tick_depth (both int/c_int, no padding change), and the compile-time offset_of!(PosixLoop, num_polls) == size_of::<InternalLoopData>() assertion in Loop.rs will still hold, but layout changes to this struct historically warrant a maintainer glance.
Other factors
The predicate placement (before us_internal_loop_pre, reading the outer current_ready_poll/num_ready_polls before the nested tick overwrites them) and the deliberate distinction from tick_depth are design choices tied to #33261 that a maintainer should confirm match the intended follow-up assertion semantics. No prior human reviews or outstanding comments; CI is still building.
The nested_dispatch_ticks field and its increment now exist only under BUN_DEBUG, mirrored in Rust with cfg(bun_debug) (both come from the Debug build type). Because this is the first build-type-conditional field in a C/Rust-mirrored struct, us_internal_loop_data_init now asserts (under ASSERT_ENABLED) that sizeof(us_internal_loop_data_t) matches the Rust mirror's size, following the existing Bun__lock__size check, so a future skew panics at startup instead of corrupting the loop struct. getEventLoopStats().nestedDispatchTicks still always exists in JS and reports 0 outside debug builds; a second, ungated test covers that shape on every build flavor.
|
Pushed a557b71: the counter is now debug-build-only, per review feedback.
An equally valid, slightly smaller shape is to keep the field unconditional and gate only the increment (no conditional layout at all) — happy to switch if a reviewer prefers that; the observable behavior is identical. |
nested_dispatch_ticks occupies what was tail padding after tick_depth, so its presence never changes sizeof(us_internal_loop_data_t) - which also means the size assertion could not have detected a BUN_DEBUG/bun_debug mismatch for this field. Drop the conditional layout entirely: the field always exists (it costs no space), only the increment compiles under BUN_DEBUG, and the mirror size check keeps its honest, general purpose.
There was a problem hiding this comment.
Thanks for taking the suggestion — the unconditional field + debug-gated increment looks right, and the reworded size-check comment is accurate now. LGTM.
Extended reasoning...
Overview
Adds a debug-only nested_dispatch_ticks diagnostic counter to the POSIX event loop and exposes it via bun:internal-for-testing's getEventLoopStats(). Touches packages/bun-usockets/src/{eventing/epoll_kqueue.c, internal/loop_data.h, loop.c}, the Rust mirror src/uws_sys/InternalLoopData.rs, the host-fn in src/jsc/event_loop.rs, the internal-for-testing TS surface, and a new test file. This is groundwork for #33261 — a detector for synchronous event-loop re-entry from inside a poll dispatch, with no user-facing behavior change.
Prior review
I previously flagged (inline on loop.c:89, now resolved) that the C↔Rust size assertion couldn't detect the BUN_DEBUG/cfg(bun_debug) skew it claimed to guard because the new int fills exactly the 4 bytes of tail padding after tick_depth. The author took the suggested fix in 2855d5b/ef3022f: the field is now unconditional in both the C struct and the Rust #[repr(C)] mirror, only the increment is #ifdef BUN_DEBUG, and the size-check comment now honestly says "size-only: it cannot see drift that hides in padding." That fully addresses my concern — there is no longer any conditional layout to skew.
Security risks
None. The counter is a debug-build integer increment on a calloc-zeroed field, exposed only through bun:internal-for-testing (which is gated to debug builds and Bun's own CI). No user input is parsed, no allocation, no new syscall, no auth/crypto/permissions surface.
Level of scrutiny
Event-loop code is normally high-scrutiny, but the actual release-build delta here is essentially nil: the new int occupies what was already tail padding (struct sizeof unchanged, no field offsets shift), the increment is compiled out, and the only new release-path code is a startup sizeof equality check under ASSERT_ENABLED following the established Bun__lock__size pattern. In debug builds it's one well-predicted compare + increment at tick entry. There is no control-flow, dispatch, or lifetime change to the poll loop itself. Given that, and that my earlier feedback was addressed exactly as suggested, I'm comfortable approving.
Other factors
- Verified
WindowsLoopalso carriesinternal_loop_data: InternalLoopData(src/uws_sys/Loop.rs:392), so the unconditional Rust read inget_active_taskscompiles on Windows and reads the calloc-zeroed 0 (Windows never runsepoll_kqueue.c). - The
#[unsafe(no_mangle)] pub(crate) static Bun__internal_loop_data__sizefollows the exact precedent ofBun__lock__sizeinsrc/threading/Mutex.rs:411-412, so the Cextern const size_twill link. - Test coverage: a debug-gated test proves the counter increments via
HTMLRewriter.transforminside aBun.servehandler (level-triggered socket, so no one-shot-loss risk), and an ungated test proves the field is present and stays 0 outside debug builds. Usesport: 0,using server, and awaits observable conditions. - The bug-hunting system found no issues; my resolved thread was the only outstanding feedback.
This branch has not been deployed
2 participants
What does this PR do?
Adds a
nested_dispatch_tickscounter to the POSIX event loop: it increments whenus_loop_run_bun_tickbegins while an outer ready-poll dispatch is still mid-batch — i.e. a poll callback (or a continuation it drained inline) synchronously waited on the event loop through one of thewaitForPromisecallers. The counter is exposed asgetEventLoopStats().nestedDispatchTicksinbun:internal-for-testing(always0on Windows, whose readiness is driven by libuv).First step of #33261: before synchronous event-loop re-entry from inside dispatch callbacks can be forbidden (and
expect().resolves/HTMLRewriter.transformde-blocked), the violation has to be observable. This counter is that detector — the follow-up work uses it to prove "this API no longer re-enters the loop", and it is the condition a future debug assertion will fire on.No behavior change: one well-predicted integer compare + increment at tick entry, no allocation.
nested_dispatch_ticksis deliberately narrower thantick_depth(which also counts re-entry from the pre/post phases): it counts only re-entry that starts while a ready-poll batch is mid-dispatch, which is exactly the state that loses one-shot events.How did you verify your code works?
test/js/bun/util/event-loop-stats.test.ts: anHTMLRewriter.transform()with an async element handler, called from inside aBun.servefetch handler, synchronously waits on the event loop from inside the server socket's poll dispatch — the counter increments. Server sockets are level-triggered, so unlike subprocess-based triggers this cannot lose one-shot events and does not depend on pidfd/waiter-thread differences across platforms. The test fails without the native change (USE_SYSTEM_BUN=1).Also verified outside the test runner with a plain script through the CLI: a fresh process reports
0, concurrent shell commands leave it at0, a normal async server handler leaves it at0, and a synchronously-waiting handler moves it to1.