fix(reports): prevent bad-date crontab from crashing the report scheduler for all tenants - #42486
Conversation
…l reports A syntactically valid but calendrically impossible crontab (e.g. "0 0 30 2 *" for Feb 30th) passes API schema validation via croniter.is_valid(), which only checks syntax. When the reports.scheduler celery-beat task later iterates all_next() for such a schedule, croniter raises an uncaught CroniterBadDateError, breaking the scheduler loop for every active report and alert across the instance until the offending schedule is fixed directly in the database. cron_schedule_window() now catches CroniterBadDateError the same way it already handles an invalid timezone: log the error and treat it as producing no upcoming executions, isolating the bad schedule instead of crashing the shared scheduler loop.
dosubot
Bot
added
alert-reports
Code Review Agent Run #ee4e2aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42486 +/- ##
==========================================
- Coverage 65.25% 65.24% -0.01%
==========================================
Files 2795 2795
Lines 157639 157642 +3
Branches 36052 36052
==========================================
- Hits 102860 102858 -2
- Misses 52802 52806 +4
- Partials 1977 1978 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
A crontab expression that's syntactically valid but can never match a real calendar date (e.g.
0 0 30 2 *for Feb 30th, or0 0 31 4 *for April 31st — April only has 30 days) passes API schema validation on report/alert create/update, becausesuperset/reports/schemas.py::validate_crontab()only checkscroniter.is_valid(), which is a purely syntactic check.Once such a schedule is saved as active, the celery-beat task
reports.scheduler(superset/tasks/scheduler.py) iterates over every activeReportScheduleand callscron_schedule_window()(superset/tasks/cron_util.py) for each, with no per-schedule isolation:Inside
cron_schedule_window, iteratingcrons.all_next(datetime)raisescroniter.CroniterBadDateErrorfor a bad-date cron (croniter's internal max-year search gives up), uncaught anywhere in the chain. Because the loop has no per-schedule isolation, one report/alert with a bad-but-syntactically-valid crontab crashes the scheduler task for every other active schedule too — on every celery-beat tick, across the whole instance — until someone finds and fixes/deletes the offending schedule directly in the database.PROBLEM
CroniterBadDateErroris a subclass ofCroniterError, itself a subclass ofValueError— a raw library exception with no Superset-level handling on this path.FIX
cron_schedule_window()already has an established precedent for degrading gracefully on bad per-schedule input a few lines above this bug: it catchesUnknownTimeZoneErrorfor an invalid timezone string and falls back to UTC with alogger.warning. This PR follows the same convention for the cron expression itself: thecrons.all_next(datetime)iteration is now wrapped intry/except CroniterBadDateError, logging an error (this case is not a graceful degrade like the timezone fallback — it means the schedule can never fire until someone fixes it, so it's logged loudly) and letting the generator end instead of propagating and crashing the whole scheduler run.This is a background celery-task loop, not an HTTP request/response path, so there's no 4xx/500 status to map to — the fix intentionally does not introduce a new
superset/exceptions.pysubclass (unlike the API-layer fixes in the related PRs below).TESTING INSTRUCTIONS
New parametrized test
test_cron_schedule_window_invalid_cron_dateadded totests/unit_tests/tasks/test_cron_util.py, covering"0 0 30 2 *"(Feb 30th) and"0 0 31 4 *"(April 31st), assertingcron_schedule_window()returns[]instead of raising.Verified empirically both ways:
croniter.croniter.CroniterBadDateError: failed to find next datepropagating out of the test.tests/unit_tests/tasks/test_cron_util.py— 36 passed, no regressions to the timezone-fallback or normal-schedule cases.ruff check/ruff format --checkclean on both changed files (verified against both the repo's ambient ruff and the CI-pinnedruff==0.9.7viauvx).mypyshows the same pre-existing missing-stub errors (croniter,pytz,freezegun.api.FakeDatetime) before and after — no new errors introduced.Tradeoffs
Additive-only: a cron that can never fire now logs an error and produces zero scheduled executions instead of crashing the whole scheduler task. No change to the failure mode for any valid crontab.
ADDITIONAL INFORMATION
Same bug class / cleanup series as #42366, #42401, #42426, #42442 (raw system/library exception reaching a live code path instead of being handled) — this one surfaces in the report-scheduling background task rather than an API request path.