Avoid scheduler crash when periodic maintenance actions fail#69487
Draft
hanxdatadog wants to merge 4 commits into
Draft
Avoid scheduler crash when periodic maintenance actions fail#69487hanxdatadog wants to merge 4 commits into
hanxdatadog wants to merge 4 commits into
Conversation
_remove_unreferenced_triggers and _reap_stale_connection_tests are registered as periodic scheduler maintenance callbacks via EventScheduler.call_regular_interval, which has no exception handling around the callback. A single transient failure (e.g. a DB statement_timeout) propagates and crashes the whole SchedulerJob. Add an opt-in catch_exceptions flag (default False, preserving current behavior) that logs and swallows an exception from the periodic action instead of letting it propagate, so one bad cycle doesn't take down the scheduler; the next cycle is still scheduled either way. Enable it for the two maintenance callbacks above.
boring-cyborg
Bot
added
the
area:Scheduler
non_fatal reads better at call sites (e.g. non_fatal=True) than the mechanism-focused catch_exceptions, and matches existing "non-fatal" terminology already used elsewhere in the codebase for the same crash-avoidance concept (see dag_processing/importers/base.py).
…pt to investigate Log "Failed to run periodic action <name> due to <exception>. Please investigate if this is persistent." instead of a bare "Exception raised in periodic action <repr>", making the log actionable on its own without needing to open the traceback to identify what failed.
_update_dag_run_state_for_paused_dags already uses "Failed to <thing> due to %s" (scheduler_job_runner.py); match that instead of inventing new phrasing. Drop "please investigate if this is persistent" -- it's not actionable (nothing here can tell whether a single occurrence is persistent) and no other log line in this codebase phrases itself as a directive to the reader. Add "will retry on the next cycle" instead, since that's genuinely new information explaining why the exception is safe to swallow.
hanxdatadog
changed the title
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
1 participant
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.
What does this PR do?
non_fatal: bool = Falseparameter toEventScheduler.call_regular_interval. WhenTrue, an exception raised by the periodicactionis logged (self.log.exception(...)) and swallowed instead of propagating; the next cycle is still scheduled either way. Default isFalse, preserving current behavior for existing callers.non_fatal=Truefor the twoSchedulerJobRunnerperiodic maintenance callbacks registered viacall_regular_intervalthat had no exception handling at all:_remove_unreferenced_triggersand_reap_stale_connection_tests.Failed to run periodic action <name> due to <exception>; will retry on the next cycleon the caught-exception path — matches the existingFailed to <thing> due to %shouse style already used by_update_dag_run_state_for_paused_dags, and states why a single occurrence is safe to ignore.Motivation:
EventScheduler.call_regular_interval's innerrepeat()closure calls the registeredactionwith no exception handling, so a single transient failure (e.g. a DBstatement_timeoutduring_remove_unreferenced_triggers's DELETE) propagates all the way up and crashes the wholeSchedulerJob. In practice this means: the scheduler process dies with a critical-level log, and every task instance it was tracking gets swept into the orphaned-task-adoption path on the next scheduler startup — a heavyweight, disruptive recovery flow triggered by what should have been a harmless, skippable cycle of a periodic cleanup job. This is inconsistent with the existing pattern elsewhere inSchedulerJobRunner—_update_dag_run_state_for_paused_dagsalready wraps its own body inexcept Exception as e: # should not fail the scheduler, and the same crash-avoidance philosophy has precedent in merged PRs like #62893 (isolating per-DagRun failures so one bad DagRun can't crash the scheduler).EventScheduleris documented as a "general purpose" utility, so rather than making it always swallow exceptions (which would silently change behavior for any future caller that wants failures to propagate), the fix is an explicit opt-in.Testing:
test_event_scheduler.py— one confirms the default (no flag) still propagates the action's exception, one confirmsnon_fatal=Trueswallows it and still schedules the next cycle.Notes:
non_fatalrather thancatch_exceptionsto be outcome-focused (this failure isn't fatal to the scheduler) rather than mechanism-focused, matching existing "non-fatal" terminology already used elsewhere in the codebase (dag_processing/importers/base.py).Was generative AI tooling used to co-author this PR?