Skip to content

Commit 26f4b79

Browse files
goingforstudying-ctrlgoingforstudying-ctrl
authored andcommitted
Ignore stale executor success when TI is queued after defer
When a task is deferred and later rescheduled into QUEUED state before the scheduler processes a stale SUCCESS event from the executor, the scheduler should ignore the stale success rather than incorrectly marking the task as succeeded. - Add guard in _process_executor_events to check current TI state - Add test for stale success after defer -> queued transition Fixes #67287
1 parent 832ecf8 commit 26f4b79

3 files changed

Lines changed: 66 additions & 3 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Ignore stale executor success when a deferred task is rescheduled into QUEUED before the scheduler processes the stale SUCCESS from the worker. (fixes #67287)

airflow-core/src/airflow/jobs/scheduler_job_runner.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,6 +1433,8 @@ def process_executor_events(
14331433
# or the TI is queued by another job. Either ways we should not fail it.
14341434
# 3) the trigger already put the TI back to scheduled (resume after defer) but the executor success
14351435
# from the worker exit after defer() has not been processed yet - should not fail it.
1436+
# 4) the trigger already put the TI back to queued (resume after defer) but the executor success
1437+
# from the worker exit after defer() has not been processed yet - should not fail it.
14361438

14371439
# All of this could also happen if the state is "running",
14381440
# but that is handled by the scheduler detecting task instances without heartbeats.
@@ -1447,9 +1449,9 @@ def process_executor_events(
14471449
ti.queued_by_job_id != job_id # Another scheduler has queued this task again
14481450
or executor.has_task(ti) # This scheduler has this task already
14491451
or (
1450-
# Resume-after-defer: trigger moved TI to scheduled (next_method set) before we saw the
1451-
# executor success from the defer exit for the same try_number.
1452-
ti.state == TaskInstanceState.SCHEDULED
1452+
# Resume-after-defer: trigger moved TI to scheduled or queued (next_method set)
1453+
# before we saw the executor success from the defer exit for the same try_number.
1454+
ti.state in (TaskInstanceState.SCHEDULED, TaskInstanceState.QUEUED)
14531455
and state == TaskInstanceState.SUCCESS
14541456
and ti.next_method is not None
14551457
)

airflow-core/tests/unit/jobs/test_scheduler_job.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -923,6 +923,66 @@ def test_process_executor_events_stale_success_when_scheduled_after_defer(
923923
tags={"dag_id": dag_id, "task_id": ti1.task_id},
924924
)
925925

926+
@mock.patch("airflow.jobs.scheduler_job_runner.TaskCallbackRequest")
927+
@mock.patch("airflow._shared.observability.metrics.stats._get_backend")
928+
def test_process_executor_events_stale_success_when_queued_after_defer(
929+
self, mock_get_backend, mock_task_callback, dag_maker
930+
):
931+
"""
932+
Trigger moved TI to queued (resume after defer) before executor success from defer exit arrived.
933+
934+
Regression for https://github.com/apache/airflow/issues/67287 — must not treat as state mismatch.
935+
The fix for #66374 (#66431) covered the scheduled-state variant; this covers the queued-state variant.
936+
"""
937+
mock_stats = mock.MagicMock(spec=StatsLogger)
938+
mock_get_backend.return_value = mock_stats
939+
dag_id = "test_process_executor_events_stale_success_queued_after_defer"
940+
task_id_1 = "dummy_task"
941+
942+
session = settings.Session()
943+
with dag_maker(dag_id=dag_id, fileloc="/test_path1/"):
944+
task1 = EmptyOperator(task_id=task_id_1)
945+
ti1 = dag_maker.create_dagrun().get_task_instance(task1.task_id)
946+
947+
executor = MockExecutor(do_update=False)
948+
task_callback = mock.MagicMock()
949+
mock_task_callback.return_value = task_callback
950+
scheduler_job = Job()
951+
session.add(scheduler_job)
952+
session.flush()
953+
self.job_runner = SchedulerJobRunner(scheduler_job, executors=[executor])
954+
955+
ti1.state = State.QUEUED
956+
ti1.next_method = "execute_callback"
957+
ti1.queued_by_job_id = scheduler_job.id
958+
ti1.try_number = 1
959+
session.merge(ti1)
960+
session.commit()
961+
962+
executor.event_buffer[ti1.key] = State.SUCCESS, None
963+
executor.has_task = mock.MagicMock(return_value=False)
964+
mock_stats.incr.reset_mock()
965+
966+
self.job_runner._process_executor_events(executor=executor, session=session)
967+
ti1.refresh_from_db(session=session)
968+
assert ti1.state == State.QUEUED
969+
self.job_runner.executor.callback_sink.send.assert_not_called()
970+
mock_stats.incr.assert_not_called()
971+
972+
# Without next_method, queued + stale success is still a mismatch (e.g. external kill).
973+
ti1.next_method = None
974+
session.merge(ti1)
975+
session.commit()
976+
977+
executor.event_buffer[ti1.key] = State.SUCCESS, None
978+
mock_stats.incr.reset_mock()
979+
980+
self.job_runner._process_executor_events(executor=executor, session=session)
981+
mock_stats.incr.assert_any_call(
982+
"scheduler.tasks.killed_externally",
983+
tags={"dag_id": dag_id, "task_id": ti1.task_id},
984+
)
985+
926986
@mock.patch("airflow.jobs.scheduler_job_runner.TaskCallbackRequest")
927987
@mock.patch("airflow._shared.observability.metrics.stats._get_backend")
928988
def test_process_executor_events_multiple_try_numbers_warns(

0 commit comments

Comments
 (0)