Skip to content

Fix LocalExecutor deadlock when a workload put blocks - #75086

Merged
potiuk merged 3 commits into
apache:mainfrom
potiuk:fix-flaky-local-executor-refill
Nov 1, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
potiuk:fix-flaky-local-executor-refill

Conversation

@potiuk

@potiuk potiuk commented Oct 19, 2026 •

Copy link
Copy Markdown
Member

related: #70526, #70545

#70545 drains the result queue once before each activity_queue.put(), but the put itself can still block forever. A worker can write a result between the drain and the put; if the activity pipe is full at that moment the put blocks, and the worker keeps reading workloads and writing results until the result pipe is full too, so each side waits on the other. Linux pipes free space only a whole page at a time, so this shows on loaded Linux CI runners (the new test_process_workloads_progresses_under_concurrent_result_refill hung at its 30s limit in job 117757985706 on an unrelated PR) and not on macOS.

The put now runs on a short-lived helper thread while the main thread keeps reading results until it completes, so the result pipe cannot fill up behind a blocked put. The workload is registered as running before it is sent, so a "running" result read while the put is in flight is not dropped. If the dispatch is interrupted (SIGTERM raises SystemExit), draining continues until the pending put completes before re-raising, so end() does not queue its shutdown messages behind an abandoned writer. If no worker is alive the interruption is re-raised at once, since nothing could take the pending write and end() sends nothing.

A workload is counted and registered as running before its put; if the put itself fails, that bookkeeping is undone and the workload stays queued, as before. While a put is blocked and fewer than parallelism workers are alive (the spawn start method has none before the first batch and starts one per check, or workers died), workers are spawned from the drain loop, since only a live worker can take the pending write. On SIGTERM during a blocked put the executor now waits until a live worker has made room, consistent with end() waiting for running tasks; a second signal still interrupts.

Trade-offs worth a look from executor maintainers:

  • one thread start per dispatched workload; the worst case for dispatching 200 workloads went from about 8 ms to about 50 ms,
  • end() still sends its shutdown messages with a plain blocking put; left as is to keep this change focused,
  • an mp.Queue (background writer thread, risky to fork workers under) or non-blocking pipe writes (multiprocessing internals) were considered and rejected.

This could not be reproduced on macOS, where pipes free space byte by byte; the Linux behaviour was checked against a model of the kernel's pipe rules, where the previous code still deadlocked in 50-97% of interleavings with a lagging worker and the new code in none.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5.5)

Generated-by: Claude Code (Opus 5.5) following the guidelines

@boring-cyborg boring-cyborg Bot added the area:Executors-core LocalExecutor & SequentialExecutor label Oct 19, 2026
@potiuk potiuk changed the title cccccbctlvggkglvgvhbftlrvgvdeugibcrukevhvvbhFix LocalExecutor deadlock when a workload put blocks Fix LocalExecutor deadlock when a workload put blocks Oct 19, 2026
@potiuk potiuk added the backport-to-v3-4-test Backport to v3-4-test label Oct 19, 2026
@potiuk potiuk added this to the Airflow 3.4.0 milestone Oct 19, 2026
@potiuk

potiuk commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

A question for executor maintainers before this leaves draft: this starts a short-lived thread for every workload put, and the edge cases around it (interruption, no live workers, failed puts) needed handling of their own.

An alternative would be one long-running feeder thread per executor that owns all writes to the activity queue: _process_workloads would hand workloads to it through an in-process queue and keep draining results while it is busy. That removes the per-put thread start and the 50 ms polling, and most of the interruption handling moves into one place (stopping the feeder in end()).

The cost is a thread living for the executor's whole lifetime, which needs care around forking workers while it runs. Would the long-running thread be the better approach here, or is the per-put thread acceptable for the volumes LocalExecutor handles?


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

@potiuk
potiuk marked this pull request as draft October 19, 2026 10:49
@potiuk
potiuk marked this pull request as ready for review October 19, 2026 15:00

@o-nikolas o-nikolas left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the per-put thread. A long-lived feeder would be alive whenever a
replacement worker is forked, so every such fork would happen in a
multi-threaded process. With the per-put thread that only overlaps when a put
is blocked and a worker has died. ~50 ms per 200 workloads seems fine for
LocalExecutor volumes.
+1 to @FrankYang0529's suggestion to cover the remaining > 0 branch of
_undo_dispatch; with that, LGTM.


Drafted-by: Kiro (claude-opus-5.5); reviewed by @o-nikolas before posting

Draining the result queue once before each put did not remove the
deadlock reported in apache#70526. A worker can write a result between the
drain and the put; if the activity pipe is full at that point the put
blocks, and the worker keeps writing results until the result pipe is
full too, so each side waits on the other. Linux pipes free space only a
page at a time, which is why the regression test from apache#70545 hangs on
loaded CI runners but not on macOS.

Make the put on a short-lived helper thread and keep reading results on
the main thread until it completes, so the result pipe can never fill up
behind a blocked put. The workload is registered as running before it is
sent, so a "running" result read while the put is in flight is not
dropped.

Generated-by: Claude Opus 5
Registering a workload before its put meant a put that raised left the
key running and counted although nothing was sent; restore the queued
workload and its bookkeeping instead. Counting each workload before its
put keeps a worker from decrementing the outstanding count below zero
when a batch is interrupted part way.

A blocked put can only complete once a live worker reads the activity
pipe, but workers were only spawned or replaced after the batch, so with
the spawn start method (or once every worker had died) a batch larger
than the pipe waited forever. Spawn them from the drain loop whenever
fewer than parallelism are alive: spawn starts one per call, so a single
busy worker would otherwise hold the put until its task finished.

Generated-by: Claude Opus 5
…nning

The test only reached the no-earlier-dispatch path, so the remaining > 0 branch of _undo_dispatch had no coverage.

Generated-by: Claude Opus 5
@potiuk
potiuk force-pushed the fix-flaky-local-executor-refill branch from c1f162d to 1d250da Compare November 1, 2026 11:41
@potiuk
potiuk enabled auto-merge (squash) November 1, 2026 12:20
@potiuk
potiuk merged commit 1d1d17b into apache:main Nov 1, 2026
71 checks passed
@potiuk
potiuk deleted the fix-flaky-local-executor-refill branch November 1, 2026 12:29
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-4-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
✅ v3-4-test PR Link

potiuk added a commit that referenced this pull request Nov 1, 2026
…5086) (#76223)

* Keep draining LocalExecutor results while a workload put is blocked

Draining the result queue once before each put did not remove the
deadlock reported in #70526. A worker can write a result between the
drain and the put; if the activity pipe is full at that point the put
blocks, and the worker keeps writing results until the result pipe is
full too, so each side waits on the other. Linux pipes free space only a
page at a time, which is why the regression test from #70545 hangs on
loaded CI runners but not on macOS.

Make the put on a short-lived helper thread and keep reading results on
the main thread until it completes, so the result pipe can never fill up
behind a blocked put. The workload is registered as running before it is
sent, so a "running" result read while the put is in flight is not
dropped.

Generated-by: Claude Opus 5

* Undo a failed LocalExecutor put and spawn workers while one is blocked

Registering a workload before its put meant a put that raised left the
key running and counted although nothing was sent; restore the queued
workload and its bookkeeping instead. Counting each workload before its
put keeps a worker from decrementing the outstanding count below zero
when a batch is interrupted part way.

A blocked put can only complete once a live worker reads the activity
pipe, but workers were only spawned or replaced after the batch, so with
the spawn start method (or once every worker had died) a batch larger
than the pipe waited forever. Spawn them from the drain loop whenever
fewer than parallelism are alive: spawn starts one per call, so a single
busy worker would otherwise hold the put until its task finished.

Generated-by: Claude Opus 5

* Cover undoing a failed put while an earlier dispatch of the key is running

The test only reached the no-earlier-dispatch path, so the remaining > 0 branch of _undo_dispatch had no coverage.
(cherry picked from commit 1d1d17b)


Generated-by: Claude Opus 5

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
vatsrahul1001 pushed a commit that referenced this pull request Nov 4, 2026
…5086) (#76223)

* Keep draining LocalExecutor results while a workload put is blocked

Draining the result queue once before each put did not remove the
deadlock reported in #70526. A worker can write a result between the
drain and the put; if the activity pipe is full at that point the put
blocks, and the worker keeps writing results until the result pipe is
full too, so each side waits on the other. Linux pipes free space only a
page at a time, which is why the regression test from #70545 hangs on
loaded CI runners but not on macOS.

Make the put on a short-lived helper thread and keep reading results on
the main thread until it completes, so the result pipe can never fill up
behind a blocked put. The workload is registered as running before it is
sent, so a "running" result read while the put is in flight is not
dropped.

Generated-by: Claude Opus 5

* Undo a failed LocalExecutor put and spawn workers while one is blocked

Registering a workload before its put meant a put that raised left the
key running and counted although nothing was sent; restore the queued
workload and its bookkeeping instead. Counting each workload before its
put keeps a worker from decrementing the outstanding count below zero
when a batch is interrupted part way.

A blocked put can only complete once a live worker reads the activity
pipe, but workers were only spawned or replaced after the batch, so with
the spawn start method (or once every worker had died) a batch larger
than the pipe waited forever. Spawn them from the drain loop whenever
fewer than parallelism are alive: spawn starts one per call, so a single
busy worker would otherwise hold the put until its task finished.

Generated-by: Claude Opus 5

* Cover undoing a failed put while an earlier dispatch of the key is running

The test only reached the no-earlier-dispatch path, so the remaining > 0 branch of _undo_dispatch had no coverage.
(cherry picked from commit 1d1d17b)


Generated-by: Claude Opus 5

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Executors-core LocalExecutor & SequentialExecutor backport-to-v3-4-test Backport to v3-4-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants