Skip to content

fix: skip schedules with invalid cron_offset instead of crashing the scheduler - #669

Open
juanmicl wants to merge 4 commits into
taskiq-python:masterfrom
juanmicl:fix/scheduler-invalid-cron-offset-crash
Open

fix: skip schedules with invalid cron_offset instead of crashing the scheduler#669
juanmicl wants to merge 4 commits into
taskiq-python:masterfrom
juanmicl:fix/scheduler-invalid-cron-offset-crash

Conversation

@juanmicl

@juanmicl juanmicl commented Sep 1, 2026 •
edited by danfimov
Loading

Copy link
Copy Markdown
Contributor

Closes: #668

is_cron_task_now called ZoneInfo(offset) outside the try/except that converts errors into CronValueError, and the scheduler loop only catches CronValueError. One schedule with a non-IANA cron_offset string (e.g. the natural spelling "UTC+3") raised ZoneInfoNotFoundError out of SchedulerLoop.run and killed the whole scheduler process.

With this change a bad offset goes through the same path as an invalid cron expression: CronValueError is raised, the loop logs the existing Cannot parse cron warning and skips that schedule.

Two regression tests cover invalid offset strings: the natural spelling "UTC+3" and a typo'd timezone name ("Europa/Madrid"). Both fail on master with ZoneInfoNotFoundError and pass with this change.

Complements #625 (which fixes duration-string offsets at model-validation time but leaves the runtime guard unhandled).

Validation: full suite passes (pytest -q, 324 tests), black, ruff and mypy clean.

@codecov

codecov Bot commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.88%. Comparing base (b5d6033) to head (fb21919).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
+ Coverage   82.86%   82.88%   +0.01%     
==========================================
  Files          69       69              
  Lines        2749     2752       +3     
==========================================
+ Hits         2278     2281       +3     
  Misses        471      471

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

danfimov
danfimov requested changes Sep 15, 2026
View reviewed changes

@danfimov danfimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for contribution)

Please fix couple minor things. And after that I think we can merge this MR.

Comment thread taskiq/cli/scheduler/run.py Outdated
Comment thread tests/cli/scheduler/test_is_cron_task_now.py Outdated
@danfimov

danfimov commented Sep 16, 2026

Copy link
Copy Markdown
Member

Please run linters locally. I see that ruff is failing in CI

@danfimov
danfimov self-requested a review September 16, 2026 10:51
@juanmicl

juanmicl commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

I ran the linters locally before pushing, with both the locked ruff (0.16.5) and the latest one, and this branch passes. The errors in the CI lint job don't come from this PR's changes, they come from the merge with master.

#627 (b5d6033, merged after this PR was opened) introduced them: 5 auto-fixable COM812 trailing comma issues in tests/cli/scheduler/test_send_with_timeout.py, and C901 on SchedulerLoop.run (11 > 10) from the send_timeout branches. I verified by running ruff on the merge tree of this PR with current master: exactly the same 6 errors, none in the files this PR touches. They slipped in because the lint job in test.yml only runs on pull_request events, so pushes to master are never linted. Every PR currently open against master should show the same failure.

If it helps, I can send a small separate PR that fixes master's lint (apply the 5 auto-fixes and extract a small helper from SchedulerLoop.run to get back under the complexity limit), so this PR and the others go green. Or I can fold it into this PR if you prefer.

@danfimov

danfimov commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Yep, found the same problem. Will merge this MR with fixes for pre-commit checks first, after that will rebase your MR and merge it too

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

Reviewers

@danfimov danfimov danfimov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

A schedule with an invalid cron_offset string crashes the whole scheduler

2 participants

@juanmicl @danfimov