Fix:Issue 921 cron improvements#975
Conversation
- Standardize cron jobs to use device-local timezone by default - Add ActiveHours support for cron schedules (start/end HH:MM) - Update CronAddTool with improved descriptions and schema - Add comprehensive tests for scheduling logic and timezone behavior Closes tinyhumansai#921 Co-authored-by: YellowSnnowmann <167776381+YellowSnnowmann@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds optional ActiveHours to cron schedules, threads it through types, storage, validation, next-run computation (timezone-aware, bounded search), RPC/tooling schemas, and tests; preserves/persists active_hours across updates and seeds. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Tool as "cron_add Tool\n(rpc/API)"
participant Validator as "validate_schedule"
participant Store as "Job Store"
participant Scheduler as "Scheduler Loop"
participant Calculator as "next_run_for_schedule"
Client->>Tool: Submit cron schedule (+ active_hours?)
Tool->>Validator: validate_schedule(expr, tz?, active_hours?)
Validator-->>Tool: validation result
Tool->>Store: persist job (Schedule::Cron incl. active_hours)
Store-->>Tool: job id
loop Poll
Scheduler->>Store: fetch due jobs
Store-->>Scheduler: due job(s)
Scheduler->>Calculator: next_run_for_schedule(from, schedule)
loop until matching active_hours (max 100k)
Calculator->>Calculator: compute next cron candidate
Calculator->>Calculator: convert candidate to local time (tz or device local)
Calculator->>Calculator: check active_hours window
end
Calculator-->>Scheduler: next valid run
Scheduler->>Store: update next_run / record run
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/openhuman/cron/types.rs (1)
67-68: Use camelCase field naming at the Rust type boundary.
active_hoursintroduces a new snake_case Rust field. If you need to keep JSON compatibility, rename the Rust field to camelCase and keep the wire key with serde rename.As per coding guidelines: "`src/openhuman/**/*.rs`: Use camelCase for variable names..."♻️ Suggested adjustment
Cron { expr: String, #[serde(default)] tz: Option<String>, - #[serde(default)] - active_hours: Option<ActiveHours>, + #[serde(default, rename = "active_hours")] + activeHours: Option<ActiveHours>, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/cron/types.rs` around lines 67 - 68, The struct field currently defined as active_hours: Option<ActiveHours> uses snake_case; rename the Rust field to camelCase (activeHours: Option<ActiveHours>) and keep JSON compatibility by adding a serde attribute mapping the wire key, e.g. #[serde(rename = "active_hours", default)] (or combine with existing attributes) so deserialization still accepts "active_hours" while the Rust API uses camelCase; update any code references to use activeHours and ensure serde import/attributes remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/cron/schedule.rs`:
- Around line 253-267: The test next_run_cron_without_tz_uses_local_by_default
is host-timezone dependent; change the expected calculation to derive the local
date from the fixed `from` instant (use `from.with_timezone(&chrono::Local)`),
then build the expected local run at 09:00 on that local date or the next day if
the local `from` time is already >= 09:00, and finally compare `next` to that
expected converted to Utc; update the test to use
`from.with_timezone(&Local).date()` (and add one day when needed) instead of
hardcoding 2026-02-16 so it works on any host timezone and still verifies
`next_run_for_schedule` for `Schedule::Cron { expr: "0 9 * * *", tz: None, ...
}`.
In `@src/openhuman/cron/seed.rs`:
- Around line 112-116: The doc comment that describes Schedule::Cron using expr
"0 7 * * *" still says "7:00 AM UTC" while tz: None now means device-local
timezone; update the function/doc comment text to indicate the schedule runs at
7:00 AM in the device-local timezone when tz is None (e.g., "7:00 AM
device-local timezone" or "7:00 AM (device-local)"), and optionally mention that
providing tz overrides this behavior; refer to Schedule::Cron, the expr "0 7 * *
*", and the tz: None field when making the change.
In `@src/openhuman/tools/impl/cron/add.rs`:
- Around line 35-54: The schema for "schedule" must be a discriminated union so
each "kind" requires the correct fields; replace the current single object with
a oneOf (or anyOf) array of three schemas discriminated by "kind" (or use JSON
Schema discriminator) where the "cron" variant requires "expr" (and optionally
"tz" and "active_hours"), the "at" variant requires "at", and the "every"
variant requires "every_ms"; update the "schedule" definition (the object that
currently has "properties" kind/expr/tz/active_hours/at/every_ms and
"required":["kind"]) to instead list three separate schemas each with
"properties":{"kind":{"const":"cron"}} / {"kind":{"const":"at"}} /
{"kind":{"const":"every"}} and the appropriate "required" arrays
(["kind","expr"], ["kind","at"], ["kind","every_ms"]) so schema validation
enforces the correct fields per kind.
---
Nitpick comments:
In `@src/openhuman/cron/types.rs`:
- Around line 67-68: The struct field currently defined as active_hours:
Option<ActiveHours> uses snake_case; rename the Rust field to camelCase
(activeHours: Option<ActiveHours>) and keep JSON compatibility by adding a serde
attribute mapping the wire key, e.g. #[serde(rename = "active_hours", default)]
(or combine with existing attributes) so deserialization still accepts
"active_hours" while the Rust API uses camelCase; update any code references to
use activeHours and ensure serde import/attributes remain.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae2b731f-8043-460d-81cb-6397a6d16797
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/openhuman/cron/mod.rssrc/openhuman/cron/ops.rssrc/openhuman/cron/ops_tests.rssrc/openhuman/cron/schedule.rssrc/openhuman/cron/scheduler_tests.rssrc/openhuman/cron/seed.rssrc/openhuman/cron/store.rssrc/openhuman/cron/store_tests.rssrc/openhuman/cron/types.rssrc/openhuman/tools/impl/cron/add.rssrc/openhuman/tools/impl/system/schedule.rs
- Revise schedule property to support three job types: repeating cron, one-shot at, and fixed interval every. - Improve descriptions and requirements for each job type, ensuring clarity on timezone and active hours. - Standardize schema to enforce required fields and disallow additional properties for better validation.
- Update the test to use local time for the `from` variable to ensure the expected next run time is consistent across different host timezones. - This change prevents machine-dependent results by expressing the time in local context before converting to UTC.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Solid work overall. The local-timezone default addresses real user pain — missed runs because 0 9 * * * was being interpreted in UTC on a Pacific-time machine. The active_hours feature is well-designed: midnight-spanning windows work correctly, the per-timezone scoping is right, the #[serde(default)] on both new fields means existing persisted rows deserialize cleanly without a schema migration, and the 100k-iteration ceiling is a sensible safety valve. Test coverage is thorough across store, ops, scheduler, and the tool layer.
Three things worth fixing before merge, and a couple of minor polish items below.
| match self { | ||
| Self::Named(timezone) => { | ||
| let localized = time.with_timezone(&timezone); | ||
| NaiveTime::from_hms_opt(localized.hour(), localized.minute(), 0).unwrap() |
There was a problem hiding this comment.
[major] Bare .unwrap() in production code.
NaiveTime::from_hms_opt(localized.hour(), localized.minute(), 0) will always succeed in practice (hour/minute from a valid DateTime are always in-range), but the project rules flag bare .unwrap() in production paths. Same thing on line 133.
Swap both to .expect("hour() and minute() from a valid DateTime are always in-range") so a hypothetical panic gives context instead of just called unwrap() on a None value.
| .next() | ||
| .ok_or_else(|| anyhow::anyhow!("No future occurrence for expression: {expr}")) | ||
| let mut current_from = from; | ||
| for _ in 0..100_000 { |
There was a problem hiding this comment.
[major] No debug logging on the active-hours filtering loop.
The existing cron scheduler uses [cron] prefixed logging throughout scheduler.rs. This new loop silently skips candidates that fall outside the active window — when a job stops firing unexpectedly, there is zero trace output to explain why.
Suggestion: add a tracing::debug!("[cron] next_run candidate {} outside active window {:?}–{:?}, advancing", next_utc, active.start, active.end) for skipped candidates, and a tracing::warn! before the 100k bail. This is exactly the kind of flow the CLAUDE.md debug logging rule is designed for.
| Some(Schedule::Cron { | ||
| expr: expression.unwrap_or(existing_expr), | ||
| tz: tz.or(existing_tz), | ||
| active_hours: existing_active, |
There was a problem hiding this comment.
[major] update_cron_job correctly preserves active_hours when updating expr or tz, but there's no parameter to explicitly set or clear active_hours through this function. The RPC cron.update path handles it via CronJobPatch, so there's no data loss — but the gap is invisible to anyone reading this function.
At minimum add a doc comment noting the intentional omission, e.g. // active_hours is preserved from the existing schedule; use CronJobPatch for direct edits.
There was a problem hiding this comment.
[major]
update_cron_jobcorrectly preservesactive_hourswhen updatingexprortz, but there's no parameter to explicitly set or clearactive_hoursthrough this function. The RPCcron.updatepath handles it viaCronJobPatch, so there's no data loss — but the gap is invisible to anyone reading this function.At minimum add a doc comment noting the intentional omission, e.g.
// active_hours is preserved from the existing schedule; use CronJobPatch for direct edits.
already resolved
| async fn scheduler_flow_runs_active_hours_job_and_reschedules_inside_window() { | ||
| let tmp = TempDir::new().unwrap(); | ||
| let config = test_config(&tmp).await; | ||
| let active_minute = Utc::now() + ChronoDuration::minutes(1); |
There was a problem hiding this comment.
[minor] Utc::now() + ChronoDuration::minutes(1) is a narrow timing window. If the test starts at MM:59.9, active_minute lands on the next hour (or even next day at 23:59), and the final assertion next_hm == active_hm can fail because next_run wraps differently.
Using +2 minutes gives more headroom, or better yet, anchor from to a fixed time and set active_hm accordingly (similar to the schedule.rs tests).
| let cron = CronExprSchedule::from_str(&normalized) | ||
| .with_context(|| format!("Invalid cron expression: {expr}"))?; | ||
| let timezone = ScheduleTimeZone::parse(tz.as_deref())?; | ||
| let active_window = active_hours.as_ref().map(ActiveWindow::parse).transpose()?; |
There was a problem hiding this comment.
[minor] ActiveWindow::parse re-parses two HH:MM strings on every call to next_run_for_schedule. Not a bug — validate_schedule guards against bad strings, and the parse cost is trivial compared to the cron evaluation — but it might surprise someone reading this hot path. A one-line comment like // Parsing is cheap; validated at job-creation time would preempt the question.
- Replace bare .unwrap() in local_time_of_day with .expect() (both Named and Local arms) so a hypothetical panic has context - Add tracing::debug! for each candidate skipped outside the active window and tracing::warn! before the 100k-iteration bail, matching the [cron] logging convention used throughout scheduler.rs - Add doc comment on update_cron_job noting that active_hours is intentionally preserved from the existing schedule; callers who need to set/clear it must use CronJobPatch via the RPC path - Widen active_hours scheduler test timing window from +1 to +2 minutes to avoid flakiness at the 59s mark - Add one-line comment explaining why ActiveWindow::parse re-parses on each next_run_for_schedule call (cheap, validated at job-creation)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/cron/schedule.rs (1)
113-150: Consider extracting common logic to reduce duplication.The
next_afterandlocal_time_of_daymethods have nearly identical implementations between theLocalandNamedvariants. While chrono'sLocalandchrono_tz::Tzdon't share a directly usable trait, you could use a macro or a helper closure to reduce the duplication.That said, this is minor—the current form is clear and only ~10 lines per method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/cron/schedule.rs` around lines 113 - 150, Both next_after and local_time_of_day duplicate the same branch logic for Self::Named(timezone) and Self::Local; extract a small helper (or closure) that maps the TimezoneVariant to a timezone-aware conversion and reuse it in both functions. For example, introduce a helper like a method on the enum (e.g., with_localized<F, R>(&self, dt: DateTime<Utc>, f: F) -> R) or a closure used by next_after (which calls cron.after(&localized_from)) and local_time_of_day (which computes localized.hour()/minute()), referencing next_after, local_time_of_day, Self::Named, Self::Local, CronExprSchedule, and NaiveTime::from_hms_opt to centralize the with_timezone logic and remove the duplicated match arms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openhuman/cron/schedule.rs`:
- Around line 113-150: Both next_after and local_time_of_day duplicate the same
branch logic for Self::Named(timezone) and Self::Local; extract a small helper
(or closure) that maps the TimezoneVariant to a timezone-aware conversion and
reuse it in both functions. For example, introduce a helper like a method on the
enum (e.g., with_localized<F, R>(&self, dt: DateTime<Utc>, f: F) -> R) or a
closure used by next_after (which calls cron.after(&localized_from)) and
local_time_of_day (which computes localized.hour()/minute()), referencing
next_after, local_time_of_day, Self::Named, Self::Local, CronExprSchedule, and
NaiveTime::from_hms_opt to centralize the with_timezone logic and remove the
duplicated match arms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83be3212-069d-48f7-9f0f-23a9a0adc9b6
📒 Files selected for processing (3)
src/openhuman/cron/ops.rssrc/openhuman/cron/schedule.rssrc/openhuman/cron/scheduler_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/cron/scheduler_tests.rs
- src/openhuman/cron/ops.rs
3 participants
Summary
tzis omitted.active_hourssupport for cron schedules, including windows that span midnight.CronAddToolschema/copy to describe timezone defaults, active hours, and scheduler polling behavior.Problem
Cron jobs without an explicit timezone defaulted to UTC, which was surprising for a personal desktop assistant. There was also no first-class way to say “only run during work hours,” forcing users to write awkward cron expressions or accept reminders outside the intended window.
The tool surface also did not make it clear that cron jobs are polled on an interval and missed runs are not caught up.
Solution
ActiveHoursand extendedSchedule::Cronwithactive_hours.next_run_for_scheduleto use device-local time whentzis not set.22:00-02:00.active_hourswhen updating cron expression/timezone through cron ops.tzandactive_hours.active_hourstoNone.Tests
cargo fmtcargo test cron::149 passedactive_hours.active_hours.Impact
tzwill now be interpreted in device-local time instead of UTC. This is an intentional behavior change aligned with the personal assistant use case.active_hoursuses the schedule timezone whentzis set, otherwise the device-local timezone.Related
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests