kaxil commented on code in PR #72054:
URL: https://github.com/apache/airflow/pull/72054#discussion_r4284785180
##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -7650,6 +7650,57 @@ def _running_counts():
assert session.scalar(select(func.count()).select_from(DagRun)) == 6
assert session.scalar(select(func.count()).where(DagRun.dag_id ==
dag1_dag_id)) == 6
+ def
test_backfill_runs_started_when_backfill_completed_despite_paused_flag(self,
dag_maker, session):
+ """
+ A completed backfill leaves is_paused=True (cancel sets both is_paused
and
+ completed_at). Its queued runs -- e.g. one cleared after cancel --
must still
+ be promoted; the stale is_paused flag must not strand them. Regression
test
+ for https://github.com/apache/airflow/issues/69658.
Review Comment:
Can you drop the issue link here? The test docstring should describe the
behavior; the PR already links the issue.
##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -7650,6 +7650,57 @@ def _running_counts():
assert session.scalar(select(func.count()).select_from(DagRun)) == 6
assert session.scalar(select(func.count()).where(DagRun.dag_id ==
dag1_dag_id)) == 6
+ def
test_backfill_runs_started_when_backfill_completed_despite_paused_flag(self,
dag_maker, session):
+ """
+ A completed backfill leaves is_paused=True (cancel sets both is_paused
and
+ completed_at). Its queued runs -- e.g. one cleared after cancel --
must still
+ be promoted; the stale is_paused flag must not strand them. Regression
test
+ for https://github.com/apache/airflow/issues/69658.
+ """
+ dag1_dag_id = "test_dag1"
+ with dag_maker(
+ dag_id=dag1_dag_id,
+ start_date=DEFAULT_DATE,
+ schedule=timedelta(days=1),
+ max_active_runs=1,
+ catchup=True,
+ ):
+ EmptyOperator(task_id="mytask")
+
+ scheduler_job = Job()
+ self.job_runner = SchedulerJobRunner(job=scheduler_job,
executors=[MockExecutor(do_update=False)])
+
+ from_date = pendulum.parse("2021-01-01")
+ to_date = pendulum.parse("2021-01-06")
+ b = _create_backfill(
+ dag_id=dag1_dag_id,
+ from_date=from_date,
+ to_date=to_date,
+ max_active_runs=3,
+ reverse=False,
+ triggering_user_name="test_user",
+ dag_run_conf={},
+ )
+
+ # Simulate a cancelled backfill: paused and terminal.
Review Comment:
Cancel fails every queued run before it sets `completed_at`, so six queued
runs under a paused, completed backfill isn't a state a real cancel leaves
behind. Could this go through `cancel_backfill`, then clear one run and assert
that run starts? That's the path the PR is fixing.
##########
airflow-core/src/airflow/models/dagrun.py:
##########
@@ -849,8 +849,14 @@ def get_queued_dag_runs_to_set_running(cls, session:
Session) -> ScalarResult[Da
# capacity for 1. this could be improved.
coalesce(running_drs.c.num_running, text("0"))
< coalesce(Backfill.max_active_runs, DagModel.max_active_runs),
- # don't set paused dag runs as running
- not_(coalesce(cast("ColumnElement[bool]", Backfill.is_paused),
False)),
+ # don't set paused dag runs as running, but a completed
backfill's
+ # is_paused flag is stale: a run cleared after the backfill was
+ # cancelled (cancel sets is_paused=True and completed_at) must
still
+ # be schedulable, otherwise it is stuck in queued forever.
+ or_(
+ Backfill.completed_at.isnot(None),
Review Comment:
I think this lets a cancelled backfill run in one case. `_create_backfill`
commits the `Backfill` row before it creates the runs, and the runs only commit
when its session closes. If someone cancels in that window, the cancel's update
finds no `BackfillDagRun` rows to fail, and it sets `completed_at` anyway.
Right now the `is_paused` check keeps those runs in queued. With this change
the scheduler would start them, including any already sitting in a DB on
upgrade.
Could `cancel_backfill` return 409 while the backfill has no
`BackfillDagRun` rows yet? `_finalize_draining_dags` already guards the same
window.
##########
airflow-core/newsfragments/72054.bugfix.rst:
##########
@@ -0,0 +1 @@
+Fix DAG runs belonging to a completed backfill getting stuck in ``queued``
forever. Cancelling a backfill sets ``is_paused=True`` and ``completed_at`` on
the backfill record. If one of that backfill's DAG runs was later cleared, it
returned to the ``queued`` state but the scheduler's promotion query skipped
any run whose backfill is paused, ignoring ``completed_at``. Such runs could
never start again. The scheduler now treats ``is_paused`` as meaningful only
while a backfill is still running, so runs tied to an already-completed
backfill are schedulable again and previously stranded runs recover
automatically.
Review Comment:
I'd drop the newsfragment. Routine bug fixes don't need one, and the PR and
commit message already record this.
--
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]
