bake: re-arm the memory visualizer timer when it fires

[robobun]

Problem

• Sending sM (subscribe to the memory visualizer topic) on /_bun/hmr, waiting about a second, then closing the socket corrupts the process-wide timer heap of a dev server on canary and debug builds.
• Debug builds abort with panic: assertion failed: self.root == v at bun_io::heap::Intrusive::remove (src/io/heap.rs:165), reached from timer::All::remove via HmrSocket::on_close -> on_unsubscribe (src/runtime/bake/dev_server/hmr_socket.rs:321 on main). The same call sits in DevServer's Drop (src/runtime/bake/DevServer.rs:1090 on main).
• Release canary builds take the prev == null branch of remove instead and delete_min() the real root of the heap: an unrelated timer (a Bun.sleep, a setTimeout, ...) is silently dropped and stays marked as armed. The repro below wedges on await Bun.sleep(300) on 1.4.0-canary.
• Cause: DevServer::emit_memory_visualizer_message_timer (src/runtime/bake/DevServer.rs:5448 on main) is an empty function. All::next() pops a due node without touching state/in_heap (every fire handler is responsible for that), so after the first tick the node is out of the heap but still reads ACTIVE/in_heap, and the unsubscribe and Drop paths remove it again.
• History: the body was gated on a cargo feature that nothing enabled, while the subscribe hook in hmr_socket.rs is gated on the BAKE_DEBUGGING_FEATURES const (canary or debug). Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184 deleted the never-compiled body and left the stub; the hook that arms the timer stayed live. Since Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184 the /_bun/memory_visualizer page is gone too, but the wire topic still arms the timer.
• on_unsubscribe also disarmed on emit_incremental_visualizer_events == 0 instead of the memory visualizer count (carried over from the Zig version). With the tick restored that would keep the timer ticking with zero subscribers when an incremental visualizer socket is open, and stop it early when a second memory visualizer socket is still connected.

Fix

• emit_memory_visualizer_message_timer marks the node FIRED before doing anything else, publishes the M frame and re-arms one tick later, the same shape as SourceMapStore::sweep_weak_refs (the other DevServer timer). It takes the raw timer pointer and recovers the owner with from_timer_ptr, like the sibling handler, so the dispatch arm passes t through.
• Arming and disarming are DevServer::arm_memory_visualizer_timer / disarm_memory_visualizer_timer, used by the subscribe hook, the unsubscribe hook (now keyed on emit_memory_visualizer_events == 0), the tick, and Drop. The inline runtime_state() copies in hmr_socket.rs are gone.
• emit_memory_visualizer_message_if_needed (called after each source map sweep) gets its body back: publish when there are subscribers.
• Audited the other 23 arms of __bun_fire_timer: every other handler sets a terminal state or re-arms on every path, so the stale node was specific to this handler. I left All::next() as is rather than also clearing in_heap on pop; that would be a timer-subsystem change affecting all handlers and both heaps, and nothing else needs it today.
• Verification: test/bake/dev/hot.test.ts, two new tests, wrapped in a canary-or-debug check because stable builds never emit the topic.
  • "ticks while subscribed and unsubscribes without disturbing other timers": waits for three M frames (the third only exists if the tick re-armed), checks they took at least 1.5s (a handler that re-inserted without moving the deadline would deliver them at once), then uses a 20ms setInterval in the fixture as the unrelated timer: a /tick request answers on its next fire, one round trip before closing the subscriber and one after. Finally re-subscribes and leaves that socket open so the harness teardown closes it with the timer armed.
  • "stays armed while another subscriber remains": two subscribers, close one, the other must keep receiving frames (fails with the old counter condition).
  • bun bd test test/bake/dev/hot.test.ts: 13/13 pass with the fix; with src/ stashed both new tests fail after one frame (received 1 memory visualizer frames); USE_SYSTEM_BUN=1 on 1.4.0-canary.1 fails the same way. The interval half on its own also distinguishes the builds: on the unfixed canary binary the interval stops firing after the close (3/3 runs), on the fixed build it keeps firing.
  • The repro below exits 0 on the fixed debug build (two M frames, sleep resolves) and aborts with the assertion above on an unfixed debug build.
  • cargo clippy -p bun_runtime --no-deps clean. CI on the rebased head (build 95709): all 13 build lanes and all 177 finished jobs green; the only non-green entries are darwin 14 test shards that expired in the queue before an agent picked them up, plus retried flakes in install/napi tests unrelated to bake.
