Repository navigation
Reset Dag autoregistration state when a Python Dag file fails to load - #74911
Conversation
potiuk
left a comment
There was a problem hiding this comment.
LGTM. I traced the paths that skipped the reset:
- the
AirflowConfigExceptionre-raise for a non-numericget_dagbag_import_timeout; - a timeout policy that raises something other than
ImportError/AttributeError, which hits theexcept Exceptionearly return; KeyboardInterruptfromparse().
In each case the stale name was left on the class, and every later DagContext.pop() hit KeyError on sys.modules[...]. The finally covers all of these. Dropping the reset from _process_modules is safe because nothing there depends on the name. The new test fails without the fix, since the second with DAG(...) raises.
CI for the last push shows a cancelled run, so it needs a full re-run before merge.
Drafted-by: Claude Opus 5.5; reviewed by potiuk before posting
e54793e to
194a2f6
Compare
The Python importer records the module it is importing so Dags created at module level get autoregistered, and drops that module from sys.modules when the import fails. The recorded name was only cleared after a successful load, so when loading raised (an invalid or failing get_dagbag_import_timeout policy, for example) the stale name stayed behind. Every later Dag context in the same process then looked it up in sys.modules and failed with KeyError, breaking Dags built by other importers such as the YAML importer. This surfaced as order-dependent failures of the YAML importer tests on xdist workers that had previously run the invalid-timeout importer test.
Set DagContext.current_autoregister_module_name inside parse() and reset it in a finally there, so the set and the reset live in one place and a failing timeout policy never leaves the name behind. Cover the KeyboardInterrupt path, which is now the case that exercises the reset.
194a2f6 to
e206681
Compare
Backport successfully created: v3-4-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
In parallel test runs, a failed Python Dag import could leave stale state that broke later imports in the same worker; the fix always clears it.
AI Summary
`PythonDagImporter` sets `DagContext.current_autoregister_module_name` before importing a Dag file, and drops that module from `sys.modules` if the import fails. The name was only reset in `_process_modules`, which is skipped when `_load_modules` raises: the `AirflowConfigException` re-raise for an invalid `get_dagbag_import_timeout` value, or the generic `except Exception` early return (for example, a timeout policy that raises). The stale name stayed on the class. After that, every `with DAG(...)` in the same process ran `DagContext.pop()`, which does `sys.modules[stale_name]` and raised `KeyError`. Dags built by other importers in the same process, such as the YAML importer, failed to build.This PR moves the reset into a
finallyaround_load_modules.How it surfaced
Three YAML importer tests failed in the
Task SDK tests / task-sdk:P3.11 testsjob of #74889 (run 39004464506). That PR moves CI onto 32-CPU runners, so xdist started 32 workers and grouped tests differently. All three failures were on workergw12:The log runs at verbosity 0, so it does not list which tests ran on each worker. The hash identifies the predecessor anyway:
8ffeae58…is the sha1 of/tmp/pytest-of-root/pytest-0/popen-gw12/test_invalid_dagbag_import_tim0/bundle/dag.py. That is the temp path ofTestPythonDagImporter::test_invalid_dagbag_import_timeout_raises_custom_exceptionongw12. The/etc/passwdcase has the same cause: theKeyErrorraised byDAG.__exit__replaces the expected "resolves outside" error.Minimal deterministic reproduction (Breeze, one process, before this fix gives 3 failed / 1 passed):
Testing
test_failed_load_does_not_break_later_dagscovers both error paths. Without the fix, both cases fail with theKeyError.task-sdk/tests/task_sdk/importersandshared/module_loading/testspass (305 tests).-n 32 --skip-db-tests(4057 passed, 6 skipped each time).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5) following the guidelines