feat(bgzf): add slice decompression and caller-driven buffer recycling - #714
Merged
Merged
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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe reader adds bounded, fixed-slice BGZF decompression with stored-frame, size, and CRC validation. The writer adds bounded buffer recycling and preserves queued blocks after write failures. Public exports expose the new decompression API and reusable decompressor type. ChangesBGZF I/O changes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Caller
participant decompress_into_slice
participant Decompressor
Caller->>decompress_into_slice: provide BGZF block and output slice
decompress_into_slice->>Decompressor: decompress DEFLATE payload
Decompressor-->>decompress_into_slice: return decompressed bytes
decompress_into_slice-->>Caller: return validated byte count
sequenceDiagram
participant InlineBgzfCompressor
participant Output
participant BufferPool
InlineBgzfCompressor->>Output: write queued compressed blocks
Output-->>InlineBgzfCompressor: return write result
InlineBgzfCompressor->>BufferPool: recycle successful eligible buffers
InlineBgzfCompressor-->>InlineBgzfCompressor: retain failed and subsequent blocks
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-runall #714 +/- ##
==============================================
Coverage ? 93.88%
==============================================
Files ? 206
Lines ? 117945
Branches ? 0
==============================================
Hits ? 110732
Misses ? 7213
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
nh/runall-04-bgzf-additive
branch
from
August 5, 2026 07:55
4000b6a to
86bacc8
Compare
Member
Author
|
@coderabbitai review |
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-bgzf/src/reader.rs`:
- Around line 548-587: Update decompress_into_slice to return Ok(0) immediately
when uncompressed_size is zero, before is_stored_block or DEFLATE dispatch;
preserve the existing output-length validation and nonzero decompression paths.
Add a regression test covering the BGZF_EOF payload with an empty output slice
and asserting successful zero-byte decompression.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd1db323-c182-41c0-bab7-928256e9dd8f
📒 Files selected for processing (3)
crates/fgumi-bgzf/src/lib.rscrates/fgumi-bgzf/src/reader.rscrates/fgumi-bgzf/src/writer.rs
Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
force-pushed
the
nh/runall-04-bgzf-additive
branch
from
August 5, 2026 18:01
86bacc8 to
bf39a45
Compare
Member
Author
|
Addressed in
One correction on the diagnosis, since it changes what the change is for. The finding states that with an empty Removing the new short-circuit leaves all 87 crate tests green, which is the same thing said another way: this is not repairing a failure. It is still worth taking, for the reason the finding is right about — One deviation from the suggested placement. The prompt says to return before the stored/DEFLATE dispatch while preserving the output-length validation; those two conflict if taken literally, because the size check sits between them. Placing the short-circuit ahead of the size check makes a caller passing a wrongly-sized slot for a zero-ISIZE block get a silent |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
added a commit
that referenced
this pull request
Aug 9, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
added a commit
that referenced
this pull request
Aug 10, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
This was referenced Aug 17, 2026
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
nh13
added a commit
that referenced
this pull request
Aug 23, 2026
#714) Three additive entry points that `fgumi-sort`'s arena engine and the pipeline BGZF steps need, landed ahead of their consumers so the crate arrives complete rather than growing under each later port. `decompress_into_slice` is the fixed-slice analogue of `decompress_block_slice_into`: it decompresses a block straight into a caller-sized `&mut [u8]` instead of appending to a `Vec`, which lets the sort ingest path decompress directly into an arena slot rather than into a staging buffer it then copies out of. It keeps the integrity guarantee its siblings provide -- the payload must exactly fill `out` and match the footer CRC32 -- and takes the same deflate stored-block fast path for level-0 input. `uncompressed_size` is the accessor that makes that contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is a `u32` sitting in the file, so sizing straight off the footer means allocating from unvalidated input -- up to 4 GiB from a corrupt block. This reads the footer off the same `&[u8]` (no owned `Vec`, so no copy of the block just to read four bytes) and bounds the claim to `MAX_UNCOMPRESSED_BLOCK_SIZE`. `decompress_into_slice` resolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge. The stored fast path is now shared three ways. `copy_stored_and_verify`'s framing checks were inlined in its body, so the slice variant would have had to duplicate them; they move to `parse_stored_frame`. The BTYPE dispatch predicate becomes `is_stored_block`, and the inflate-then-verify tail becomes `deflate_into_slice_and_verify`, both shared with `decompress_and_verify`. Extracting only the framing check would have left the same drift exposure one level up. `InlineBgzfCompressor::recycle_buffer` lets a `take_blocks` consumer hand a drained block buffer back to the pool. Only `write_blocks_to` recycled before, so a consumer driving the compressor with `write_all` + `flush` + `take_blocks` left the pool permanently empty and allocated a fresh output `Vec` for every block. `write_blocks_to` now routes through the same method. The pool is bounded on both axes -- count and buffer capacity -- because bounding the count alone would let one oversized `Vec` handed in by a caller sit there for the compressor's lifetime. Rewriting `write_blocks_to` to call `recycle_buffer` (a `&mut self` method) means draining into a temporary rather than holding a borrow on `completed_blocks`. That makes the write-error path recoverable, so it is handled rather than left as-is: the failing block and its tail are restored to the queue instead of being dropped by the `Drain` guard. They are documented as safe to inspect, not to replay -- `write_all` can commit part of the failing block before erroring, and the whole block is re-queued, so writing the queue again would repeat those bytes inside a gzip member.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2 of the
feat-runalllanding series, on top of #697. Three additive entry points infgumi-bgzfthat later phases consume, landed ahead of their callers so the crate arrives complete rather than growing under each subsequent port.Do not read this as a port of
feat-runall'sfgumi-bgzf.git diff main-runall feat-runall -- crates/fgumi-bgzfreads +512/−578, but almost all of the deletions aremainwork thatfeat-runallpredates:header.rsin full (#659's single BGZF header predicate), theCargo.tomlconversion to workspace dependencies, and every CHANGELOG entry from 0.3.1 through 0.5.0. Applying that diff would silently revert #614 and #659. The three additions were hand-ported instead, andBGZF_HEADER_SIZEis taken fromheader.rsrather than reintroduced as a local constant.What lands
decompress_into_slice— the fixed-slice analogue ofdecompress_block_slice_into. It decompresses a block straight into a caller-sized&mut [u8]rather than appending to aVec, so the sort ingest path can decompress directly into an arena slot instead of into a staging buffer it then copies out of. Same integrity guarantee as its siblings: the payload must exactly filloutand match the footer CRC32, and level-0 input takes the same deflate stored-block fast path.parse_stored_frame— extracted, not new.copy_stored_and_verifyhad its framing checks inlined in its body, so the slice variant would have had to duplicate them. Both copy paths now call the one parser, which is what keeps theVecand slice entry points from drifting apart on what counts as a well-formed stored frame.uncompressed_size— the accessor that makesdecompress_into_slice's contract safe to satisfy. A caller has to size the slot before calling, and ISIZE is au32sitting in the file, so sizing straight off the footer means allocating from unvalidated input — up to 4 GiB from a corrupt block. This reads the footer off the same&[u8](no ownedVec, so no copying the block to read four bytes) and bounds the claim toMAX_UNCOMPRESSED_BLOCK_SIZE.decompress_into_sliceresolves the size through it too, so what a caller allocates and what the decompressor accepts cannot diverge.parse_stored_frame,is_stored_block,deflate_into_slice_and_verify— extracted, not new.copy_stored_and_verifyhad its framing checks inlined in its body, so the slice variant would have had to duplicate them. The BTYPE dispatch predicate and the inflate-then-verify tail were duplicated for the same reason. All three are now shared withdecompress_and_verify, so theVecand fixed-slice entry points cannot drift on what a well-formed stored frame is or on the exact-fill invariant.InlineBgzfCompressor::recycle_buffer— lets atake_blocksconsumer hand a drained block buffer back to the pool. Onlywrite_blocks_torecycled before, so a consumer driving the compressor withwrite_all+flush+take_blocksleft the pool permanently empty and allocated a fresh outputVecfor every block.pub use libdeflater::Decompressor— so a consumer can name the type everydecompress_*entry point takes without declaring its ownlibdeflaterdependency, and so the version it names is necessarily the one this crate decompresses with.Two things here are not purely additive
Both fall out of the additions rather than being bundled with them, but they are behaviour changes and should be reviewed as such.
write_blocks_tonow bounds the pool and survives a write error. Routing it throughrecycle_buffermeans draining into a temporary rather than holding a borrow oncompleted_blocks, sincerecycle_buffertakes&mut self. Two consequences. The pool is now capped atMAX_POOLED_BUFFERS; previously this method pushed one buffer per block written with nothing capping the growth, so the "bounded" claim inrecycle_buffer's docs would have been false the moment the two paths disagreed. And the write-error path became recoverable, so it is handled: the failing block and its tail are restored to the queue instead of being dropped by theDrainguard. The oldfor block in self.completed_blocks.drain(..) { output.write_all(&block.data)?; }silently discarded every unwritten block on error, and the error says nothing about how far the write got.decompress_into_slicevalidatesout.len()against the footer ISIZE up front. The doc stated this precondition; nothing enforced it. Both decompress paths compare againstout.len()on the assumption that it is the ISIZE, so a mis-sized slot was reported as a fault in the block — the stored path claimedBGZF stored block ISIZE mismatch: footer=105, LEN=100for a block whose footer says 100. That names a value the footer does not contain and sends a reader after file corruption that isn't there, when the defect is in the arena that sized the slot.write_blocks_to's retained blocks are documented as safe to inspect, not to replay.io::Write::write_allloops overwrite, advancing past eachOk(n), so it can commit part of the failing block before an error surfaces — and the whole block is re-queued, not the unwritten remainder. Writing the queue again would repeat those bytes inside a gzip member and produce a stream no BGZF reader can decode.Both new functions land without callers
decompress_into_slicegets its first consumer at P3 (arena slots) and P4 (fgumi-pipeline-io'sspill_decompress).recycle_buffer's eventual consumer is the pipeline BGZF compress step.Worth naming explicitly:
crates/fgumi-sort/src/worker_pool.rs:2193is already atake_blocksconsumer of exactly the shaperecycle_bufferwas written for, and it still allocates a freshVecper block. It is left alone here because P3 rewrites that file wholesale; wiring it now would be undone in two phases. That gap is sequencing, not an oversight.Verification
cargo ci-fmt,cargo ci-lint,cargo ci-docclean; 7554 tests pass, 27 skipped.Every guard here was mutation-tested rather than assumed — revert the fix, confirm a named test fails:
case_2_bad_crc_storedcase_3_short_fillcase_4_isize_above_max>→>=)uncompressed_size_accepts_the_block_maximumcase_4_isize_above_max,case_7_too_short_blocktest_recycle_buffer_refuses_an_oversized_buffertest_recycle_buffer_repopulates_the_poolwrite_blocks_tobypass the captest_write_blocks_to_recycles_up_to_the_capThe first two matter most: both mutations previously left the whole suite green, so neither integrity check this crate advertises was actually pinned.
Two constants were measured, not assumed.
MAX_POOLED_BUFFER_BYTES(130560) against real block buffers, which are 65376 bytes at levels 0/6/12 on incompressible input — so no legitimate buffer is refused. And dropping the pop-siteclear()is safe becausebgzf0.4'sresize_uninitopens withVec::clear, i.e. the compressor clears unconditionally.Still outstanding from P1
fgumi-sortdoes not yet have thetest-utilsfeature. The tracker files it under P1, but it is inert until P3 gateschunk_sorter.rson it, so it belongs in that phase rather than shipping here as a dead feature flag.Summary by CodeRabbit
New Features
Bug Fixes
Performance