Vamsi-klu commented on code in PR #66296:
URL: https://github.com/apache/airflow/pull/66296#discussion_r3393071399
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -284,12 +284,12 @@ def _do_delete(
session.execute(delete)
session.commit()
- except BaseException as e:
- raise e
+ except BaseException:
+ session.rollback()
+ raise
finally:
if target_table is not None and skip_archive:
- bind = session.get_bind()
- target_table.drop(bind=bind)
+ target_table.drop(bind=session.connection())
Review Comment:
Added a comment explaining it: binding the drop to `session.get_bind()`
checks out a second pooled connection, and on MySQL its `DROP TABLE` blocks
forever on the metadata lock still held by this session's failed transaction —
the #66177 hang. Reusing the session's own connection avoids that.
---
Drafted with Claude Code; reviewed by @Vamsi-klu before posting
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -284,12 +284,12 @@ def _do_delete(
session.execute(delete)
session.commit()
- except BaseException as e:
- raise e
+ except BaseException:
+ session.rollback()
Review Comment:
Done — rollback is wrapped in `with suppress(Exception):` and
`test_do_delete_propagates_original_error_when_rollback_fails` pins that the
original delete error (not the rollback error) propagates. On the residual: if
the connection is genuinely dead the `finally` drop can still raise, but Python
chains the original error as `__context__` so it stays in the traceback; left
unguarded to keep the diff scoped — can add a guard if you prefer.
---
Drafted with Claude Code; 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
