fix: RepartitionExec deadlock when its spill pool holds two open files by adriangb · Pull Request #24891 · apache/datafusion

adriangb · 2026-09-02T21:28:48Z

Which issue does this PR close?

Closes RepartitionExec deadlocks under a memory limit when its spill pool holds two open files #24883

Rationale for this change

Under a memory limit, a grouped aggregation on two or more partitions can stop and never complete. The query does not return and does not give an error. All worker threads wait. The issue has the full analysis.

In short: when RepartitionExec cannot reserve memory for a batch, it writes the batch to a spill pool and sends a marker. The reader then blocks on the pool until it gets a batch. When two input tasks spill at the same time, the pool has two open files. The reader reads only the oldest file. When that file has no unread batch, the reader waits for it. But the batch that the reader needs is in the newer file. When each channel has data, the gate closes and the writers wait too. Nothing can wake the reader.

What changes are included in this PR?

The PR has three commits, in test-first order.

test: reproduce the RepartitionExec spill pool deadlock with a SQL query. This is the query from the issue as a test in datafusion/core/tests/memory_limit/repartition_mem_limit.rs. It makes 12 attempts and stops each one after 20 seconds. On main, attempt 0 does not complete and the test fails with a message.
test: add a scenario fuzzer for the spill pool contract. This adds a test double that can hold a writer inside its first disk write, delay the reads of a file, or fail them. It adds a test that holds writer 1 while writer 2 pushes a batch, a test that a failed read gives an error to the reader, and spill_pool_scenario_fuzz, which runs random scenarios from a seed and checks the pool contract: after overlapping pushes the reader can read each pushed batch without more writer activity, it does not signal EOF while a writer is alive, it gives exactly the pushed batches (in order for one writer), and it releases the files. The first two tests are red on this commit. The extended CI job runs 1000 seeds.
fix: RepartitionExec deadlock when its spill pool holds two open files. SpillPoolReader now keeps all the files that it received, oldest first, and gives the first available batch. It skips a file that has no unread batch. It waits for the oldest file that has an unread batch when the read of that file is not complete, so one writer keeps FIFO order. The writer side does not change. The tests of the two commits before it become green.

The fuzzer finds each known fault of this pool that a test can reach:

