fix(cron): honor heartbeat active hours when the scheduler fires - #1208
fix(cron): honor heartbeat active hours when the scheduler fires#1208Lstarsky0 wants to merge 2 commits into
Conversation
`heartbeat.active_hours` had no effect. `is_within_active_hours` exists and
is documented, but nothing in the crate called it — the heartbeat is a plain
`Every { every_ms }` cron job, and `process_due_jobs` runs whatever is due
without consulting the window. So a server configured for 07:00–23:00 kept
taking heartbeat turns through the night.
`CronService` now carries the window alongside the wake cooldown it already
holds, and `process_due_jobs` rolls the heartbeat's schedule forward instead
of running it when the current time falls outside. Rolling forward rather
than leaving it due matters: a skipped job that stays due would be retried
on every tick. Explicit `wake()` calls are not gated, since those respond to
something that already happened.
Callers that pass no window are unaffected.
Closes moltis-org#1205
Greptile SummaryThe PR gates scheduled heartbeat jobs using the configured active-hours window while leaving explicit wake requests unaffected.
Confidence Score: 4/5The PR is not yet safe to merge because runtime heartbeat configuration updates still leave scheduled execution enforcing the old active-hours window until restart.
Files Needing Attention: crates/gateway/src/server/prepare_core.rs, crates/gateway/src/methods/services/core.rs, crates/cron/src/service.rs
|
| Filename | Overview |
|---|---|
| crates/cron/src/heartbeat.rs | Adds clock-independent tests for full-day and empty active-hours windows. |
| crates/cron/src/service.rs | Adds scheduled-heartbeat gating and advances skipped jobs to their next interval. |
| crates/cron/src/service/tests.rs | Adds service-level tests covering skipped, permitted, and unconfigured heartbeat execution. |
| crates/gateway/src/server/prepare_core.rs | Passes a startup snapshot of active hours into CronService, while the previously reported runtime-update synchronization issue remains. |
Reviews (2): Last reviewed commit: "fix(cron): honor heartbeat active hours ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rate_limit_config, | ||
| wake_cooldown_ms, | ||
| events_queue, | ||
| Some(config.heartbeat.active_hours.clone()), |
There was a problem hiding this comment.
When a user changes heartbeat.active_hours through heartbeat.update, the handler persists and reports the new setting but scheduled execution continues reading the value cloned into CronService at startup, causing heartbeats to run during newly excluded hours or remain suppressed during newly allowed hours until restart.
Knowledge Base Used: Onboarding and Scheduled Agent Runs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be88510916
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rate_limit_config, | ||
| wake_cooldown_ms, | ||
| events_queue, | ||
| Some(config.heartbeat.active_hours.clone()), |
There was a problem hiding this comment.
This passes a one-time clone into CronService, but the existing heartbeat.update RPC only replaces state.inner.heartbeat_config and patches the cron job; it never updates this new service field. Consequently, changing the start, end, or timezone through the web UI/API appears to save successfully but the scheduler continues using the startup window until the gateway is restarted. Store the window in shared mutable state or expose an updater that heartbeat.update calls.
Useful? React with 👍 / 👎.
| && job.state.next_run_at_ms.is_some_and(|t| t <= now) | ||
| && job.state.running_at_ms.is_none() | ||
| { | ||
| if job.id == "__heartbeat__" && !heartbeat_may_run { |
There was a problem hiding this comment.
When an explicit wake() occurs outside active hours, it only sets the heartbeat's next_run_at_ms to now, after which the same process_due_jobs path reaches this condition and skips it. Thus exec-completion wakes from prepare_core/post_state.rs and CronWakeMode::Now wakes from execute_job are both suppressed despite the new heartbeat_may_run_now contract stating explicit wakes are not gated; the queued event waits until a later scheduled heartbeat instead of being handled immediately. Preserve the wake source so this gate applies only to periodic firings.
Useful? React with 👍 / 👎.
| job.state.next_run_at_ms = | ||
| compute_next_run(&job.schedule, now).unwrap_or(None); |
There was a problem hiding this comment.
For custom intervals whose phase never intersects the active window, recomputing only from now can suppress the heartbeat forever rather than letting it resume when the window opens. For example, a heartbeat due daily at 02:00 with active hours 08:00–24:00 is advanced to 02:00 the next day on every skip and never runs. Schedule the skipped heartbeat at the next window opening, or otherwise ensure its next due time falls within an active period.
Useful? React with 👍 / 👎.
|
On the startup-snapshot point: it is real, but it is what So the gap is inherited rather than introduced here. Making the window live means giving |
|
@greptileai review |
The gate added in this PR drops two kinds of run it should not. An explicit wake is lost, not deferred. `wake()` does not execute the heartbeat; it sets `next_run_at_ms = Some(now)` and notifies the timer, so the run happens in `process_due_jobs` -- which is where the gate is. Outside the window the gate overwrote that due time, so the wake was gone rather than delayed. Both paths look identical by then, both being a due `next_run_at_ms`, so the difference now travels beside them: a flag `wake()` raises and the run it asks for lowers. The doc comment claiming wakes were never gated described an intention, not the code. A heartbeat on a whole-day interval never runs again. Rolling the schedule forward by its own interval calls `compute_next_run`, which with `anchor_ms: None` returns `now + every_ms`, so `heartbeat.every = "24h"` lands on the same excluded wall-clock time every day forever. The skip now aims at the next opening of the window instead. Leaving `next_run_at_ms` untouched so the job simply stays due would be the smaller change and is wrong: `ms_until_next_wake` computes the sleep with `saturating_sub`, so a due job yields a zero-length sleep and the timer loop spins for as long as the window is shut. `ms_until_active_hours` is therefore documented to never return zero. Behaviour of `is_within_active_hours` itself is deliberately unchanged here, including its handling of an unparseable bound.
|
Pushed fixes for the two open bot comments. Checked both against the code first — one is worse than described, the other can't happen the way it's described but does happen another way. Explicit wakes. Real, and not only suppression. Skipped heartbeats never resuming. The daily-cron example can't occur — the heartbeat job is created and updated as Worth flagging for whoever reviews: the smaller-looking fix, leaving On the startup snapshot — it's inherited from Unrelated to the review, but found while checking it: the default active-hours window has no effect at all — #1223. Doesn't block this, but it does mean the feature is a no-op on a default config until that's settled. |
2 participants
Closes #1205.
Summary
heartbeat.active_hoursnever had any effect.is_within_active_hoursis written, documented and tested incrates/cron/src/heartbeat.rs, but nothing in the crate calls it — the heartbeat is a plainEvery { every_ms }cron job, andprocess_due_jobsruns whatever is due without consulting the window. A server configured for 07:00–23:00 kept taking heartbeat turns through the night, which is what @IlyaBizyaev observed.CronServicenow carries the window next to the wake cooldown it already holds, threaded fromconfig.heartbeatinprepare_core.rsthe same way. When the heartbeat comes up due outside the window,process_due_jobsrolls its schedule forward instead of running it. Rolling forward rather than leaving it due matters — a skipped job that stayed due would be retried on every tick.Explicit
wake()calls are not gated. Those respond to something that already happened, and the cooldown there is a separate concern.User-facing impact worth calling out.
ActiveHoursConfig::default()is08:00–24:00, so anyone who never configured[heartbeat.active_hours]has effectively been running 24/7 and will now stop between 00:00 and 08:00. That is the default the config template ships and the reference documents ("heartbeats only run during this window"), so this brings the code in line with the docs rather than changing the contract — but it is a behaviour change on upgrade for those users.start = "00:00"withend = "24:00"covers every minute if someone wants the old behaviour back.The three existing
is_within_active_hourstests say "We can't assert exact behavior without controlling time", which is why this went unnoticed — two of them discard the result and only check for the absence of a panic. Two windows are in fact clock-independent: an equal start and end leaves no minute innow >= start && now < end, and00:00–24:00covers all of them. The new tests use those, so nothing here depends on when CI runs.Validation
Completed
cargo test -p moltis-cron --lib— 140 passed, including 5 new testscargo test -p moltis-gateway --lib— 640 passedcargo check --workspace --lib --testscargo fmt --all -- --checkjust lintTwo closed-loop rounds on identical file hashes. Disabling the gate alone fails only
heartbeat_outside_active_hours_is_skipped_and_rescheduled; the in-window and no-config-window tests keep passing, so the new test is specific to the fix rather than to the surrounding scaffolding.The service-level tests call
process_due_jobsdirectly and assert on state set synchronously under the write lock, so they need no timer and no sleep.Remaining
./scripts/local-validate.sh <PR>Manual QA
Not exercised against a running server. The observable change is that a heartbeat coming up due outside the window logs
skipping heartbeat — outside active hoursat debug level and itsnext_run_at_msadvances by one interval instead of a turn being taken;__heartbeat__run history should show no entries inside the excluded hours.