You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kjh0623 (via GitHub)" <gi...@apache.org> on 2026/07/14 03:09:13 UTC
[PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
kjh0623 opened a new pull request, #69845:
URL: https://github.com/apache/airflow/pull/69845
…istered
_update_import_errors() eagerly evaluated ParseImportError.full_file_path() to build the on_new/on_existing_dag_import_error listener arguments, even when no listener was registered. full_file_path() instantiates the DAG bundle via DagBundlesManager().get_bundle(), which for some bundle types (e.g. the Git bundle) resolves an Airflow Connection. Performing that work inside the DAG-parsing transaction can disturb it and roll back the not-yet-flushed ParseImportError row, so the DAG ends up flagged as broken (has_import_errors) while no import error is ever shown in the UI.
Guard both listener notifications with get_listener_manager().has_listeners so full_file_path() is only resolved when a listener is actually registered. This fixes the silent drop in the common (no-listener) case and avoids the wasted bundle instantiation on every import error.
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4965022774
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our [Contributors' Guide](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "dhkim1920 (via GitHub)" <gi...@apache.org>.
dhkim1920 commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4971064589
Thanks for fixing the no-listener case.
I think there may still be a similar transaction issue when any listener plugin is registered.
`ListenerManager.has_listeners` only checks whether any listener plugin exists, not whether an import-error hook is actually implemented. That means `ParseImportError.full_file_path()` is still evaluated while building the hook arguments, even if none of the registered listeners handle import errors.
For bundle types like the Git bundle, calling `full_file_path()` can instantiate the bundle and resolve an Airflow Connection inside the parsing transaction. If that ends up rolling back the session, the pending import-error row could still be lost.
Would it be reasonable to pass the already-known `bundle_path` down the parsing call chain and build the listener filename with `str(bundle_path / relative_fileloc)` instead? `DagFileInfo` and `DagBag` already have that information, so it avoids initializing the bundle again inside the transaction regardless of which listeners are registered.
As a side benefit, the existing-error path would no longer need the extra `select(ParseImportError)` query just to construct the listener filename.
I tried this approach locally with a registered listener for both new and existing import errors, and the listener still received the expected filename while the `ParseImportError` row remained persisted without calling `full_file_path()`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't re-instantiate the DAG bundle inside the parsing transaction when recording import errors [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-5000215975
Thanks @Vamsi-klu for the thorough recheck — all points addressed in 364a7d0:
sync_bag_to_db() now forwards the path: it passes bundle_path=dagbag.bundle_path to update_dag_parsing_results_in_db(), so the CLI / dag.test() path no longer falls back to full_file_path() inside the transaction.
End-to-end regression test: added test_sync_bag_to_db_does_not_call_full_file_path, which builds a real DagBag over a broken DAG file and goes through sync_bag_to_db() (not the private helper). full_file_path is patched with side_effect=AssertionError, so the test fails immediately if any production caller stops forwarding bundle_path.
Existing-error branch coverage: added test_existing_import_error_listener_uses_bundle_path_without_full_file_path, which pre-creates a ParseImportError row and asserts on_existing_dag_import_error receives the bundle_path-based filename without hitting full_file_path() or the extra select.
Belt and braces: while revisiting this I also restored a cheap has_listeners early exit in _update_import_errors. Threading bundle_path covers the known callers, but any caller that doesn't thread it down would still have hit the bundle-instantiating fallback in the no-listener case — the original bug. With the gate, the listener arguments aren't computed at all when no listener plugin is registered (covered by test_import_error_persisted_without_listener_or_bundle_path).
Title / body / AI disclosure: refreshed to describe the current bundle_path implementation, and the disclosure now names the tool.
Ready for CI whenever a maintainer can approve the workflows.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-5000004693
I rechecked the updated bundle-path implementation. The new direction is better, but one production caller still does not forward the path. `sync_bag_to_db()` already has `dagbag.bundle_path` and uses it while constructing `files_parsed`, but its call to `update_dag_parsing_results_in_db()` omits `bundle_path`. That path can therefore still fall back to `ParseImportError.full_file_path()` inside the transaction.
Could you pass `bundle_path=dagbag.bundle_path` there as well and add a regression test through `sync_bag_to_db()` that makes `full_file_path()` fail if called? The current test invokes the private `_update_import_errors()` helper directly with a supplied path, so it stays green if a production caller stops forwarding that path. Coverage for an existing import-error row would also protect the other changed listener branch.
The title and body still describe the superseded `has_listeners` implementation, and the AI disclosure does not name the tool, so those should be refreshed before rereview. Only the WIP and Mergeable gates have run on this head; substantive CI is still needed.
---
Drafted-by: Codex (GPT-5); reviewed by @Vamsi-klu before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4966077308
Had a look through this and the guard reads right to me. `full_file_path()` in `models/errors.py` does `DagBundlesManager().get_bundle(...).path`, so gating both notification calls in `_update_import_errors` behind `get_listener_manager().has_listeners` keeps that bundle instantiation out of the parsing transaction when no listener is registered, which is the common case. Nice that it reuses the same `has_listeners` check already in `dagbag.py` instead of inventing a new signal. The regression test mocking `full_file_path` with `assert_not_called()` and then asserting the `ParseImportError` row still persists lands right on the behavior that was broken, and as a bonus the existing-error branch now skips the extra `select(ParseImportError)` query that only ever existed to feed the listener arg.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4966113355
Thanks for the review!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4975658395
Thanks @dhkim1920, that's a great catch — you're right that `has_listeners` only tells us whether *any* listener plugin is registered, not whether an import-error hook is actually implemented, so `full_file_path()` (and the bundle instantiation it triggers) could still run inside the parsing transaction whenever any listener exists.
I've updated the PR to your suggested approach. The already-known bundle path (`DagFileInfo.bundle_path`) is now threaded down through `persist_parsing_result` → `update_dag_parsing_results_in_db` → `_update_import_errors`, and the listener filename is built with `str(bundle_path / relative_fileloc)`. So the bundle is no longer re-instantiated inside the transaction regardless of which listeners are registered, and — as you noted — the existing-error path no longer needs the extra `select(ParseImportError)` that only existed to feed the listener argument. `full_file_path()` is kept only as a fallback for callers that don't thread the bundle path.
The regression test now registers an import-error listener and asserts that `full_file_path()` is not called while the listener still receives the correct `bundle_path`-based filename and the `ParseImportError` row is persisted.
@Vamsi-klu thanks for the earlier review as well — this approach makes the `has_listeners` guard unnecessary, so I've dropped it in favor of threading `bundle_path`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dag-processing): don't drop import errors when no listener is reg… [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #69845:
URL: https://github.com/apache/airflow/pull/69845#issuecomment-4999930706
I took another pass over the updated bundle-path version. The new direction makes sense, but one production entry point still falls back to the behavior this PR is trying to remove. `sync_bag_to_db()` already has `dagbag.bundle_path` and uses it while building `files_parsed`, but it does not pass that path to `update_dag_parsing_results_in_db()`. Dag reserialization and the CLI Dag-loading path can therefore still reach `full_file_path()` inside the transaction.
Could you pass `bundle_path=dagbag.bundle_path` there as well and add a regression test through `sync_bag_to_db` that asserts `full_file_path()` is not called? Coverage for an existing error with a supplied path would also be useful; that branch looks correct by inspection, but the current test covers only a new error and bypasses the production call chain.
The PR title and body also need a refresh before rereview: the title is truncated and uses a `fix(...)` prefix, while the body still describes the superseded `has_listeners` implementation and the AI disclosure does not name the tool.
---
Drafted-by: Codex (GPT-5); reviewed by @Vamsi-klu before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org