fix(sort): write SS sub-sort tag as <sort-order>:<sub-sort> (R2-HDR-01) - #514
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Walkthrough
ChangesHeader sub-sort contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Jul 10, 2026
Closed
…/samtools parity
fgbio (`SamOrder.applyTo`: `sortOrder.name() + ":" + ss`) and samtools write the
`SS` header tag as `<sort-order>:<sub-sort>` — e.g. `queryname:natural`,
`unsorted:template-coordinate`. fgumi wrote the bare sub-sort (`natural`,
`template-coordinate`), which diverges from both parity targets: fgbio's
`SamOrder.apply` strips everything up to the first colon
(`s.substring(s.indexOf(':') + 1)`), so a bare `SS:natural` is not re-recognized
as a queryname order. (Template-coordinate happened to round-trip by luck.)
`SortOrder::header_ss_tag` now returns the `<sort-order>:<sub-sort>` form, and
`create_output_header` uses it for both the queryname and template-coordinate
branches. The `<sort-order>` prefix is the value written to the `SO` tag
(`queryname` for queryname sub-sorts, `unsorted` for template-coordinate).
No reader regression: `is_template_coordinate_sorted` already splits on the
first colon and accepts both the prefixed and bare forms (mirroring fgbio), so
fgumi-, fgbio-, and samtools-written headers are all accepted. There is no reader
that inspects the queryname sub-sort value.
R2-HDR-01.
nh13
force-pushed
the
nh/fix-sort-ss-header-prefix
branch
from
July 10, 2026 04:16
63711ff to
6004b3c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
==========================================
- Coverage 91.17% 91.11% -0.06%
==========================================
Files 78 78
Lines 51916 51916
==========================================
- Hits 47333 47303 -30
- Misses 4583 4613 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
added a commit
that referenced
this pull request
Jul 10, 2026
simulate grouped-reads / mapped-reads hand-formatted the `SS` sub-sort tag as a
bare literal. Reuse `SortOrder::TemplateCoordinate.header_{so,go,ss}_tag()` so the
emitted `@HD SS` matches the <sort-order>:<sub-sort> form this PR establishes for
`sort`, keeping every SS-writing site on one canonical mapping. Stacked on #514
(depends on its keys.rs prefix).
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
nh13
added a commit
that referenced
this pull request
Jul 13, 2026
…ORT3-10) The `@HD SS` sub-sort value for a lexicographic queryname sort was written as `lexicographic`, which is not the SAM-spec / samtools-ecosystem spelling (`lexicographical` — the value htsjdk/Picard emit and parsers recognize). The tag is write-only (fgumi determines sort order from `SO`/`GO` and the CLI `--order`, never by parsing an input `SS`), so this is a pure spelling correction with no functional round-trip effect. `header_ss_tag()` now returns `lexicographical`. To avoid a UX papercut — a user seeing `SS:queryname:lexicographical` in output and passing it back — `--order queryname::lexicographical` is accepted as an alias for `queryname::lexicographic`. The CLI vocabulary and the type's `Display` keep the shorter `lexicographic` spelling. Composes with the separate `<SO>:<sub-sort>` prefix fix (SORT3-01, #514/#531 in lib.rs): after both, the header reads `SS:queryname:lexicographical`. Verified: `fgumi sort --order queryname` emits `SS:lexicographical` on this branch.
nh13
added a commit
that referenced
this pull request
Jul 13, 2026
…ORT3-10) The `@HD SS` sub-sort value for a lexicographic queryname sort was written as `lexicographic`, which is not the SAM-spec / samtools-ecosystem spelling (`lexicographical` — the value htsjdk/Picard emit and parsers recognize). The tag is write-only (fgumi determines sort order from `SO`/`GO` and the CLI `--order`, never by parsing an input `SS`), so this is a pure spelling correction with no functional round-trip effect. `header_ss_tag()` now returns `lexicographical`. To avoid a UX papercut — a user seeing `SS:queryname:lexicographical` in output and passing it back — `--order queryname::lexicographical` is accepted as an alias for `queryname::lexicographic`. The CLI vocabulary and the type's `Display` keep the shorter `lexicographic` spelling. Composes with the separate `<SO>:<sub-sort>` prefix fix (SORT3-01, #514/#531 in lib.rs): after both, the header reads `SS:queryname:lexicographical`. Verified: `fgumi sort --order queryname` emits `SS:lexicographical` on this branch.
nh13
added a commit
that referenced
this pull request
Jul 18, 2026
…ORT3-10) The `@HD SS` sub-sort value for a lexicographic queryname sort was written as `lexicographic`, which is not the SAM-spec / samtools-ecosystem spelling (`lexicographical` — the value htsjdk/Picard emit and parsers recognize). The tag is write-only (fgumi determines sort order from `SO`/`GO` and the CLI `--order`, never by parsing an input `SS`), so this is a pure spelling correction with no functional round-trip effect. `header_ss_tag()` now returns `lexicographical`. To avoid a UX papercut — a user seeing `SS:queryname:lexicographical` in output and passing it back — `--order queryname::lexicographical` is accepted as an alias for `queryname::lexicographic`. The CLI vocabulary and the type's `Display` keep the shorter `lexicographic` spelling. Composes with the separate `<SO>:<sub-sort>` prefix fix (SORT3-01, #514/#531 in lib.rs): after both, the header reads `SS:queryname:lexicographical`. Verified: `fgumi sort --order queryname` emits `SS:lexicographical` on this branch.
nh13
added a commit
that referenced
this pull request
Jul 18, 2026
…ORT3-10) (#567) The `@HD SS` sub-sort value for a lexicographic queryname sort was written as `lexicographic`, which is not the SAM-spec / samtools-ecosystem spelling (`lexicographical` — the value htsjdk/Picard emit and parsers recognize). The tag is write-only (fgumi determines sort order from `SO`/`GO` and the CLI `--order`, never by parsing an input `SS`), so this is a pure spelling correction with no functional round-trip effect. `header_ss_tag()` now returns `lexicographical`. To avoid a UX papercut — a user seeing `SS:queryname:lexicographical` in output and passing it back — `--order queryname::lexicographical` is accepted as an alias for `queryname::lexicographic`. The CLI vocabulary and the type's `Display` keep the shorter `lexicographic` spelling. Composes with the separate `<SO>:<sub-sort>` prefix fix (SORT3-01, #514/#531 in lib.rs): after both, the header reads `SS:queryname:lexicographical`. Verified: `fgumi sort --order queryname` emits `SS:lexicographical` on this branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 9, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 10, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 23, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fgbio (
SamOrder.applyTo:sortOrder.name() + ":" + ss) and samtools write theSSheader tag as<sort-order>:<sub-sort>— e.g.queryname:natural,unsorted:template-coordinate. fgumi wrote the bare sub-sort (natural,template-coordinate), diverging from both parity targets: fgbio'sSamOrder.applystrips everything up to the first colon (s.substring(s.indexOf(':') + 1)), so a bareSS:naturalheader is not re-recognized as a queryname order. (Template-coordinate round-tripped only by luck.)This is round-2 audit finding R2-HDR-01.
What changed
SortOrder::header_ss_tagnow returns the<sort-order>:<sub-sort>form (queryname:lexicographic,queryname:natural,unsorted:template-coordinate).create_output_headeruses it for both the queryname and template-coordinate branches (the template-coordinate branch previously hard-coded the bare value).No reader regression
is_template_coordinate_sortedalready splits theSSvalue on the first colon and accepts both the prefixed and bare forms (mirroring fgbio's reader), so headers written by fgumi, fgbio, and samtools are all accepted bygroup/merge. There is no reader that inspects the queryname sub-sort value.Testing
header_ss_tagunit tests (fgumi-sort + sort command) and the external-sort output-header test to the prefixed values.create_output_headertest pinning the end-to-endSO/GO/SStriple for queryname and template-coordinate.ci-fmtandci-lintclean.Summary by CodeRabbit
Bug Fixes
<sort-order>:<sub-sort>values.Tests