fix(sort)!: enforce --max-temp-files on the arena spill path - #993
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters (1)
⚙️ Run configurationConfiguration used: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe sort pipeline now consolidates spill runs when the configured live-run limit is reached. It carries key-kind metadata through spill events, reports consolidation and merge-source statistics, and includes tests for bounded runs and unchanged sorted output. ChangesSort spill consolidation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SpillGather
participant SpillWrite
participant RunMergerDyn
participant SortMerge
SpillGather->>SpillWrite: Send spill blocks with key kind
SpillWrite->>RunMergerDyn: Consolidate selected runs
RunMergerDyn-->>SpillWrite: Return merge progress
SpillWrite->>SortMerge: Announce surviving runs
Suggested labels: Merge Risk: ⚪ Minimal · up to The reported temporary-file durability concern does not block merging. Normal checks can proceed. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #993 +/- ##
==========================================
+ Coverage 96.26% 96.27% +0.01%
==========================================
Files 294 296 +2
Lines 148266 148827 +561
==========================================
+ Hits 142725 143283 +558
- Misses 5541 5544 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Since the sort command moved onto the chain builder and the legacy engine was removed from it, --max-temp-files was resolved and logged but never enforced: SpillWrite opened a merge slot for every spilled run as it closed, so a sort held one descriptor and a slot's read-ahead per run until the end and merged all runs in one pass. A sort spilling more runs than `ulimit -n` failed with EMFILE (#991). - SpillWrite keeps closed runs as paths and opens slots only at AllAnnounced, after the run count is bounded. It fails closed if input drains with runs never announced, or if anything arrives after AllAnnounced, so a truncated upstream cannot finish "successfully" without those runs' records. - When the live-run count reaches the limit, a contiguous window of runs is merged into one, chosen to minimise bytes rewritten per slot freed. Nothing is merged below the limit, and one pass just past it (for L <= 64) matches RawExternalSorter's single L/2-wide merge. - The merge (fgumi_sort::run_consolidate) uses the arena spill kernels and shares the final merge's record-framing helpers. It runs cooperatively, bounded per try_run by both records and bytes, so the detached writer never blocks and the stall monitor keeps seeing progress even on long reads. - Spill blocks carry their SpillKeyKind, since the template-coordinate key width is only chosen at runtime. - The sort summary reports consolidations and merge sources beside the runs written; SortStats::runs_written keeps meaning runs written. The phase-timing roll-up reports consolidation time and no longer labels merge read-ahead as consolidation. Merging only contiguous runs, placed at their range's position, keeps the final merge's tie-break order, so output is byte-identical to an unbounded sort: tested across all four orders, limits 2/4/8, 1 and 4 threads, every template-coordinate key lane, unmapped records, and a fused runall sort -> group chain. A sort spilling several times more runs than `ulimit -n` now completes with identical output. BREAKING CHANGE: fgumi-pipeline-io's SpillBlockEvent::Block gains a required key_kind field, and SpillWrite now emits SpillReady for its surviving runs at AllAnnounced (with slot_count set to their number) instead of as each run closes. RunMergerDyn::step takes a byte budget.
b524294 to
c41ae4f
Compare
|
@coderabbitai review |
|
This branch was successfully deployed
1 participant
Fixes #991.
What
Since
fgumi sortmoved onto the chain builder,--max-temp-fileswas resolved and logged but never enforced.SpillWriteopened a merge slot (an open file plus read-ahead) for every spilled run as it closed, and the final merge fanned in over all of them. A sort that spilled more runs thanulimit -nfailed withToo many open files (os error 24).This PR restores the bound on the arena spill path:
SpillWriterecords closed runs as paths and opens their slots only atAllAnnounced, once the run count is bounded. The merge cannot start earlier anyway. The trade-off is that the merge starts on cold slots, without the per-run prefetch that used to happen during spilling.RunStackmerges the contiguous window of 2..=f runs (f = clamp(L/2, 2, 32)) that rewrites the fewest bytes per slot freed, and repeats until the count is back under the limit. Nothing is merged below the limit. For L <= 64, one pass just past the limit is the same single L/2-wide mergeRawExternalSorterdoes. When the limit is hit repeatedly, it rewrites several times less than the oldest-half policy (simulated: 0.95x vs 10.3x the input at L=64 with 700 runs).fgumi_sort::run_consolidateis built on the arena spill kernels and shares the final merge's record-framing helpers.SpillWritedrives it a bounded batch pertry_run, capped by both records and bytes, so the detached writer never blocks and the stall monitor keeps seeing progress. While a merge is in flight, no input is taken, so upstream back-pressures.SpillBlockEvent::Blockcarries aSpillKeyKind, because the template-coordinate key width is only chosen at runtime.SpillWriteerrors if input drains with runs never announced, or if anything arrives afterAllAnnounced.Consolidations: N (Xs)andMerge sources: MbesideSpill runs:, and each merge logsConsolidating N spill runs (...). The phase-timing roll-up has a real consolidation bucket and no longer labels merge read-ahead as consolidation.SortStats::runs_writtenstill means runs written.Correctness
Only contiguous runs are merged, and the merged run takes the range's lowest
file_id, so the final merge's tie-break (run order) is unchanged and output is byte-identical to an unbounded sort. New tests compare decompressed records,@PGaside, against an unbounded reference across:--key-types none|cb|mi|full);runallsort -> group chain, whereSpillWriteis pool-scheduled.Unit tests cover the merge kernel (stable tie order, both codecs, truncation, the byte budget), the policy (live count < L after every push, run order preserved, no merge below the limit, legacy-equivalent single pass, rewrite volume vs the legacy policy), and the new fail-closed paths.
The issue's failure mode is pinned directly. Under
ulimit -n 32, a sort that spills more than twice as many runs as the limit now completes with identical output. Before the fix, the same sort under a lowulimit -nfailed with EMFILE inSpillWrite.Breaking
In
fgumi-pipeline-io:SpillBlockEvent::Blockgains a requiredkey_kindfield.SpillWriteemitsSpillReadyfor its surviving runs atAllAnnounced(withslot_countset to their number) rather than as each run closes.RunMergerDyn::steptakes a byte budget.Risk
unsafe: none added.--max-memory.SpillWrite. Sorts that consolidate repeatedly will spend time there; measuring it on the spill-consolidation benchmarks is the next step.Risk verdict: Output changes: none intended; integration tests compare bounded and unbounded sort records, including tied-key order. Unsafe: none added; no CLAUDE.md allowlist update is indicated. Memory/backpressure: changed; the live-run cap bounds merge fan-in, and consolidation runs in bounded cooperative steps.
Fix: Enforce
--max-temp-fileson the arena spill path by consolidating adjacent runs before final merge.Spill runs stay closed until announced. Consolidation preserves run order and fails closed on incomplete or late spill input. Sort summaries now report consolidations and merge sources.
The
fgumi-pipeline-ioAPI changes include a requiredkey_kindonSpillBlockEvent::Blockand changedSpillReadytiming.RunMergerDyn::stepnow accepts record and byte budgets.Tests were added for record preservation, consolidation, multiple sort key types, fused
runall, and sorting under a low file-descriptor limit. Test execution results were not provided.