Skip to content

fix: enforce fair memory limits across sibling reservations - #25172

Draft
sunchao wants to merge 1 commit into
apache:dev/chao/codex/pr25172-replay-headroomfrom
sunchao:codex/upstream-consumer-accounting
Draft

sunchao wants to merge 1 commit into
apache:dev/chao/codex/pr25172-replay-headroomfrom
sunchao:codex/upstream-consumer-accounting

Conversation

@sunchao

@sunchao sunchao commented Sep 10, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Fixes FairSpillPool admission for sibling reservations and an already-full pool. Depends on #25383; this draft is based on that PR's branch so its diff contains only accounting enforcement and its tests. Merge the prerequisite first, then rebase and retarget this PR to main.

Rationale for this change

An operator with a 100 MiB allowance can hold 60 MiB in one reservation and 40 MiB in another. Checking an empty sibling alone incorrectly permits further allocation. Similarly, registering another consumer does not create free memory when the pool is already full.

What changes are included in this PR?

FairSpillPool tracks combined usage by consumer ID and admits fallible growth only when both the consumer's fair share and the pool's remaining capacity permit it. Splitting, transferring, and dropping reservations preserve that accounting. The consumer map also supplies the consumer count, avoiding a redundant counter.

The existing permanent-pressure regression now requires a bounded resource error and cleanup once a competing operator takes the remaining memory.

What is the testing strategy for this PR?

Six focused pool regressions cover sibling totals, fair shares, split/take/drop, a newly registered consumer against an exhausted pool, infallible-growth accounting, and integer overflow. The aggregate pressure test checks exhaustion after output has begun, peak reservation bounds, and memory/disk release.

Validated on top of #25383:

  • 35 memory-pool tests, 2,263 physical-plan tests, and 2,281 core/CLI tests passed.
  • The permanent-pressure regression passed with the required resource error, bounded peak reservations, and complete memory/disk cleanup.
  • All 520 SQL logic test files passed.
  • cargo fmt --all, strict all-target/all-feature Clippy, and the full ./dev/rust_lint.sh passed.

Local validation uses Rust 1.98.1 and upstream revision 22651d24 with its unchanged dependency lockfile; newer main's dependency versions are unavailable in the local registry. The prerequisite merges cleanly with current main. After it merges, rebase this accounting change and rerun CI against main.

Are there any user-facing changes?

Requests through try_grow are rejected when combined reservations exceed the consumer allowance or the pool is full. Infallible grow keeps its existing contract. No public API is added.

@github-actions github-actions Bot added core Core DataFusion crate execution Related to the execution crate physical-plan Changes to the physical-plan crate labels Sep 10, 2026
@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.94%. Comparing base (1ff4126) to head (b075256).

Additional details and impacted files
@@                            Coverage Diff                             @@
##           dev/chao/codex/pr25172-replay-headroom   #25172      +/-   ##
==========================================================================
- Coverage                                   82.02%   81.94%   -0.08%     
==========================================================================
  Files                                        1136     1136              
  Lines                                      439539   429614    -9925     
  Branches                                   439539   429614    -9925     
==========================================================================
- Hits                                       360522   352067    -8455     
+ Misses                                      58242    56491    -1751     
- Partials                                    20775    21056     +281     

☔ 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.

@sunchao
sunchao force-pushed the codex/upstream-consumer-accounting branch from 9480bfa to b075256 Compare September 16, 2026 16:16
@sunchao
sunchao changed the base branch from main to dev/chao/codex/pr25172-replay-headroom September 16, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate execution Related to the execution crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants