You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kaxil (via GitHub)" <gi...@apache.org> on 2026/09/15 09:50:40 UTC
[PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
kaxil opened a new pull request, #73173:
URL: https://github.com/apache/airflow/pull/73173
Follow-up to #70923, which restored the coverage check for `db clean`. That check compares table *names* against the config, not the columns the config names, so it cannot catch this.
`airflow db clean --dag-ids` or `--exclude-dag-ids` silently purges nothing from three tables. Each names a `dag_id` column it does not have: `task_reschedule.dag_id` was dropped in 3.0.0, `deadline.dag_id` in 3.1.0, and `asset_event` has only ever had `source_dag_id`. The query fails on a missing column, `run_cleanup` suppresses a failing table into a warning, and the command exits 0. An operator who always scopes by Dag has been growing those three tables forever while cleanup reported success.
`deadline` and `task_reschedule` now reach their Dag the way the schema does, through `dag_run` and `task_instance`.
**Why scope them rather than just drop the filter**, which would be the smaller diff: `deadline` is cleaned as a dependent of `dag_run` precisely so its rows are archived before the `ON DELETE CASCADE` removes them. An unscoped pass would purge deadlines belonging to the very Dags whose runs `--exclude-dag-ids` is preserving, turning a silent no-op into silent data loss.
**`--exclude-dag-ids` now also treats a NULL dag id as eligible.** A row attributed to no Dag is not one of the excluded Dags' rows, but `NOT IN` alone yields NULL for it and retains it forever. That is every `job` row, since core never sets `Job.dag_id`, and it becomes reachable for `asset_event` and `log` too. This changes behaviour for those tables, in the direction the flag already documents.
The guard test is the part that stops this recurring. It only ever called `run_cleanup` without Dag filters, and the Dag filter is the only thing that dereferences these column names, so all three configs drifted unnoticed across two releases. It is now parametrized over the filter combinations and fails on each of them.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #73173:
URL: https://github.com/apache/airflow/pull/73173#issuecomment-5706258925
Two PRs were already working on this, both are tagged "closes: #69030", and I've been working with those authors. #66350 has been open since 4 May and has been through several rounds of review. #71732 came in August and was cherry-picked into #66350 to avoid the two conflicting. Same three tables, same cause, same fix.
Your `--exclude-dag-ids` handling is a real catch, and broader than anything either PR had. But that's multiple months of working with a new contributor sidestepped when a search for those table names would have turned up both of those PRs.
Please don't close #66350. It also re-applies `skip_if_referenced` on the DELETE which fixes a race condition where a `task_instance` which points at a `dag_version` might appear between the archive INSERT and the DELETE, and it has a guard asserting every `dag_id_column_name` in `config_dict` is a real column of its table, to catch this class of config drift directly rather than through the filter path. Neither of those is in #73173, so I've asked @jakubmatyszewski to rescope his PR down to those two.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #73173:
URL: https://github.com/apache/airflow/pull/73173#discussion_r4025381256
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -517,7 +583,31 @@ def _build_query(
if dag_ids:
conditions.append(base_table_dag_id_col.in_(dag_ids))
if exclude_dag_ids:
- conditions.append(base_table_dag_id_col.not_in(exclude_dag_ids))
+ # A NULL dag id belongs to no Dag, so it is not one of the excluded Dags' rows and stays
+ # eligible. NOT IN alone would yield NULL for it and silently retain it forever -- which
+ # is every `job` row, since core never sets Job.dag_id.
+ conditions.append(
+ or_(base_table_dag_id_col.is_(None), base_table_dag_id_col.not_in(exclude_dag_ids))
+ )
+ elif (dag_ids or exclude_dag_ids) and dag_id_scope is not None:
+ fk_col = base_table.c[dag_id_scope.fk_column]
+ referenced = table(
+ dag_id_scope.referenced_table,
+ column(dag_id_scope.referenced_pk_column),
+ column(dag_id_scope.referenced_dag_id_column),
+ )
+
+ def _rows_for(target_dag_ids: list[str]):
+ return select(referenced.c[dag_id_scope.referenced_pk_column]).where(
+ referenced.c[dag_id_scope.referenced_dag_id_column].in_(target_dag_ids)
+ )
+
+ if dag_ids:
+ conditions.append(fk_col.in_(_rows_for(dag_ids)))
+ if exclude_dag_ids:
+ # A NULL foreign key belongs to no Dag, so it is not one of the excluded Dags' rows and
+ # stays eligible. Testing it with NOT IN alone would yield NULL and silently retain it.
Review Comment:
Trimmed, the second one now just points at the first, which keeps the `job` detail.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #73173:
URL: https://github.com/apache/airflow/pull/73173#discussion_r4024859157
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -145,6 +171,20 @@ def __post_init__(self):
schema=self.schema_name,
)
+ if self.dag_id_scope is not None:
+ if self.dag_id_column_name is not None:
+ raise ValueError(
+ f"_TableConfig for table {self.table_name!r} sets both dag_id_column_name and "
+ f"dag_id_scope; a table is scoped to a Dag either by its own column or through a "
+ f"foreign key, not both."
+ )
+ if self.dag_id_scope.fk_column not in self.orm_model.c.keys():
+ raise ValueError(
+ f"_TableConfig for table {self.table_name!r} sets dag_id_scope but its "
+ f"fk_column {self.dag_id_scope.fk_column!r} is not one of its columns; "
+ f"add {self.dag_id_scope.fk_column!r} to extra_columns."
+ )
Review Comment:
Should we have tests for `ValueError`?
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -517,7 +583,31 @@ def _build_query(
if dag_ids:
conditions.append(base_table_dag_id_col.in_(dag_ids))
if exclude_dag_ids:
- conditions.append(base_table_dag_id_col.not_in(exclude_dag_ids))
+ # A NULL dag id belongs to no Dag, so it is not one of the excluded Dags' rows and stays
+ # eligible. NOT IN alone would yield NULL for it and silently retain it forever -- which
+ # is every `job` row, since core never sets Job.dag_id.
+ conditions.append(
+ or_(base_table_dag_id_col.is_(None), base_table_dag_id_col.not_in(exclude_dag_ids))
+ )
+ elif (dag_ids or exclude_dag_ids) and dag_id_scope is not None:
+ fk_col = base_table.c[dag_id_scope.fk_column]
+ referenced = table(
+ dag_id_scope.referenced_table,
+ column(dag_id_scope.referenced_pk_column),
+ column(dag_id_scope.referenced_dag_id_column),
+ )
+
+ def _rows_for(target_dag_ids: list[str]):
+ return select(referenced.c[dag_id_scope.referenced_pk_column]).where(
+ referenced.c[dag_id_scope.referenced_dag_id_column].in_(target_dag_ids)
+ )
+
+ if dag_ids:
+ conditions.append(fk_col.in_(_rows_for(dag_ids)))
+ if exclude_dag_ids:
+ # A NULL foreign key belongs to no Dag, so it is not one of the excluded Dags' rows and
+ # stays eligible. Testing it with NOT IN alone would yield NULL and silently retain it.
Review Comment:
Almost same explained on line 586 - 588, one can be trimmed
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #73173:
URL: https://github.com/apache/airflow/pull/73173#discussion_r4025383233
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -145,6 +171,20 @@ def __post_init__(self):
schema=self.schema_name,
)
+ if self.dag_id_scope is not None:
+ if self.dag_id_column_name is not None:
+ raise ValueError(
+ f"_TableConfig for table {self.table_name!r} sets both dag_id_column_name and "
+ f"dag_id_scope; a table is scoped to a Dag either by its own column or through a "
+ f"foreign key, not both."
+ )
+ if self.dag_id_scope.fk_column not in self.orm_model.c.keys():
+ raise ValueError(
+ f"_TableConfig for table {self.table_name!r} sets dag_id_scope but its "
+ f"fk_column {self.dag_id_scope.fk_column!r} is not one of its columns; "
+ f"add {self.dag_id_scope.fk_column!r} to extra_columns."
+ )
Review Comment:
Added two, `test_table_config_rejects_both_dag_id_column_and_scope` and `test_table_config_dag_id_scope_requires_fk_column`. Both fail with DID NOT RAISE if the validation block is removed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #73173:
URL: https://github.com/apache/airflow/pull/73173#issuecomment-5706379488
> Two PRs were already working on this, both are tagged "closes: #69030", and I've been working with those authors. #66350 has been open since 4 May and has been through several rounds of review. #71732 came in August and was cherry-picked into #66350 to avoid the two conflicting. Same three tables, same cause, same fix.
>
> Your `--exclude-dag-ids` handling is a real catch, and broader than anything either PR had. But that's multiple months of working with a new contributor sidestepped when a search for those table names would have turned up both of those PRs.
>
> Please don't close #66350. It also re-applies `skip_if_referenced` on the DELETE which fixes a race condition where a `task_instance` which points at a `dag_version` might appear between the archive INSERT and the DELETE, and it has a guard asserting every `dag_id_column_name` in `config_dict` is a real column of its table, to catch this class of config drift directly rather than through the filter path. Neither of those is in #73173, so I've asked @jakubmatyszewski to rescope his PR down to those two.
Commented on that PR too, thanks Dennis
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix airflow db clean silently skipping three tables when scoped to Dags [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #73173:
URL: https://github.com/apache/airflow/pull/73173
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org