feat: optional start anchor for recurring cron jobs - #180
Open
joaosouz4dev wants to merge 1 commit into
Open
Conversation
Today schedule_type "every" always fires for the first time at
(now + interval_s), which makes it impossible to schedule a recurring
job aligned to wall-clock time without first writing a one-shot
"at" job that re-arms itself on every fire. Concrete pain point: a
user wants "every day at 10:00 local time", chains 4 tools to compute
that offset, and the LLM still tends to get it wrong.
This patch lets callers pass at_epoch alongside interval_s for an
"every" job. When set and in the future, the first fire happens at
at_epoch and subsequent fires follow interval_s as usual. Existing
callers that pass only interval_s see no change.
main/cron/cron_service.c use at_epoch as initial next_run when set
main/tools/tool_cron.c accept and validate at_epoch for "every"
main/tools/tool_registry.c document the new optional usage so the LLM
picks it up via the tool schema
📝 WalkthroughWalkthroughThis PR adds optional start-anchor support for recurring cron jobs. The ChangesOptional start anchor for recurring cron jobs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/cron/cron_service.c`:
- Around line 318-325: Startup sets job->next_run using naive now +
job->interval_s which ignores anchor-aware logic in compute_initial_next_run(),
causing anchor drift when job->next_run <= 0; modify the startup path to call
compute_initial_next_run(job, now) (or the equivalent anchor-aware helper)
whenever a recurring enabled job has next_run <= 0 or job->at_epoch is set, and
assign its result to job->next_run instead of using now + job->interval_s so the
anchor semantics are preserved.
In `@main/tools/tool_cron.c`:
- Around line 71-83: The code currently ignores a present but non-numeric
"at_epoch" (found via cJSON_GetObjectItem -> start_at) and proceeds as if
absent; change the logic so that when start_at is non-NULL and
!cJSON_IsNumber(start_at) you treat this as an input error for recurring jobs:
construct an explanatory error message into output (similar style to the
existing snprintf), call cJSON_Delete(root), and return ESP_ERR_INVALID_ARG
instead of falling through to schedule now+interval_s; keep the existing numeric
branch that converts start_at->valuedouble to anchor and assigns job.at_epoch
when cJSON_IsNumber succeeds.
🪄 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: 7521f530-0d15-4d80-8a46-280b1f8b4e69
📒 Files selected for processing (3)
main/cron/cron_service.cmain/tools/tool_cron.cmain/tools/tool_registry.c
Comment on lines
+318
to
+325
| /* Optional start anchor: if at_epoch is set and in the future, use it | ||
| * as the first fire time. Lets callers say "every 86400s starting at | ||
| * tomorrow 10:00 local time" instead of "now + 24h". */ | ||
| if (job->at_epoch > now) { | ||
| job->next_run = job->at_epoch; | ||
| } else { | ||
| job->next_run = now + job->interval_s; | ||
| } |
There was a problem hiding this comment.
Avoid anchor logic drift by reusing compute_initial_next_run() at startup.
compute_initial_next_run() is now anchor-aware, but startup recompute still uses now + interval_s for recurring jobs. If an enabled job has next_run <= 0, this can bypass anchor semantics.
Proposed fix
- /* Recompute next_run for all enabled jobs that don't have one */
+ /* Recompute next_run for all enabled jobs that don't have one */
time_t now = time(NULL);
for (int i = 0; i < s_job_count; i++) {
cron_job_t *job = &s_jobs[i];
if (job->enabled && job->next_run <= 0) {
- if (job->kind == CRON_KIND_EVERY) {
- job->next_run = now + job->interval_s;
- } else if (job->kind == CRON_KIND_AT && job->at_epoch > now) {
- job->next_run = job->at_epoch;
- }
+ compute_initial_next_run(job);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/cron/cron_service.c` around lines 318 - 325, Startup sets job->next_run
using naive now + job->interval_s which ignores anchor-aware logic in
compute_initial_next_run(), causing anchor drift when job->next_run <= 0; modify
the startup path to call compute_initial_next_run(job, now) (or the equivalent
anchor-aware helper) whenever a recurring enabled job has next_run <= 0 or
job->at_epoch is set, and assign its result to job->next_run instead of using
now + job->interval_s so the anchor semantics are preserved.
Comment on lines
+71
to
+83
| cJSON *start_at = cJSON_GetObjectItem(root, "at_epoch"); | ||
| if (start_at && cJSON_IsNumber(start_at)) { | ||
| int64_t anchor = (int64_t)start_at->valuedouble; | ||
| time_t now = time(NULL); | ||
| if (anchor <= now) { | ||
| snprintf(output, output_size, | ||
| "Error: at_epoch %lld is not in the future (now=%lld)", | ||
| (long long)anchor, (long long)now); | ||
| cJSON_Delete(root); | ||
| return ESP_ERR_INVALID_ARG; | ||
| } | ||
| job.at_epoch = anchor; | ||
| } |
There was a problem hiding this comment.
Reject invalid at_epoch types for recurring jobs.
If at_epoch is present but not numeric, this path silently ignores it and schedules now + interval_s, which masks input errors.
Proposed fix
cJSON *start_at = cJSON_GetObjectItem(root, "at_epoch");
- if (start_at && cJSON_IsNumber(start_at)) {
+ if (start_at) {
+ if (!cJSON_IsNumber(start_at)) {
+ snprintf(output, output_size,
+ "Error: 'at_epoch' must be a unix timestamp number");
+ cJSON_Delete(root);
+ return ESP_ERR_INVALID_ARG;
+ }
int64_t anchor = (int64_t)start_at->valuedouble;
time_t now = time(NULL);
if (anchor <= now) {
snprintf(output, output_size,
"Error: at_epoch %lld is not in the future (now=%lld)",
(long long)anchor, (long long)now);
cJSON_Delete(root);
return ESP_ERR_INVALID_ARG;
}
job.at_epoch = anchor;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/tools/tool_cron.c` around lines 71 - 83, The code currently ignores a
present but non-numeric "at_epoch" (found via cJSON_GetObjectItem -> start_at)
and proceeds as if absent; change the logic so that when start_at is non-NULL
and !cJSON_IsNumber(start_at) you treat this as an input error for recurring
jobs: construct an explanatory error message into output (similar style to the
existing snprintf), call cJSON_Delete(root), and return ESP_ERR_INVALID_ARG
instead of falling through to schedule now+interval_s; keep the existing numeric
branch that converts start_at->valuedouble to anchor and assigns job.at_epoch
when cJSON_IsNumber succeeds.
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
Today
schedule_type: "every"always fires for the first time at(now + interval_s). Practically this makes it impossible to schedule a recurring job aligned to a wall-clock time without writing a one-shot"at"job whose message re-creates itself on every fire — a fragile pattern that depends on the LLM remembering to chaincron_addagain.Concrete pain point I hit while configuring a daily routine on a real device: the user asked for "every weekday at 10:00 local time". The LLM correctly computed the epoch for the next 10:00, called
cron_addwith bothschedule_type: "every"andat_epoch: <future-ts>, and the API silently ignoredat_epoch— the job was scheduled for now + 86400 (~3:26 AM local) instead. The first failure mode is silent, the second requires teaching the LLM a multi-step workaround, both are avoidable.This PR lets callers pass
at_epochalongsideinterval_sfor an"every"job. When set and in the future, the first fire happens atat_epochand every subsequent fire followsinterval_s. No behavior change for callers that only passinterval_s.if (job->kind == CRON_KIND_EVERY) { - job->next_run = now + job->interval_s; + if (job->at_epoch > now) { + job->next_run = job->at_epoch; // honor caller-supplied anchor + } else { + job->next_run = now + job->interval_s; + } }What's in the diff
main/cron/cron_service.c—compute_initial_next_run()honorsat_epochfor"every"jobs when in the future.main/tools/tool_cron.c—cron_addreads optionalat_epochfor"every", validates it's in the future, stores it on the job.main/tools/tool_registry.c— the tool schema description forat_epochnow documents the new optional usage so the LLM discovers it through the tool catalogue rather than the user having to spell it out every time.No new field on
cron_job_t, no migration ofcron.jsonon disk —at_epochalready existed for"at"jobs and was simply unused by"every"ones. Persisted jobs from before this change keep working unchanged.Test plan
cron_add({schedule_type:"every", interval_s:60, message:"x"})still firesnow + 60s, then every 60s after.cron_add({schedule_type:"every", interval_s:86400, at_epoch:<tomorrow 10:00 UTC>, message:"x"})first fires at the supplied timestamp, then every 24h after.at_epochin the past for an"every"job returns the same "is in the past" error already used for"at"jobs."every"job created withat_epochsurvives a reboot with the rightnext_run.Summary by CodeRabbit
New Features
Documentation