vatsrahul1001 commented on code in PR #74861:
URL: https://github.com/apache/airflow/pull/74861#discussion_r4296817406
##########
airflow-core/src/airflow/utils/sqlalchemy.py:
##########
@@ -693,7 +693,12 @@ class CommitProhibitorGuard:
def __init__(self, session: Session):
self.session = session
- def _validate_commit(self, _):
+ def _validate_commit(self, session):
+ # before_commit fires for a SAVEPOINT release too, not just a real
commit. Allow it
+ # through: a savepoint release ends neither the outer transaction nor
its row locks, so
+ # it can't break the HA locks this guard protects.
+ if session.in_nested_transaction():
Review Comment:
Good catch -- narrowed the comment to say only session.commit() and
savepoint exits are covered, per your edge case.
Re: the connection-level commit/savepoint/release_savepoint events -- that'd
remove the ambiguity entirely, but I kept this to the Session-level
before_commit check for now since it's the minimal change for the actual bug
(DynamicRegion's savepoint release), and switching event layers would need the
guard to track the right connection across the session's lifetime rather than
just listening on the session object. Happy to take it to the connection-level
events as a follow-up if you'd rather land that instead of the narrowed comment
-- your call.
##########
airflow-core/tests/unit/utils/test_sqlalchemy.py:
##########
@@ -219,6 +219,20 @@ def test_prohibit_commit_specific_session_only(self):
other_session.execute(text("SELECT 1"))
other_session.commit()
+ def test_prohibit_commit_allows_savepoint_release(self):
Review Comment:
Added a scheduler-level test
(test_do_scheduling_creates_task_instances_for_scheduler_created_mapped_run)
that goes through the real _do_scheduling, not a session cast as the guard.
Confirmed it reproduces the bug: reverting just the sqlalchemy.py fix makes it
fail with the exact production traceback (UNEXPECTED COMMIT, same
DynamicRegion.get_or_create_many() -> begin_nested() path) and assert [] == [0,
1] -- zero TaskInstances created. Passes with the fix restored.
--
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]
