Skip to content

Make sure stale file handler doesn't crash the DagFileProcessorManager#69324

Merged
dabla merged 5 commits into
apache:mainfrom
dabla:fix/stale-file-handler-dag-processor
Jul 9, 2026
Merged

Make sure stale file handler doesn't crash the DagFileProcessorManager#69324
dabla merged 5 commits into
apache:mainfrom
dabla:fix/stale-file-handler-dag-processor

Conversation

@dabla

@dabla dabla commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

As described in issue #69321, this PR makes sure a stale file handler doesn't crash the DagFileProcessorManager. The OSError should be catched and logged as warning instead.


Was generative AI tooling used to co-author this PR?
  • [ x ] Yes (please specify the tool below)

Claude Opus 4.6


  • Read the 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) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@dabla dabla requested review from amoghrajesh and removed request for ephraimbuddy and jedcunningham July 3, 2026 10:06

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one nit if you want to address it.

Comment thread airflow-core/src/airflow/dag_processing/manager.py Outdated
@dabla dabla requested a review from potiuk July 6, 2026 08:04

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — small, correctly-scoped fix for the stale-NFS-handle crash in #69321. Routing all three logger_filehandle.close() sites through a close() that swallows OSError is the right call, and reaching for LoggingMixin's self.log rather than self.process_log (which writes through the very handle that just went stale) is a nice touch.

A couple of non-blocking nits:

  1. One inline comment on the terminate_orphan_processes test — it mocks the whole processor, so it doesn't actually exercise the OSError swallow (the other two tests do).
  2. The commit subjects use refactor: Conventional-Commit prefixes, which Airflow disallows (check-no-conventional-commit-message). It's a commit-msg-stage hook so it isn't gating CI, and the squash-merge uses the clean PR title — but worth rewording/squashing to keep history tidy.

Optional follow-up (out of scope here): a stale handle can also raise on writes via structlog.BytesLogger, not just on close — worth keeping in mind if the crash recurs from a different frame.


This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.

Comment thread airflow-core/tests/unit/dag_processing/test_manager.py Outdated
@potiuk potiuk force-pushed the fix/stale-file-handler-dag-processor branch from 054ffc0 to e4163a4 Compare July 8, 2026 11:10
@potiuk

potiuk commented Jul 8, 2026

Copy link
Copy Markdown
Member

rebased it as well - fell free to merge with/without nit

@dabla

dabla commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

rebased it as well - fell free to merge with/without nit

Thanks for the review @potiuk will fix nits before merging.

@dabla dabla merged commit d9bfef0 into apache:main Jul 9, 2026
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants