Skip to content

Pass the scheduler's session to set_state when processing executor events - #73213

Closed
namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-executor-events-session
Closed

namanjain24-sudo wants to merge 1 commit into
apache:mainfrom
namanjain24-sudo:fix-executor-events-session

Conversation

@namanjain24-sudo

Copy link
Copy Markdown
Contributor

SchedulerJobRunner.process_executor_events calls ti.set_state() without session in two places: when a cleared (RESTARTING) task instance is reported as successfully terminated, and when the Dag of a finished task instance cannot be loaded. TaskInstance.set_state is @provide_session and settings.Session is scoped, so create_session() returns the scheduler's own session and commits and closes it on exit. This is the same mechanism as #67850 and #71968.

process_executor_events runs inside with create_session() in _run_scheduler_loop, not under prohibit_commit, so nothing raises. Instead, in the middle of an executor-event batch:

  • the scheduler's transaction is committed early, which releases the FOR UPDATE SKIP LOCKED row locks taken on the batch's task instances;
  • close() detaches the task instances loaded for the batch, so changes made to them afterwards are never written. In a local run with one RESTARTING → SUCCESS event and four QUEUED events carrying an external executor id, none of the four external_executor_id values reached the database without this change, and all four did with it (SQLite and PostgreSQL 16).

This passes the scheduler's session at both call sites, as _enqueue_task_instances_with_queued_state already does for its own ti.set_state() call.

Tests:

  • test_process_executor_events_sets_state_in_callers_transaction covers both paths: the state change has to roll back with the caller's transaction. On main both cases fail (the task instance is already committed as None / failed); with this change they pass.
  • The rest of test_scheduler_job.py and prek, including mypy for airflow-core, pass locally on SQLite and PostgreSQL 16.

Not covered here. In the same loop, executor.send_callback() reaches DatabaseCallbackSink.send, which is also @provide_session and gets no session. So when the task instance has an on_failure_callback / on_retry_callback (reproduced locally with this change applied) or email configured, the scheduler's transaction is still committed at that call. _maybe_requeue_stuck_ti and _purge_task_instances_without_heartbeats call send_callback() the same way. Fixing that means either adding a session argument to BaseExecutor.send_callback and BaseCallbackSink.send, which executors that override send_callback would then have to accept, or handing the session to the sink directly, as Trigger already does with DatabaseCallbackSink().send(callback=request, session=session). I kept this PR to the set_state calls and am happy to follow up with whichever approach you prefer.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: a Gen-AI coding assistant, following the guidelines. I reviewed the change and ran the checks above.

@namanjain24-sudo
namanjain24-sudo force-pushed the fix-executor-events-session branch from c79adcb to 58d3e70 Compare September 16, 2026 07:18
@namanjain24-sudo
namanjain24-sudo marked this pull request as ready for review September 16, 2026 07:18
…ents

process_executor_events called ti.set_state() without a session when a
cleared task instance is reported terminated and 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.
@namanjain24-sudo
namanjain24-sudo force-pushed the fix-executor-events-session branch from 58d3e70 to b9d0f3f Compare September 21, 2026 13:07
@potiuk potiuk added the closed because of open PR limit Closed as a one-time step of introducing the open pull request limit label Sep 25, 2026
@potiuk

potiuk commented Sep 25, 2026

Copy link
Copy Markdown
Member

Hello @namanjain24-sudo - thank you for your contributions to Apache Airflow!

The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 7 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:

These pull requests stay open because maintainers are already engaged in them - they count towards your limit:

This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are.

What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or gh pr reopen <PR_NUMBER> --repo apache/airflow. Reopen the ones you are ready to follow through - keep them rebased, respond to review comments and fix failing checks.

While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack.

Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@namanjain24-sudo

Copy link
Copy Markdown
Contributor Author

Superseded by #73810 — this PR kept failing to reopen (both gh pr reopen and the UI button errored with a generic "Could not open the pull request"), so per @potiuk's guidance on Slack I pushed/opened fresh from the same branch instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scheduler closed because of open PR limit Closed as a one-time step of introducing the open pull request limit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants