Skip to content

Prevent interval endpoint overflow and remove stray adjust-mods stderr output - #706

Open
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-small-output-defects
Open

Prevent interval endpoint overflow and remove stray adjust-mods stderr output#706
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/fix-small-output-defects

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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

Fixes #671.

Summary

  • Removes an unconditional dbg! call from adjust-mods so successful commands no longer print internal source state to stderr.
  • Prevents ReferenceIntervalBatchesFeeder endpoint arithmetic from overflowing near u32::MAX.
  • Adds focused regressions for clean stderr, readable adjusted BAM output, exact near-limit interval coverage, and bounded termination.

Severity

Severity: Low — diagnostics and extreme-coordinate reliability

Rationale: The stderr leak is routine but does not change the output BAM. The interval defect can panic, wrap coverage backwards, or fail to terminate, but it requires an uncommon accepted-input coordinate and interval-size combination near the u32 limit. Neither defect is known to change routine biological calls or statistics.

Root cause

Adjust::run executes dbg!(&self.cpg) unconditionally before motif parsing. Separately, ReferenceIntervalBatchesFeeder::next_batch evaluates start + self.interval_size in u32 before clipping the result to the contig end. The addition therefore panics in debug builds and wraps in release builds before min can constrain it.

Implementation

The two fixes remain separate commits:

  1. Remove the stray dbg! expression without changing intended logging.
  2. Replace endpoint addition with start.saturating_add(self.interval_size), then retain the existing clip to the contig end.

The saturating addition is local to the faulty endpoint expression and avoids a coordinate-type or scheduling refactor.

Preserved behavior

  • Adjust/collapse/ignore/CpG behavior and the emitted BAM records remain unchanged.
  • Intentional logging and error diagnostics remain unchanged.
  • Ordinary interval partitions remain unchanged because their additions do not saturate.

Non-goals

  • No migration from u32 coordinates, broad arithmetic audit, interval-size policy change, or scheduler refactor.
  • No change to MM/ML tags, BAM record order, extract accounting, writer finalization, or dependency resolution.
  • No performance claim.

Behavior before and after

Case Before After Expected oracle
Successful adjust-mods --ignore h Writes a readable BAM but leaks &self.cpg = false and a Rust source location to stderr Writes the same readable BAM without the internal diagnostic No &self.cpg or modbam_util/subcommands.rs signature in stderr
Reference interval [u32::MAX-9, u32::MAX) with interval size 8 Debug panic or release-mode wrapped/nonterminating coverage Emits two monotonic intervals and terminates [MAX-9, MAX-1), [MAX-1, MAX); exactly nine covered bases
Ordinary interval and adjust inputs Existing successful behavior Unchanged Existing integration suite remains green

Testing

Test environment

  • Revision tested: 8eb4867b309a3f0eac4317d16652780ea3cec506
  • Tree tested and worktree state: 477b0986db3b77a24d2d19363c8a3dbbac4f2329; clean after testing
  • Toolchain: rustc/cargo 1.90.0
  • Platform: macOS 26.6, arm64 Apple Silicon
  • Reference binary: installed modkit 0.6.4
  • External tools: none required for the focused regressions
  • Dependency resolution identity: ignored worktree Cargo.lock SHA-256 f9d3389f8fc63c9ad653dad3fbf43a1c26cc54008f0a39e06e71ff6455a1e9b5; rust-htslib 0.46.0, hts-sys 2.2.0, tempfile 3.27.0
Test layer Exact command, fixture, or matrix Result and evidence
Core: parent-red regression Exact parent 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c; issue reproducer for adjust-mods, plus replay of test_reference_interval_batches_near_u32_max Parent emits the internal dbg! signature; debug overflow panics, and release behavior violates the bounded exact-coverage oracle
Core: focused regression TMPDIR=/private/tmp/modkit-pr-small-output-test-tmp cargo test -p mod_kit interval_chunks::interval_chunks_tests::test_reference_interval_batches_near_u32_max -- --exact --nocapture --test-threads=1 1 passed, 0 failed; exact two intervals, monotonicity, nine-base coverage, and termination verified
Core: affected CLI/integration test TMPDIR=/private/tmp/modkit-pr-small-output-test-tmp cargo test -p modkit --test test_adjust_mods test_adjust_mods_does_not_dump_cpg_to_stderr -- --exact --nocapture --test-threads=1 using tests/resources/bc_anchored_10_reads.sorted.bam 1 passed, 0 failed; command succeeds, output BAM is readable and nonempty, and stderr lacks both internal signatures
Core: applicable full workspace gate TMPDIR=/private/tmp/modkit-pr-small-output-test-tmp cargo test --workspace --all-targets --no-fail-fast -- --test-threads=1 181 passed, 14 ignored, 0 failed; test_adjust_mods target contributed 15 passed
Core: formatting/diff checks rustfmt --edition 2021 --check modkit-core/src/interval_chunks.rs modkit-core/src/modbam_util/subcommands.rs modkit/tests/test_adjust_mods.rs; git diff --check upstream/master...HEAD Both passed; clean three-file diff, 90 insertions and 3 deletions

Tests not performed

  • No large real-data or performance benchmark was run because the production changes are one diagnostic removal and one constant-time checked endpoint operation, with no performance claim.
  • No broad coordinate-arithmetic audit was performed; it is outside this issue's scope.

Scientific validation

  • Population/eligibility invariant: no record eligibility or modification-call population changes.
  • Count/category conservation invariant: the adjusted BAM remains readable and nonempty; existing adjust tests remain green.
  • Coordinate/strand/interval invariant: the near-limit interval union is exactly the requested nine-base half-open interval, without gaps, overlaps, reversal, or overrun.
  • Determinism invariant: the exact interval partition and clean-stderr assertions are deterministic.
  • Independent oracle or specialist review: exact boundary arithmetic was independently reviewed and the isolated full gate was rerun on the frozen head.

Output and compatibility

  • User-visible change: one unintended stderr diagnostic disappears; extreme endpoints no longer panic or wrap.
  • Expected output differences: no BAM-content difference is expected for ordinary adjust-mods inputs.
  • Byte-identical controls: existing affected integration tests remain green; the PR does not alter record-writing code.
  • CLI/API/schema compatibility: no option, public Rust API, file schema, or log-level contract changes.
  • Partial-output or failure semantics: no writer or failure-policy changes.

Reviewer guide

  1. Review the two parent expressions identified in issue adjust-mods leaks debug state to stderr and interval batching can overflow at u32 endpoints #671.
  2. Review the one-line removal in modbam_util/subcommands.rs and the one-line saturating addition in interval_chunks.rs.
  3. Review the bounded endpoint fixture for exact half-open coverage and termination.
  4. Rerun the two focused canaries listed above.
  5. Confirm the two commits remain independent and the diff contains no broader coordinate or adjust refactor.

Checklist

  • The issue contains reproducible observed and expected behavior.
  • The change is limited to the linked issue's approved scope.
  • The regression is demonstrably red on the exact parent revision.
  • All tests actually performed are listed above with their results.
  • Unrun or inapplicable tests are disclosed.
  • Scientific counts/statistics and output compatibility are explicitly checked.
  • Formatting and diff-hygiene checks pass, or unrelated findings are documented.
  • No generated data, private sample identifiers, or unrelated changes are 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.

adjust-mods leaks debug state to stderr and interval batching can overflow at u32 endpoints

1 participant