Skip to content

Correct repair matching, ordering, and atomic output - #711

Open
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/publish-repair-ordering-atomic
Open

Correct repair matching, ordering, and atomic output#711
SuhasSrinivasan wants to merge 4 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/publish-repair-ordering-atomic

Conversation

@SuhasSrinivasan

@SuhasSrinivasan SuhasSrinivasan commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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

Summary

This PR contains the complete repair-specific correctness package. It fixes three related failure modes in one reviewable branch:

  • ambiguous overlapping donor placements are rejected instead of projecting MM/ML calls from an arbitrary occurrence;
  • repaired records retain acceptor/query-name order, and a missing donor no longer suppresses later repairable acceptors;
  • fatal reader, worker, writer, and output-validation failures are joined and returned without publishing a partial BAM or overwriting a valid destination.

The changes are consolidated because they all modify the same repair record-matching, scheduling, and output lifecycle. No pileup, extract, entropy, DMR, or probability code is included.

Severity

High — scientific output integrity and reliability.

The affected paths can silently place modification probabilities at the wrong acceptor positions, omit repairable records, retain a query-name-sorted header while reordering records, or report success after an incomplete output.

Bugs and fixes

Bug Previous behavior Corrected behavior
Overlapping sequence placements ACAC in ACACAC was treated as unique because non-overlapping matching missed the second start at offset 2 A second occurrence, overlapping or not, makes the record an ordinary nonfatal repair rejection
Donor gap Acceptor read1,read2,read3 plus donor read1,read3 could emit only read1 One ordered repaired-or-rejected outcome is produced per acceptor; read3 is retained
Worker completion order Multi-worker output followed completion order under an SO:queryname header Ordinals restore acceptor-relative order independently of worker count
Fatal decode/write/finalization error Work could hang, return partial success, or mutate an existing destination Work is cancelled and joined; a private staged BAM is validated and atomically published only on success

Unique forward/reverse repair and MM/ML/MN projection semantics are unchanged.

Implementation

  • A crate-private bounded ordered scheduler handles ordinals, cancellation, first-error preservation, panic capture, and joined shutdown.
  • Repair matching groups comparator-equivalent donor names while requiring exact QNAME bytes and advances the appropriate stream after gaps.
  • memchr::memmem::Finder performs at most two bounded searches so overlapping placements are detected without allocating all matches.
  • Output is written to a private same-filesystem staging directory, then checked for file identity, canonical BGZF EOF, complete decode, and exact output cardinality before synchronization and atomic replacement.
  • New files follow ordinary 0666 & umask semantics; replacing an existing regular file preserves its permissions. Symlinks and nonregular destinations are rejected before staging.

Testing

Tested exact revision: 23709cf3aff0da3aeec6beb02498629e0920aedf
Tree: c5658ba199e533083f9bae4195d73772583820a8
Platform/toolchain: macOS arm64; rustc/cargo 1.90.0; rust-htslib 0.46.0 and hts-sys 2.2.0.

  • Full serial workspace gate: 204 passed, 14 ignored, 0 failed.
  • Repair units: 12 passed.
  • Ordered-scheduler units: 10 passed.
  • Repair CLI integrations: 6 passed.
  • Parent-red overlap fixture: upstream accepts the ambiguous ACACAC/ACAC placement; this branch rejects it.
  • Parent-red gap fixture: upstream omits later read3; this branch emits read1,read3 in acceptor order and counts one rejected record.
  • Public repair fixture at threads 1/2/3/8: all fixed outputs contain 11 records and are byte-identical, 3,142 bytes, SHA-256 52b8a3a227b08d0cd98d79c826421199971f5c2e2feead0e3d5c6441e25dc813. The result also matches installed modkit 0.6.4 at one worker.
  • Failure-injection matrix: truncated donor/acceptor, worker error/panic, consumer error/panic, Nth-write failure, corrupted staged BAM, and staged-inode substitution all terminate, preserve an existing sentinel or leave an absent destination absent, join workers, and clean staging.
  • Destination matrix: ordinary new file, existing mode 0404, live/dangling symlink, FIFO, and directory.
  • Empty donor, empty acceptor, and both empty are rejected by existing modBAM/sequence validation before substring matching; exact diagnostics are regression-tested.
  • git diff --check, targeted rustfmt --check, clean-worktree, ancestry, and Cargo.lock checks passed.

The exact combined head was independently reviewed for Rust ownership/cancellation, matching, diagnostics, output lifecycle, and test preservation. No blocker remained.

Output compatibility

  • Expected output changes are limited to ambiguous placements, donor gaps, completion-order permutations, and fatal-output paths.
  • Unique placements and existing valid single-worker fixture output remain byte-identical.
  • There is no CLI, BAM header, or tag-schema change.
  • Per-record biological repair rejection remains nonfatal; infrastructure and I/O failures are fatal.

Reviewer guide

  1. Review the overlap tests and two Finder searches in repair_tags.rs.
  2. Review the acceptor/donor merge and one-outcome-per-acceptor invariant.
  3. Review ordered_scheduler.rs for bounded ordering, cancellation, first-error preservation, and joins.
  4. Review staged-output validation and atomic publication.
  5. Run cargo test -p mod_kit repair_tags::tests -- --test-threads=1, cargo test -p mod_kit ordered_scheduler::tests -- --test-threads=1, and cargo test -p modkit --test test_repair -- --test-threads=1 as focused canaries.

Non-goals

  • No heuristic or alignment-based choice among ambiguous placements.
  • No duplicate-flagged donor policy change or corrupt-input repair.
  • No CRAM policy change.
  • No crash/power-loss durability claim, parent-directory fsync, or ACL/xattr/owner/group preservation.
  • No successful-path performance claim; complete validation and synchronization add deliberate I/O before publication.

@SuhasSrinivasan SuhasSrinivasan changed the title Preserve repair order and publish output atomically on fatal errors Correct repair matching, ordering, and atomic output Aug 11, 2026
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