Skip to content

fix(sort): resolve --max-temp-files once and bake it into the chain spec - #891

Merged
nh13 merged 1 commit into
mainfrom
nh/sort-maxtemp-resolve
Sep 1, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/sort-maxtemp-resolve

Conversation

@nh13

@nh13 nh13 commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

What

Resolves fgumi sort --max-temp-files once and bakes the result into the chain spec, instead of resolving Auto twice from two independent RLIMIT_NOFILE snapshots.

Before this change, execute_sort resolved --max-temp-files Auto for the startup banner (self.resolved_max_temp_files(soft_nofile)), while build_sort_chain_spec baked the raw MaxTempFiles::Auto into SortOptions — so the chain builder re-resolved it against a second, independent RLIMIT_NOFILE snapshot. The two snapshots agree in practice (the descriptor limit is stable within a process), but they are logically independent.

build_sort_chain_spec now takes a resolved_max_temp_files: usize and bakes SortOptions.max_temp_files = MaxTempFiles::Fixed(resolved). execute_sort passes the value it already computed for the banner, so the banner and the chain share a single resolution. The chain builder's own Auto branch is unchanged (other callers keep it); sort now always hands it a Fixed value.

The banner's auto → N framing is intentionally unchanged (it still receives the raw self.max_temp_files for framing plus the resolved number).

Correctness

  • Output is unchanged: the cutover parity suite (test_sort_cutover_parity) passes byte-identical (19/19, all four sort orders) against the saved pre-cutover baseline binary, confirming the baked-Fixed value matches what the builder resolved before.
  • A new unit assertion pins the fix: with --max-temp-files auto, the built spec carries MaxTempFiles::Fixed(resolved), not Auto — proving the chain is handed a single resolution.
  • No new unsafe. Full gate green (fmt, clippy -D warnings -W pedantic, doc -D warnings, test suite).

Addresses a CodeRabbit finding raised on #890.

Risk: command output changes: none, pinned by the cutover parity suite; unsafe changes: none, so the CLAUDE.md allowlist is unchanged; memory, queue, thread, and backpressure policy changes: none.

Fix: Resolve Auto once in execute_sort and store the result as MaxTempFiles::Fixed. The startup banner and chain builder now use the same RLIMIT_NOFILE snapshot.

Tests verify the resolved value in the chain specification. Formatting, linting, documentation, and test gates pass.

build_sort_chain_spec previously left SortOptions.max_temp_files as the raw
MaxTempFiles::Auto/Fixed setting, so the chain builder re-resolved it from
its own RLIMIT_NOFILE snapshot independently of the one already taken for
the startup banner. The two snapshots could disagree. Resolve it once in
execute_sort and bake the resolved usize into SortOptions as
MaxTempFiles::Fixed before building the spec, so the banner and the chain
always agree on the same resolution.
@nh13
nh13 deployed to github-actions September 1, 2026 05:30 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: c0edc12f-af1b-4048-a9f3-a404ff6ccc84

📥 Commits

Reviewing files that changed from the base of the PR and between 6b17c3c and c05d2cc.

📒 Files selected for processing (1)
  • src/lib/commands/sort.rs

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


Walkthrough

The sort command now resolves the maximum temporary-file count once and passes it into the chain specification as a fixed value. Tests verify the generated options while preserving the command’s Auto setting.

Changes

Sort temporary-file limit resolution

Layer / File(s) Summary
Apply and verify the resolved limit
src/lib/commands/sort.rs
build_sort_chain_spec stores the resolved limit as MaxTempFiles::Fixed. execute_sort passes the value from the shared RLIMIT_NOFILE snapshot. The wiring test verifies the generated fixed option.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c05d2

The change resolves the temporary-file limit once and uses that fixed value consistently when building the sort chain, preserving output behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested labels: fgumi sort

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required Conventional Commit format, names the affected sort command, and accurately describes resolving --max-temp-files once and storing it in the chain specification.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@nh13

nh13 commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@nh13

nh13 commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.96%. Comparing base (6b17c3c) to head (c05d2cc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   92.96%   92.96%   -0.01%     
==========================================
  Files         299      299              
  Lines      150342   150345       +3     
==========================================
  Hits       139769   139769              
- Misses      10573    10576       +3     

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

@nh13
nh13 added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main with commit ae9e011 Sep 1, 2026
17 checks passed
@nh13
nh13 deleted the nh/sort-maxtemp-resolve branch September 1, 2026 09:04
@nh13 nh13 mentioned this pull request Sep 1, 2026

This branch was successfully deployed

1 active deployment
github-actions — c05d2cc8 Deployed Sep 1, 2026 by nh13 via coverage #4087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant