Shared text and DMR output failures can be silently discarded

[SuhasSrinivasan]

Summary

Several modkit-owned text-output paths can report success, panic, or return the wrong error after writing incomplete output. Shared writers use a single Write::write call where Rust permits a legal short write, buffered outputs often have no fallible command-level finalization step, and single-site DMR segmentation logs some output errors without returning them.

The same lifecycle gap affects pairwise and threaded DMR paths: an earlier processing or write failure can bypass output finalization, while thread joins use expect and can panic before the command can finish its writers. These behaviors reproduce at current upstream revision 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c with deterministic in-memory fault-injection writers and segmenters.

Severity

Severity: Medium — output integrity and reliability

Rationale: A legal short write can truncate a row, and a late buffered-write or DMR-segmenter failure can leave incomplete output while the command reports success. The triggering I/O failures are uncommon on healthy local storage, and deterministic scientific output is unchanged when all writes succeed, but affected output cannot be assumed complete solely from a zero exit status.

User and scientific impact

• Affected result or workflow: uncompressed modkit-owned text output used by pileup, extract/reporting utilities, sampled-probability artifacts, and DMR pairwise, single-site, isoform, and gene-transcript output paths.
• Direction of error: truncated rows or final buffered blocks, a missing final DMR segment, false success after a segmenter failure, panic during output handling, or replacement of the first causal error by a later lifecycle failure.
• Likely exposure: uncommon and environment-dependent for I/O failures; data- or execution-path-dependent for DMR segmenter and threaded-pipeline failures.
• Detectability or workaround: a panic or nonzero exit is visible, but a swallowed flush or segmenter error may only appear in a log. Writing to reliable local storage and validating expected rows/checksums reduces risk but does not repair the success contract.

Affected versions and environment

• Released version: modkit 0.6.4
• Development revision: 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c
• Operating system and architecture: macOS 26.6, arm64
• Output formats: uncompressed TSV, BED, bedMethyl, HTML, and other modkit-owned text artifacts
• Reproduction method: deterministic Rust Write and DMR-segmenter fault injection; no private sequencing data is required

Steps to reproduce

Minimal input

The first reproducer uses a legal writer that accepts at most two bytes from each write call:

#[derive(Clone)]
struct ShortWriter {
    bytes: Arc<Mutex<Vec<u8>>>,
    max_write: usize,
}

impl Write for ShortWriter {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        let n = buf.len().min(self.max_write);
        self.bytes.lock().unwrap().extend_from_slice(&buf[..n]);
        Ok(n)
    }

    fn flush(&mut self) -> io::Result<()> {
        Ok(())
    }
}

Inside writers.rs, wrap it in TsvWriter and write one complete row:

let sink = ShortWriter {
    bytes: Arc::new(Mutex::new(Vec::new())),
    max_write: 2,
};
let observed = sink.bytes.clone();
let mut writer = TsvWriter { writer: sink };
let expected = b"alpha\tbeta\n";

assert_eq!(writer.write(expected).unwrap(), expected.len());
assert_eq!(*observed.lock().unwrap(), expected);

For finalization, replace the sink with a Write implementation whose write accepts all bytes but whose flush returns io::Error::other("flush failed"). Exercise an in-scope uncompressed buffered pileup/text writer through its normal command-facing trait and assert that finalization returns that error rather than allowing it to be discarded during Drop.

For the DMR path, use a stub implementing the private DmrSegmenter trait with independently selectable errors from add, run_current_chunk, and clean_up. A minimal failing case is:

fn add(&mut self, _: &[ChromToSingleScores]) -> anyhow::Result<()> {
    Err(anyhow!("stub segmenter add failed"))
}

Pass an empty score batch through the same helper/caller used by SingleSiteDmrAnalysis::run and require the exact error to reach the caller. Repeat with run_current_chunk returning "stub segmenter final chunk failed".

Commands

On the affected revision, add the short-write snippet above as a writer_tests regression and run it with the workspace's actual package name:

git switch --detach 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c
cargo test -p mod_kit writer_tests -- --nocapture

The complete proposed carrier adds the segmenter/finalization fault-injection modules. Their verification commands are:

git switch --detach 096b04636d77e0d8cf8dd74021f953df2819004b
cargo test -p mod_kit writer_tests -- --nocapture
cargo test -p mod_kit segmenter_error_tests -- --nocapture
cargo test -p mod_kit output_finalization_tests -- --nocapture

The carrier commands are fix-green verification, not claims that those test modules already exist at the affected parent. All sinks are in-memory and deterministic, so no special filesystem, disk-full condition, or timing race is needed.

Control or independent oracle

Rust's Write::write contract permits a successful call to consume fewer than buf.len() bytes. A caller that requires a complete row must therefore retry until the entire buffer has been accepted, as write_all does.

Likewise, BufWriter may defer the underlying I/O until flush or drop. Errors encountered by Drop cannot be returned to the command. A command may report success only after every independently flushable, in-scope uncompressed text writer has been explicitly flushed. Complete writes are also required when feeding compressed writers, but ParCompress EOF, worker, and Drop failures remain excluded. If processing already failed, in-scope finalization should still be attempted without replacing the first causal error.

