Fix airflow db clean hang on MySQL when delete fails#66296
Conversation
|
@Vamsi-klu can you close your PR, I have made the fix 4 days ago and waiting for the #PR-66249 approval for merge and close this issue. |
9967327 to
6e2e0bf
Compare
6e2e0bf to
7dec317
Compare
|
@Vamsi-klu A few things need addressing before review — see our Pull Request quality criteria.
No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
89e743d to
35eb8d4
Compare
|
Gentle bump on this PR. CI is green now and the PR is labeled ready for maintainer review. Could a maintainer please take another look when available and let me know if anything else is needed? Thanks. Drafted-by: Codex (GPT-5); reviewed by @Vamsi-klu before posting |
ephraimbuddy
left a comment
There was a problem hiding this comment.
I think this will solve the hang but the cleanup would still raise on every dag with run history since dagversion is fk-pinned to task_instance.
The newsfragments is not necessary.
35eb8d4 to
de47db7
Compare
|
Addressed the review and rebased on main:
On dag_version being FK-pinned by task_instance: this PR only converts the hang into a fast, visible failure — #68339 is the right place for skipping pinned rows. To keep the two PRs composable, the two backend regression tests here now disable row-exclusion config on a copied Drafted with Claude Code; reviewed by @Vamsi-klu before posting |
|
#66249 was closed on 2026-05-07 without being merged, and #66177 is still open, so this PR remains the active fix for the hang. It has since been through maintainer review (see @ephraimbuddy's comments above) and CI is green. Happy to coordinate if you plan to reopen yours, but as it stands there is no overlapping open PR. Drafted with Claude Code; reviewed by @Vamsi-klu before posting |
|
Hi @ephraimbuddy @potiuk , could you take a maintainer pass on this PR when you have a moment? The PR is cleanly rebased, and I finished the merge-conflict cleanup. It now preserves the behavior fix and includes a regression test for the |
de47db7 to
f0eecec
Compare
f0eecec to
7972b0c
Compare
aaron-y-chen
left a comment
There was a problem hiding this comment.
Hi, thanks for the fix! While testing this, I found a small edge case where the cleanup path can still mask the original delete error. Would you mind fixing it in this PR as well? :)
The check-newsfragment-pr-number CI job requires the newsfragment filename to match the PR number, not the linked issue number.
Keeps the hang-regression tests meaningful once dag_version cleanup learns to skip FK-pinned rows (apache#68339).
When _do_delete unwinds from a failed DELETE, dropping the archive table in the finally block could itself raise (e.g. a dead connection on MySQL). Python makes a finally-raised error the top-level exception, so that cleanup error would replace the original delete failure -- and an OperationalError from the drop could then be downgraded to a warning by _suppress_with_logging, silently masking the real IntegrityError. Guard the cleanup so it only propagates on the success path; on the failure path it is logged and the original error survives.
7972b0c to
5a98dd5
Compare
|
Good catch, thanks! Fixed: the Added Drafted-by: Claude Code (Opus 4.8); reviewed by @Vamsi-klu before posting |
ephraimbuddy
left a comment
There was a problem hiding this comment.
We shouldn't have PR numbers on tests doc strings
Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>
|
Hi @ephraimbuddy , Addressed the review comment regarding the removal of PR numbers from test docstrings. The documentation has been updated to be cleaner and maintain standard practices for the codebase. |
The previous run failed because the Mongo provider test fixture could not connect to its temporary mongod container. No source change is needed; this commit retriggers CI so the PR can use a fresh signal.
* Fix airflow db clean hang on MySQL when delete fails (apache#66177) * Rename newsfragment to match PR number The check-newsfragment-pr-number CI job requires the newsfragment filename to match the PR number, not the linked issue number. * Remove newsfragment per review * Suppress rollback errors so the original delete error propagates * Explain why the archive table drop reuses the session connection * Force the FK-failure path in backend regression tests via config copy Keeps the hang-regression tests meaningful once dag_version cleanup learns to skip FK-pinned rows (apache#68339). * Guard archive-table cleanup so it can't mask the original delete error When _do_delete unwinds from a failed DELETE, dropping the archive table in the finally block could itself raise (e.g. a dead connection on MySQL). Python makes a finally-raised error the top-level exception, so that cleanup error would replace the original delete failure -- and an OperationalError from the drop could then be downgraded to a warning by _suppress_with_logging, silently masking the real IntegrityError. Guard the cleanup so it only propagates on the success path; on the failure path it is logged and the original error survives. * Update airflow-core/tests/unit/utils/test_db_cleanup.py Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com> * Retry CI after provider compat startup failure The previous run failed because the Mongo provider test fixture could not connect to its temporary mongod container. No source change is needed; this commit retriggers CI so the PR can use a fresh signal. --------- Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com> Co-authored-by: Ramachandra Nalam <nalamvamsi13@gmail.com>
Fixes a MySQL hang in
airflow db clean --skip-archivewhen a delete fails on a foreign-key constraint.On the failure path,
_do_delete()now rolls back the session before dropping the temporary archive table, and the drop uses the session's active connection instead of checking out a new pooled connection. This prevents metadata-lock contention from leaving the cleanup command stuck indefinitely.Also adds:
Validation:
prek run --from-ref main --stage pre-commitprek run --from-ref main --stage manualairflow-core/tests/unit/utils/test_db_cleanup.pyin Breeze (57 passed, 1 skipped)breeze ci selective-check --commit-ref HEADindicates the shared test utility change expands the CI matrix beyond what was practical to run locallycloses: #66177
Was generative AI tooling used to co-author this PR?
Generated-by: Codex; Claude Code (Fable 5) following the guidelines
Important
🛠️ Maintainer triage note for @Vamsi-klu · by
@potiuk· 2026-07-02 17:46 UTCHelpful heads-up from the maintainers — please address before this PR can be reviewed (see the Pull Request quality criteria):
main. Please rebase onto the latestmainand resolve them.The ball is in your court — you've been assigned to this PR. Fix the above, then mark it Ready for review.
Automated triage — may be imperfect; a maintainer takes the next look.