• Rebased onto main after bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 merged: it had changed the arm site's ms_from_now to ForceRealTime, and that line now lives in arm_memory_visualizer_timer, so the helper uses ForceRealTime (required, since bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 moved the dev server timers into the real-clock heap). Remaining overlap: bake: unsubscribe the HMR socket from topics dropped by a re-subscribe #37878 fixes the unreachable else if in the subscribe handler and changes the same counter line in on_unsubscribe; this PR's hunk subsumes that line, so whichever lands second has a small rebase.
• If the visualizers are not coming back after Narrow crate-internal Rust visibility across all targets and delete the code it proves dead #36184, deleting the M/v hooks, the timer and the two orphaned *_visualizer.html files is the alternative; this PR takes the smaller route of making the code that is still live consistent.

Background

• EventLoopTimer is an intrusive node embedded in its owner (here DevServer.memory_visualizer_timer) and linked into timer::All.timers, a pairing heap. All::drain_timers pops each due node with delete_min() and calls the owner's handler by tag; popping clears the node's links but not its state/in_heap fields, which each handler updates itself (set FIRED/CANCELLED, or re-insert through All::update).
• All::remove trusts in_heap and calls the heap's remove, which uses a null prev link to mean "this node is the root". A popped node has null links, so removing it removes whatever the real root is; debug builds assert first.
• The memory visualizer is a canary/debug-only HMR topic: the dev server publishes a memory breakdown frame (M) on subscribe and then once a second while any socket is subscribed. BAKE_DEBUGGING_FEATURES is IS_CANARY || IS_DEBUG, so this is reachable on published canaries from anything that can open a WebSocket to the dev server.

Repro (from the report)

index.html loading app.ts, plus:

import html from "./index.html";
const srv = Bun.serve({ routes: { "/": html }, development: true, port: 0, hostname: "127.0.0.1" });
await (await fetch("http://127.0.0.1:" + srv.port + "/")).text();
const t0 = Date.now();
setTimeout(() => console.log("user setTimeout(2500) fired", Date.now() - t0), 2500);
const ws = new WebSocket("ws://127.0.0.1:" + srv.port + "/_bun/hmr");
await new Promise(r => (ws.onopen = r));
ws.send("sM");
await Bun.sleep(1500);
ws.close();
await Bun.sleep(300); // never resolves on 1.4.0-canary; assertion failure on a debug build
console.log("sleeping 4 s"); await Bun.sleep(4000); console.log("OK woke up");
srv.stop(true); process.exit(0);

1.4.0-canary.1+da3851e57: prints user setTimeout(2500) fired 2501, then hangs until killed.

Unfixed debug build:

panic: assertion failed: self.root == v
<bun_io::heap::Intrusive<...>>::remove            src/io/heap.rs:165
<bun_runtime::timer::TimerHeap>::remove           src/runtime/timer/mod.rs:319
<bun_runtime::timer::All>::remove                 src/runtime/timer/mod.rs:827
HmrSocket::on_unsubscribe                          src/runtime/bake/dev_server/hmr_socket.rs:321
HmrSocket::on_close                                src/runtime/bake/dev_server/hmr_socket.rs:338

Fixed debug build: M message 1, M message 2 one second later, sleep(300) resolved, OK woke up, exit 0.

Earlier version of the test

The first revision armed the unrelated timer with a 500ms Bun.sleep behind a /sleep route. Review pointed out that on a slow worker the sleep could finish before the close frame arrived, at which point that half of the test proved nothing; the interval replaces it so a timer is pending for the whole unsubscribe by construction.

---

no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bake/dev/hot.test.ts

[coderabbitai]

Walkthrough

Changes

The memory visualizer timer now has centralized arm, disarm, callback, and re-arm logic in DevServer. HMR subscription handling uses these helpers. Debug tests cover periodic frames and multiple subscribers.

Memory visualizer timer