For single-site DMR output, an error from either DmrSegmenter::add or run_current_chunk means the segmentation artifact is incomplete; logging that error is not an adequate success result.

Observed behavior

At 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c:

TsvWriter::write("alpha\tbeta\n") returns 2 and stores only "al".
Some bedMethyl writer failures reach unwrap() and panic instead of returning Err.
RecordingWriter::drop ignores a final flush error.
DmrSegmenter::add errors are logged and processing continues.
DmrSegmenter::run_current_chunk errors are logged before the command can return Ok.
Pairwise/threaded DMR error paths do not consistently join, clean up, and flush before returning.

All results are deterministic under the fault-injection sinks. Successful-output controls remain byte-identical after replacing the incomplete writes and adding explicit finalization.

Expected behavior

• Every modkit-owned text row/header/artifact is either written completely or produces a returned error.
• A command reports success only after every independently flushable, in-scope uncompressed text writer has been explicitly and successfully flushed.
• An earlier processing or write error remains the returned causal error, while cleanup, thread joins, and writer flushes are still attempted in a deterministic order.
• DMR segmenter add, final-chunk, cleanup, writer, and pipeline-join failures return a nonzero command result rather than being logged and discarded or converted into a panic.
• If DmrSegmenter::add may already have emitted part of a chunk before failing, finalization does not retry that chunk and duplicate output.
• Successful output bytes, scientific values, row ordering, and schemas remain unchanged.

Root-cause evidence

• TsvWriter::write forwards one potentially short write call, and file headers do the same, at writers.rs:721-760.
• Sampled-probability HTML artifacts use one write call and have no explicit command-visible finalization at writers.rs:1145-1191.
• RecordingWriter::drop explicitly discards its flush result at writers.rs:1268-1287.
• Phased pileup output unwraps formatting, write, and join results at writers.rs:1407-1473.
• Single-site DMR logs and discards both segmenter-add and final-chunk errors at single_site.rs:300-355.
• Pairwise DMR can return an earlier error without finalizing its writer at pairwise.rs:248-309.
• Threaded DMR joins use expect, so a panic can bypass fallible output completion, at subcommands.rs:1413-1429 and subcommands.rs:1803-1875.

Proposed fix scope

• Use complete writes for modkit-owned text rows, headers, and probability artifacts.
• Add an explicit fallible finish contract to command-facing pileup and output writers for their in-scope ordinary-writer flushes, and make stdout writer construction fallible when it emits a header.
• Call finalization on both success and earlier-error paths. Attempt every owned cleanup/join/flush step, but preserve the first causal error.
• Propagate single-site DMR segmenter errors, finalize pending output in the correct order, and avoid retrying a possibly partially emitted chunk after add fails.
• Join all owned DMR threads and convert thread panics into returned errors before final writer completion.
• Organize the implementation as two readily reviewable commit groups within this issue: shared text-write/finalization contracts, followed by DMR lifecycle/thread-join propagation.
• Add short-write, failing-flush, first-error precedence, final-segment, and DMR lifecycle regressions.

Non-goals

• No claim that gzp::ParCompress worker, EOF, Drop, or encoder-finalization errors can all be returned safely; that requires an upstream-capability/API decision and should remain separately documented.
• No claim that rust-htslib BAM close/EOF errors are covered.
• No transactional output rollback, atomic replacement, fsync, or deletion of a partial file after failure.
• No broad cancellation rewrite for every producer/consumer pipeline.
• No scientific, statistical, schema, ordering, threshold, or performance change.

Acceptance criteria

• Legal short-write sinks receive exact complete rows and headers without changing successful bytes.
• In-scope write and ordinary-writer flush failures are returned rather than panicking or being discarded during drop.
• Every independently flushable, in-scope uncompressed command-facing text writer is explicitly flushed before success is reported.
• Single-site DMR add and final-chunk failures return their exact causal error; pending output is finalized without duplicating a partially emitted chunk.
• Pairwise and threaded DMR paths attempt joins, cleanup, and flushes on both success and failure while preserving the first error.
• Deterministic lifecycle tests cover short writes, header errors, write errors, flush errors, segmenter errors, thread panics, and finalization order.
• Existing successful text and DMR output remains byte-identical across applicable thread counts.
• Focused tests, the core suite, and the full workspace suite pass.

Reproduction artifacts

Artifact	Size	SHA-256	Notes
In-memory ShortWriter	not applicable	not applicable	Deterministically accepts at most 2 or 3 bytes per legal write call
In-memory failing-flush writer	not applicable	not applicable	Deterministically returns io::Error from flush
Stub DMR segmenter	not applicable	not applicable	Independently injects add, final-chunk, and cleanup errors

Related work

• Fresh dependency resolution selects incompatible hts-sys bindings #665 / Pin hts-sys to bindings compatible with rust-htslib 0.46 #666 address the separate fresh-build dependency incompatibility and do not change output lifecycle behavior.
• Proposed PR: to be linked only after the implementation has been rebased onto the accepted dependency resolution and the complete test gate passes.
• Deferred upstream dependency: fallible completion of gzp::ParCompress output is intentionally outside this issue and remains documented for later upstream follow-up.

[SuhasSrinivasan]

The complete code change is in ready-for-review PR #697. 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.