Skip to content

fix: preserve scheduled reminders when an edit is invalid - #10075

Open
beemines wants to merge 1 commit into
AstrBotDevs:masterfrom
beemines:fix/preserve-cron-jobs-on-invalid-edits
Open

beemines wants to merge 1 commit into
AstrBotDevs:masterfrom
beemines:fix/preserve-cron-jobs-on-invalid-edits

Conversation

@beemines

@beemines beemines commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

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 * * 1 to 60 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

  • Validate the prospective enabled job before writing to SQLite or replacing its live schedule.
  • Reuse the existing trigger construction for validation and scheduling, preserving timezone fallback, standard crontab weekday conversion, and one-shot timestamp handling.
  • Keep disabled jobs editable, including disabling or correcting malformed legacy rows.
  • Add tests through the real Dashboard service, SQLite database, and APScheduler. Invalid edits preserve the original name, payload, expression, deadline, scheduled callback, and restart recovery. Valid edits and disabling remain supported.

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.

  • This is not a breaking change.

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:

python -m pytest tests/unit/test_cron_edit_validation.py tests/unit/test_cron_manager.py tests/unit/test_cron_service.py tests/unit/test_cron_tools.py tests/unit/test_cron_context_compression.py -q
83 passed, 1 warning

ruff format .
1 file reformatted, 503 files left unchanged

ruff check .
All checks passed!

git diff --check
passed

The warning is the existing audioop deprecation. 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 commit ac34058beecc55448949d3223b8792775c1edccc is 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

  • New-feature discussion: not applicable; this repairs a rejected edit's side effects.
  • Reproduction, verification steps, and actual results are included above.
  • No new dependencies.
  • No malicious code.

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.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants