Skip to content

chore(bam): remove two dead pipeline completion flags - #657

Merged
nh13 merged 1 commit into
mainfrom
nh/chore-remove-dead-pipeline-done-flags
Jul 26, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/chore-remove-dead-pipeline-done-flags

Conversation

@nh13

@nh13 nh13 commented Jul 25, 2026 •

Copy link
Copy Markdown
Member

Follow-up to #656, found while auditing BamPipelineState's completion graph. Independent of that PR — this branches from main and touches none of the same lines.

Two *_done flags were dead:

Flag State
decompress_done Declared and initialized, then never touched — never set, never read.
decode_done Set in exactly one place, read nowhere. Write-only state that looked load-bearing.

Neither is missed

  • Decompress is tracked downstream through batches_decompressed, and FindBoundaries infers its completion from batches_boundary_processed == next_read_serial — nothing can be processed that was not decompressed first.
  • Decode is tracked by Group through batches_grouped == batches_boundary_found.

Both are recorded as comments where the fields were, so the next reader doesn't go looking for a flag that was deliberately removed.

Why decode_done is worth deleting rather than leaving

Its guard was boundary_done && q2b_boundaries.is_empty(), which does not account for a batch sitting in a worker's held_boundaries. Anyone who wired the flag up to a consumer would have reintroduced exactly the stranded-batch bug #656 fixes in FindBoundaries — the flag is inert today only by accident. Deleting it removes the trap instead of leaving a loaded gun behind a plausible-looking name.

Behavior

Unchanged. Dropping the store left the else if arm that records Q2b starvation as the only live branch, so the two state.stats() blocks fold into one. The recording condition is the De Morgan equivalent of the original (!(boundary_done && q2b empty)), and the comment now explains why an empty Q2b past that point is the terminal state rather than a stall.

Both fields were pub on BamPipelineState — internal pipeline machinery with no reader outside the module.

cargo ci-test (6586 tests), ci-fmt, ci-lint, and ci-doc all pass.

Coverage

Rewriting that stats arm dropped patch coverage to 0% on the three lines it touches — state.stats() is None unless stats collection is enabled, and nothing turned it on, so the arm had never been exercised. It is covered now by test_decode_records_q2_starvation_only_while_boundaries_may_arrive, which also turns the "condition unchanged" claim above into something checked rather than asserted: inverting the condition fails both cases.

Summary by CodeRabbit

  • Bug Fixes
    • Improved BAM processing completion and queue-handling behavior.
    • Reduced incorrect starvation reporting when no additional boundaries can arrive.
    • Improved pipeline end-of-step detection using batch and boundary progress.

@nh13
nh13 temporarily deployed to github-actions July 25, 2026 17:37 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 36d2f2f1-cc03-4332-b080-cb0269028c11

📥 Commits

Reviewing files that changed from the base of the PR and between 0f97f40 and da5d018.

📒 Files selected for processing (1)
  • src/lib/unified_pipeline/bam.rs

Walkthrough

BAM pipeline completion tracking now relies on decompressed/grouped counters and boundary state instead of explicit flags. Decode starvation accounting and its test cases were updated for empty Q2b queues and boundary arrival conditions.

Changes

BAM completion tracking

Layer / File(s) Summary
Counter-based completion state
src/lib/unified_pipeline/bam.rs
Removes decompress_done and decode_done from BamPipelineState and its constructor, documenting counter-based completion semantics.
Boundary-aware decode termination
src/lib/unified_pipeline/bam.rs
Updates decode starvation accounting to use boundary_done and q2b_boundaries, and revises tests for the resulting behavior.

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

Possibly related PRs

  • fulcrumgenomics/fgumi#656: Changes how boundary_done is set, which directly affects the updated decode completion semantics.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing two unused BAM pipeline completion flags.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/chore-remove-dead-pipeline-done-flags

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

@codecov

codecov Bot commented Jul 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.65%. Comparing base (0f97f40) to head (da5d018).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #657   +/-   ##
=======================================
  Coverage   93.64%   93.65%           
=======================================
  Files         175      175           
  Lines      107492   107487    -5     
=======================================
- Hits       100664   100662    -2     
+ Misses       6828     6825    -3     

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

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

`decompress_done` was declared and initialized and never touched again --
never set, never read. `decode_done` was set in one place and read nowhere,
so it was write-only state that looked load-bearing.

Neither is missed. `Decompress` is tracked downstream through
`batches_decompressed`, and `FindBoundaries` infers its completion from
`batches_boundary_processed == next_read_serial`, since nothing can be
processed that was not decompressed first. `Decode` is tracked by `Group`
through `batches_grouped == batches_boundary_found`. Both replacement
comments record that, so the next reader does not go looking for the flag.

Removing `decode_done` matters for more than line count: its guard
(`boundary_done && q2b_boundaries.is_empty()`) does not account for a batch
sitting in a worker's `held_boundaries`, so anyone who wired the flag up to
a consumer would have reintroduced the stranded-batch bug that was just
fixed in `FindBoundaries`. Deleting it removes that trap rather than leaving
a loaded gun behind a plausible-looking name.

Dropping the store leaves the `else if` arm that recorded Q2b starvation as
the only live branch, so the two `state.stats()` blocks fold into one. The
recording condition is unchanged -- `!(boundary_done && q2b empty)` becomes
the De Morgan equivalent -- and the comment now says why an empty Q2b past
that point is the terminal state rather than a stall.

That arm had no test, so rewriting it dropped patch coverage to zero on the
three lines it touches: `state.stats()` is `None` unless stats collection is
enabled, which nothing turned on. It is covered now, which also turns the
"condition unchanged" claim above into something checked rather than
asserted -- inverting the condition fails both cases.

Both fields were `pub` on `BamPipelineState`, which is internal pipeline
machinery with no reader outside this module.
@nh13
nh13 force-pushed the nh/chore-remove-dead-pipeline-done-flags branch from 737e11c to da5d018 Compare July 25, 2026 17:46
@nh13
nh13 temporarily deployed to github-actions July 25, 2026 17:46 — with GitHub Actions Inactive
@nh13

nh13 commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 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 commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 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 merged commit db79ee6 into main Jul 26, 2026
14 checks passed
@nh13
nh13 deleted the nh/chore-remove-dead-pipeline-done-flags branch July 26, 2026 05:42
@nh13 nh13 mentioned this pull request Jul 26, 2026

This branch was previously deployed

1 inactive deployment
github-actions — da5d0185 Deployed Jul 25, 2026 by nh13 via coverage #3064
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