Layer / File(s)	Summary
Timer lifecycle and dispatch src/runtime/bake/DevServer.rs, src/runtime/dispatch.rs	DevServer manages the one-second timer, emits frames only when subscribers exist, re-arms after callbacks, and cleans up on drop. Dispatch passes the raw timer pointer.
Subscription-driven timer integration src/runtime/bake/dev_server/hmr_socket.rs	HMR subscriptions arm the timer, and the timer is disarmed when visualizer subscriptions reach zero.
Visualizer timer regression coverage test/bake/dev/hot.test.ts	Debug and canary tests validate frame delivery, timer re-arming, safe unsubscribe behavior, and continued delivery to remaining subscribers.

Possibly related PRs

• oven-sh/bun#37878: Modifies memory visualizer timer lifecycle and subscription cleanup in the same HMR socket code.
• oven-sh/bun#37946: Overlaps in memory visualizer timer scheduling and event-loop timestamp handling.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary timer re-arming change.
Description check	✅ Passed	The description explains the problem, fix, background, verification steps, and test results in substantial detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 5:05 AM PT - Aug 14th, 2026}

❌ @robobun, your commit 832afbb has some failures in Build #95709 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37936

That installs a local version of the PR into your bun-37936 executable, so you can run:

bun-37936 --bun

[claude]

I reviewed this PR and didn't find any bugs. Because it touches the process-wide timer heap via unsafe container-of recovery and raises an open question (fix vs. delete the visualizer topic after #36184, plus overlap with #37878), a human look is still worthwhile.

Checked: emit_memory_visualizer_message_timer sets FIRED before any path can call disarm, matching the sweep_weak_refs sibling; All::update handles the re-insert-when-ACTIVE case so arm is safe from all three call sites.
Checked: the unconditional re-arm after publish — the subscriber count is >0 when the tick fires (arm/disarm and fire are all JS-thread), and uWS publish does not synchronously invoke on_close, so the debug_assert(emit_memory_visualizer_events > 0) holds.
Checked: the new tests await observable events (frame counts, waitForLine, response body) rather than sleeps, wire onerror/onclose to reject, and gate on hasBakeDebuggingFeatures.

Extended reasoning...

Overview

This PR fixes a timer-heap corruption in the bake dev server's memory visualizer. The previous emit_memory_visualizer_message_timer was an empty stub, so when the intrusive EventLoopTimer node was popped by All::next() its state/in_heap fields were never updated; a subsequent unsubscribe or Drop then remove()d a node the pairing heap no longer contained, discarding whatever timer was actually at the root (assertion failure in debug, silent hang in release canary). The fix marks the node FIRED, publishes the frame, and re-arms one tick later — the same shape as SourceMapStore::sweep_weak_refs. Arm/disarm are factored into DevServer methods used from the subscribe hook, unsubscribe hook (now correctly keyed on emit_memory_visualizer_events instead of the incremental-visualizer counter), the tick, and Drop. The dispatch arm now passes the raw *mut EventLoopTimer and the handler recovers the owner via from_timer_ptr. Two new tests in test/bake/dev/hot.test.ts cover re-arming, cross-timer non-interference on unsubscribe, and the multi-subscriber counter fix.

Security risks

None identified. The topic is gated behind BAKE_DEBUGGING_FEATURES (canary/debug only) and the change restores intended behavior rather than adding new surface. The unsafe blocks follow the established from_timer_ptr container-of pattern already used by the sibling DevServerSweepSourceMaps arm, and the safety contract is documented.

Level of scrutiny

Moderate-to-high. While the diff is small and follows an existing pattern, it interacts with the process-wide intrusive timer heap through raw pointers, and the failure mode of getting this wrong is silent corruption of unrelated timers. I verified against timer::All::update/remove that the state machine is consistent (update removes-then-inserts if ACTIVE, so arm is idempotent; disarm early-returns on non-ACTIVE), and confirmed the publish call cannot synchronously drive the subscriber count to zero before the unconditional re-arm. The tests are well-constructed: they await frame counts and server output rather than sleeping, wire error/close events to reject the awaited step, and use WAIT_MULTIPLIER for the deadline.

Other factors

