Skip to content

Commit 84fca1f

Browse files
Pass the scheduler's session to set_state when processing executor events
process_executor_events calls ti.set_state() without a session when the Dag of a finished task instance cannot be loaded. set_state is @provide_session and settings.Session is scoped, so create_session() returned the scheduler's own session and committed and closed it on exit, in the middle of the batch. A second call site with the same bug (a cleared task instance reported terminated) was superseded by #73554, which replaced that whole code path with TaskInstance.complete_restart(), already passing the scheduler's session correctly.
1 parent 28e7c89 commit 84fca1f

2 files changed

Lines changed: 34 additions & 1 deletion

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1671,7 +1671,7 @@ def process_executor_events(
16711671
task = dag.get_task(ti.task_id)
16721672
except Exception:
16731673
cls.logger().exception("Marking task instance %s as %s", ti, state)
1674-
ti.set_state(state)
1674+
ti.set_state(state, session=session)
16751675
continue
16761676
ti.task = task
16771677
if task.has_on_retry_callback or task.has_on_failure_callback:

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,39 @@ def test_retired_attempt_events_do_not_modify_replacement(
759759
assert ti.try_number == 2
760760
assert ti.external_executor_id == ("current_worker" if include_current else "replacement")
761761

762+
def test_process_executor_events_sets_state_in_callers_transaction(self, dag_maker):
763+
"""
764+
Setting a task instance's state must not commit the caller's transaction.
765+
766+
``settings.Session`` is scoped, so ``ti.set_state()`` without ``session`` resolved to the
767+
scheduler's own session and ``create_session()`` committed and closed it on exit. That released
768+
the scheduler's row locks mid-batch and detached the task instances still to be processed, so
769+
changes made to them afterwards were never written.
770+
771+
This covers the Dag-not-found path: the Dag can't be loaded, so the task instance is marked
772+
with the executor's reported state directly, without a session it would otherwise resolve to
773+
the scheduler's own scoped session and commit early.
774+
"""
775+
session = settings.Session()
776+
with dag_maker(dag_id="test_executor_events_callers_transaction", fileloc="/test_path1/"):
777+
task1 = EmptyOperator(task_id="test_task", retries=2)
778+
ti1 = dag_maker.create_dagrun().get_task_instance(task1.task_id)
779+
ti1.state = TaskInstanceState.QUEUED
780+
session.merge(ti1)
781+
session.commit()
782+
783+
executor = MockExecutor(do_update=False)
784+
job_runner = SchedulerJobRunner(Job(), executors=[executor])
785+
job_runner.scheduler_dag_bag = mock.MagicMock()
786+
job_runner.scheduler_dag_bag.get_dag_for_run.side_effect = Exception("failed")
787+
executor.event_buffer[ti1.key] = State.FAILED, None
788+
789+
job_runner._process_executor_events(executor=executor, session=session)
790+
session.rollback()
791+
792+
ti1.refresh_from_db(session=session)
793+
assert ti1.state == TaskInstanceState.QUEUED
794+
762795
@mock.patch("airflow.jobs.scheduler_job_runner.TaskCallbackRequest")
763796
@mock.patch("airflow._shared.observability.metrics.stats._get_backend")
764797
def test_process_executor_events_with_no_callback(self, mock_get_backend, mock_task_callback, dag_maker):

0 commit comments

Comments
 (0)