feat: wall-clock cron support in UI, anchor interval crons to last execution on restart - #361
Merged
Merged
Conversation
…ecution on restart Channels never used wall-clock cron expressions because the UI had no support for them and the LLM prompts were too soft about preferring cron_expr over interval_secs. Backend: - Add CronStore::last_execution_times() to batch-query last run timestamps - Scheduler::register_with_anchor() uses last execution time to compute the correct first-tick delay for interval crons after restart, preventing skipped or duplicate firings - Add anchored_initial_delay() that sleeps for the remainder of the interval or fires after 2s jitter when overdue - Add cron_expr to the overview API CronJobInfo struct and SQL query Frontend: - Rework create/edit modal with schedule mode toggle (Cron Expression / Interval), cron expression input with preset buttons, timeout field - Replace emoji action buttons with HugeIcons (Pause/Play/Flash/Pencil/Delete) - Display cron expressions in job cards and overview section - Add formatCronSchedule helper, improve formatDuration for clean units - Add cron_expr and timezone to API types, wire through CronListResponse Prompts: - Add Cron section to channel system prompt with explicit guidance to always use cron_expr and common patterns - Strengthen cron tool description: always use cron_expr, deprecate interval_secs in parameter docs
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds cron expression support and timezone metadata across API, UI, scheduler, and storage; makes cron creation accept cron_expr or interval, adds timeout handling; introduces anchor-aware scheduler using last-execution times; updates frontend cron UI and formatting; includes large live shell streaming feature and docs updates. 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}
![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
🧹 Nitpick comments (1)
src/cron/scheduler.rs (1)
652-678: Consider randomizing the jitter for overdue jobs.The anchored delay logic is sound. However, the fixed 2-second delay for overdue jobs means all overdue jobs fire simultaneously after a restart, which could cause a mini thundering-herd if many jobs are overdue.
A small randomization (e.g., 1-4 seconds) would spread the load:
💡 Optional: Randomize jitter for overdue jobs
+use rand::Rng; + fn anchored_initial_delay( interval_secs: u64, anchor: Option<chrono::DateTime<chrono::Utc>>, ) -> Duration { if let Some(last_run) = anchor { let now = chrono::Utc::now(); let elapsed = (now - last_run).num_seconds().max(0) as u64; if elapsed >= interval_secs { // Overdue — fire soon with a small jitter to avoid thundering herd - Duration::from_secs(2) + let jitter_secs = rand::thread_rng().gen_range(1..=4); + Duration::from_secs(jitter_secs) } else { Duration::from_secs(interval_secs - elapsed) } } else { interval_initial_delay(interval_secs) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 652 - 678, anchored_initial_delay currently returns a fixed 2s for overdue jobs which can still create a thundering herd; change the overdue branch in anchored_initial_delay to return a small randomized jitter (e.g., 1–4 seconds) instead of Duration::from_secs(2) by generating a random u64 in that range (use rand::thread_rng().gen_range(1..=4) or equivalent) and wrapping it in Duration::from_secs; if the rand crate isn’t already a dependency, add it to Cargo.toml and import rand::Rng at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prompts/en/channel.md.j2`:
- Around line 81-83: Update the prompt text to avoid absolutist language:
clarify that cron_expr refers to a strict 5-field cron expression and should be
preferred for wall-clock schedules (e.g., "0 * * * *"), but keep interval_secs
as a supported create input for fixed-duration cadences that 5-field cron cannot
express; remove “always use”/“do not use” phrasing and explicitly warn the model
not to emit 6-field cron expressions while recommending cron_expr when aligning
to real-world clock boundaries; reference the API fields cron_expr and
interval_secs (create input) so the guidance matches the implementation.
In `@prompts/en/tools/cron_description.md.j2`:
- Line 1: The prompt text currently forbids interval_secs even though the tool
(see src/tools/cron.rs functions handling requests around lines 45-81 and
183-230) still accepts it; update prompts/en/tools/cron_description.md.j2 to
stop outright banning interval_secs and instead state that cron_expr (5-field
wall-clock cron) is preferred and recommended, but interval_secs is supported as
a legacy/fallback for fixed cadences that 5-field cron cannot express; keep
guidance about using run_once for one-time reminders and show examples for both
cron_expr and interval_secs usage so callers know valid encodings.
---
Nitpick comments:
In `@src/cron/scheduler.rs`:
- Around line 652-678: anchored_initial_delay currently returns a fixed 2s for
overdue jobs which can still create a thundering herd; change the overdue branch
in anchored_initial_delay to return a small randomized jitter (e.g., 1–4
seconds) instead of Duration::from_secs(2) by generating a random u64 in that
range (use rand::thread_rng().gen_range(1..=4) or equivalent) and wrapping it in
Duration::from_secs; if the rand crate isn’t already a dependency, add it to
Cargo.toml and import rand::Rng at the top of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31f2f6e6-6fde-4752-9132-4bb0ee20bf5a
📒 Files selected for processing (11)
interface/src/api/client.tsinterface/src/lib/format.tsinterface/src/routes/AgentCron.tsxinterface/src/routes/AgentDetail.tsxprompts/en/channel.md.j2prompts/en/tools/cron_description.md.j2src/api/agents.rssrc/cron/scheduler.rssrc/cron/store.rssrc/main.rssrc/tools/cron.rs
…ut and delivery target picker The cron crate requires 7-field expressions (sec min hour dom month dow year) but we were validating for 5-field standard cron then passing it directly to Schedule::from_str, causing every wall-clock cron to fail with 'Invalid cron expression'. Fix: normalize_cron_expr, the API validator, and the tool validator all now expand 5-field input to 7-field (prepend '0' seconds, append '*' year) before parsing. The original 5-field form is stored in the DB. Also: - Rework modal to side-by-side two-column layout (max-w-4xl) - Replace delivery target text input with a channel dropdown populated from api.channels(), with Custom... fallback for manual entry - Show agent name inline beside each channel option - Exclude cron and link channels from the dropdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
src/api/cron.rs (1)
226-234: Duplicated cron expansion logic.The 5-field to 7-field expansion (
format!("0 {expr} *")) is duplicated here and insrc/cron/scheduler.rs(lines 646 and 710). The scheduler already has anexpand_cron_exprhelper function. Consider exposing it as a public utility to avoid triple duplication.Suggested approach
In
src/cron/scheduler.rs, make the helper public:-fn expand_cron_expr(expr: &str) -> String { +pub fn expand_cron_expr(expr: &str) -> String {Then in
src/api/cron.rs:+use crate::cron::scheduler::expand_cron_expr; + if let Some(expr) = cron_expr { let field_count = expr.split_whitespace().count(); if field_count != 5 { return Err(( StatusCode::BAD_REQUEST, format!("cron_expr must have exactly 5 fields (got {field_count}): '{expr}'"), )); } - let expanded = format!("0 {expr} *"); + let expanded = expand_cron_expr(expr); cron::Schedule::from_str(&expanded).map_err(|error| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/cron.rs` around lines 226 - 234, The cron expansion "0 {expr} *" is duplicated; make the helper in scheduler.rs public and reuse it: change the expand_cron_expr helper in src/cron/scheduler.rs to pub (e.g., pub fn expand_cron_expr(expr: &str) -> String or Result<String, _> matching its current behavior), then replace the local inline expansion in the API handler with a call to cron::scheduler::expand_cron_expr(expr) (or the appropriate module path) and use its returned value when calling cron::Schedule::from_str; ensure any error types/signatures are adapted so the API's map_err still produces the same BAD_REQUEST message.interface/src/routes/AgentCron.tsx (2)
460-481: Active hours UI defaults may confuse users.When
active_start_houris empty (first load), the NumberStepper displays 0h. ButformDataToRequestcorrectly treats empty strings as undefined (no active hours restriction). Consider showing a placeholder or disabled state when hours aren't set, to avoid implying the job runs only at hour 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentCron.tsx` around lines 460 - 481, The NumberStepper components for active hours (used with formData.active_start_hour and formData.active_end_hour) currently coerce empty strings to 0/23 and thus visually imply a restriction; update the UI so empty values are rendered as an empty/placeholder or disabled state instead of 0/23 — e.g., pass undefined/null (or a placeholder prop) to NumberStepper when formData.* is '' and ensure the onChange still writes back strings, keeping formDataToRequest logic unchanged; target the NumberStepper usages in this Field and adjust rendering to show a placeholder like "—" or disable the control until a value is set.
209-213: Defensive validation is good, but consider user feedback.The validation logic at line 211 duplicates the button's
disabledcondition. Since the button is disabled when invalid, this early return is purely defensive. Consider whether showing an error toast would improve UX if someone bypasses the disabled state (e.g., via keyboard).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentCron.tsx` around lines 209 - 213, The handleSave function currently silently returns when validation fails (checking formData.id, formData.prompt, formData.delivery_target, and cron_expr) which duplicates the button's disabled logic; instead surface a clear error to the user when validation blocks submission—update handleSave to perform the same checks and, on failure, call the app's notification/toast helper (or set a local error state) to show a descriptive error message (e.g., "Please complete all required fields" or specific missing-field text) before returning, then only call saveMutation.mutate(formDataToRequest(formData)) when validation passes; keep the same validation conditions but replace the silent early return with a user-facing error via the existing UI notification approach.src/tools/cron.rs (1)
238-242: Same duplicated expansion logic.This is the same 5-to-7-field expansion pattern found in
src/api/cron.rsandsrc/cron/scheduler.rs. Reuse theexpand_cron_exprhelper from scheduler.rs.Suggested fix
+use crate::cron::scheduler::expand_cron_expr; + if let Some(expr) = cron_expr.as_deref() { let field_count = expr.split_whitespace().count(); if field_count != 5 { return Err(CronError(format!( "'cron_expr' must have exactly 5 fields (got {field_count}): '{expr}'" ))); } - let expanded = format!("0 {expr} *"); + let expanded = expand_cron_expr(expr); cron::Schedule::from_str(&expanded) .map_err(|error| CronError(format!("invalid 'cron_expr' '{expr}': {error}")))?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 238 - 242, Replace the duplicated 5-to-7-field expansion (the format!("0 {expr} *") + cron::Schedule::from_str) with a call to the shared helper expand_cron_expr from scheduler.rs: call expand_cron_expr(expr) to get the expanded expression, then pass that to cron::Schedule::from_str and propagate errors as before; import or qualify expand_cron_expr into scope if necessary. Ensure you remove the temporary expanded variable and keep the existing error mapping logic (CronError(...)) intact when calling cron::Schedule::from_str.src/cron/scheduler.rs (1)
532-533: Consider anchoring cold re-enabled jobs to their last execution.When cold-re-enabling a job, the code passes
Nonefor the anchor. If the job has execution history in the store, it could be fetched to anchor the first tick correctly. This would prevent potential immediate firing if the job was disabled for a long time.However, this may be intentional if re-enabled jobs should start fresh. Adding a brief comment documenting the design choice would clarify intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/scheduler.rs` around lines 532 - 533, The current call to start_timer(job_id, None) when re-enabling a job can cause an immediate tick because it doesn't anchor to prior execution; update the logic in scheduler.rs to fetch the job's last execution time from the store (using the same job_id) and pass that timestamp as Some(anchor) to start_timer if present, falling back to None only if no history exists; alternatively, if starting fresh is intentional, add a concise comment at the start_timer(job_id, None) call explaining that the design purposely ignores prior runs and always starts a fresh schedule so reviewers understand the choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design-docs/interactive-shell.md`:
- Around line 17-23: Update the fenced code blocks that show the failing shell
commands (e.g., the blocks containing "Shell: npm create vite@latest myapp --
--template react" and "Shell: npx create-vite@latest myapp --template react") to
include appropriate language tags (bash/sh for shell output, text/json/rust as
applicable for other blocks) so they satisfy markdownlint MD040; apply the same
change to the other affected blocks mentioned (the blocks around the other
failing Shell snippets) to keep the document lint-clean.
- Around line 114-115: The docs currently describe streaming raw lines via
stream_lines and emitting ProcessEvent::ToolOutput before any scrubbing, which
risks leaking secrets; update the implementation and docs to make redaction
mandatory in the live-output path by running the redaction/scrub function inside
stream_lines (before creating/emitting ProcessEvent::ToolOutput) so every
emitted line is scrubbed in real time; also ensure the same change is applied to
other streaming spots referenced (the block around the lines mentioned and the
ToolCompleted emission) so ToolOutput and ToolCompleted both use the redacted
buffer/ShellOutput and the implementation order in the docs reflects
redaction-first then emit.
- Around line 114-115: The current stream_lines helper uses
AsyncBufReadExt::lines(), which misses prompts without trailing newlines; update
stream_lines to read raw chunks (e.g., AsyncReadExt::read or poll_read into a
buffer) and emit ProcessEvent::ToolOutput for each chunk, accumulate bytes into
the String returned as ShellOutput, and on quiesce/exit ensure any remaining
partial buffer is flushed as a final ToolOutput so waiting_for_input and live
streaming receive prompt text; ensure the same ShellOutput is returned after
collecting all chunks and that the quiesce path explicitly converts and emits
any partial line before returning.
- Around line 177-205: The liveToolOutput buffer (liveToolOutput.current[key])
in handlers.tool_output can grow without bound; implement a fixed-size
ring/rolling buffer or enforce a max lines/bytes cap inside handlers.tool_output
so when pushing event.line you trim older entries (e.g., keep last N lines or
last M bytes) to prevent unbounded growth, and ensure the same capped buffer is
cleared/replaced in the tool_completed handler; update the ToolCall.tsx
rendering logic (which reads liveOutput) to continue showing the truncated
liveOutput and still switch to spinner when liveOutput is empty.
- Around line 215-238: The quiesce timer currently only resets on stdout
activity (stdout_reader.next_line()), so activity on stderr can be ignored and
cause premature interactive detection; update the loop handling around
QUIESCE_TIMEOUT/overall_timeout to also listen to stderr activity (e.g.,
stderr_reader.next_line()) or otherwise merge stdout and stderr into a single
activity signal, and ensure both readers reset the quiesce timer before setting
interactive_detected and calling child.kill(). Keep the existing overall_timeout
branch and preserve behavior on Ok(None)/Err for each reader.
In `@interface/src/routes/AgentCron.tsx`:
- Around line 334-357: The two toggle buttons that set formData.schedule_mode
("Cron Expression" and "Interval") lack accessibility state; update the markup
around the buttons and the buttons themselves (the container and the onClick
handlers that call setFormData) to expose the selected state to assistive tech
by adding appropriate ARIA attributes (e.g., role="tablist" or role="group" on
the container and aria-pressed or aria-selected on each button based on
formData.schedule_mode === "cron" / "interval"), ensure each button has an
accessible name (already present) and maintain keyboard focus/activation
behavior by keeping them as buttons.
In `@src/cron/scheduler.rs`:
- Around line 668-684: The fixed 2-second fallback in anchored_initial_delay
causes a thundering herd when many overdue interval jobs restart; modify
anchored_initial_delay (and keep interval_initial_delay) so that instead of
returning a constant Duration::from_secs(2) it returns a small randomized
jittered Duration (e.g., random seconds or milliseconds within a configurable
small range) using a RNG (e.g., rand::thread_rng and a uniform distribution) to
spread startup firing times; ensure the jitter range is small and bounded and
that the function still returns a Duration to preserve existing callers.
---
Nitpick comments:
In `@interface/src/routes/AgentCron.tsx`:
- Around line 460-481: The NumberStepper components for active hours (used with
formData.active_start_hour and formData.active_end_hour) currently coerce empty
strings to 0/23 and thus visually imply a restriction; update the UI so empty
values are rendered as an empty/placeholder or disabled state instead of 0/23 —
e.g., pass undefined/null (or a placeholder prop) to NumberStepper when
formData.* is '' and ensure the onChange still writes back strings, keeping
formDataToRequest logic unchanged; target the NumberStepper usages in this Field
and adjust rendering to show a placeholder like "—" or disable the control until
a value is set.
- Around line 209-213: The handleSave function currently silently returns when
validation fails (checking formData.id, formData.prompt,
formData.delivery_target, and cron_expr) which duplicates the button's disabled
logic; instead surface a clear error to the user when validation blocks
submission—update handleSave to perform the same checks and, on failure, call
the app's notification/toast helper (or set a local error state) to show a
descriptive error message (e.g., "Please complete all required fields" or
specific missing-field text) before returning, then only call
saveMutation.mutate(formDataToRequest(formData)) when validation passes; keep
the same validation conditions but replace the silent early return with a
user-facing error via the existing UI notification approach.
In `@src/api/cron.rs`:
- Around line 226-234: The cron expansion "0 {expr} *" is duplicated; make the
helper in scheduler.rs public and reuse it: change the expand_cron_expr helper
in src/cron/scheduler.rs to pub (e.g., pub fn expand_cron_expr(expr: &str) ->
String or Result<String, _> matching its current behavior), then replace the
local inline expansion in the API handler with a call to
cron::scheduler::expand_cron_expr(expr) (or the appropriate module path) and use
its returned value when calling cron::Schedule::from_str; ensure any error
types/signatures are adapted so the API's map_err still produces the same
BAD_REQUEST message.
In `@src/cron/scheduler.rs`:
- Around line 532-533: The current call to start_timer(job_id, None) when
re-enabling a job can cause an immediate tick because it doesn't anchor to prior
execution; update the logic in scheduler.rs to fetch the job's last execution
time from the store (using the same job_id) and pass that timestamp as
Some(anchor) to start_timer if present, falling back to None only if no history
exists; alternatively, if starting fresh is intentional, add a concise comment
at the start_timer(job_id, None) call explaining that the design purposely
ignores prior runs and always starts a fresh schedule so reviewers understand
the choice.
In `@src/tools/cron.rs`:
- Around line 238-242: Replace the duplicated 5-to-7-field expansion (the
format!("0 {expr} *") + cron::Schedule::from_str) with a call to the shared
helper expand_cron_expr from scheduler.rs: call expand_cron_expr(expr) to get
the expanded expression, then pass that to cron::Schedule::from_str and
propagate errors as before; import or qualify expand_cron_expr into scope if
necessary. Ensure you remove the temporary expanded variable and keep the
existing error mapping logic (CronError(...)) intact when calling
cron::Schedule::from_str.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d73ba561-351c-462f-a986-417d3afb7a90
📒 Files selected for processing (5)
docs/design-docs/interactive-shell.mdinterface/src/routes/AgentCron.tsxsrc/api/cron.rssrc/cron/scheduler.rssrc/tools/cron.rs
…rompt language - main.rs: log warning on last_execution_times() failure instead of silently falling back to empty map via unwrap_or_default() - store.rs: use ? propagation and explicit Option<String> for last_executed_at instead of unwrap_or_default() which could silently drop anchoring data on column/type changes - scheduler.rs: warn when last_executed_at is present but parsing fails, so format mismatches don't go unnoticed - channel.md.j2: soften from 'Always use / Do not use' to 'Prefer cron_expr' with interval_secs as a supported fallback for cadences that 5-field cron cannot express (e.g. every 90 minutes) - cron_description.md.j2: same softening — prefer cron_expr, but interval_secs is supported as legacy/fallback
rktmeister
pushed a commit
to rktmeister/spacebot
that referenced
this pull request
Mar 11, 2026
…l-clock-and-ui-rework feat: wall-clock cron support in UI, anchor interval crons to last execution on restart
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
0 9 * * *) and "Interval" (demoted, with a drift warning). Job cards display cron expressions in monospace, and emoji action buttons (⏸▶⚡✎✕) are replaced with HugeIcons. Timezone is shown in the stats bar.MAX(executed_at)fromcron_executionsfor each interval-based job and computes the first-tick delay from that timestamp. If overdue, fires after 2s jitter; if not yet due, sleeps for the remainder. Prevents skipped or duplicate firings after restarts.cron_exprwith common patterns. Tool description andinterval_secsparameter explicitly marked as deprecated.Changes
Backend (Rust)
src/cron/store.rs— newlast_execution_times()method (batchMAX(executed_at)grouped bycron_id)src/cron/scheduler.rs— newregister_with_anchor(),anchored_initial_delay(),start_timer()now accepts optional anchor timestampsrc/main.rs— startup usesregister_with_anchorwith last execution timessrc/api/agents.rs—CronJobInfonow includescron_expr, SQL query updatedsrc/tools/cron.rs—interval_secsparameter description marked DEPRECATEDFrontend (TypeScript)
interface/src/routes/AgentCron.tsx— full rework: schedule mode toggle, cron expression input with 9 presets, timeout field, HugeIcon action buttons, monospace schedule displayinterface/src/routes/AgentDetail.tsx—CronSectionshowscron_exprviaformatCronScheduleinterface/src/api/client.ts—cron_expr,timeout_secs,timezoneadded to typesinterface/src/lib/format.ts— newformatCronSchedule(), improvedformatDuration()Prompts
prompts/en/channel.md.j2— new "Cron (Scheduled Tasks)" sectionprompts/en/tools/cron_description.md.j2— strengthened to "always usecron_expr"Testing
just gate-prpasses — 460 tests, clippy clean, fmt clean, no migration changes.Note
This PR introduces wall-clock cron expression support to the UI and fixes interval-based cron restart timing by anchoring to the last execution timestamp. The cron UI now defaults to standard cron expressions with preset shortcuts, relegates interval mode to secondary status, and uses modern icons. System prompts are updated to guide the LLM away from deprecated interval scheduling. All delivery gates pass with no migration changes required.
Written by Tembo for commit de6308c. This will update automatically on new commits.