feat(cron): add strict wall-clock schedule support - #247
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Comment on lines
+602
to
+614
| let trimmed = expr.trim(); | ||
| if trimmed.is_empty() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| Schedule::from_str(trimmed).map_err(|error| { | ||
| crate::error::Error::Other(anyhow::anyhow!( | ||
| "invalid cron expression '{trimmed}': {error}" | ||
| )) | ||
| })?; | ||
|
|
||
| Ok(Some(trimmed.to_string())) | ||
| } |
Contributor
There was a problem hiding this comment.
If we intend to support only 5-field cron syntax (as docs/comments say), it might be worth enforcing that explicitly here so we don’t accidentally accept 6/7-field (seconds/year) expressions.
Suggested change
| let trimmed = expr.trim(); | |
| if trimmed.is_empty() { | |
| return Ok(None); | |
| } | |
| Schedule::from_str(trimmed).map_err(|error| { | |
| crate::error::Error::Other(anyhow::anyhow!( | |
| "invalid cron expression '{trimmed}': {error}" | |
| )) | |
| })?; | |
| Ok(Some(trimmed.to_string())) | |
| } | |
| let trimmed = expr.trim(); | |
| if trimmed.is_empty() { | |
| return Ok(None); | |
| } | |
| let field_count = trimmed.split_whitespace().count(); | |
| if field_count != 5 { | |
| return Err(crate::error::Error::Other(anyhow::anyhow!( | |
| "cron expression must have exactly 5 fields (got {field_count}): '{trimmed}'" | |
| ))); | |
| } | |
| Schedule::from_str(trimmed).map_err(|error| { | |
| crate::error::Error::Other(anyhow::anyhow!( | |
| "invalid cron expression '{trimmed}': {error}" | |
| )) | |
| })?; | |
| Ok(Some(trimmed.to_string())) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/config.rs`:
- Around line 2141-2142: When loading/parsing the TOML into the struct that
contains the fields cron_expr and interval_secs, normalize cron_expr by trimming
whitespace and converting empty or all-whitespace strings to None so you never
carry an Option<String> with an empty schedule into runtime; update the
TOML-to-config conversion path (e.g. the Config::from_toml / deserialize logic
that populates cron_expr) to perform s = s.trim(); if s.is_empty() set cron_expr
= None otherwise set Some(s.to_string()); apply the same normalization where
similar schedule parsing occurs (the other occurrence around the block
referenced at lines ~3754-3758).
In `@src/cron/store.rs`:
- Line 77: The row decoding currently swallows errors by calling
row.try_get::<Option<String>, _>("cron_expr").ok().flatten() (and the same
pattern used elsewhere), which masks DB decode/schema issues; change the mapping
closure to return a Result and propagate errors instead of returning None—either
call row.try_get::<Option<String>, _>("cron_expr")? to let the error bubble up,
or extract into a helper like fn get_optional_string(row, col) ->
Result<Option<String>, E> and use that; if mapping multiple rows, use the
collect::<Result<Vec<_>, _>>() pattern so any decode error is returned to the
caller rather than silently discarded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (11)
README.mddocs/content/docs/(configuration)/config.mdxdocs/content/docs/(features)/cron.mdxmigrations/20260226000001_cron_expression.sqlprompts/en/tools/cron_description.md.j2src/api/cron.rssrc/config.rssrc/cron/scheduler.rssrc/cron/store.rssrc/main.rssrc/tools/cron.rs
Comment on lines
+2141
to
2142
| cron_expr: Option<String>, | ||
| interval_secs: Option<u64>, |
Contributor
There was a problem hiding this comment.
Normalize cron_expr when loading TOML to reject empty schedules.
Right now an empty/whitespace cron_expr is passed through unchanged. Normalizing here avoids carrying an invalid “set” schedule into runtime behavior.
🧹 Suggested fix
- cron_expr: h.cron_expr,
+ cron_expr: h
+ .cron_expr
+ .map(|expr| expr.trim().to_string())
+ .filter(|expr| !expr.is_empty()),Also applies to: 3754-3758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 2141 - 2142, When loading/parsing the TOML into
the struct that contains the fields cron_expr and interval_secs, normalize
cron_expr by trimming whitespace and converting empty or all-whitespace strings
to None so you never carry an Option<String> with an empty schedule into
runtime; update the TOML-to-config conversion path (e.g. the Config::from_toml /
deserialize logic that populates cron_expr) to perform s = s.trim(); if
s.is_empty() set cron_expr = None otherwise set Some(s.to_string()); apply the
same normalization where similar schedule parsing occurs (the other occurrence
around the block referenced at lines ~3754-3758).
| .map(|row| CronConfig { | ||
| id: row.try_get("id").unwrap_or_default(), | ||
| prompt: row.try_get("prompt").unwrap_or_default(), | ||
| cron_expr: row.try_get::<Option<String>, _>("cron_expr").ok().flatten(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the structure and size of the file
wc -l src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 87
🏁 Script executed:
# View the specific lines mentioned in the review (77, 168) with context
sed -n '70,85p' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 949
🏁 Script executed:
# View line 168 with context
sed -n '160,175p' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 929
🏁 Script executed:
# Search for all .ok().flatten() patterns in the file
rg -n '\.ok\(\)\.flatten\(\)' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 250
🏁 Script executed:
# Also check for the specific pattern mentioned in the review
rg -n 'cron_expr:\s*row\.try_get' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 250
🏁 Script executed:
# Get the full context of the first occurrence (lines 70-90)
sed -n '70,90p' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 1180
🏁 Script executed:
# Get full context of second occurrence (lines 160-180)
sed -n '160,180p' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 1161
🏁 Script executed:
# Check for other .ok() uses on try_get to see if pattern is consistent
rg -n '\.try_get.*\.ok\(\)' src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 829
🏁 Script executed:
# Check the imports to verify error handling dependencies available
head -30 src/cron/store.rsRepository: spacedriveapp/spacebot
Length of output: 1010
Remove silent error discarding on cron_expr row decoding.
Lines 77 and 168 use .ok().flatten() to convert try_get failures into None, which silently discards database decoding errors and masks data/schema issues.
- cron_expr: row.try_get::<Option<String>, _>("cron_expr").ok().flatten(),
+ cron_expr: row.try_get::<Option<String>, _>("cron_expr")?,Consider using a separate helper function or the collect::<Result<Vec<_>>>() pattern to properly propagate errors from the row mapping closure instead of silently dropping them.
Per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them."
Also applies to line 168.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/store.rs` at line 77, The row decoding currently swallows errors by
calling row.try_get::<Option<String>, _>("cron_expr").ok().flatten() (and the
same pattern used elsewhere), which masks DB decode/schema issues; change the
mapping closure to return a Result and propagate errors instead of returning
None—either call row.try_get::<Option<String>, _>("cron_expr")? to let the error
bubble up, or extract into a helper like fn get_optional_string(row, col) ->
Result<Option<String>, E> and use that; if mapping multiple rows, use the
collect::<Result<Vec<_>, _>>() pattern so any decode error is returned to the
caller rather than silently discarded.
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cron/scheduler.rs (1)
597-613:⚠️ Potential issue | 🟠 MajorEnforce exactly 5 fields for
cron_exprduring normalization.
normalize_cron_exprcurrently checks “parseable” only. IfSchedule::from_straccepts 6/7-field variants, this can violate the documented 5-field contract and change runtime behavior.Proposed fix
fn normalize_cron_expr(cron_expr: Option<String>) -> Result<Option<String>> { let Some(expr) = cron_expr else { return Ok(None); }; let trimmed = expr.trim(); if trimmed.is_empty() { return Ok(None); } + + let field_count = trimmed.split_whitespace().count(); + if field_count != 5 { + return Err(crate::error::Error::Other(anyhow::anyhow!( + "cron expression must have exactly 5 fields (got {field_count}): '{trimmed}'" + ))); + } Schedule::from_str(trimmed).map_err(|error| { crate::error::Error::Other(anyhow::anyhow!( "invalid cron expression '{trimmed}': {error}" )) })?; Ok(Some(trimmed.to_string())) }For cron crate version 0.12.0, what cron field counts does `Schedule::from_str` accept (5 vs 6/7 fields), and does it permit seconds/year fields?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 597 - 613, normalize_cron_expr currently only validates parsability via Schedule::from_str but must enforce the documented 5-field contract; update normalize_cron_expr to count whitespace-separated fields from the trimmed expr and return an Err(crate::error::Error::Other(...)) if the count is not exactly 5 (use a clear message like "cron expression must have exactly 5 fields: found N"). Keep the existing empty/None handling and still call Schedule::from_str for semantic validation, and reference the function name normalize_cron_expr and the Schedule::from_str call when making the change.
🧹 Nitpick comments (1)
src/cron/scheduler.rs (1)
202-203: Rename abbreviated localjvariables for guideline compliance.Please replace
jwith explicit names likejobs_guard/job_entryin these blocks for readability and consistency.As per coding guidelines, "
**/*.rs: Don't abbreviate variable names. Usequeuenotq,messagenotmsg,channelnotch."Also applies to: 257-259, 306-307, 319-320, 335-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 202 - 203, The local read guard variable currently named `j` is too abbreviated; rename it to a descriptive name such as `jobs_guard` (or `jobs_lock`) where you call `jobs.read().await` and update the subsequent `match j.get(&job_id)` to `match jobs_guard.get(&job_id)`; likewise rename any short `j` occurrences in the other similar blocks (e.g., the ones surrounding `job_id`, `jobs.read().await`, and `match ... .get(...)` at the other spots) and rename short variables used for map entries to `job_entry` for clarity, keeping all uses consistent so the code compiles.
🤖 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/cron/scheduler.rs`:
- Around line 616-623: The function interval_initial_delay should guard against
interval_secs == 0 to avoid a modulo-by-zero panic: update the condition that
currently reads `if interval_secs < 86400 && 86400 % interval_secs == 0` to
first check `interval_secs != 0` (e.g. `if interval_secs != 0 && interval_secs <
86400 && 86400 % interval_secs == 0`) or add an early return when interval_secs
== 0 (e.g. return Duration::from_secs(0)); adjust the control flow in the
function (referencing interval_initial_delay and the variable interval_secs) so
no modulo is executed with zero and a sensible Duration::from_secs(0) is
returned for the zero case.
---
Duplicate comments:
In `@src/cron/scheduler.rs`:
- Around line 597-613: normalize_cron_expr currently only validates parsability
via Schedule::from_str but must enforce the documented 5-field contract; update
normalize_cron_expr to count whitespace-separated fields from the trimmed expr
and return an Err(crate::error::Error::Other(...)) if the count is not exactly 5
(use a clear message like "cron expression must have exactly 5 fields: found
N"). Keep the existing empty/None handling and still call Schedule::from_str for
semantic validation, and reference the function name normalize_cron_expr and the
Schedule::from_str call when making the change.
---
Nitpick comments:
In `@src/cron/scheduler.rs`:
- Around line 202-203: The local read guard variable currently named `j` is too
abbreviated; rename it to a descriptive name such as `jobs_guard` (or
`jobs_lock`) where you call `jobs.read().await` and update the subsequent `match
j.get(&job_id)` to `match jobs_guard.get(&job_id)`; likewise rename any short
`j` occurrences in the other similar blocks (e.g., the ones surrounding
`job_id`, `jobs.read().await`, and `match ... .get(...)` at the other spots) and
rename short variables used for map entries to `job_entry` for clarity, keeping
all uses consistent so the code compiles.
Comment on lines
+616
to
+623
| fn interval_initial_delay(interval_secs: u64) -> Duration { | ||
| if interval_secs < 86400 && 86400 % interval_secs == 0 { | ||
| let now_unix = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_secs(); | ||
| let remainder = now_unix % interval_secs; | ||
| let secs_until = if remainder == 0 { |
Contributor
There was a problem hiding this comment.
Guard interval_secs == 0 to avoid modulo-by-zero panic.
At Line 617, 86400 % interval_secs will panic when interval_secs is 0.
Proposed fix
fn interval_initial_delay(interval_secs: u64) -> Duration {
+ if interval_secs == 0 {
+ return Duration::from_secs(1);
+ }
if interval_secs < 86400 && 86400 % interval_secs == 0 {
let now_unix = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.unwrap_or_default()
.as_secs();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn interval_initial_delay(interval_secs: u64) -> Duration { | |
| if interval_secs < 86400 && 86400 % interval_secs == 0 { | |
| let now_unix = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| let remainder = now_unix % interval_secs; | |
| let secs_until = if remainder == 0 { | |
| fn interval_initial_delay(interval_secs: u64) -> Duration { | |
| if interval_secs == 0 { | |
| return Duration::from_secs(1); | |
| } | |
| if interval_secs < 86400 && 86400 % interval_secs == 0 { | |
| let now_unix = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap_or_default() | |
| .as_secs(); | |
| let remainder = now_unix % interval_secs; | |
| let secs_until = if remainder == 0 { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/scheduler.rs` around lines 616 - 623, The function
interval_initial_delay should guard against interval_secs == 0 to avoid a
modulo-by-zero panic: update the condition that currently reads `if
interval_secs < 86400 && 86400 % interval_secs == 0` to first check
`interval_secs != 0` (e.g. `if interval_secs != 0 && interval_secs < 86400 &&
86400 % interval_secs == 0`) or add an early return when interval_secs == 0
(e.g. return Duration::from_secs(0)); adjust the control flow in the function
(referencing interval_initial_delay and the variable interval_secs) so no modulo
is executed with zero and a sensible Duration::from_secs(0) is returned for the
zero case.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cron/scheduler.rs (1)
623-625:⚠️ Potential issue | 🔴 CriticalGuard zero interval before modulo to avoid timer-task panic.
Line 624 performs
86400 % interval_secs; ifinterval_secs == 0, this panics and terminates the scheduler task for that job.🐛 Proposed fix
fn interval_initial_delay(interval_secs: u64) -> Duration { + if interval_secs == 0 { + return Duration::from_secs(1); + } if interval_secs < 86400 && 86400 % interval_secs == 0 { let now_unix = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .as_secs();#!/bin/bash set -euo pipefail # 1) Confirm modulo-by-zero risk is present in interval_initial_delay. rg -n -C3 'fn interval_initial_delay|86400 % interval_secs|interval_secs == 0' src/cron/scheduler.rs # 2) Inspect ingestion/validation paths for interval_secs to verify whether zero can still enter # via non-API sources (config/store/startup restore). rg -n -C3 'interval_secs|MIN_CRON_INTERVAL_SECS|default_interval|cron_expr' \ src/api/cron.rs src/tools/cron.rs src/cron/scheduler.rs src/config.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 623 - 625, The function interval_initial_delay must guard against interval_secs == 0 before performing 86400 % interval_secs to avoid a panic; update interval_initial_delay to check interval_secs == 0 first (in the branch that currently checks interval_secs < 86400) and return an appropriate Duration (e.g., zero delay or the intended default) or skip the modulo logic when zero, so that the modulo operation is never executed with a zero divisor; locate and update the interval_initial_delay function to implement this early check and ensure any callers expecting a non-zero delay still behave correctly.
🧹 Nitpick comments (1)
src/cron/scheduler.rs (1)
200-214: Use descriptive guard variable names in the timer loop.Line 202 uses
jfor the jobs map guard; rename to something explicit (e.g.,jobs_guard) for readability and consistency.As per coding guidelines: Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 200 - 214, Rename the short guard variable `j` inside the timer loop to a descriptive name (e.g., `jobs_guard`) where the code does `let j = jobs.read().await;` and update its uses in the match that checks `j.get(&job_id)` and the pattern `Some(j)` to avoid abbreviation; ensure you also update the cloned variable `j.clone()` to use a clear name (e.g., `job_entry`) so the block using `jobs.read().await`, the `match` arms, and the `tracing::debug!` calls reference the new descriptive identifiers (`jobs_guard`, `job_entry`, etc.) consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cron/scheduler.rs`:
- Around line 623-625: The function interval_initial_delay must guard against
interval_secs == 0 before performing 86400 % interval_secs to avoid a panic;
update interval_initial_delay to check interval_secs == 0 first (in the branch
that currently checks interval_secs < 86400) and return an appropriate Duration
(e.g., zero delay or the intended default) or skip the modulo logic when zero,
so that the modulo operation is never executed with a zero divisor; locate and
update the interval_initial_delay function to implement this early check and
ensure any callers expecting a non-zero delay still behave correctly.
---
Nitpick comments:
In `@src/cron/scheduler.rs`:
- Around line 200-214: Rename the short guard variable `j` inside the timer loop
to a descriptive name (e.g., `jobs_guard`) where the code does `let j =
jobs.read().await;` and update its uses in the match that checks
`j.get(&job_id)` and the pattern `Some(j)` to avoid abbreviation; ensure you
also update the cloned variable `j.clone()` to use a clear name (e.g.,
`job_entry`) so the block using `jobs.read().await`, the `match` arms, and the
`tracing::debug!` calls reference the new descriptive identifiers (`jobs_guard`,
`job_entry`, etc.) consistently.
rktmeister
pushed a commit
to rktmeister/spacebot
that referenced
this pull request
Mar 11, 2026
…l-clock-schedules feat(cron): add strict wall-clock schedule support
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cron_exprsupport across config, cron tool, API, scheduler, and storage so cron jobs can run on strict wall-clock schedulescron_expris set, while preservinginterval_secsas a backward-compatible legacy pathcron_jobs.cron_exprand update README/config/cron docs to document strict scheduling and migration-safe compatibilityTesting
cargo checkcargo fmt --allcargo test --lib cron::scheduler -- --nocaptureNote
This PR introduces first-class cron expression support across the entire cron job stack. The
croncrate (v0.12) dependency is added to handle standard 5-field cron syntax parsing. New jobs can specify exact wall-clock times viacron_expr(e.g.,0 9 * * *for daily at 9am), while existing interval-based jobs remain fully supported. The scheduler intelligently routes between cron-expression logic (computing next fire in the configured timezone) and interval-based logic (clock-aligned for sub-daily intervals). A non-breaking migration adds thecron_exprcolumn to thecron_jobstable. All changes flow through config, tools, API, storage layer, and documentation.Written by Tembo for commit 57f44ce. This will update automatically on new commits.