Skip to content

feat(pipeline-io): add the fgumi-pipeline-io crate (P4) - #732

Merged
nh13 merged 3 commits into
main-runallfrom
nh/runall-08-pipeline-io
Aug 9, 2026
Merged

feat(pipeline-io): add the fgumi-pipeline-io crate (P4)#732
nh13 merged 3 commits into
main-runallfrom
nh/runall-08-pipeline-io

Conversation

@nh13

@nh13 nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member

Ports the BAM-pipeline I/O layer of the typed-step pipeline from origin/feat-runall f50df03b: the source, sink, and sort step building blocks plus the record-batch / BGZF-block buffer types they exchange.

Stacked on main-runall (fa866159). Three commits, each self-contained and green on its own:

  1. feat(pipeline-io): add the fgumi-pipeline-io crate — the crate, plus its entry in the publish list.
  2. chore(sort): drop stale allow(dead_code) suppressions — the removals from chore: remove stale #[allow(dead_code)] attributes now that the code is used #562 that were correctly absent while the code was unused.
  3. test(pipeline-io): cover record framing, step wiring, and spill plumbing — coverage work, and the soak-suite gating.

This is machinery only — nothing constructs it yet

fgumi sort still drives the legacy file-to-file sorter. Routing the command through the arena chain is root-crate wiring (#476) and lands in P5/P6. The crate is exercised by its own tests rather than in production, exactly as the phasing implies.

Not byte-identical to feat-runall — read this before porting from that branch again

An earlier revision of this PR claimed the crate was byte-identical to origin/feat-runall except for a few named adaptations. That is no longer true, and the claim has been removed from the commit message as well. A future port from feat-runall should expect conflicts at the sites below.

Toolchain / workspace drift, no behaviour change:

  • ArenaPool::try_acquire already returns the pooled wrapper on this base, so the manual PooledSegmentedBuf::pooled wrap is dropped (feat(sort): add the arena pool and block-offset planner #719's hang guard is why pooled is private here).
  • Three collapsible_if sites rewritten as let-chains rather than suppressed, per the repo's rule about adopting newer-compiler idioms.
  • Three rustdoc intra-doc links fixed. One was a genuine doc inaccuracy: FindBoundariesAndSort's doc described the wrong sort entry point.

Behaviour changes made under review, which feat-runall does not have:

  • TemplateChunks::push no longer panics. It returned unreachable!() on a template lane-variant change. The --key-types variant is global to a sort, so a change means phase 1 and the merge disagree about key width and the merge emits silently mis-ordered output. It now returns io::Result with an InvalidData error naming both variants, threaded through MemoryChunksByKind::push and absorb_phase2_event — the same fail-closed treatment ensure_single_lane and build_driver already had.
  • emptiest_first_order skips drained slots. The registry is append-only, so a slot that reached queue_eof otherwise stays in the refill scan for the rest of the run, taking its FIFO lock on every dispatch only to be rejected. Scheduling-only — an EOF slot cannot progress, so no output changes.
  • ReadBgzfBlocks drops its Arc/atomic. It is a Serial step, so the runtime drives one shared instance and never clones it; the reader and finished flag are plain fields, the misleading "was clone() called?" panic message is gone, and the reader is released at end of stream rather than held for the whole run.
  • The residual-emission rule is extracted from ChunkSorter::take_residual_chunks into a free function so it can be unit-tested without building an accumulator. Same rule, same behaviour.

Scope: fgumi-sort-cli is deliberately excluded

Deferred to P5. It is lib-only and every consumer on feat-runall is root-crate, so after this PR nothing in the workspace would depend on it. fgumi-pipeline-io does not need it, and landing it would have forced widening fgumi_sort::create_output_header to pub ahead of any production caller.

Test coverage

The port arrived at 80% patch coverage with the gap concentrated in the code that decides whether sorted output is byte-identical — boundaries.rs, the record-framing scanner, was at 31.6% of lines and 22% of functions with a single test. It is now at ~99% lines / 100% functions.

Two existing assertions turned out to be weaker than they looked, and both are fixed:

  • synthesize_sized_records built every record with tid = -1, so every coordinate key was RawCoordinateKey::unmapped() (u64::MAX), pos was never compared, and the coordinate parity cases would have passed with a broken key comparison. Records are now mapped across four references, with every eighth left unmapped for tie coverage.
  • The queryname branch only length-checked the RecordBatch output, so a framing divergence between SortMerge<BlockOutput> and SortMerge<RecordBatchOutput> that preserved record count would pass. It now compares both framings as multisets.

The multi-minute soak / matrix / proptest suites move behind the crate's new stress-tests feature, matching the root crate's existing convention: the default test target drops from ~30s to ~2s for this crate, and cargo ci-test-stress still runs them. The coverage job explicitly enables that feature — those suites reach ~200 lines nothing else covers, so measuring without them under-reports coverage for code that is tested, just not on the PR-latency path.

Verification

cargo ci-fmt && cargo ci-lint && cargo ci-tag-literals
RUSTDOCFLAGS="-D warnings" cargo ci-doc
cargo ci-test          # 8127 passed, 30 skipped
cargo ci-test-stress   # 8154 passed, 30 skipped
RUSTFLAGS="--cfg loom" cargo test -p fgumi-sort --test loom_merge_slots --release   # 5/5
./scripts/publish-crates.sh --check

cargo check is not sufficient here — it misses the collapsible_if errors (clippy runs --all-features --all-targets) and every rustdoc error. publish-order is CI-only and is not part of the local cargo ci-* set; #733 adds cargo ci-publish-order so that gap closes.

Related: syncing main-runall with main is a separate change

main-runall is behind origin/main. That sync is deliberately not part of this PR.

From a trial rebase, both conflicts are additive — fgumi-sort/src/lib.rs (mod merge_phases from #706 vs mod merge_slots from #718) and Cargo.lock (rustix from #705 vs smallvec from #720), each resolved by keeping both sides. It also surfaces a latent upstream defect: #706's merge_phases.rs links crate::external::SortPhaseTimer, a private struct in a private module, which does not resolve. It is invisible on main because that ci-doc alias lacks --document-private-items (added by #722, present only here), so the sync will fail ci-doc under -D warnings until it is fixed. Taking that flag on main would stop the class accumulating unseen.

Risk: command output: none (fgumi sort remains unchanged); unsafe: none, with no CLAUDE.md allowlist change; memory bounds and queue/backpressure policy: changed through bounded pipeline settings and stress tests.

  • Added fgumi-pipeline-io with typed BAM source, sink, boundary, buffer, and sort-step building blocks.
  • Added BGZF/raw writers, BAM readers, record batches, decompressed blocks, and boundary validation.
  • Added a complete typed sort pipeline with spill compression, gathering, writing, decompression, and merging.
  • Added safeguards for truncated input, template variants, drained slots, ordering, and backpressure.
  • Added extensive unit, parity, loom, stress, documentation, lint, and publish checks.
  • Added the crate to workspace and publish configuration. Root-crate integration remains deferred.

@nh13
nh13 temporarily deployed to github-actions August 9, 2026 18:00 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8f015ca2-e3f1-4c22-9f93-e72a5b24a77c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR adds the fgumi-pipeline-io workspace crate. It provides BAM input and output, record-boundary handling, sorting, spill processing, merging, pipeline protocols, and validation tests.

Changes

Pipeline contracts and BAM I/O

Layer / File(s) Summary
Pipeline contracts and BAM I/O
Cargo.toml, crates/fgumi-pipeline-io/..., crates/fgumi-pipeline-io/src/source/*
The workspace adds fgumi-pipeline-io. The crate defines pipeline data types, BAM boundary scanning, EOF validation, and BGZF block input helpers.

Sort pipeline

Layer / File(s) Summary
Sort protocol and buffering
crates/fgumi-pipeline-io/src/sort/{protocol.rs,sort_buffer.rs,spill_gather.rs}
Typed sort events, arena-backed buffering, bounded output, and ordered spill framing are added.
Spill processing
crates/fgumi-pipeline-io/src/sort/{compress_spill.rs,spill_block_compress.rs,spill_write.rs}
Sorted chunks are compressed and written into codec-framed temporary spill files.
Spill decompression and merge
crates/fgumi-pipeline-io/src/sort/{spill_decompress.rs,merge.rs}
Spill slots support bounded inline or block-parallel decompression. SortMerge validates events and emits record or framed-block batches.
Pipeline validation
crates/fgumi-pipeline-io/src/sort/tests.rs, crates/fgumi-pipeline-io/src/sort/*/tests.rs
Tests cover parity, codecs, spill regimes, concurrency, ordering, backpressure, malformed protocol states, and failure handling.

File sinks

Layer / File(s) Summary
BAM and raw sinks
crates/fgumi-pipeline-io/src/sink/*
WriteBgzfFile supports eager or deferred headers. WriteRawFile writes block bytes and appends its trailer after clean drain.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested labels: fgumi sort, continuous-integration

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses a valid Conventional Commit format and clearly describes the addition of the fgumi-pipeline-io crate.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.38883% with 283 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main-runall@fa86615). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/fgumi-pipeline-io/src/sort/merge.rs 84.76% 87 Missing ⚠️
crates/fgumi-pipeline-io/src/sort/spill_gather.rs 73.96% 44 Missing ⚠️
crates/fgumi-pipeline-io/src/source/read_bam.rs 85.06% 33 Missing ⚠️
crates/fgumi-pipeline-io/src/sort/spill_write.rs 78.15% 26 Missing ⚠️
crates/fgumi-pipeline-io/src/sink/write_bgzf.rs 89.95% 23 Missing ⚠️
...fgumi-pipeline-io/src/sort/spill_block_compress.rs 71.21% 19 Missing ⚠️
crates/fgumi-pipeline-io/src/sink/write_raw.rs 78.57% 18 Missing ⚠️
...tes/fgumi-pipeline-io/src/sort/spill_decompress.rs 92.30% 18 Missing ⚠️
...rates/fgumi-pipeline-io/src/sort/compress_spill.rs 92.13% 7 Missing ⚠️
crates/fgumi-pipeline-io/src/sort/sort_buffer.rs 98.54% 4 Missing ⚠️
... and 2 more

❌ Your patch check has failed because the patch coverage (89.38%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@              Coverage Diff               @@
##             main-runall     #732   +/-   ##
==============================================
  Coverage               ?   93.91%           
==============================================
  Files                  ?      228           
  Lines                  ?   125670           
  Branches               ?        0           
==============================================
  Hits                   ?   118028           
  Misses                 ?     7642           
  Partials               ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 6959657 to 4f453ab Compare August 9, 2026 18:12
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 18:12 — with GitHub Actions Inactive
@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 4f453ab to 30cebba Compare August 9, 2026 18:17
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 18:17 — with GitHub Actions Inactive
@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fgumi-pipeline-io/src/boundaries.rs`:
- Around line 306-355: Add focused tests for the boundary state machine methods
scan, find_boundaries, and finish, covering cross-call record reassembly with
correct offsets, split-header handling with header_skipped remaining false,
finish returning UnexpectedEof for both 1–3 trailing bytes and truncated record
bodies, and new_no_header accepting a zero-block-size record. Verify complete
successful output and error behavior without testing implementation shape.

In `@crates/fgumi-pipeline-io/src/sort/merge.rs`:
- Around line 226-250: Update TemplateChunks::push to return io::Result<()> and
replace variant-change unreachable! branches with InvalidData errors; propagate
the result through MemoryChunksByKind::push and absorb_phase2_event. In
crates/fgumi-pipeline-io/src/sort/merge/tests.rs lines 1-52, add coverage that
pushes K24 followed by Cb32 and asserts ErrorKind::InvalidData.

In `@crates/fgumi-pipeline-io/src/sort/protocol.rs`:
- Around line 187-217: Add COVERAGE tests for SpillBlockEvent covering Block,
Residual, and AllAnnounced. Assert both the inherent ordinal and
<SpillBlockEvent as Ordered>::ordinal for every variant, and verify heap_size
includes Block byte capacity, Residual chunk heap usage, and the fixed base for
AllAnnounced. This should pin the delegation behavior and queue accounting.

In `@crates/fgumi-pipeline-io/src/sort/sort_buffer.rs`:
- Around line 262-265: Update the documentation for ChunkSorter::from_sorter to
describe its actual constructors and only the u32 reference-count conversion
error it can raise, removing the nonexistent into_*_chunk_sorter and rayon-pool
claims. Correct the finalize documentation so had_spills is described as
selecting template-coordinate residual chunking, consistent with
take_residual_chunks and MemoryChunkErased::TemplateCoordinate.
- Around line 203-232: Add a unit-test module covering take_residual_chunks for
both MemoryChunkErased::TemplateCoordinate and Coordinate chunks, across empty
and non-empty residuals with had_spills true and false. Assert that only an
empty template-coordinate residual with spills is retained, while empty
coordinate residuals are dropped and all non-empty residuals are retained.

In `@crates/fgumi-pipeline-io/src/sort/spill_decompress.rs`:
- Around line 259-278: Update try_fill_some_slot to exclude slots whose
queue_eof is already set before calling emptiest_first_order, so EOF slots are
not cloned, ordered, or locked during refill scans. Preserve the existing
scheduling and refill behavior for non-EOF slots.

In `@crates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rs`:
- Around line 12-20: Add a test alongside
zero_output_byte_limit_normalizes_reorder_window that constructs
SortSpillDecompress with SortDecompressTuning.block_batch set to 0 and asserts
new() clamps step.tuning.block_batch to 1. Also verify a positive block_batch,
such as 8, remains unchanged.

In `@crates/fgumi-pipeline-io/src/sort/spill_gather/tests.rs`:
- Around line 12-19: Add a TemplateCoordinate-specific test alongside the
existing coord_chunk tests, constructing the template chunk and exercising
frame_record_at across the same thresholds. Assert the established packing
invariants: records remain unsplit, each block stays within the size bound, and
concatenated framed blocks reconstruct the original records exactly.

In `@crates/fgumi-pipeline-io/src/sort/spill_write/tests.rs`:
- Around line 98-119: Add tests covering the remaining ensure_no_open_file call
sites in the spill writer tests: verify SpillBlockEvent::AllAnnounced returns an
error containing “still open” when a non-final block leaves the file open, and
verify ensure_no_open_file("input drained") returns the same error for an
unterminated spill. Follow residual_while_file_open_errors’ setup and
assertions.

In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 1191-1231: Gate the long-running soak and proptest suites,
including block_parallel_soak_matrix_matches_legacy and the other
multi-iteration soak tests in this module, behind a dedicated soak feature or
#[ignore]. Keep them runnable in a nightly job while ensuring the default test
target excludes these expensive suites and retains the fast parity tests.
- Around line 1430-1445: Add a test near coordinate_memory_chunk_event named
test_sort_merge_fails_closed_on_shared_memory_chunk_arc that constructs one
MemoryChunkErased inside an Arc, emits a MemoryChunk event using
Arc::clone(&shared), then completes the protocol with AllAnnounced and calls
collect_merge_batches. Assert the merge returns an error whose message contains
“unexpectedly shared,” covering the Arc::try_unwrap failure path.
- Around line 611-625: Update the unstable branch of the sort comparison around
the block_out, legacy_out, and batch_out assertions to compare batch_out with
block_out as multisets, not only by length. Preserve the existing unstable
ordering behavior by sorting cloned outputs with sort_unstable before asserting
equality, while retaining the legacy oracle comparison.
- Around line 1452-1483: Extend
test_sort_merge_does_not_over_reserve_output_buffers to retain the existing
single-chunk fast-path case and add a separate two-memory-chunk case with
memory_chunk_count: 2 that forces build_driver and the SortMergeState::Merging
path. Apply the same emitted-record and total-heap assertions to the merge-path
case so next_batch’s buffer sizing is covered.
- Around line 1246-1253: Update the synthesized records in the coordinate-order
test loop around make_bam_bytes so some records use valid reference IDs,
allowing ref_id and pos to affect ordering, while retaining some ref_id = -1
records to preserve equal-key/unmapped coverage. Ensure the generated records no
longer all share RawCoordinateKey::unmapped().

In `@crates/fgumi-pipeline-io/src/source/read_bam.rs`:
- Around line 253-295: COVERAGE: Replace the direct access to step.reader and
step.next_serial in read_bam_from_reader_round_trips_bytes with a StepCtx-driven
test that invokes ReadBgzfBlocks::try_run. Use a mock context whose output push
is rejected once, then accepted, to exercise the held-slot retry and Contention
outcome; continue running until the reader reaches finished and verify the
subsequent Finished return. Also assert the uncompressed_size overflow guard
using input or context data that exceeds the supported size.
- Around line 34-42: Update ReadBgzfBlocks to remove Arc wrapping from its
reader and finished fields, replacing them with the appropriate plain field
types and adjusting all constructors and accesses accordingly; preserve
synchronization where the Mutex is still required. Replace the misleading “was
clone() called?” panic text with a message describing the actual invalid state
or operation.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 77f8c188-a40c-43b9-bb53-63c5a0e337c2

📥 Commits

Reviewing files that changed from the base of the PR and between fa86615 and 30cebba.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock, !**/*.lock
  • crates/fgumi-pipeline-io/benches/serial_ingest.rs is excluded by !**/benches/**
📒 Files selected for processing (32)
  • Cargo.toml
  • crates/fgumi-bam-io/src/prefetch_reader.rs
  • crates/fgumi-pipeline-io/Cargo.toml
  • crates/fgumi-pipeline-io/src/boundaries.rs
  • crates/fgumi-pipeline-io/src/lib.rs
  • crates/fgumi-pipeline-io/src/sink/mod.rs
  • crates/fgumi-pipeline-io/src/sink/write_bgzf.rs
  • crates/fgumi-pipeline-io/src/sink/write_raw.rs
  • crates/fgumi-pipeline-io/src/sort/arena_ingest.rs
  • crates/fgumi-pipeline-io/src/sort/compress_spill.rs
  • crates/fgumi-pipeline-io/src/sort/compress_spill/tests.rs
  • crates/fgumi-pipeline-io/src/sort/merge.rs
  • crates/fgumi-pipeline-io/src/sort/merge/tests.rs
  • crates/fgumi-pipeline-io/src/sort/mod.rs
  • crates/fgumi-pipeline-io/src/sort/protocol.rs
  • crates/fgumi-pipeline-io/src/sort/sort_buffer.rs
  • crates/fgumi-pipeline-io/src/sort/spill_block_compress.rs
  • crates/fgumi-pipeline-io/src/sort/spill_block_compress/tests.rs
  • crates/fgumi-pipeline-io/src/sort/spill_decompress.rs
  • crates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rs
  • crates/fgumi-pipeline-io/src/sort/spill_gather.rs
  • crates/fgumi-pipeline-io/src/sort/spill_gather/tests.rs
  • crates/fgumi-pipeline-io/src/sort/spill_write.rs
  • crates/fgumi-pipeline-io/src/sort/spill_write/tests.rs
  • crates/fgumi-pipeline-io/src/sort/tests.rs
  • crates/fgumi-pipeline-io/src/source/mod.rs
  • crates/fgumi-pipeline-io/src/source/read_bam.rs
  • crates/fgumi-pipeline-io/src/types.rs
  • crates/fgumi-sort/src/memory_probe.rs
  • crates/fgumi-sort/src/segmented_buf.rs
  • crates/fgumi-sort/src/tmp_dir_alloc.rs
  • scripts/publish-crates.sh
💤 Files with no reviewable changes (4)
  • crates/fgumi-bam-io/src/prefetch_reader.rs
  • crates/fgumi-sort/src/tmp_dir_alloc.rs
  • crates/fgumi-sort/src/segmented_buf.rs
  • crates/fgumi-sort/src/memory_probe.rs

Comment thread crates/fgumi-pipeline-io/src/boundaries.rs
Comment thread crates/fgumi-pipeline-io/src/sort/merge.rs
Comment thread crates/fgumi-pipeline-io/src/sort/protocol.rs
Comment thread crates/fgumi-pipeline-io/src/sort/sort_buffer.rs
Comment thread crates/fgumi-pipeline-io/src/sort/sort_buffer.rs Outdated
Comment thread crates/fgumi-pipeline-io/src/sort/tests.rs
Comment thread crates/fgumi-pipeline-io/src/sort/tests.rs
Comment thread crates/fgumi-pipeline-io/src/sort/tests.rs
Comment thread crates/fgumi-pipeline-io/src/source/read_bam.rs
Comment thread crates/fgumi-pipeline-io/src/source/read_bam.rs Outdated
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 18:45 — with GitHub Actions Inactive
@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 9ed8228 to 96e6835 Compare August 9, 2026 20:19
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 20:19 — with GitHub Actions Inactive
@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fgumi-pipeline-io/src/sort/sort_buffer.rs`:
- Around line 277-282: Update the documentation comments near from_sorter to
remove the nonexistent SortAccum reference and identify ChunkSorter instead.
Revise the # Errors section to state only the u32 reference-sequence-count
conversion as an error source, removing the infallible
TemplateArenaAccumulator::from_header claim.

In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 1395-1421: Update run_merge_over_events to delegate its pipeline
construction and execution to collect_merge_batches instead of duplicating the
Phase2EventSource → SortMerge<RecordBatchOutput> → VecSink wiring and thread
configuration. Preserve run_merge_over_events’ existing Vec<Vec<u8>> result by
flattening the batches returned from collect_merge_batches, and reuse that
helper’s byte-limit handling rather than hardcoding a separate limit.

In `@crates/fgumi-pipeline-io/src/source/read_bam.rs`:
- Line 297: Update the rustdoc comment above the relevant BAM-writing function
to format the identifiers `record_count` and `path` with backticks, while
preserving the existing description and return tuple meaning.
- Around line 336-339: Remove the blocks.sort_by_key call in the test setup
after std::mem::take(&mut *received.lock()) so assertions inspect the queue’s
actual delivery order. Preserve the existing assertions and add or flag missing
COVERAGE for the ordering-regression case as requested.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 03d2f20b-9def-4886-8068-0c9d75038b30

📥 Commits

Reviewing files that changed from the base of the PR and between 9ed8228 and 96e6835.

📒 Files selected for processing (12)
  • .cargo/config.toml
  • crates/fgumi-pipeline-io/Cargo.toml
  • crates/fgumi-pipeline-io/src/sort/merge.rs
  • crates/fgumi-pipeline-io/src/sort/merge/tests.rs
  • crates/fgumi-pipeline-io/src/sort/protocol.rs
  • crates/fgumi-pipeline-io/src/sort/sort_buffer.rs
  • crates/fgumi-pipeline-io/src/sort/spill_decompress.rs
  • crates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rs
  • crates/fgumi-pipeline-io/src/sort/spill_gather/tests.rs
  • crates/fgumi-pipeline-io/src/sort/spill_write/tests.rs
  • crates/fgumi-pipeline-io/src/sort/tests.rs
  • crates/fgumi-pipeline-io/src/source/read_bam.rs

Comment thread crates/fgumi-pipeline-io/src/sort/sort_buffer.rs Outdated
Comment thread crates/fgumi-pipeline-io/src/sort/tests.rs
Comment thread crates/fgumi-pipeline-io/src/source/read_bam.rs Outdated
Comment thread crates/fgumi-pipeline-io/src/source/read_bam.rs Outdated
@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 96e6835 to c877377 Compare August 9, 2026 20:42
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 20:42 — with GitHub Actions Inactive
@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from c877377 to 1a9c3eb Compare August 9, 2026 20:46
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 20:46 — with GitHub Actions Inactive
@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fgumi-pipeline-io/src/sort/sort_buffer.rs`:
- Around line 342-368: Update the sorter ownership flow in ingest_one_batch so a
push error restores the moved sorter to self.sorter before propagating the
error. Ensure try_run cannot interpret the post-error state as finalized or
Finished, while preserving the existing error propagation and retaining buffered
records.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a9524cf8-2e8f-449f-b839-b1e2e1828201

📥 Commits

Reviewing files that changed from the base of the PR and between 96e6835 and 1a9c3eb.

📒 Files selected for processing (4)
  • .github/workflows/check.yml
  • crates/fgumi-pipeline-io/src/sort/sort_buffer.rs
  • crates/fgumi-pipeline-io/src/sort/tests.rs
  • crates/fgumi-pipeline-io/src/source/read_bam.rs

Comment thread crates/fgumi-pipeline-io/src/sort/sort_buffer.rs Outdated
@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 1a9c3eb to 829d5d0 Compare August 9, 2026 22:27
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 22:27 — with GitHub Actions Inactive
@nh13

nh13 commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 1-5: Update the module documentation above the sort tests to
describe the current four-step chain driven by the helpers: SortBuffer →
CompressSpill → SortSpillDecompress → SortMerge. Remove the outdated
SortAndSpill reference while preserving the note about RawExternalSorter::sort
as the parity oracle.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 61869372-84ee-4549-9b21-bf19ac4dae2c

📥 Commits

Reviewing files that changed from the base of the PR and between 1a9c3eb and 829d5d0.

📒 Files selected for processing (2)
  • crates/fgumi-pipeline-io/src/sort/sort_buffer.rs
  • crates/fgumi-pipeline-io/src/sort/tests.rs

Comment thread crates/fgumi-pipeline-io/src/sort/tests.rs Outdated
nh13 added 3 commits August 9, 2026 15:40
Adds the BAM-pipeline I/O layer for the typed-step pipeline: the source,
sink, and sort building blocks plus the record-batch and BGZF-block buffer
types exchanged between steps.

  * source — BAM ingest (ReadBgzfBlocks, read_bam*) turning a reader into
    decompressed blocks.
  * sink   — BAM output (WriteBgzfFile).
  * sort   — the in-pipeline sort steps (SortBuffer, CompressSpill,
    SortSpillDecompress, SortMerge) over the arena ingest, spill-write,
    spill-gather and boundary-finding machinery.
  * types  — RecordBatch, DecompressedBlock, BgzfBlock and friends.

Nothing constructs these steps yet: `fgumi sort` still drives the legacy
file-to-file sorter, and routing the command through the arena chain is
root-crate wiring that lands separately. This commit adds the machinery
only, so the crate is exercised by its own tests rather than in production.

Ported from origin/feat-runall f50df03, but NOT byte-identical to it. The
divergences are listed below so a future port from that branch knows where
to expect conflicts.

Toolchain / workspace drift (no behaviour change):

  * ArenaPool::try_acquire already returns the pooled wrapper on this base,
    so the manual PooledSegmentedBuf::pooled wrap is dropped.
  * Three collapsible_if sites rewritten as let-chains rather than
    suppressed, per the repo's rule about adopting newer-compiler idioms.
  * Three rustdoc intra-doc links fixed. One was a genuine doc inaccuracy:
    FindBoundariesAndSort's doc described the wrong sort entry point.

Behaviour changes made under review, which feat-runall does not have:

  * TemplateChunks::push returned unreachable!() on a template lane-variant
    change. The --key-types variant is global to a sort, so a change means
    phase 1 and the merge disagree about key width and the merge would emit
    silently mis-ordered output. It now returns io::Result with an
    InvalidData error naming both variants, threaded through
    MemoryChunksByKind::push and absorb_phase2_event — the same fail-closed
    treatment ensure_single_lane and build_driver already had.
  * SortSpillDecompress::emptiest_first_order now filters slots that have
    reached queue_eof before reading FIFO lengths. The registry is
    append-only, so a drained slot otherwise stays in the scan for the rest
    of the run, taking its FIFO lock on every dispatch only to be rejected.
    Scheduling-only: an EOF slot cannot progress, so no output changes.
  * ReadBgzfBlocks held its reader and finished flag in an Arc<Mutex<..>>
    and an AtomicBool. It is a Serial step, so the runtime drives one shared
    instance and never clones it; both are now plain fields, the misleading
    "was clone() called?" panic message is gone, and the reader is released
    at end of stream instead of held for the whole run.
  * The residual-emission rule is split out of ChunkSorter::take_residual_chunks
    into a free function so it can be unit-tested without building an
    accumulator. Same rule, same behaviour.

Also registers the crate in scripts/publish-crates.sh, after fgumi-sort and
before fgumi-consensus. The list is topologically ordered and CI's
publish-order job fails when a workspace crate is missing from it; the new
crate depends on fgumi-bam-io, fgumi-bgzf, fgumi-pipeline-core,
fgumi-raw-bam, and fgumi-sort, so that is the earliest valid position.
These six attributes predate the pipeline-io machinery and no longer
suppress anything: clippy over --all-features --all-targets is clean
without them. Removing them restores dead-code coverage for
SegmentedBuf's inherent impl, TmpDirAllocator::with_recheck_interval and
mark_full, PrefetchReader::bytes_consumed and consumer_stalls, and the
memory-debug-gated print_mi_stats.
The ported crate arrived at 80% patch coverage, below the 90% gate, and the
gap was concentrated in the code that decides whether sorted output is
byte-identical.

boundaries.rs was the worst of it: 31.6% of lines and only 22% of functions,
with a single test covering `bam_header_len` and nothing at all for the
scanner that decides where records begin and end. It now sits at ~99% lines
and 100% functions, with cases for header skipping, cross-block record
carryover, trailing partial records, incomplete headers, and every `finish`
rejection path. The existing sequential-assert test was refactored into an
rstest case table so a failure names the scenario.

Also covered: `ReadBgzfBlocks` driven through a real pipeline (its `try_run`
had no coverage — the previous test hand-rolled an equivalent read loop
instead, so it validated `read_raw_blocks` rather than the step, and has been
removed), the SpillWrite / SpillGather / SpillBlockCompress / ReadBlocks /
InflateToArena step wiring, `SpillWrite::open_file`'s refusal to reuse a
file_id, `SpillGather`'s dense ordinal minting and empty-chunk skip, the
TemplateCoordinate framing path (the one variant that does not share
frame_keyed_record_into), `SpillBlockEvent`'s heap_size and its Ordered
delegation (which guards an infinite-recursion trap), the residual-emission
rule including its template-coordinate exception, the block_batch clamp, the
shared-Arc fail-closed guard, and the over-reserve guard on the k-way merge
path rather than only the fast path.

Two fixes to the tests themselves, both of which made existing assertions
weaker than they looked:

  * synthesize_sized_records built every record with tid = -1, so
    extract_coordinate_key_inline returned RawCoordinateKey::unmapped()
    (u64::MAX) for all of them, `pos` was never compared, and the coordinate
    parity cases collapsed into one equal-key bucket — they would have passed
    with a broken key comparison. Records are now mapped across four
    references, with every eighth left unmapped to keep tie coverage.
  * The queryname branch only length-checked the RecordBatch output, so a
    framing divergence between SortMerge<BlockOutput> and
    SortMerge<RecordBatchOutput> that preserved record count would pass. It
    now compares the two framings as multisets.

The multi-minute soak, matrix, and proptest suites move behind the crate's
new `stress-tests` feature, matching the root crate's existing convention:
the default test target drops from ~30s to ~2s for this crate, and
`cargo ci-test-stress` still runs them. The coverage job explicitly enables
that feature — those suites reach ~200 lines nothing else covers, so
measuring without them under-reports coverage for code that is tested, just
not on the PR-latency path.

Where a StepCtx is required the crate's existing convention is followed:
test the StepCtx-free core, or drive the step through Pipeline::builder.
All test data is constructed programmatically; no fixtures are committed.
@nh13
nh13 force-pushed the nh/runall-08-pipeline-io branch from 829d5d0 to efc8406 Compare August 9, 2026 22:40
@nh13
nh13 temporarily deployed to github-actions August 9, 2026 22:40 — with GitHub Actions Inactive
@nh13
nh13 merged commit f42f6cd into main-runall Aug 9, 2026
16 of 17 checks passed
@nh13
nh13 deleted the nh/runall-08-pipeline-io branch August 9, 2026 22:47
nh13 added a commit that referenced this pull request Aug 10, 2026
* feat(pipeline-io): add the fgumi-pipeline-io crate

Adds the BAM-pipeline I/O layer for the typed-step pipeline: the source,
sink, and sort building blocks plus the record-batch and BGZF-block buffer
types exchanged between steps.

  * source — BAM ingest (ReadBgzfBlocks, read_bam*) turning a reader into
    decompressed blocks.
  * sink   — BAM output (WriteBgzfFile).
  * sort   — the in-pipeline sort steps (SortBuffer, CompressSpill,
    SortSpillDecompress, SortMerge) over the arena ingest, spill-write,
    spill-gather and boundary-finding machinery.
  * types  — RecordBatch, DecompressedBlock, BgzfBlock and friends.

Nothing constructs these steps yet: `fgumi sort` still drives the legacy
file-to-file sorter, and routing the command through the arena chain is
root-crate wiring that lands separately. This commit adds the machinery
only, so the crate is exercised by its own tests rather than in production.

Ported from origin/feat-runall f50df03, but NOT byte-identical to it. The
divergences are listed below so a future port from that branch knows where
to expect conflicts.

Toolchain / workspace drift (no behaviour change):

  * ArenaPool::try_acquire already returns the pooled wrapper on this base,
    so the manual PooledSegmentedBuf::pooled wrap is dropped.
  * Three collapsible_if sites rewritten as let-chains rather than
    suppressed, per the repo's rule about adopting newer-compiler idioms.
  * Three rustdoc intra-doc links fixed. One was a genuine doc inaccuracy:
    FindBoundariesAndSort's doc described the wrong sort entry point.

Behaviour changes made under review, which feat-runall does not have:

  * TemplateChunks::push returned unreachable!() on a template lane-variant
    change. The --key-types variant is global to a sort, so a change means
    phase 1 and the merge disagree about key width and the merge would emit
    silently mis-ordered output. It now returns io::Result with an
    InvalidData error naming both variants, threaded through
    MemoryChunksByKind::push and absorb_phase2_event — the same fail-closed
    treatment ensure_single_lane and build_driver already had.
  * SortSpillDecompress::emptiest_first_order now filters slots that have
    reached queue_eof before reading FIFO lengths. The registry is
    append-only, so a drained slot otherwise stays in the scan for the rest
    of the run, taking its FIFO lock on every dispatch only to be rejected.
    Scheduling-only: an EOF slot cannot progress, so no output changes.
  * ReadBgzfBlocks held its reader and finished flag in an Arc<Mutex<..>>
    and an AtomicBool. It is a Serial step, so the runtime drives one shared
    instance and never clones it; both are now plain fields, the misleading
    "was clone() called?" panic message is gone, and the reader is released
    at end of stream instead of held for the whole run.
  * The residual-emission rule is split out of ChunkSorter::take_residual_chunks
    into a free function so it can be unit-tested without building an
    accumulator. Same rule, same behaviour.

Also registers the crate in scripts/publish-crates.sh, after fgumi-sort and
before fgumi-consensus. The list is topologically ordered and CI's
publish-order job fails when a workspace crate is missing from it; the new
crate depends on fgumi-bam-io, fgumi-bgzf, fgumi-pipeline-core,
fgumi-raw-bam, and fgumi-sort, so that is the earliest valid position.

* chore(sort): drop stale allow(dead_code) suppressions

These six attributes predate the pipeline-io machinery and no longer
suppress anything: clippy over --all-features --all-targets is clean
without them. Removing them restores dead-code coverage for
SegmentedBuf's inherent impl, TmpDirAllocator::with_recheck_interval and
mark_full, PrefetchReader::bytes_consumed and consumer_stalls, and the
memory-debug-gated print_mi_stats.

* test(pipeline-io): cover record framing, step wiring, and spill plumbing

The ported crate arrived at 80% patch coverage, below the 90% gate, and the
gap was concentrated in the code that decides whether sorted output is
byte-identical.

boundaries.rs was the worst of it: 31.6% of lines and only 22% of functions,
with a single test covering `bam_header_len` and nothing at all for the
scanner that decides where records begin and end. It now sits at ~99% lines
and 100% functions, with cases for header skipping, cross-block record
carryover, trailing partial records, incomplete headers, and every `finish`
rejection path. The existing sequential-assert test was refactored into an
rstest case table so a failure names the scenario.

Also covered: `ReadBgzfBlocks` driven through a real pipeline (its `try_run`
had no coverage — the previous test hand-rolled an equivalent read loop
instead, so it validated `read_raw_blocks` rather than the step, and has been
removed), the SpillWrite / SpillGather / SpillBlockCompress / ReadBlocks /
InflateToArena step wiring, `SpillWrite::open_file`'s refusal to reuse a
file_id, `SpillGather`'s dense ordinal minting and empty-chunk skip, the
TemplateCoordinate framing path (the one variant that does not share
frame_keyed_record_into), `SpillBlockEvent`'s heap_size and its Ordered
delegation (which guards an infinite-recursion trap), the residual-emission
rule including its template-coordinate exception, the block_batch clamp, the
shared-Arc fail-closed guard, and the over-reserve guard on the k-way merge
path rather than only the fast path.

Two fixes to the tests themselves, both of which made existing assertions
weaker than they looked:

  * synthesize_sized_records built every record with tid = -1, so
    extract_coordinate_key_inline returned RawCoordinateKey::unmapped()
    (u64::MAX) for all of them, `pos` was never compared, and the coordinate
    parity cases collapsed into one equal-key bucket — they would have passed
    with a broken key comparison. Records are now mapped across four
    references, with every eighth left unmapped to keep tie coverage.
  * The queryname branch only length-checked the RecordBatch output, so a
    framing divergence between SortMerge<BlockOutput> and
    SortMerge<RecordBatchOutput> that preserved record count would pass. It
    now compares the two framings as multisets.

The multi-minute soak, matrix, and proptest suites move behind the crate's
new `stress-tests` feature, matching the root crate's existing convention:
the default test target drops from ~30s to ~2s for this crate, and
`cargo ci-test-stress` still runs them. The coverage job explicitly enables
that feature — those suites reach ~200 lines nothing else covers, so
measuring without them under-reports coverage for code that is tested, just
not on the PR-latency path.

Where a StepCtx is required the crate's existing convention is followed:
test the StepCtx-free core, or drive the step through Pipeline::builder.
All test data is constructed programmatically; no fixtures are committed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant