Repository navigation
fix(bam-io): propagate errors writing the final BGZF EOF block - #1046
Conversation
BgzfWriterEnum::finish on the single-threaded path flushed and then dropped the noodles BGZF writer, leaving the EOF block to Drop, which discards the write error. A failure at the very end of the stream (e.g. disk full) produced an output without its EOF marker while the command exited 0. Finish by value via noodles Writer::finish so the error propagates, and flush the underlying writer on both paths. Errors reported only when the file is closed are still not checked. No change to output on success; failures now surface.
|
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 configuration
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
==========================================
- Coverage 96.47% 96.45% -0.03%
==========================================
Files 299 299
Lines 152214 152264 +50
==========================================
+ Hits 146854 146868 +14
- Misses 5360 5396 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch was successfully deployed
1 participant
Summary
BgzfWriterEnum::finishon the single-threaded path flushed the last data block and then dropped the noodles BGZF writer, leaving the EOF block toDrop, which discards the write error. A failure writing that final block (e.g. the disk filling at the very end) left an output without its EOF marker while the command exited 0.finishnow uses the consuming noodlesWriter::finish, so the EOF write error is returned, and then flushes the underlying writer on both the single- and multi-threaded paths. The multi-threaded path already propagated the writer thread's EOF error; it gains only the final inner flush.No change to output on success: exactly one EOF block is still written.
Audit
Other finish paths in
fgumi-bam-ioalready propagate:IndexingBamWriter::finish(vendored multithreaded writer,?on the join result) and the SAM-input BGZF framers (try_finish()?). No production caller relies on dropping aBgzfWriterEnum/RawBamWriterinstead of callingfinish.Limitation
finishsurfaces errors returned bywriteandflush, including the final EOF block. The underlying file is dropped rather than closed explicitly, so errors the OS reports only atclose()(e.g. deferred write-back on a network filesystem) are still not checked; that is left for a separate change.Tests
test_bgzf_writer_finish_propagates_eof_write_error: a sink that rejects the EOF-block write makesfinish()return that error (1 and 2 threads); fails onmainfor the single-threaded path.test_bgzf_writer_finish_propagates_inner_flush_error: a failing final flush of the underlying writer, after the EOF block is written, surfaces.test_bgzf_writer_finish_writes_exactly_one_eof_block: success path ends with exactly one EOF block (guards against finishing and then double-finishing on drop).