Repository navigation
Conversation
Closing an output file synced its data to storage before the checked close(2), which made the final close wait out the whole write-back: closing a 49 GB coordinate-sorted output took 61.5 s with the sync against 20-28 s without it (c7g.4xlarge, gp3). fgumi 0.7.0 did not sync its outputs, and this restores that behaviour deliberately. It differs from htslib, and so samtools, whose bgzf_close reaches fdatasync through hflush. Durability against a host crash is left to the caller. OutputFile::close is now just the checked close. The sync, the skip list of "sync unsupported" errnos it needed, and the OutputFile::unsynced constructor (added in #1047 and never released) are removed; callers use OutputFile::from. The temp-file close before a rename (BAI index, sort merge output) is likewise close-only, and "sync/close" error messages now say "close". Those renames stay atomic against a failure of the process, but no longer against a host crash or power loss, and their docs now say so. Kept from the change that added the sync: the checked close and its error reporting (EINTR treated as success), short-write detection through the checked flush and close of every output sink, the pooled writers' failure when the I/O writer ends short of the blocks its PermitPool issued, and the rejection of block serials the pool did not issue. Two outputs still sync, for reasons of their own and unchanged here: the metrics writer's temp-and-rename (fgumi-metrics, small files) and review's staged grouped BAM. A test scans every source file in the workspace and fails if the set of sync_data, sync_all, fdatasync, fsync or F_FULLFSYNC calls differs from those two, so re-adding the sync to OutputFile::close (or anywhere else) fails it, and so does removing an allowed one without updating the list. Changes output for: none. An error the OS reports only after close (deferred write-back) is no longer surfaced; errors reported at close still fail the command.
|
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
WalkthroughOutput files now check close errors without syncing data first. The change removes ChangesOutput close handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested labels: Merge Risk: ⚪ Minimal · up to Output close no longer waits for data to sync, as intended. The checked-close behavior remains, and no issue requiring a fix before merge was identified. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title uses valid Conventional Commit syntax, has the allowed perf type, uses a lowercase imperative description, and accurately states the main change. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
- Coverage 96.60% 96.59% -0.02%
==========================================
Files 304 304
Lines 155704 155953 +249
==========================================
+ Hits 150419 150635 +216
- Misses 5285 5318 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Closing an output file fdatasync-ed it before the checked
close(2)(added in #1047), so the final close waited out the whole write-back: closing a 49 GB coordinate-sorted output took 61.5 s with the sync against 20–28 s without it (c7g.4xlarge, gp3). That tail was the largest source of run-to-run variance in the sort benchmarks. fgumi 0.7.0 did not sync its outputs, and this restores that behaviour deliberately. It differs from htslib, and so samtools, whosebgzf_closereachesfdatasyncthroughhflush. Durability against a host crash is left to the caller.What changed
OutputFile::closeis now just the checked close. The sync, the list of "sync unsupported" errnos it needed, andOutputFile::unsynced(added in fix(io): sync and check close of output files #1047, never released) are removed; callers useOutputFile::from.sync_data/sync_all/fdatasync/fsync/F_FULLFSYNCcalls differs from the two that remain (the metrics writer's temp-and-rename andreview's staged grouped BAM, both unchanged here).Kept from #1047
The checked close and its error reporting (EINTR treated as success), short-write detection through the checked flush and close of every output sink, the pooled writers' failure when the I/O writer ends short of the blocks its
PermitPoolissued, and the rejection of block serials the pool did not issue.Output: unchanged. An error the OS reports only after close (deferred write-back) is no longer surfaced; errors reported at close still fail the command.