Pool version	Unit tests of that version	Fuzzer
Before #23522 (has #23447)	16 of 16 pass	Seeds 0 and 1: stall with unread batches. Seed 4: stall after the drop of all writers (the #23447 signature).
`main` (has #24883)	pass	Seed 0: stall with 5 of 13 batches unread.
A draft of this fix that skips a file with a read in progress	1 failure	Seed 2: FIFO violation with one writer, `[1, 3, 4, 2, ...]`.
This fix	pass	500 seeds pass.

What is the testing strategy for this PR?

The reproducer makes 12 attempts, each with a 20-second limit. With the fix, 156 runs and 3000 instrumented attempts gave 0 stalls.
The memory limit must be small enough that RepartitionExec spills, and that window is narrow: at 4 MB the inner repartition spills in 10 of 10 runs, and at 6 MB and more it never spills. Thus the test also sums the spill_count metric of the RepartitionExec nodes and fails if no attempt spilled. A later change that stops the query from spilling makes the test say so, instead of passing while it covers nothing.
At 4 MB the greedy pool sometimes refuses an allocation of the final aggregate. That is a correct result of the limit and not a fault, so the test accepts a resources-exhausted error. One attempt in 3000 failed in this way.
The fuzzer runs 50 seeds by default, in approximately three seconds. DATAFUSION_SPILL_POOL_FUZZ_ITERATIONS sets the count, and DATAFUSION_SPILL_POOL_FUZZ_SEED replays one seed.
test_read_error_is_reported_to_the_reader covers the error paths of SpillPoolFile::poll_file, which had no test before.
The 33 memory-limit tests, the 111 spill and repartition unit tests, and the spill pool doctests pass. cargo fmt and cargo clippy --all-targets --all-features -- -D warnings pass on the two changed crates.

I also wrote this reproducer as a sqllogictest file and then removed it. sqllogictest has no time limit, so a return of the deadlock stops the file until the CI job reaches its time limit, instead of a quick failure with a message. It also cannot say "these rows, or this resource error", so the file failed 1 run in 400 (3200 queries) on the legitimate resources-exhausted error. The limit cannot be relaxed to avoid this, because at 6 MB and more nothing spills, and a smaller count of distinct values also stops the spill (with 97 distinct values 7 of 10 runs do not spill, and with 53 none do).

I also evaluated a second fix that keeps one open write file for each pool. It passes the same tests. It serializes writers that spill at the same time on the file lock, which #23522 avoided on purpose. It is not part of this PR.

Are there any user-facing changes?

No.

The branch-54 line has the same fault, because #23654 backported the same file model. A backport of this fix is necessary.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RauD5PqPCp4y92RUZUD6C9

adriangb

Small notes

Copilot

🔵 Needs a closer look

Spill coordination is concurrency-sensitive, and the seed replay overflow remains unresolved.

Pull request overview

Fixes a RepartitionExec deadlock caused by concurrent spill files under memory pressure.

Changes:

Reads available batches across active spill files.
Adds SQL, regression, and fuzz coverage.
Expands extended-CI fuzzing to 1,000 scenarios.

File summaries

File	Review
`datafusion/sqllogictest/test_files/repartition_memory_spill.slt`	Adds SQL-level spill and result validation.
`datafusion/physical-plan/src/spill/spill_pool.rs`	Fixes multi-file coordination and adds concurrency tests. Moderate (1 vote): seed-range addition can overflow for accepted `u64` replay values.
`datafusion/core/tests/memory_limit/repartition_mem_limit.rs`	Adds a timeout-based regression test.
`.github/workflows/extended.yml`	Runs 1,000 spill-pool fuzz scenarios.

Review details

Files reviewed: 4/4 changed files
Comments generated: 1
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

codecov-commenter · 2026-09-02T21:56:30Z

Codecov Report

❌ Patch coverage is 88.59416% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.60%. Comparing base (da89c7c) to head (b7a9555).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/physical-plan/src/spill/spill_pool.rs	88.59%	25 Missing and 18 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #24891     +/-   ##
=========================================
  Coverage   81.60%   81.60%             
=========================================
  Files        1123     1123             
  Lines      408898   411514   +2616     
  Branches   408898   411514   +2616     
=========================================
+ Hits       333670   335806   +2136     
- Misses      55625    55928    +303     
- Partials    19603    19780    +177

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Three changes from the review of apache#24891. `spill_pool_scenario_fuzz` added the iteration count to the first seed. That addition can overflow when a person replays a seed near the maximum of a `u64`. It now uses `saturating_add`. The SLT file asserted the spill with `<slt:ignore>` markers around the metrics of `RepartitionExec`. The matcher splits the expected text on the marker and then finds each fragment in order in the full snapshot, not in one line. The assertion thus did not say that the spilled bytes of that operator are in KB, only that the text `KB,` comes after the text `spilled_bytes=` somewhere in the plan. It also needs the spilled bytes to stay in the KB range; 25 samples gave 34.9 KB to 67.8 KB, and a change to the batch sizes could move the value to B or MB and fail the test for no good reason. The EXPLAIN ANALYZE block is gone, and the Rust test now sums the `spill_count` metric of the `RepartitionExec` nodes and fails if no attempt spilled. That is the property the block tried to assert. The read hook of the test double can now fail a read, and the new test `test_read_error_is_reported_to_the_reader` makes sure that a spill file that cannot be read gives an error to the reader instead of stopping it. `RepartitionExec` waits on this stream after each spilled marker, so an error that gets lost here stops the query in the same way as the deadlock that this module guards against. The error arms of `SpillPoolFile::poll_file` had no test before this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RauD5PqPCp4y92RUZUD6C9

Copilot

🔵 Needs a closer look

A moderate fuzz-seed iteration issue remains unresolved, alongside a test-coverage nit.

Review details

Suppressed comments (1)

datafusion/physical-plan/src/spill/spill_pool.rs:1750

The injected failure is returned by read_stream() itself, so this test only exercises stream construction at poll_file line 638. It never covers the refactored path where an existing reader stream yields Some(Err(_)) at line 660. Return the injected error as the first stream item instead so the test validates that polling path.

            let delay = (self.read_hook)()?;
            let mut inner = Some(self.inner.read_stream()?);
            Ok(Box::pin(
                futures::stream::once(tokio::time::sleep(delay))
                    .flat_map(move |_| inner.take().expect("polled once")),
            ))

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Balanced

Add the query from issue apache#24883 as a test. It is a grouped COUNT(DISTINCT) on a Utf8View column, with a 4 MB memory limit, two partitions and 64-row batches. Those values send `RepartitionExec` into its spill path. On the current code, attempt 0 does not complete. The test stops each attempt after 20 seconds and fails with a message. It makes 12 attempts, because one attempt that completes is not sufficient for a pass. The memory limit must be small enough that `RepartitionExec` spills, and that window is narrow: at 4 MB the inner repartition spills in 10 of 10 runs, and at 6 MB and more it never spills. Thus the test also sums the `spill_count` metric of the `RepartitionExec` nodes and fails if no attempt spilled. If a later change stops the query from spilling, the test says so instead of passing while it covers nothing. At 4 MB the greedy memory pool sometimes refuses an allocation of the final aggregate. That is a correct result of the limit and not a fault, so the test accepts a resources-exhausted error. In 3000 attempts, one attempt failed in this way. The module is not compiled with `force_hash_collisions`. That feature makes every hash 0, so the hash `RepartitionExec` sends all rows to one output partition. The gate of the distributor channels closes only when each channel holds data, thus the deadlock cannot occur and the test cannot show it. The feature also makes the group-by hash table degenerate, which made one attempt of this query approximately 36 times slower and thus longer than the limit above. A sqllogictest file is not suitable for this test. sqllogictest has no time limit, so a return of the deadlock stops the file until the CI job reaches its time limit, instead of a quick failure with a message. It also cannot say "these rows, or this resource error", so it would fail approximately 1 CI run in 400. Ref: apache#24883 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RauD5PqPCp4y92RUZUD6C9

The spill pool had three faults since its unbuffered reads: apache#20683, apache#23447 and apache#24883. In each fault, two writers were in `push_batch` at the same time. No unit test could cause that overlap. This commit adds a test double and three tests. `IoHookFactory` is a `TempFileFactory` that runs a hook before each disk write and consults a second hook before each read. A writer writes while it holds the lock of its file. Thus a hook that blocks can hold a writer at the point where it has a file checked out, a read delay makes the bytes of an older file arrive after those of a newer file, and a read hook can also fail a read. `test_reader_does_not_wait_on_drained_file_while_another_has_data` holds writer 1 inside its first write while writer 2 pushes a batch. The reader must then give both batches while both writers are alive. `test_read_errors_are_reported_to_the_reader` fails the read of a spill file in the two possible ways: the file does not open, and the file opens and then gives an error as the first item of its stream. Those are the two error paths of `SpillPoolFile::poll_file`, and the reader must give the error in both cases. `RepartitionExec` waits on this stream after each spilled marker, so an error that gets lost here stops the query in the same way as a deadlock. `spill_pool_scenario_fuzz` runs random scenarios from a seed: one to four writers, file rotation after each batch, after some batches or never, and one to six phases. In each phase a random set of writers pushes one to three batches each. A pauser holds each writer of the phase inside its first write, so the pushes overlap. Then the reader reads a random number of the available batches, which can leave a backlog for a later phase. Some phases drop a writer. Each scenario checks the contract that `RepartitionExec` depends on: - When no push is in progress, the reader can read each pushed batch without more writer activity. - The reader does not signal EOF while a writer is alive. - After the last writer is dropped, the reader gives the backlog and then EOF. - The reader gives exactly the pushed batches. With one writer, it gives them in push order. - Disk usage is zero after the reader is dropped. The first two tests are red on this commit. The fuzzer also fails on these versions of the pool: - The code before apache#23522. Seed 4 shows the apache#23447 stall after the drop of all writers, and seeds 0 and 1 show the apache#24883 stall. All 16 pool tests of that time pass on that code. - A draft of the fix in the next commit that skips a file with a read in progress. Seed 2 shows a FIFO violation with one writer. The default of 50 seeds takes approximately three seconds. The extended CI job runs 1000 seeds. `DATAFUSION_SPILL_POOL_FUZZ_SEED` replays one seed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RauD5PqPCp4y92RUZUD6C9

In non-preserve-order mode, all input tasks of a `RepartitionExec` share one spill pool for each output partition. When two input tasks spilled at the same time, the pool had two open spill files. The `SpillPoolReader` read only the oldest file. When that file had no unread batch and was not finished, the reader waited for it. But the batch that the reader needed was in the newer file. When each distributor channel had data, the gate closed. Then both input tasks stopped in `send`, no sink was dropped, and nothing could wake the reader. The reader now keeps all the files that it received, oldest first. It gives the first batch that is available in one of them. It skips a file that has no unread batch. It waits for the oldest file that has an unread batch when the read of that file is not complete. Thus one writer gets its batches in FIFO order. This commit makes the tests of the two commits before it green. The fuzzer passes 500 seeds. Ref: apache#24883 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RauD5PqPCp4y92RUZUD6C9

adriangb · 2026-09-03T15:11:54Z

@2010YOUY01 @kosiew would one of you mind reviewing this change? Thanks!

kosiew

@adriangb
Thanks for working on this. The fix looks good to me, and I like the additional direct regression, error, fuzz, and end-to-end coverage. I have one non-blocking suggestion to make the direct concurrency regression test a little more deterministic.

kosiew · 2026-09-04T09:04:20Z

+        });
+        // Let writer 2 either complete (into a second file) or block on writer
+        // 1's file lock, depending on the pool implementation.
+        std::thread::sleep(Duration::from_millis(200));


Could we replace the fixed 200 ms sleep with a synchronization point that confirms writer 2 has acquired or created its file before releasing writer 1? On a heavily loaded runner, writer 2 might not get scheduled until after writer 1 is released. In that case it could reuse writer 1's returned file, leaving the test with only one file. The test could then pass on the old implementation without actually exercising the two-open-file case. This is just test hardening, since the fuzzer gives us additional coverage as well.

Addressed in b7a9555

…test The two-open-file regression test slept 200 ms to let writer 2 push while writer 1 was held inside its first disk write. On a loaded runner writer 2 could be scheduled only after writer 1 was released, and would then reuse writer 1's returned file. The run would pass with a single open file, which says nothing about the case the test is for. The write hook now reports every write other than the held one. A write gets there only once its writer holds a file of its own, so the test waits for that report before it releases writer 1, and then asserts that two files were written. Ref: apache#24891 (comment) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

adriangb · 2026-09-04T15:38:01Z

Thanks for the review @kosiew !

adriangb requested a balanced review from Copilot September 2, 2026 21:29

github-actions Bot added development-process Related to development process of DataFusion core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Sep 2, 2026

Copilot started reviewing on behalf of adriangb September 2, 2026 21:30 View session

adriangb commented Sep 2, 2026

View reviewed changes

