kaxil commented on code in PR #74861:
URL: https://github.com/apache/airflow/pull/74861#discussion_r4296552098
##########
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:
`in_nested_transaction()` says a savepoint is open, not that the transaction
being committed is the savepoint. The root `SessionTransaction` fires
`before_commit` before it closes its open subtransactions, so
`session.begin_nested(); session.get_transaction().commit()` now returns early
on both events and issues a real COMMIT, where the old guard raised (checked
against the SQLAlchemy in uv.lock). Nothing under `prohibit_commit` commits a
transaction handle today, so it isn't live, but could the comment say only
`session.commit()` and savepoint exits are covered? If you'd rather close it,
the connection-level `commit` event fires on COMMIT and never on RELEASE
SAVEPOINT, so the guard wouldn't need the special case at all.
##########
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:
This pins the guard on its own. Since nothing in the scheduler suite caught
this, could you also add a scheduler test that has `_do_scheduling` (which
installs the real guard) create a run for a scheduled `.expand()` Dag and
asserts the mapped TIs exist? The `_create_dagruns_for_dags` tests pass the
session itself as the guard (`cast("CommitProhibitorGuard", session)`), so they
can't see this. It'd also be worth asserting here that `session.commit()`
inside an open `begin_nested()` still raises, since only SQLAlchemy's event
order makes that true today.
--
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]
