hanxdatadog commented on PR #69487: URL: https://github.com/apache/airflow/pull/69487#issuecomment-4974101232
> Confirmed the root cause: repeat() in call_regular_interval calls action() with no exception handling, and both maintenance callbacks it guards -- _remove_unreferenced_triggers (a raw DELETE with synchronize_session=fetch) and _reap_stale_connection_tests (a row-locked SELECT plus UPDATE loop and flush) -- have no internal error handling. timers.run(blocking=False) is called inside the while loop of _run_scheduler_loop with no try/except, so a transient DB error (statement_timeout, deadlock, dropped connection) propagates out and takes down the SchedulerJob for what should be a skippable cleanup cycle. The opt-in flag is the right call since EventScheduler is general-purpose and default=False keeps every existing caller identical. except Exception (not bare except) correctly still lets KeyboardInterrupt/SystemExit through, and self.enter() is the last line of repeat() outside the try, so the next cycle is always rescheduled. Worth noting: both callbacks are @provide_session-deco rated, so the session is already rolled back and closed by the time the exception reaches the except -- swallowing doesn't leave a half-open transaction. This matches the existing precedent in _update_dag_run_state_for_paused_dags. Regression coverage is meaningful: the non_fatal test fails without the guard (asserts no raise plus queue growth 1 to 2), and the default-path test pins that the unguarded behavior still propagates. > > One non-blocking question: self.log.exception logs a full traceback at ERROR every cycle, so a persistent (rather than transient) failure will repeat the traceback each interval and the cleanup will silently never succeed. Given the coarse interval that's probably acceptable, but did you consider log.warning without the traceback, or logging once, to avoid steady-state log noise? Hi @Vamsi-klu Thank you for the comment. I just made the log at the warning level. For the change's motivation, exactly as you described, we hit some crashes in some supposably non-fatal cleaning up schedule actions. here is to provide some general option for these kind of non-fatal scheduled actions. With the flag, the goal is to not let scheduler crash (making some tasks enter orphan adoption/drop path unnecessarily) meanwhile giving the maintainers enough attentions to take actions to address them shortly (warning level logs with stacktrace) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
