fix(cron): add .catch() re-arm and watchdog to prevent runtime timer chain death#73355
fix(cron): add .catch() re-arm and watchdog to prevent runtime timer chain death#73355SkywingsWang wants to merge 1 commit into
Conversation
…chain death When the cron setTimeout chain breaks at runtime (e.g. due to macOS App Nap timer coalescing for background processes), the scheduler silently stops firing. This was observed after ~2.5 days of continuous gateway uptime on macOS with Apple Silicon. Changes: 1. Add armTimer(state) in the .catch() handler of both armTimer() and armRunningRecheckTimer() setTimeout callbacks. If onTimer() ever rejects without reaching its finally block, the chain is now re-established instead of permanently broken. 2. Add startCronWatchdog() — an independent setInterval-based watchdog (every 5 minutes) that detects when nextWakeAtMs is past-due by more than MAX_TIMER_DELAY_MS and force-triggers onTimer + re-arms the chain. setInterval is more resilient to OS-level timer deferral than chained setTimeout because libuv treats it as a persistent/repeating timer. 3. Wire up the watchdog in start()/stop() lifecycle. 4. Add tests for watchdog detection, no-false-positive, cleanup, and disabled-cron scenarios. Fixes openclaw#73166
Greptile SummaryThis PR adds two resilience mechanisms to the cron scheduler: a Confidence Score: 4/5Safe to merge — the watchdog and re-arm logic are sound; the double-arm edge case is benign in practice due to the Only P2 findings: a double-
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 738-745
Comment:
**Double `armTimer` call on rejection inside `try` block**
When `onTimer` rejects from within its own `try` block, the `finally` clause at line 967 (`state.running = false; armTimer(state)`) still executes first — JS `finally` always runs on async rejection. The `.catch()` handler then calls `armTimer` a second time, leaking timer A (from `finally`) and replacing `state.timer` with timer B. Both subsequently fire; whichever fires second hits the `state.running` guard and calls `armRunningRecheckTimer`, cancelling and re-creating the tracked timer. Net effect: spurious extra timer ticks in the error path.
The `.catch()` re-arm only uniquely adds value for the narrow edge case where code before the `try` block (specifically `armRunningRecheckTimer` on line 819) throws synchronously. Consider guarding against that specific case, or checking `!state.timer` before re-arming in `.catch()` to avoid the double-arm:
```ts
void onTimer(state).catch((err) => {
state.deps.log.error({ err: String(err) }, "cron: timer tick failed");
if (!state.timer) {
armTimer(state);
}
});
```
The same applies to the watchdog's `.catch()` handler and `armRunningRecheckTimer`'s `.catch()` handler.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/cron/service/timer.watchdog.test.ts
Line: 38-62
Comment:
**Test doesn't exercise the `.catch()` re-arm behavior**
This test only verifies that calling `armTimer` twice sets a new timer reference — it doesn't simulate a rejection path at all. It manually clears the timer and calls `armTimer` directly, bypassing the `.catch()` handler entirely. A meaningful test would mock `onTimer` to reject (or inject a failing dep) so that the `.catch()` branch is actually exercised and `state.timer` is confirmed to be re-set after the rejection.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(cron): add .catch() re-arm and watch..." | Re-trigger Greptile |
| void onTimer(state).catch((err) => { | ||
| state.deps.log.error({ err: String(err) }, "cron: timer tick failed"); | ||
| // Re-arm the timer even when onTimer rejects outside its own finally | ||
| // block. Without this, any unhandled rejection permanently breaks | ||
| // the setTimeout chain, silently killing the scheduler. | ||
| // See: https://github.com/openclaw/openclaw/issues/73166 | ||
| armTimer(state); | ||
| }); |
There was a problem hiding this comment.
armTimer call on rejection inside try block
When onTimer rejects from within its own try block, the finally clause at line 967 (state.running = false; armTimer(state)) still executes first — JS finally always runs on async rejection. The .catch() handler then calls armTimer a second time, leaking timer A (from finally) and replacing state.timer with timer B. Both subsequently fire; whichever fires second hits the state.running guard and calls armRunningRecheckTimer, cancelling and re-creating the tracked timer. Net effect: spurious extra timer ticks in the error path.
The .catch() re-arm only uniquely adds value for the narrow edge case where code before the try block (specifically armRunningRecheckTimer on line 819) throws synchronously. Consider guarding against that specific case, or checking !state.timer before re-arming in .catch() to avoid the double-arm:
void onTimer(state).catch((err) => {
state.deps.log.error({ err: String(err) }, "cron: timer tick failed");
if (!state.timer) {
armTimer(state);
}
});The same applies to the watchdog's .catch() handler and armRunningRecheckTimer's .catch() handler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 738-745
Comment:
**Double `armTimer` call on rejection inside `try` block**
When `onTimer` rejects from within its own `try` block, the `finally` clause at line 967 (`state.running = false; armTimer(state)`) still executes first — JS `finally` always runs on async rejection. The `.catch()` handler then calls `armTimer` a second time, leaking timer A (from `finally`) and replacing `state.timer` with timer B. Both subsequently fire; whichever fires second hits the `state.running` guard and calls `armRunningRecheckTimer`, cancelling and re-creating the tracked timer. Net effect: spurious extra timer ticks in the error path.
The `.catch()` re-arm only uniquely adds value for the narrow edge case where code before the `try` block (specifically `armRunningRecheckTimer` on line 819) throws synchronously. Consider guarding against that specific case, or checking `!state.timer` before re-arming in `.catch()` to avoid the double-arm:
```ts
void onTimer(state).catch((err) => {
state.deps.log.error({ err: String(err) }, "cron: timer tick failed");
if (!state.timer) {
armTimer(state);
}
});
```
The same applies to the watchdog's `.catch()` handler and `armRunningRecheckTimer`'s `.catch()` handler.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // Arm an initial timer | ||
| armTimer(state); | ||
| expect(state.timer).not.toBeNull(); | ||
|
|
||
| // Simulate a scenario where onTimer would fail: clear the timer | ||
| // reference and verify the .catch handler re-arms it. | ||
| // We test this indirectly by checking that after a timer fire + rejection, | ||
| // a new timer is set. | ||
| const originalTimer = state.timer; | ||
| clearTimeout(state.timer!); | ||
| state.timer = null; | ||
|
|
||
| // Directly call armTimer to verify it sets a timer | ||
| armTimer(state); | ||
| expect(state.timer).not.toBeNull(); | ||
| expect(state.timer).not.toBe(originalTimer); | ||
| }); | ||
| }); | ||
|
|
||
| describe("startCronWatchdog", () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
.catch() re-arm behavior
This test only verifies that calling armTimer twice sets a new timer reference — it doesn't simulate a rejection path at all. It manually clears the timer and calls armTimer directly, bypassing the .catch() handler entirely. A meaningful test would mock onTimer to reject (or inject a failing dep) so that the .catch() branch is actually exercised and state.timer is confirmed to be re-set after the rejection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/timer.watchdog.test.ts
Line: 38-62
Comment:
**Test doesn't exercise the `.catch()` re-arm behavior**
This test only verifies that calling `armTimer` twice sets a new timer reference — it doesn't simulate a rejection path at all. It manually clears the timer and calls `armTimer` directly, bypassing the `.catch()` handler entirely. A meaningful test would mock `onTimer` to reject (or inject a failing dep) so that the `.catch()` branch is actually exercised and `state.timer` is confirmed to be re-set after the rejection.
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 12:57 AM ET / 04:57 UTC. Summary PR surface: Source +68, Tests +188. Total +256 across 4 files. Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against b9933b2ec119. Label changesLabel justifications:
Evidence reviewedPR surface: Source +68, Tests +188. Total +256 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
does the |
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
When the cron
setTimeoutchain breaks at runtime, the scheduler silently stops firing and never recovers until a gateway restart. This was observed after ~2.5 days of continuous gateway uptime on macOS (Apple Silicon) with no errors logged.Root cause: The
.catch()handler inarmTimer()andarmRunningRecheckTimer()logs the error but does NOT callarmTimer(state), so ifonTimer()ever rejects without reaching itsfinallyblock, the timer chain is permanently broken.Contributing factor: On macOS, background processes (like the gateway running as a launchd agent) are subject to App Nap and timer coalescing. While
MAX_TIMER_DELAY_MS = 60sshould be short enough for the clampedsetTimeoutto survive, the OS can still defer callbacks in edge cases, and if the chain breaks once, there is no recovery mechanism.Changes
1. Re-arm in
.catch()handler (timer.ts)Both
armTimer()andarmRunningRecheckTimer()now callarmTimer(state)inside.catch(), ensuring the timer chain is never permanently broken:2. Independent watchdog
setInterval(timer.ts)Added
startCronWatchdog()— asetInterval-based watchdog (every 5 minutes) that detects whennextWakeAtMsis past-due by more thanMAX_TIMER_DELAY_MSand force-triggersonTimer+ re-arms the chain.setIntervalis more resilient to OS-level timer deferral than chainedsetTimeoutbecause libuv treats it as a persistent/repeating timer.3. Lifecycle integration (
ops.ts,state.ts)start()and stopped instop()_stopWatchdogfield toCronServiceState4. Tests (
timer.watchdog.test.ts)cronEnabled: falseRelationship to other PRs
start()->runMissedJobs()throwing kills the timer. That is a startup-time issue.The two fixes are complementary and non-overlapping.
Fixes #73166