ferruzzi commented on code in PR #66350:
URL: https://github.com/apache/airflow/pull/66350#discussion_r3994190662
##########
airflow-core/tests/unit/utils/test_db_cleanup.py:
##########
@@ -551,6 +655,78 @@ def
test_dag_version_cleanup_skips_versions_pinned_by_task_instance(self):
assert latest_id in remaining # kept by keep_last
assert orphan_id not in remaining # old and unreferenced -> pruned
+ def test_do_delete_skip_if_referenced_guards_against_race(self):
+ """_do_delete must not issue a DELETE that violates an ON DELETE
RESTRICT FK.
+
+ Simulates a race where a dag_version row passes the SELECT filter (no
TI
+ references it at archive-creation time) but a TI referencing it is
inserted
+ before the DELETE runs. The skip_if_referenced guard on the DELETE
itself
+ must leave the row in place instead of failing with IntegrityError.
+ """
+ from airflow.utils.db import reflect_tables
+
+ base_date = pendulum.DateTime(2020, 1, 1,
tzinfo=pendulum.timezone("UTC"))
+ bundle_name = f"race-test-{uuid4()}"
+ dag_id = f"race_dag_{uuid4()}"
+
+ with create_session() as session:
+ session.add(DagBundleModel(name=bundle_name))
+ session.flush()
+ session.add(DagModel(dag_id=dag_id, bundle_name=bundle_name))
+ session.flush()
+
+ dv = DagVersion(
+ dag_id=dag_id,
+ version_number=1,
+ bundle_name=bundle_name,
+ created_at=base_date,
+ last_updated=base_date,
+ )
+ session.add(dv)
+ session.flush()
+ dv_id = dv.id
+
+ # Manually create an archive table containing this dag_version row,
+ # simulating the CTAS step that ran before the TI was inserted.
+ archive_name = f"{ARCHIVE_TABLE_PREFIX}dag_version__race_test"
+ session.execute(
+ text(f"CREATE TABLE {archive_name} AS SELECT * FROM
dag_version WHERE id = '{dv_id}'")
Review Comment:
FWIW, my Claude agrees with your Claude that `bindparams` was the right
answer here, ~~but suggests `bindparams(dag_id=dag_id)`. :sweat_smile: It's
a bit outside my wheelhouse though, so a human confirmation would be required
before I feel comfortable approving/merging.~~
Edit: Looking with fresh eyes, Claude was wrong with that suggestion. You
got it right. This is why we review Claude comments :D
--
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]
