Skip cron jobs with invalid schedules instead of 500-ing every cron mutation#243
Merged
Conversation
…utation A single job with an out-of-range schedule field (e.g. hour 45) made every cron enable/disable/update return 500: sync_jobs re-adds all enabled jobs on each mutation, and _add_job_to_scheduler only guarded the field-count check, not CronTrigger's value validation. The propagated ValueError also broke the mutation's own file-write follow-through, so the frontend never refetched and the edit dialog kept showing the stale cached value (the timeout "reverts to 900" symptom). Validate the trigger inside the guard so a malformed schedule is logged and skipped, letting the rest of the sync proceed.
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
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.
Context
The cron editor's pencil kept reverting a job's timeout to its template default (900), and edits appeared not to save. Root cause is a backend 500, not the UI.
hn-ai-dailyhad an out-of-range schedule06 45 * * *(hour 45; max is 23).sync_jobsre-adds every enabled job on every cron mutation, and_add_job_to_scheduler(kaisho/cron/scheduler.py) only guarded the field-count check (_cron_kwargs), notCronTrigger's value validation. So that one malformed job made every cron enable/disable/update throw aValueError→ 500.The mutation writes the file before
sync_jobs, so the value did persist to disk — but the request 500'd, the frontend's React Query mutation errored, no refetch happened, and the edit dialog kept showing the stale cached value. That is the "timeout reverts to 900" symptom.Why this matters
One bad job silently bricks all cron mutations, including the save that would fix the bad job — a deadlock the user can only escape by hand-editing
jobs.yaml.Fix
Validate the
CronTriggerinside the guard in_add_job_to_scheduler, so a malformed schedule (wrong field count or out-of-range value) is logged and skipped instead of aborting the whole sync. The remaining jobs schedule normally and mutations return 200.Adds
tests/test_cron_scheduler.pycovering: valid job registers, out-of-range hour is skipped without raising, wrong field count is skipped, and a bad job in a set does not block the good ones.Out of scope
POST/PATCH) so invalid schedules can't be stored in the first place — worth a follow-up.