fix(cli): reject colliding output paths across every command output - #845
Conversation
PR #689 guarded only `--output` vs `--rejects`, and only in the consensus commands. The same collision — two of a command's outputs resolving to one destination — was unguarded for `--stats`, `--metrics`, histograms, and the `--metrics PREFIX` expansions, and is worse there: the write happens after the BAM is complete, so `fgumi simplex -o out.bam --stats out.bam` writes a correct BAM and then truncates it to a TSV. Generalize the pairwise guard into a set-based `reject_output_collisions` that takes every output path a command will open as `(path, flag)` pairs and rejects any two that resolve to one destination (stdout may be named at most once; the null device stays exempt). Wire it into simplex, duplex, codec, filter, correct, downsample, group (including the three `--metrics PREFIX` files, `-f`, and `-g`), dedup, and clip, each handing the guard its full output list before any writer opens. Also fix `open_pipeline_output` to delegate to `open_output_writer` rather than returning a `LineWriter`-backed `std::io::stdout()`, which tears every BGZF flush at each 0x0a; `open_output_writer` is the single place the `-` convention is honoured for BAM output. Addresses #715 (items 2 and 3). Item 1 — moving identity into the writer layer with a `dev`+`ino` check to catch symlink/hardlink/case-insensitive collisions a pre-open path comparison cannot — remains open as a follow-up.
|
@coderabbitai review |
|
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #845 +/- ##
========================================
Coverage 94.46% 94.46%
========================================
Files 187 187
Lines 115301 115405 +104
========================================
+ Hits 108921 109021 +100
- Misses 6380 6384 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch was successfully deployed
1 participant
Problem
PR #689 added
reject_colliding_outputs, but it only compared--outputagainst--rejects, and only in the consensus commands. The same collision — two of a command's outputs resolving to one destination — was unguarded for--stats,--metrics, histograms, and the--metrics PREFIXexpansions. It is arguably worse there, because the secondary write happens after the BAM is complete:The inventory across the CLI found ~11 commands with 2–5 output paths and no guard across the other pairs (e.g.
group's-Mprefix files landing on--output,dedup's three metrics files,downsample's two histograms).Addresses #715 (items 2 and 3).
What changed
Generalized the guard (item 2).
reject_colliding_outputs(output, secondary, flag)becomes a set-basedreject_output_collisions(&[(path, flag)]): given every output a command will open as(path, flag-label)pairs, it rejects any two that resolve to the same destination. stdout may be named by at most one target; the null device stays exempt; the identity comparison and its error messages reuse the existing pre-open(canonical parent, file name)logic, with wording neutralized for non-BAM outputs.Wired every multi-output command to hand the guard its full output list before any writer opens:
--output,--rejects,--stats--output,--rejects,--metrics--output,--rejects,--histogram-kept,--histogram-rejected--output,-f,-g, and the three--metrics PREFIXfiles--output,--metrics,--family-size-histogram,--duplication-ladder--output,--metricsFixed
open_pipeline_output(item 3). It re-implemented the stdout dispatch and returned aLineWriter-backedstd::io::stdout(), which tears every BGZF flush at each0x0a. It now delegates toopen_output_writer— the single place the-convention is honoured for BAM output, which returns a block-buffered stdout handle.Tests
common.rs): the existing output-vs-rejects cases, migrated to the set signature, plus new cases for a collision between any two outputs (--statson--output,--metricson--rejects), an all-distinct set, an empty set, and more-than-one-stdout.test_streaming_output.rs): a new case per command (simplex/duplex/codec/filter--stats; correct/dedup/clip--metrics; group--family-size-histogram) runs-o out.bam <flag> out.bamand asserts it is rejected, the error names the flag and the path, and the output BAM is never created (proving the guard fires beforeFile::createtruncates).Verified: full suite (
cargo ci-test) 3532 passed / 0 failed;cargo ci-lint(-D warnings) andcargo ci-fmtclean.Deliberately out of scope
open_output_writerreturnsdev/ino; reject a second open matching an already-open inode) to catch symlink / hardlink / case-insensitive collisions a pre-open path comparison cannot. The author deferred it from fix(sort, fastq, extract): stream-o -to stdout instead of creating a file named-#689 as an API change across the fivecreate_*_bam_writerhelpers plus the pipeline; it belongs in its own PR, so Output-collision detection is approximate, and --stats/--metrics are unguarded #715 stays open for it.simulatecommands.fastq-reads(-1/-2/--truth) andgrouped-reads(-o/--truth) are feature-gated test tooling where a collision yields a bad test file rather than silent production data loss. Easy to add if wanted.