Comment thread datafusion/sqllogictest/test_files/repartition_memory_spill.slt Outdated

Comment thread datafusion/sqllogictest/test_files/repartition_memory_spill.slt Outdated

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread datafusion/physical-plan/src/spill/spill_pool.rs Outdated

github-actions Bot removed the sqllogictest SQL Logic Tests (.slt) label Sep 2, 2026

adriangb force-pushed the claude/datafusion-24883-fixes-jeha12 branch from 0776f74 to bd1c0c0 Compare September 2, 2026 22:54

adriangb requested a balanced review from Copilot September 3, 2026 05:23

Copilot started reviewing on behalf of adriangb September 3, 2026 05:23 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Comment thread datafusion/physical-plan/src/spill/spill_pool.rs

adriangb force-pushed the claude/datafusion-24883-fixes-jeha12 branch from bd1c0c0 to eac4063 Compare September 3, 2026 05:41

claude added 3 commits September 3, 2026 15:02

adriangb force-pushed the claude/datafusion-24883-fixes-jeha12 branch from eac4063 to 7b6bc88 Compare September 3, 2026 15:08

adriangb requested review from 2010YOUY01 and kosiew September 3, 2026 15:11

kosiew approved these changes Sep 4, 2026

View reviewed changes

adriangb enabled auto-merge September 4, 2026 15:38

adriangb added this pull request to the merge queue Sep 4, 2026

Merged via the queue into apache:main with commit 09a2aff Sep 4, 2026
41 checks passed

adriangb deleted the claude/datafusion-24883-fixes-jeha12 branch September 4, 2026 16:16

Navigation Menu

Uh oh!

fix: RepartitionExec deadlock when its spill pool holds two open files - #24891

fix: RepartitionExec deadlock when its spill pool holds two open files#24891
adriangb merged 4 commits into
apache:mainfrom
pydantic:claude/datafusion-24883-fixes-jeha12

adriangb commented Sep 2, 2026 •

edited

Loading

Uh oh!

adriangb left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

codecov-commenter commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

adriangb commented Sep 3, 2026

Uh oh!

kosiew left a comment •

edited

Loading

Uh oh!

kosiew Sep 4, 2026

Uh oh!

adriangb Sep 4, 2026

Uh oh!

adriangb commented Sep 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

adriangb commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

codecov-commenter commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Uh oh!

adriangb commented Sep 3, 2026

Uh oh!

kosiew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb commented Sep 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adriangb commented Sep 2, 2026 •

edited

Loading

codecov-commenter commented Sep 2, 2026 •

edited

Loading

kosiew left a comment •

edited

Loading