Repository navigation
fix(io): sync and check close of output files - #1047
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughOutput writers now use checked sinks that report flush, sync, and close failures. BAM, pipeline, simulation, and sorting paths use shared output handling. Sorting also tracks issued compression-job serials and detects missing or unissued results. ChangesOutput finalization and sort tracking
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~50 minutes Change: Bug fix Suggested labels: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established; the checked output-finalization paths are consistent with the intended behavior. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
==========================================
- Coverage 96.55% 96.55% -0.01%
==========================================
Files 300 303 +3
Lines 153875 154588 +713
==========================================
+ Hits 148577 149255 +678
- Misses 5298 5333 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
The pooled writers' io_writer_loop fails on a gap in the block serials it receives, but a lost final block leaves no gap: when its compress job is abandoned (a pool shutdown while the block is queued or compressing, or a worker panicking mid-compress) its result sender is dropped and the writer sees a clean end of input, so it stamped an EOF marker onto a truncated stream. Block serials are now issued by the writer's PermitPool, which counts them. Once its input closes, the writer fails if it wrote fewer blocks than were issued. It also rejects a block whose serial the pool did not issue, so a producer that bypasses the count fails on its first block instead of silently disabling the check. Changes output for: none on success; a truncated sort output or spill chunk now fails the command.
Output files were flushed and then dropped. On Unix File::flush is a no-op and dropping a File discards the close(2) result, so errors reported only after the last write (write-back EIO, or ENOSPC/EDQUOT flushed at close on NFS) were lost and the command exited 0. Add OutputFile and the OutputSink trait to fgumi-bam-io. Closing an OutputFile syncs its data (regular files only) and closes it with a checked nix::unistd::close (EINTR counts as success). As in htslib, a sync failing with EINVAL, ENOTSUP or EOPNOTSUPP is logged and skipped, as is ENOTTY (macOS F_FULLFSYNC on a filesystem without it); any other sync error fails. Stdout is a duplicated descriptor whose close is checked but which is not synced. Every production output now finishes this way: BAM writers (BgzfWriterEnum, IndexingBamWriter), the WriteBgzfFile/WriteRawFile pipeline sinks (WriteRawFile now opens - and /dev/stdout as the BAM sink does), the sort output writer, and the simulate FASTQ and TSV writers (close errors name the file). write_bai_index and the sort merge output close their temp before the rename, through a shared persist_after_close; the merge temp is not synced twice. Spill files, which this process reads back, are unchanged. Public API change: fgumi_bam_io::open_output_writer now returns Box<dyn OutputSink> instead of Box<dyn Write + Send>; callers should call close() when done. New: OutputFile, OutputSink, close_buffered, open_output_sink, persist_after_close. nix is now a dependency of fgumi-bam-io on all Unix targets, not only Linux. Changes output for: none on success; sync/close errors now fail the command (except unsupported sync).
40bab20 to
3fb5145
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Builds on #1046 (merged). Two commits, each building and passing tests on its own.
1.
fix(sort): fail when the I/O writer ends short of the blocks submittedThe sort's
io_writer_loopalready failed on a gap in the block serials it received, but a lost final block left no gap. If its compress job was abandoned (a worker panic, or the pool shutting down before the block was written), its result sender was dropped and the writer saw a clean end of input, so it stamped an EOF marker onto a truncated stream.Block serials are now issued by the writer's
PermitPool, which counts them (issue_serial). Once its input closes, the writer fails if it wrote fewer blocks than were issued. It also rejects any block whose serial the pool did not issue. A producer that bypasses the count therefore fails on its first block and can't silently disable the check. No production success path drops the pool early, so in practice this guards the worker-panic case.2.
fix(io): sync and check close of output filesOutput files were finished by flushing and then dropping the
File. On UnixFile::flushis a no-op and dropping aFilediscards the result ofclose(2). Errors the OS reports only after the lastwritewere lost, and the command exited 0 with a short or corrupt output. On NFS,ENOSPC/EDQUOT/EIOfrom flushing dirty pages first appear atclose. On local filesystems a write-backEIOis reported only byfsync/fdatasync.Every production output file is now finished with
sync_data(regular files only) and then a checked close (nix::unistd::close;EINTRcounts as success). This follows htslib'sbgzf_close. Like htslib'sfd_flush(hfile.c), a sync that fails withEINVAL,ENOTSUPorEOPNOTSUPPmeans "sync not supported" (e.g.F_FULLFSYNCon an SMB mount on macOS).ENOTTYis treated the same way, because macOS returns it fromF_FULLFSYNCon filesystems with no handler for it; htslib calls plainfsync, so it doesn't seeENOTTY. In that case the sync is logged at debug and skipped, and the checked close still runs. Any other sync error fails.Design
fgumi-bam-iogainsOutputFile(sync + checked close;OutputFile::unsyncedskips the sync), theOutputSinktrait (Write + Sendplusclose(self: Box<Self>)),close_buffered,open_output_sink(stdout for-//dev/stdout, otherwise anOutputFile), andpersist_after_close(close a temp, then rename it).OutputFile::unsynced). It is flushed and its close is checked, but it is never synced, and fd 1 stays open. Pipes and/dev/nullskip the sync but still get the checked close.fgumi_bam_io::open_output_writernow returnsBox<dyn OutputSink>instead ofBox<dyn Write + Send>. Callers should callclose()when done; dropping still works but discards errors.nixis now a dependency offgumi-bam-ioon all Unix targets, not just Linux.Sites
BgzfWriterEnum::finishandIndexingBamWriter::finish.WriteBgzfFile(every chain command's BAM output). The inline.baiis written only after the BAM closed cleanly.WriteRawFile(fgumi fastqoutput) now opens throughopen_output_sink.-and/dev/stdoutnow get the same checked stdout close as the BAM sink; before, they used a flush-onlyio::stdout()or reopened/dev/stdout.write_bai_indexand the sort'sMergeOutputTarget::persistboth usepersist_after_close, so the temp is closed before the rename and a failure leaves nothing at the destination. The merge temp is synced once, by its writer, not again before the rename.PooledBamWritercloses its output. Spill chunks are still just dropped, because this process reads them back.FastqWriter(both arms),ParallelGzipWriter, and the truth/includelist TSVs. Close errors name the file.Out of scope: spill and run files, telemetry outputs, and
fgumi-metrics'write_metrics_atomic. That function alreadysync_alls before its rename, and sharing the helper would add afgumi-metrics -> fgumi-bam-iodependency.Cost
Local measurements (macOS, where
sync_dataisF_FULLFSYNC): 6-8 ms for 100 MB and 8-70 ms for 2 GiB. The checked close takes about 10 µs. This has not been measured on EBS yet. Before merging, the plan is one fgumi-benchmarks AWScorerun on main (with #1046) and one on this branch.Changes output for
None on success; output bytes are identical. Sync/close errors now fail the command (except unsupported sync), and a truncated sort output or spill chunk now fails the sort.
Tests
StagingBufferserials come from the pool.output.rs:EBADFfor a descriptor closed underneath it, for both synced and unsynced files.EINVAL/ENOTSUP/EOPNOTSUPP/ENOTTYfrom sync are skipped and the file is still closed, whileEIO,ENOSPC,EDQUOTandEBADFstay fatal.EINTRmaps to success./dev/nulland pipes close cleanly.open_output_sinkhandles both stdout spellings and creates files.persist_after_closecloses before renaming, and a failure leaves the destination untouched and removes the temp.write_bai_indexandMergeOutputTarget::persistwith a failing close leave no output and no temp.BgzfWriterEnumwith 1 and 2 threads,IndexingBamWriter,WriteBgzfFile(with no.baiafter a failed close),WriteRawFile,PooledBamWriterplain and indexing,ParallelGzipWriter, andFastqWritersingle- and multi-threaded. Each checks that a close error surfaces, and that on success the sink is closed once with one EOF block.open_output_writermatches the same stream written in memory.EBADFtests close a raw descriptor, so they skip unless nextest's process-per-test mode is set.Risk verdict: Command output: none to grouping, consensus, sort order, corrected UMIs, or metrics; reported byte-identity tests pin output bytes.
unsafe: none added or modified; noCLAUDE.mdallowlist update is indicated. Memory bounds, queue capacity, and thread/backpressure policy: none.Fix: Detect missing final sort blocks and propagate output sync and close errors.
The supplied change summary reports tests for the new error paths and output identity. The shell output does not independently confirm the diff or test results.