fix(sort): use monotonic counter for chunk file naming during consolidation

[nh13]

Summary

• Chunk files were named using chunk_files.len(), which decreases after consolidation drains entries from the vector via drain(..merge_count). This caused new chunks to collide with existing non-consolidated chunk files, silently overwriting them and losing records during the final merge.
• Replaced chunk_files.len() with a monotonic chunk_counter in all four sort methods: coordinate, coordinate-with-index, queryname, and template-coordinate.
• Added tests that sort with a tiny memory limit (1 KB) and low max_temp_files (4) to force consolidation and verify all records survive.

Test plan

• New unit tests: test_sort_{coordinate,queryname,template_coordinate}_with_consolidation_preserves_all_records
• All 1920 existing tests pass (cargo ci-test)
• cargo ci-fmt clean
• cargo ci-lint clean

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.65%. Comparing base (7bccd98) to head (61d6cde).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   84.29%   85.65%   +1.36%     
==========================================
  Files         128      128              
  Lines       51978    51990      +12     
==========================================
+ Hits        43814    44533     +719     
+ Misses       8164     7457     -707     

☔ View full report in Codecov by Sentry.
📢 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.

[coderabbitai]

Warning

Rate limit exceeded

@nh13 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 12 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4d33d817-03fb-4fd8-8bc3-f078e94101b5

📥 Commits

Reviewing files that changed from the base of the PR and between e60e7d6 and 61d6cde.

📒 Files selected for processing (1)

• src/lib/sort/raw.rs

📝 Walkthrough

Walkthrough

Chunk filename generation in the raw sorter was changed to use a separate monotonically increasing chunk_counter instead of deriving names from chunk_files.len(). Each affected sort phase now holds local chunk_counter state to avoid name collisions when consolidation mutates the chunk list. The sort_coordinate_with_index lint suppression was expanded to include clippy::too_many_lines. A new consolidation-focused test was added that exercises all three SortOrder variants with a tiny memory limit and low max_temp_files, forcing spills and consolidation and asserting total record count and that multiple chunks were produced.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: replacing chunk file naming based on vector length with a monotonic counter to prevent collisions during consolidation.
Description check	✅ Passed	The description directly relates to the changeset, explaining the bug, solution, and testing approach for the chunk file naming fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/nh/chunk-naming-collision

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/sort/raw.rs`:
- Around line 2236-2243: The assertions that compare stats.output_records (which
is derived from input counts) can miss regressions that drop records; update the
tests that currently assert stats.output_records and stats.chunks_written
(references: stats, stats.output_records, stats.chunks_written) to instead open
and count records in the produced BAM file ("output.bam") and assert that the
on-disk record count equals the expected value (expected = (num_pairs * 2) as
u64). Replace the three assertion sites noted (around the current blocks using
stats.output_records) so they validate by reading/parsing output.bam and
counting records rather than relying on stats fields. Ensure the test fails if
the file is missing or the counted records != expected.
- Around line 2207-2309: The coordinate-consolidation test needs to exercise the
index-writing path; update
test_sort_coordinate_with_consolidation_preserves_all_records to accept a
write_index parameter (rstest cases false and true) and pass it into the sorter
builder (call .write_index(write_index) on the RawExternalSorter before .sort).
Keep the other consolidation tests unchanged; ensure the SortOrder::Coordinate
case is run with both write_index = false and write_index = true so the
coordinate index-writing path is covered.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c17bce99-0d28-4c92-8fd8-92b2a6734cf0

📥 Commits

Reviewing files that changed from the base of the PR and between 7bccd98 and 8d932d1.

📒 Files selected for processing (1)

• src/lib/sort/raw.rs

[coderabbitai]

♻️ Duplicate comments (2)

src/lib/sort/raw.rs (2)

2207-2211: ⚠️ Potential issue | 🟡 Minor

Missing coverage for coordinate sort with index.

SortOrder::Coordinate exercises sort_coordinate_optimized, but the write_index=true path (sort_coordinate_with_index) also received the fix. Add a case with write_index: true for coordinate sort.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sort/raw.rs` around lines 2207 - 2211, The coordinate sort path with
index isn't covered: update the parameterization for
test_sort_with_consolidation_preserves_all_records to include a case that
exercises SortOrder::Coordinate with write_index = true so
sort_coordinate_with_index gets tested; specifically add a test case (or modify
the rstest cases) to pass a configuration/flag enabling write_index for the
Coordinate case so the branch in sort_coordinate_optimized that delegates to
sort_coordinate_with_index is executed and validated.

---

2239-2245: ⚠️ Potential issue | 🟠 Major

Assertion won't catch dropped records.

stats.output_records is assigned from stats.total_records (Lines 807, 956, 1085, 1225), not from actual writes. Count records from the output BAM to verify no data loss.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sort/raw.rs` around lines 2239 - 2245, The assertion comparing
stats.output_records to expected is unreliable because stats.output_records is
derived from stats.total_records, not actual writes; update the test to open the
produced BAM output (use the same output filename and your BAM reader used
elsewhere) and count records from the file, then assert that that file-record
count equals expected; keep the existing check that stats.chunks_written >= 4
but replace or augment assert_eq!(stats.output_records, expected, ...) with a
read-and-count step that asserts file_record_count == expected and fail with a
message indicating data loss if they differ (referencing stats, expected,
stats.chunks_written, and stats.output_records to locate the code).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/lib/sort/raw.rs`:
- Around line 2207-2211: The coordinate sort path with index isn't covered:
update the parameterization for
test_sort_with_consolidation_preserves_all_records to include a case that
exercises SortOrder::Coordinate with write_index = true so
sort_coordinate_with_index gets tested; specifically add a test case (or modify
the rstest cases) to pass a configuration/flag enabling write_index for the
Coordinate case so the branch in sort_coordinate_optimized that delegates to
sort_coordinate_with_index is executed and validated.
- Around line 2239-2245: The assertion comparing stats.output_records to
expected is unreliable because stats.output_records is derived from
stats.total_records, not actual writes; update the test to open the produced BAM
output (use the same output filename and your BAM reader used elsewhere) and
count records from the file, then assert that that file-record count equals
expected; keep the existing check that stats.chunks_written >= 4 but replace or
augment assert_eq!(stats.output_records, expected, ...) with a read-and-count
step that asserts file_record_count == expected and fail with a message
indicating data loss if they differ (referencing stats, expected,
stats.chunks_written, and stats.output_records to locate the code).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8be7b0e5-246a-415d-b5d3-b30a30cc8afe

📥 Commits

Reviewing files that changed from the base of the PR and between 8d932d1 and e60e7d6.

📒 Files selected for processing (1)

• src/lib/sort/raw.rs