lohitkolluri commented on code in PR #67900:
URL: https://github.com/apache/airflow/pull/67900#discussion_r3353183297
##########
airflow-core/src/airflow/models/backfill.py:
##########
@@ -653,7 +653,7 @@ def _create_backfill(
triggering_user_name=triggering_user_name,
)
session.add(br)
- session.commit()
+ session.flush()
Review Comment:
Thanks for the suggestion — it's a cleaner approach than the early
`session.commit()`. I've implemented it:
- Added `with_row_locks()` on the DagModel query before the `num_active`
check. On PG/MySQL, `FOR UPDATE` serializes concurrent backfill creation for
the same dag — requests queue up behind the lock, so they can't both see
`num_active=0`. On SQLite it's a no-op (single-writer), so no change in
behavior there.
- Replaced `session.commit()` with `session.flush()` — the flush populates
the auto-generated backfill ID for the `backfill_dag_run` FK, while keeping
everything in one atomic transaction. The outer `create_session()` context
commits everything together, so if DagRun creation fails, the backfill is
rolled back too — no more orphaned backfill.
Re deadlock concern: I traced the call paths inside `_create_backfill`. The
DagModel `FOR UPDATE` is on a single row (`WHERE dag_id = ?`), acquired before
any DagRun-level locks. The only other lock-taking path (`with_row_locks` in
`_create_backfill_dag_run_non_partitioned`) locks DagRun rows, not DagModel
rows, so lock ordering is consistent. No deadlock risk.
Pushed in the latest commit — mind taking another look?
--
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]
