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

[SuhasSrinivasan]

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

Summary

Probability collection does not consistently propagate fatal failures from BAM/CRAM readers, indexed-region feeders, and worker threads. Depending on the path, modkit can hang, report success with a partial probability histogram, or delay reporting a worker panic until unrelated work has finished.

This shared code affects sample-probs and probability collection used by summary, extract, and pileup workflows.

Severity

Severity: High — scientific correctness and reliability

Rationale: A partial histogram can be accepted as complete and used to select modification-probability thresholds. That can change downstream filtering and reported modification results without a clear indication that the sampled input was incomplete. Hangs and delayed panic reporting are visible reliability failures, but successful partial results are not readily detectable from the output alone.

User and scientific impact

• Affected result or workflow: probability histograms from sample-probs, summary probability collection, extract automatic thresholds, pileup automatic thresholding, and the indexed-unmapped fallback.
• Direction of error: incomplete sample membership, potentially shifted thresholds, partial output reported as successful, or nontermination.
• Likely exposure: data-dependent for BAM/CRAM decode and worker failures; deterministic when zero worker threads are requested.
• Detectability or workaround: a timeout detects hangs, but a successfully returned partial histogram has no completeness marker. Pre-validating alignment files reduces decoder risk but does not cover feeder, worker, or panic paths.

Affected versions and environment

• Released version: modkit 0.6.4.
• Development revision: 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c.
• Operating system and architecture used for verification: macOS on Apple Silicon.
• Input formats: indexed and streaming BAM/CRAM probability-collection paths.

Steps to reproduce

Zero-worker command

From the modkit repository, run the released binary against the public test fixture with a five-second watchdog:

tests/resources/HG002_small.ch20._other.sorted.bam is 1,643,297 bytes and has SHA-256 4410f8d96700202321b3e98a64c68316e390d0b38041a61e17df0ec0f442d138.

python3 - <<'PY'
import subprocess

command = [
    "modkit", "sample-probs",
    "tests/resources/HG002_small.ch20._other.sorted.bam",
    "--threads", "0",
    "--no-sampling",
    "--suppress-progress",
]

try:
    result = subprocess.run(command, timeout=5, check=False)
    print(f"exit status: {result.returncode}")
except subprocess.TimeoutExpired:
    print("timed out after 5 seconds")
PY

Deterministic failure injection

Focused Rust regressions exercise the remaining paths without relying on filesystem corruption or scheduling luck:

• a records iterator yields valid records and then a synthetic BAM/CRAM read error;
• an indexed-region feeder returns a synthetic error;
• a probability worker returns an error;
• with two synchronized workers, one panics while the surviving worker is offered 10,000,000 intervals.

The tests invoke the same shared probability-collection functions used by the commands above.

Observed behavior

• --threads 0 does not complete within the five-second watchdog.
• Reader errors can be counted as rejected biological records, allowing a partial histogram to be returned.
• Feeder and worker errors can be logged or discarded instead of becoming the command result.
• A worker panic can leave another worker processing the remaining input before the caller learns of the failure.

The two-worker panic regression exceeded a five-second outer watchdog on the affected implementation.

Control or independent oracle

A transport or decode error means the requested probability sample was not completely read. The command must return a nonzero result and must not expose the partial histogram as successful output. Likewise, zero workers is an invalid configuration and should fail immediately rather than waiting on channels that can never produce a result.

Per-record biological parsing failures that are already classified as rejected records remain a separate, nonfatal category.

As a successful-input control, the proposed correction and installed modkit 0.6.4 were each run on tests/resources/bc_anchored_10_reads.sorted.bam with --threads 2 --no-sampling --suppress-progress, once with its index and once with --ignore-index. The four resulting TSV files were each 145 bytes with SHA-256 81ec4043bda3b0c7a84522379b56cafd4ddb9192f0e55ad96344661187b7395d; fixed-versus-installed byte comparisons succeeded in both modes.

Expected behavior

• Reject zero worker threads immediately with a clear error.
• Return the first fatal reader, feeder, worker, or worker-panic error observed by the coordinator, with useful context.
• Stop scheduling new work promptly after a fatal failure, close the owned channels, and join every started thread.
• Discard partial histograms after a fatal failure.
• Preserve the current rejection accounting for nonfatal per-record biological parsing failures.
• Leave successful sampling, thresholds, histogram values, ordering, and output bytes unchanged.

Root-cause evidence

• Relevant code: modkit-core/src/sample_probs/mod.rs.
• The concurrent collector merges successful worker histograms without consistently retaining and returning fatal producer or worker results.
• BAM/CRAM iterator errors are folded into rejected-record counts on streaming and indexed-unmapped paths rather than propagated as read failures.
• Requesting zero workers leaves the channel pipeline without a consumer that can make progress.
• A panicking worker drops only its own result-sender clone; the collector can wait for surviving workers to finish before inspecting join results.
• Parent-revision regressions reproduce each failure. The synchronized two-worker panic test is timing-bounded and fails before the correction, then completes in about 0.02 seconds afterward.

Proposed fix scope

• Validate that the worker count is nonzero.
• Propagate BAM/CRAM iterator, indexed feeder, and worker failures with path-specific context.
• Retain the first fatal error observed by the coordinator, cancel further scheduling, discard partial aggregation, and join all owned threads.
• Convert Rust unwind panics at the worker processing boundary into the same returned-error and cancellation path.
• Add focused regressions for zero workers, streaming and indexed failures, worker errors, and prompt multi-worker panic cancellation.

Non-goals

• No change to the sampling estimand, seed, fraction, fixed-count behavior, record eligibility, histogram schema, or automatic-threshold algorithm.
• No retry or repair of corrupt BAM/CRAM input, CRAM reference-policy change, or validation of input outside the sampled regions or quota.
• No transactional output or atomic-finalization guarantee.
• No attempt to interrupt an htslib read already in progress or to recover from process-aborting panics.
• No CPU-performance claim for successful inputs; the change only bounds work after an observed failure.

Acceptance criteria

• Zero workers returns a deterministic, immediate, nonzero error.
• Streaming, indexed-region, and indexed-unmapped read failures return a causal error and no successful partial histogram.
• Feeder and worker errors return the first fatal cause observed by the coordinator after all started threads are joined.
• A panic in one of at least two workers cancels pending work promptly and returns a worker-panic error without processing the full remaining feed.
• Nonfatal per-record biological rejections retain their existing accounting.
• Valid indexed and streaming controls remain byte-identical to modkit 0.6.4.
• Focused tests and the full workspace suite pass.

Reproduction artifacts

Artifact	Size	SHA-256	Notes
tests/resources/HG002_small.ch20._other.sorted.bam	1,643,297 bytes	4410f8d96700202321b3e98a64c68316e390d0b38041a61e17df0ec0f442d138	Public zero-worker CLI reproducer
tests/resources/bc_anchored_10_reads.sorted.bam	5,527 bytes	4441acbe1ad59caf6ab5d56bf18097d8999c08351395219c919cb863652d85f0	Valid indexed/streaming byte-identity control
Synthetic fallible record iterator	not applicable	not applicable	Deterministic reader-error injection
Synthetic indexed feeder and worker failures	not applicable	not applicable	Deterministic propagation checks
Synchronized two-worker panic harness	not applicable	not applicable	Deterministic prompt-cancellation check

Related work

• Proposed PR: to be linked after publication.
• Dependency-resolution PR Pin hts-sys to bindings compatible with rust-htslib 0.46 #666 affects fresh lockfile resolution and publication timing only; it does not cause or fix probability failure propagation.

[SuhasSrinivasan]

The complete code change is in ready-for-review PR #701. Closing this standalone catalog issue in line with the maintainer's request that these submissions be represented by PRs rather than separate AI-generated issues.