Repository navigation
refactor(pipeline): extract pipeline::core into fgumi-pipeline-core crate (120s → 6s compile-fail test) - #440
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 (6)
WalkthroughExtracts the typed-step pipeline execution engine from Changesfgumi-pipeline-core crate extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
nh13
force-pushed
the
nh/extract-pipeline-core
branch
from
June 19, 2026 18:31
2b9a50a to
30e751c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-runall #440 +/- ##
==============================================
Coverage ? 93.80%
==============================================
Files ? 119
Lines ? 50555
Branches ? 0
==============================================
Hits ? 47425
Misses ? 3130
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…rate
The `pipeline::core` typed-step engine is dependency-light (ahash,
crossbeam-queue, parking_lot, log, and a single noodles::sam type) but lived in
the `fgumi` crate, so the trybuild compile-fail tests — which only exercise
`pipeline::core` trait/type bounds — recompiled the entire fgumi dependency
graph (noodles-bam, sort, consensus, mimalloc, …) in trybuild's isolated
sandbox. That made `pipeline_core_compile_fail` take >120s in CI.
Move the 24-file `src/lib/pipeline/core` subtree into a new
`crates/fgumi-pipeline-core` crate and re-export it as `pipeline::core`, so
every existing `crate::pipeline::core::…` path is unchanged. The compile-fail
test + cases move with it and now build against just this crate:
`pipeline_core_compile_fail` drops from ~120s to ~6s.
- Re-export: `pub use fgumi_pipeline_core as core;` in `pipeline/mod.rs`.
- Visibility: `PipelineBuilder::{append_source,append_step,append_step2}` go
`pub(crate)` -> `pub` (the `chains` layer in `fgumi` drives them across the
new crate boundary).
- Two `process2` pipeline-run tests that reached into `pipeline::steps` move
from the builder test module to `steps/process.rs` (their natural home).
- New crate carries `#![deny(unsafe_code)]`; the documented erased.rs
`#[allow(unsafe_code)]` sites and CLAUDE.md paths are updated.
157 core unit tests pass; all workspace test targets compile; fmt + clippy
(pedantic) clean.
nh13
force-pushed
the
nh/extract-pipeline-core
branch
from
June 19, 2026 18:44
30e751c to
687e3b2
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-core/src/signal.rs`:
- Around line 147-158: The cancel() method has a race condition where the state
transitions to STATE_CANCELLED before payload.set() completes, allowing
outcome() to return None while is_done() is true. Instead of directly
transitioning to STATE_CANCELLED in the compare_exchange call, first use a CAS
to transition to an intermediate non-terminal blocking state (to claim exclusive
writer access), then call payload.set(), then perform a second CAS to transition
to the terminal STATE_CANCELLED state. This ordering ensures is_done() observers
cannot observe the done state before the payload is actually set. Apply the same
two-phase transition pattern to the record_error method as well.
In `@src/lib/pipeline/steps/process.rs`:
- Around line 1187-1189: The assertions checking evens_received and
odds_received using load with AtomicOrd::Relaxed only validate item counts, not
the actual values routed to each sink. This allows bugs that misroute, drop,
duplicate, or swap values while preserving counts to pass silently. Modify the
test to collect the actual values received in each branch (not just counts) and
assert that the sorted exact expected sets match for each sink. Apply this fix
to both assertion blocks at lines 1187-1189 and 1220-1222 to validate complete
record identity rather than just totals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 460c1917-6b06-4b8b-b6d0-330276090372
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (36)
CLAUDE.mdCargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rssrc/lib/pipeline/mod.rssrc/lib/pipeline/steps/process.rs
…quality issues Follow-up commit (kept separate from the verbatim move so that move stays a clean, reviewable rename) fixing issues surfaced by review of the moved code: - signal.rs: `PipelineSignal::cancel` set the payload unconditionally even when it lost the state CAS to a concurrent `record_error`. If `record_error` won the CAS (state -> ERROR) but had not yet published its payload, an unconditional `payload.set(Cancelled)` could win the OnceLock and leave state==ERROR while outcome()==Cancelled. Guard the set behind the CAS, exactly as `record_error` already does, so the state-transition winner is the payload writer. (Not unit-tested: OnceLock masks the inconsistency in every sequential ordering; deterministically forcing the interleaving needs loom — see the inline note.) - held.rs: `HeldSlot::put` guarded the already-occupied invariant with `debug_assert!`, so a contract violation in a release build would silently OVERWRITE (drop) the held record. Promote to `assert!` — the check is one predictable branch on the back-pressure path; silent record loss must not pass. - tests.rs: three pipeline tests asserted only the SUM of collected values, which passes for any multiset with the same total (missing mis-paired/corrupted values). Assert the sorted multiset instead; switch DrainReproSink from a bare count to collecting values so drop/dup/corruption is caught, not just an off-by-N total. All 157 fgumi-pipeline-core tests pass; fmt + clippy (pedantic) clean.
nh13
force-pushed
the
nh/extract-pipeline-core
branch
from
June 19, 2026 21:38
9e62c27 to
eff004f
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
added a commit
that referenced
this pull request
Jun 19, 2026
A review of `TypedStep2::build_output_set` asked why it doesn't apply the Serial/Exclusive reorder-collapse that `TypedStep::build_output_set` has. Document that the asymmetry is intentional and add a test that pins it. The single-input collapse is sound because of a universal property: a Serial single-input step consumes one already-ordered stream, so any input ordinal it propagates onto an output is emitted in push order, making the reorder stage redundant. A two-input MERGE has no such guarantee — it interleaves two branches and can push a `ByItemOrdinal` output out of ordinal order even under serial execution, so its reorder stage is load-bearing. Collapsing it would silently deliver records out of order. Today's production Step2s happen not to need it (`PairRawFastq` uses `BranchOrdering::None`; `ZipperMergeStep` assigns fresh sequential ordinals), but the framework cannot assume that for an arbitrary merge. Adds `step2_serial_byitemordinal_output_is_reordered_not_collapsed`, which pushes a Serial Step2's ordered output out of ordinal order and asserts the consumer receives it in ordinal order — it fails if the single-input collapse is ever added to the Step2 path (verified). Stacked on the fgumi-pipeline-core extraction (#440); rebases onto feat-runall once that merges.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
added a commit
that referenced
this pull request
Jun 19, 2026
A review of `TypedStep2::build_output_set` asked why it doesn't apply the Serial/Exclusive reorder-collapse that `TypedStep::build_output_set` has. Document that the asymmetry is intentional and add a test that pins it. The single-input collapse is sound because of a universal property: a Serial single-input step consumes one already-ordered stream, so any input ordinal it propagates onto an output is emitted in push order, making the reorder stage redundant. A two-input MERGE has no such guarantee — it interleaves two branches and can push a `ByItemOrdinal` output out of ordinal order even under serial execution, so its reorder stage is load-bearing. Collapsing it would silently deliver records out of order. Today's production Step2s happen not to need it (`PairRawFastq` uses `BranchOrdering::None`; `ZipperMergeStep` assigns fresh sequential ordinals), but the framework cannot assume that for an arbitrary merge. Adds `step2_serial_byitemordinal_output_is_reordered_not_collapsed`, which pushes a Serial Step2's ordered output out of ordinal order and asserts the consumer receives it in ordinal order — it fails if the single-input collapse is ever added to the Step2 path (verified). Stacked on the fgumi-pipeline-core extraction (#440); rebases onto feat-runall once that merges.
nh13
added a commit
that referenced
this pull request
Jun 20, 2026
A review of `TypedStep2::build_output_set` asked why it doesn't apply the Serial/Exclusive reorder-collapse that `TypedStep::build_output_set` has. Document that the asymmetry is intentional and add a test that pins it. The single-input collapse is sound because of a universal property: a Serial single-input step consumes one already-ordered stream, so any input ordinal it propagates onto an output is emitted in push order, making the reorder stage redundant. A two-input MERGE has no such guarantee — it interleaves two branches and can push a `ByItemOrdinal` output out of ordinal order even under serial execution, so its reorder stage is load-bearing. Collapsing it would silently deliver records out of order. Today's production Step2s happen not to need it (`PairRawFastq` uses `BranchOrdering::None`; `ZipperMergeStep` assigns fresh sequential ordinals), but the framework cannot assume that for an arbitrary merge. Adds `step2_serial_byitemordinal_output_is_reordered_not_collapsed`, which pushes a Serial Step2's ordered output out of ordinal order and asserts the consumer receives it in ordinal order — it fails if the single-input collapse is ever added to the Step2 path (verified). Stacked on the fgumi-pipeline-core extraction (#440); rebases onto feat-runall once that merges.
nh13
added a commit
that referenced
this pull request
Jul 10, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 11, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 11, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 12, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 12, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 12, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 12, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 13, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `PipelineBuilder` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (builder.rs); drop the struct-level allow and the stale rationale. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 20, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `Pipeline::cancel_handle` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (chains/builder.rs); drop the struct-level allow and the stale rationale. `chunk_size` feeds `in_flight_unmapped_budget` (not batch sizing), so the field doc making that stale claim is corrected alongside it. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 26, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `Pipeline::cancel_handle` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (chains/builder.rs); drop the struct-level allow and the stale rationale. `chunk_size` feeds `in_flight_unmapped_budget` (not batch sizing), so the field doc making that stale claim is corrected alongside it. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 26, 2026
The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `Pipeline::cancel_handle` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (chains/builder.rs); drop the struct-level allow and the stale rationale. `chunk_size` feeds `in_flight_unmapped_budget` (not batch sizing), so the field doc making that stale claim is corrected alongside it. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
nh13
added a commit
that referenced
this pull request
Jul 27, 2026
…#544) The incremental `pipeline::core → fgumi-pipeline-core` extraction (#440) and the AAM wiring left `#[allow(dead_code)]` attributes and "wired up by Phase 1/2" / "exercised only by tests" comments on items that are, in fact, called from production now. The allows silence nothing and the comments misdescribe the code, hiding whether a genuinely-dead item exists. Verified each against `cargo clippy -D warnings` (the removals are only safe because the compiler agrees the items are live): - `CancelHandle::from_signal` (signal.rs) — called by `Pipeline::cancel_handle` (builder.rs); drop the allow, point the doc at the real caller. - `build_branch_byte_aware` (handles.rs) — called by production `build_single_queues`; drop the allow, correct the "tests only" doc. - `OutputHandles::new` (step.rs) — called by `TypedStep::wrap_outputs_view` (erased.rs); drop the allow, correct the "Phase 1 Task 15" doc. - `ResolvedAligner` (aligner.rs) — every field is read when the AAM stage is wired (chains/builder.rs); drop the struct-level allow and the stale rationale. `chunk_size` feeds `in_flight_unmapped_budget` (not batch sizing), so the field doc making that stale claim is corrected alongside it. `ResolvedAlignerMode` keeps its `allow(dead_code)`, but with an accurate comment: its `Preset` payload is consumed only through the derived `Debug` (info-logged), which the never-read lint flags — so the allow is legitimate, not stale. The remaining findings/{04,06,07,16} F items (retry_held / try_run dedup, CompressSpill↔SpillCompress rename, SortPhase1Event/SortPhase2Event structural merge, SpillReady.path removal) are churny type/rename refactors that would conflict with the in-flight feat-runall→main reconcile for little gain; they are deferred to the reconcile. The decompress_into_slice ISIZE cross-check (findings/07 Q1) is already covered by the exact-fill + CRC32 verification (findings/16), so no change is needed there.
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
Extracts the
pipeline::coretyped-step engine from thefgumicrate into a newcrates/fgumi-pipeline-corecrate, re-exported aspipeline::coreso every existingcrate::pipeline::core::…path is unchanged.Why
The
trybuildcompile-fail test (pipeline_core_compile_fail) runs >120s in CI (flaggedSLOW [>120.000s]). The cases only exercisepipeline::coretrait/type bounds, but trybuild compiles each in an isolated sandbox that links the entirefgumidependency graph —noodles-bam,fgumi-sort,fgumi-consensus,mimalloc, etc. — from scratch, none of which the cases need.pipeline::coreis actually dependency-light:ahash,crossbeam-queue,parking_lot,log, and a singlenoodles::sam::Headertype. Moving it to its own crate lets the compile-fail tests build against just that small crate.Result:
pipeline_core_compile_faildrops from ~120s to ~6s.What changed
crates/fgumi-pipeline-core(24 files, the formersrc/lib/pipeline/coresubtree).gittracks the moves as renames.fgumire-exports it:pub use fgumi_pipeline_core as core;inpipeline/mod.rs. No call sites change.PipelineBuilder::{append_source, append_step, append_step2}changepub(crate)→pub— thechainslayer (still infgumi) drives them across the new crate boundary. No other visibility changes were needed.process2pipeline-run tests that reached intopipeline::stepsmove from the builder test module tosteps/process.rs(their natural home — the core crate can't depend on the steps layer)..stderrsnapshots regenerated; case imports rewritten tofgumi_pipeline_core::….#![deny(unsafe_code)]; the documentederased.rs#[allow(unsafe_code)]sites and the CLAUDE.md unsafe-allowlist paths / framework reference are updated to the new location.Verification
fgumi-pipeline-coreunit tests pass; the 2 relocatedprocess2tests pass.cargo test --workspace --no-run).cargo fmt --checkandcargo ci-lint(clippy pedantic,-D warnings) clean.pipeline_core_compile_fail: ~6s (was >120s).Follow-up (out of scope)
The crate still pulls
noodlesfor onesam::Headertype inheader.rs. Abstracting that single touch-point would let the crate dropnoodlesentirely and bring the compile-fail test to true single-digit-second cold builds.Note
Targets
feat-runallbecausepipeline::coreexists only there (the chains pipeline rewrite); it is not onmain.Summary by CodeRabbit
Bug Fixes
New Features
Tests
process2fan-out and drop behavior, with deterministic value checking.