The PR description explicitly flags two decisions a maintainer should weigh: (1) whether to keep this fix or instead delete the memory-visualizer topic entirely now that #36184 removed the UI page, and (2) overlap with #37878 on the on_unsubscribe hunk requiring a rebase for whichever lands second. It also notes the alternative of clearing in_heap inside All::next() was deliberately not taken. These are reasonable calls but are exactly the kind of scope/architecture judgments that benefit from a human sign-off. Given the native-code + unsafe + shared-timer-subsystem nature of the change, deferring rather than auto-approving.

[coderabbitai]

Actionable comments posted: 2

🤖 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/bake/dev/hot.test.ts`:
- Around line 632-635: Replace the fixed Bun.sleep in the response handler with
a server-controlled pending promise or release gate, and trigger its release
only after the subscriber close handshake completes at the later unsubscribe
step. Keep the unrelated timer pending throughout unsubscribe so the test
verifies disarming memory_visualizer_timer does not clear other pending timers.
- Around line 662-709: Update the WebSocket failure handling around fail,
waitUntil, and handle.close to retain a terminal socket error even when no wait
is active. Ensure every active or subsequently started wait rejects with that
error, clear the active deadline before rejecting, and make close() reject on
socket error as well as resolve on the close handshake without losing the
failure handler.

🪄 Autofix

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: e8ea8569-0c54-4e86-a5b4-ab18030de9b2

📥 Commits

Reviewing files that changed from the base of the PR and between 165dc9f and f05d69b.

📒 Files selected for processing (4)

• src/runtime/bake/DevServer.rs
• src/runtime/bake/dev_server/hmr_socket.rs
• src/runtime/dispatch.rs
• test/bake/dev/hot.test.ts

[claude]

I reviewed this and found no bugs. The fix follows the sibling SourceMapStore::sweep_weak_refs pattern exactly, the counter fix in on_unsubscribe is correct, and the tests are well-constructed. Deferring to a human because it touches unsafe intrusive-heap timer state, overlaps with #37878/#37946 (rebase coordination), and the PR itself flags a delete-vs-fix design choice a maintainer should call.

What was reviewed:

• emit_memory_visualizer_message_timer: sets FIRED before any work, then re-arms via update() — matches the contract every other __bun_fire_timer arm upholds; from_timer_ptr is already declared via impl_timer_owner!.
• disarm guards on state == ACTIVE, so the Drop refactor is behaviorally identical and the FIRED-then-rearm window is safe.
• Tests: failure paths latched, awaits are on observable conditions, gated on the same predicate as BAKE_DEBUGGING_FEATURES, and the /tick interval keeps an unrelated timer in the heap through unsubscribe by construction.

Extended reasoning...

Overview

The PR fixes a timer-heap corruption in the bake dev server: the DevServerMemoryVisualizerTick fire handler was an empty stub, so after All::next() popped the node it stayed marked ACTIVE/in_heap, and a later remove() on unsubscribe or Drop removed the wrong node from the process-wide heap (assertion in debug, silent unrelated-timer drop in release). The fix gives the handler a body (set FIRED, publish, re-arm), centralizes arm/disarm on DevServer, corrects the unsubscribe counter (emit_incremental_visualizer_events → emit_memory_visualizer_events), and restores emit_memory_visualizer_message_if_needed. The dispatch arm passes the raw *mut EventLoopTimer so the handler recovers the owner via the existing impl_timer_owner!-generated from_timer_ptr, mirroring SourceMapStore::sweep_weak_refs.

Security risks

None identified. The topic is gated on BAKE_DEBUGGING_FEATURES (canary/debug only). No new user-controlled input reaches allocation or path handling; the wire message body is a fixed-layout memory report already implemented in emit_memory_visualizer_message.

Level of scrutiny

Medium-high. The feature itself is debug/canary-only, but the bug it fixes corrupts the process-wide timer heap, and the fix relies on the same invariant every other fire handler upholds (handler owns state/in_heap after pop). I verified: All::update() handles the remove-if-ACTIVE-then-insert path, so arm after FIRED is correct; disarm early-returns when state != ACTIVE, so the Drop path is unchanged and a mid-tick unsubscribe (if it could ever happen — it's all JS-thread) wouldn't double-remove. The unconditional re-arm in the tick is safe because disarm runs synchronously on the JS thread when the last subscriber leaves, so the timer cannot fire with zero subscribers.

Other factors

• All prior review feedback (comment-cop on verbose comments, CodeRabbit on the fixed-sleep race and unwired socket-failure paths) is addressed and resolved in b07ae33.
• Tests were verified by the author to fail on the unfixed canary and pass on the fix; the interval-based /tick design keeps an unrelated timer pending through unsubscribe by construction, addressing the earlier flakiness concern.
• The robobun CI failures (winsysroot clang warning, x64-asan build tarball fetch) are unrelated infrastructure noise per the PR description and the failure paths shown.
• Deferring rather than approving because: (1) unsafe pointer-recovery + intrusive-heap state on a process-wide structure warrants a maintainer look even when the pattern is copied from a sibling; (2) the author explicitly notes overlaps with #37878 and #37946 that need merge-order coordination; (3) the PR raises delete-the-feature as an alternative, which is a maintainer call.

[claude]

I reviewed this PR and didn't find any bugs. Because it fixes heap corruption in the process-wide intrusive timer heap via unsafe pointer recovery, and overlaps with #37878/#37946 that need rebase coordination, a human look would still be worthwhile.

What was reviewed:

• The tick handler shape matches the sibling SourceMapStore::sweep_weak_refs (set FIRED, publish, re-arm via update()); from_timer_ptr is the existing impl_timer_owner! at DevServer.rs:432.
• The emit_memory_visualizer_events > 0 invariant holds when the tick fires (arm on 0→1, disarm on 1→0, JS-thread-only), so the debug_assert in emit_memory_visualizer_message is satisfied.
• Test WebSocket helper: failure latching, deadline cleanup on both resolve/reject, and the interval-based unrelated-timer check all look sound.

Extended reasoning...

Overview

The PR fixes a timer-heap corruption bug in the bake dev server's memory visualizer topic. The empty emit_memory_visualizer_message_timer stub left the popped intrusive node marked ACTIVE/in_heap, so unsubscribe or Drop removed it a second time — a debug assertion in bun_io::heap::Intrusive::remove, or in release, silently deleting whatever timer was actually at the heap root. The fix gives the handler a body (set FIRED, publish, re-arm), centralizes arm/disarm helpers, corrects the unsubscribe counter (emit_memory_visualizer_events instead of emit_incremental_visualizer_events), restores emit_memory_visualizer_message_if_needed, and updates the dispatch arm to pass the raw timer pointer. Two new tests in test/bake/dev/hot.test.ts cover re-arm, unrelated-timer preservation across unsubscribe, multi-subscriber, and teardown.

Security risks

The bug being fixed is itself a memory-safety issue (heap corruption reachable from a WebSocket client on canary/debug builds). The fix introduces new unsafe code: from_timer_ptr container_of recovery of &mut DevServer from the timer field pointer. This follows the existing impl_timer_owner!(DevServer; from_timer_ptr => memory_visualizer_timer) at line 432 and mirrors SourceMapStore::sweep_weak_refs exactly. No new attack surface is added.

Level of scrutiny

High. This touches the process-wide intrusive timer heap and its state-machine invariants via unsafe pointer arithmetic. A mistake here corrupts unrelated timers across the whole runtime. The fix is small and pattern-matched against a working sibling, and the PR description demonstrates the author traced the root cause thoroughly (including auditing all 23 other __bun_fire_timer arms), but the blast radius of a regression is large enough that a maintainer should sign off.

Other factors

• All prior review threads (comment-cop on comment length, CodeRabbit on test timing/failure paths) are resolved; the test was reworked from a fixed Bun.sleep to a 20ms interval so the unrelated timer is pending by construction.
• Tests were verified to fail on unfixed canary and pass with the fix; gated on hasBakeDebuggingFeatures so stable builds skip them.
• The PR notes overlaps with #37878 (same on_unsubscribe counter line) and #37946 (ms_from_now mock mode, now inside arm_memory_visualizer_timer) — whichever lands second needs a small rebase, which a human should coordinate.
• CI build failures reported by robobun are infrastructure issues (winver.h clang warning, dependency tarball download failures) unrelated to this diff, per the PR description.