Conversation
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. A faulty validation or rescheduling path could leave a reminder running at the wrong time or cause a notification to be missed or sent incorrectly, and an already-triggered notification would not be undone by reverting the code. The affected jobs and schedules are bounded and can generally be corrected or rerun.
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Reviewed the latest head and the validation ordering is correct. update_job() builds a candidate from the persisted row and validates its trigger before any SQLite update or scheduler replacement when the job is enabled; disabled legacy rows remain editable. _build_trigger() is shared by validation and scheduling, preserving timezone, weekday normalization, and one-shot handling. The regression suite covers invalid cron edits, malformed run-once timestamps, invalid activation, valid edits, disablement, callback payload preservation, and restart recovery. I found no remaining correctness blockers.
Editing an enabled scheduled reminder with an invalid cron expression currently returns an error after overwriting its database row and removing its working schedule. For example, changing
0 9 * * 1to60 9 * * *in the Dashboard fails to save, but the original reminder no longer runs. Restarting cannot recover it because the stored expression is now invalid. Malformed one-shot timestamps and enabling invalid legacy jobs have the same write-before-validation problem at the manager boundary.Changes
This addresses rejected edits/activation. It does not change job creation, introduce automatic retries, change misfire policy, or provide a transaction across SQLite and APScheduler for arbitrary runtime failures.
Verification steps and results
Windows, Python 3.12; base
bd046ed29914ee559e9bf47676ccb71a84f747ba.Before the production fix, the new regression file reported 5 failed, 1 passed. The failures demonstrate durable state being changed despite the rejected edit: invalid field count/minute/weekday, malformed one-shot timestamp, and failed activation of an invalid disabled row.
After the fix:
The warning is the existing
audioopdeprecation. The regression tests use a paused real scheduler and temporary on-disk SQLite; agent execution is stubbed, with the original scheduled callback exercised directly. No live model or messaging service is called. Remote CI for commitac34058beecc55448949d3223b8792775c1edcccis complete: all 23 checks passed, including the actual test steps on Ubuntu, Windows, and macOS, formatting, CodeQL, dashboard CI, and smoke tests. Unit-test run.Related work
Checked prior cron error propagation (#7513), editing support (#6516 / #6518 / #7445), and the open security-hardening work (#9951). Reporting a scheduling error does not undo the previous database write/removal; this patch validates before those mutations. No matching open fix was found in the duplicate search.
Checklist
AI assistance
Codex assisted with investigation, implementation, regression tests, and this description. The reported tests were executed locally. This new finding has no maintainer-assigned priority label; no claim is made that it resolves other reports of missed reminders.