Skip to content

Propagate probability sampling failures - #701

Open
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-probability-failure-propagation
Open

Propagate probability sampling failures#701
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-probability-failure-propagation

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review status: Author-reviewed and ready for ONT review.

Fixes #677.

Summary

  • Return indexed and streaming BAM/CRAM read failures instead of hanging or accepting partial probability histograms.
  • Join every owned worker and propagate worker errors/panics promptly, including after a receiver-side failure.
  • Reject a zero-worker configuration before starting the pipeline.
  • Add deterministic feeder, reader, worker, panic, cancellation, and valid-output regressions.

Severity

Severity: High — scientific correctness and reliability

Rationale: A partial histogram can be accepted as complete and used to select thresholds, silently changing downstream modification calls. Hangs and delayed panics are visible failures, but successful partial results are not.

Root cause

Indexed workers and the streaming collector had different error contracts. Reader errors could terminate iteration as if it were complete, receiver errors could return without coordinating owned threads, and a panic could leave other workers blocked on channels. A zero-worker configuration had no producer capable of completing the protocol.

Implementation

  • Make streaming probability collection consume fallible records and preserve infrastructure errors separately from skippable record/tag rejections.
  • Track the first fatal indexed pipeline error, close channels, and join all owned threads before returning.
  • Add explicit panic cancellation so blocked siblings observe shutdown promptly.
  • Validate worker count before pipeline construction.

Preserved behavior

  • Malformed biological records that the existing policy skips remain nonfatal and counted as rejections.
  • Valid indexed and streaming histograms remain byte-identical.

Non-goals

  • No sampling-identity, edge-filter, or threshold-estimator redesign.
  • No command-wide cancellation taxonomy beyond probability collection.

Behavior before and after

Case Before After Oracle
Truncated streaming BAM Hang or incomplete success Contextual read error Nonzero return, no partial histogram
Indexed worker error/panic Partial result or delayed/hung return First fatal error after prompt cancellation/join Child exits within timeout
Zero workers Pipeline can wait forever Immediate validation error No workers started
Valid input Existing histogram Unchanged Exact indexed/streaming byte identity

Testing

Environment: macOS 26.6 arm64; rustc/cargo 1.90.0; ignored test-only Cargo.lock SHA-256 78876ab4a98da30caad167744d1c8a0875c27edad7b0b3ad5f8a1891f78604ea resolving hts-sys 2.2.0. Cargo.lock is not in the diff.

  • Revision/tree: c2e91a2 / 4eba7cff4d08b6abba2c6dd4369f381295d0fb67; clean tracked worktree.
  • Parent-red injected reader/feeder/worker failures reproduced partial success, hangs, and panic-delayed cancellation.
  • cargo test --offline --locked -p mod_kit sample_probs::tests -- --test-threads=1: 14 passed, 0 failed. Panic messages printed by child-process regressions are expected evidence.
  • Parent panic case exceeded its timeout; the fixed case returned in approximately 0.02 seconds.
  • Valid indexed and streaming output was exactly 145 bytes with SHA-256 ending 81ec4043…b7395d.
  • Exact-head cargo test --offline --locked --workspace --all-targets -- --test-threads=1: 193 active tests passed, 14 declared ignored, 0 failed.
  • Truncated indexed/streaming fixtures, short/long feeds, zero workers, worker error, worker panic, receiver error, and valid aggregation controls passed.
  • git diff --check upstream/master...HEAD passed; worktree clean.
  • Repository-wide stable cargo fmt --all -- --check reports unchanged upstream formatting plus nightly-only settings; no unrelated rewrite was made.

Tests not performed

  • cargo clippy was not run.
  • Broad performance/RSS benchmarking was not run; cancellation latency was measured only in the injected panic regression.

Scientific validation

  • A threshold histogram is returned only after every required input/worker stage succeeds.
  • Skippable malformed-record policy remains distinct from fatal reader infrastructure errors.
  • No accepted failure path returns a partial histogram.
  • Valid indexed/streaming populations remain identical.

Output and compatibility

  • Failure cases now return nonzero errors instead of hangs or partial success.
  • Valid output bytes, schemas, and CLI options are unchanged.
  • This PR can be reviewed independently of deterministic sampling and edge-filter PRs; overlapping sample_probs edits may require an ordinary rebase whichever lands second.

Reviewer guide

  1. Review the fatal-versus-skippable error boundary in sample_probs/mod.rs.
  2. Review channel closure and unconditional thread joins.
  3. Rerun the 14-test focused command.

Checklist

  • The issue contains reproducible observed and expected behavior.
  • The change is limited to probability reader/worker lifecycle.
  • Parent-red and fix-green evidence is recorded.
  • All material tests and expected panic output are listed.
  • Partial-result and valid-output invariants are checked.
  • Diff hygiene passed; unrelated format findings are disclosed.
  • No private data, generated lockfile, or unrelated change is included.

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.

Probability sampling can hang or return partial histograms after reader and worker failures

1 participant