fix: guarantee heartbeat timer re-arm with try/finally by MiloStack · Pull Request #52270 · openclaw/openclaw

MiloStack · 2026-03-22T13:52:38Z

Summary

Wraps the heartbeat run() body in try/finally so scheduleNext() is called on every exit path
Removes all manual scheduleNext() calls inside run() — the finally block is the single re-arm point
Preserves the requests-in-flight exemption (wake layer handles retry for this case)

Root Cause

run() has multiple exit paths that skip scheduleNext(), permanently killing the heartbeat timer. The most common trigger is the requests-in-flight early return at line 1141, but any unhandled rejection in the async chain can also break the re-arm.

The cron system doesn't have this bug because armTimer() is always called in the onTimer() catch handler and uses a single-layer timer design.

Why not remove `.unref()`?

PRs #31226 and #31161 proposed removing .unref() from the timer. That's the wrong fix — .unref() is the correct Node.js pattern for background schedulers. The timer dies because scheduleNext() is never called, not because .unref() causes the timer to be GC'd. We confirmed this by patching out .unref() and observing the timer still doesn't fire.

`requests-in-flight` handling

PR #47107 proposed a similar try/finally but was flagged by Greptile for a regression: finally { scheduleNext() } causes a tight spin loop when requests are in flight (delay=0 since nextDueMs already passed).

This PR uses a requestsInFlight flag to skip the re-arm for that specific case. The wake layer (heartbeat-wake.ts) already handles retry scheduling via schedule(DEFAULT_RETRY_MS, "retry") — calling scheduleNext() would race with the wake layer's 1s retry and bypass the cooldown.

The existing tests (heartbeat-runner.scheduler.test.ts) cover both the retry and schedule-preservation behavior for requests-in-flight.

Testing

Tested on OpenClaw 2026.3.14-dev, Node 24.14.0, Docker, 5 agents:

Set heartbeat interval to 5m, disabled external watchdog
First heartbeat fired at exactly the 5m mark
Multiple agents running and communicating autonomously
Timer re-armed after each cycle (confirmed via debug logging)
Ran for 30+ minutes without timer death

Fixes #31139
Fixes #45772

greptile-apps · 2026-03-22T13:55:29Z

Greptile Summary

This PR fixes a long-standing heartbeat timer liveness bug by wrapping the run() body in a try/finally block so scheduleNext() is guaranteed to be called on every exit path. Previously, the requests-in-flight early return at line 1141 (and any unhandled rejection in the async chain) would silently skip scheduleNext(), permanently killing the interval timer. The fix is sound and well-scoped.

Key changes:

A single finally { if (!requestsInFlight) scheduleNext(); } replaces all prior manual scheduleNext() call-sites inside run()
A new requestsInFlight boolean correctly gates the re-arm for the requests-in-flight case, where the wake layer already schedules a 1 s retry — preventing a spin-loop race
scheduleNext() itself already calls clearTimeout on any live timer (lines 982–984), so there is no risk of stacking duplicate timers when finally fires
Early-exit guards at the top of run() (stopped / heartbeats disabled / no agents) remain outside the try block and do not call scheduleNext(), matching the previous behaviour; the updateConfig path re-arms the timer when conditions change

Confidence Score: 5/5

This PR is safe to merge — it is a clean, targeted fix with no new logic paths and correct handling of all edge cases.
The fix is mechanically correct. Every prior explicit scheduleNext() site now reaches the finally block instead, the requestsInFlight flag faithfully preserves the one case where re-arming must be skipped, and the pre-existing timer deduplication in scheduleNext() (clearTimeout guard) prevents any double-firing risk. No new state is introduced beyond the single boolean, and the change is well-covered by the existing scheduler tests the PR description references.
No files require special attention.

_{Reviews (1): Last reviewed commit: "fix: wrap heartbeat run() in try/finally..." | Re-trigger Greptile}

MiloStack · 2026-03-25T11:18:19Z

@steipete this is a legitimate fix apologies for the tag but i feel like resolving these discord / telegram flapping issues are of utmost importance. Especially after a major Node version bump when you jumped from Node version 22 to 24

The heartbeat scheduler silently dies after the first batch because run() has multiple exit paths that skip scheduleNext(). Once any path returns without re-arming the timer, heartbeats stop permanently. The most common trigger is the requests-in-flight early return, but any unhandled rejection in the async chain between runOnce() and the manual scheduleNext() calls can also kill the timer. Fix: wrap the run() body in try/finally so scheduleNext() is called on every exit path. All manual scheduleNext() calls inside are removed — the finally block is the single re-arm point. The requests-in-flight case is excluded via a flag because the wake layer (heartbeat-wake.ts) already handles retry scheduling for this case with DEFAULT_RETRY_MS. Calling scheduleNext() here would register a 0ms timer that races with the wake layer's 1s retry. Fixes openclaw#31139 Fixes openclaw#45772 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…oStack

…oStack

grp06 · 2026-03-27T01:29:38Z

Merged via squash.

Prepared head SHA: cd0bcc2fb86a1fae99689cb7be09ebc1768e3e53
Merge commit: b33ad4d7cb45b27faf7b21cf8333b5c5f630fc07

Thanks @MiloStack!

@grp06