feat(compressors): add streaming compression crate - #722
Conversation
Import the compressed crate as compressors and integrate it with the Oxidizer workspace dependency, documentation, coverage, and mutation conventions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cd05f9b-fab4-477e-bec1-5dd8aca5034a
Restore the imported interoperability fixtures byte-for-byte after text normalization altered their binary contents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cd05f9b-fab4-477e-bec1-5dd8aca5034a
Use repository spelling conventions, format uncommon numeric ratios as code, and regenerate the crate README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cd05f9b-fab4-477e-bec1-5dd8aca5034a
Add behavior-focused tests to close every uncovered line reported by the official two-config coverage gate (lcov-all-features.info and lcov-no-default.info), and add or extend tests to catch every mutant cargo mutants reported missed for the compressors package. Coverage: - Restructure Wrapper::expects_zlib_header to drop its unreachable Gzip match arm instead of excluding it; Gzip decompressors are never pooled, so the arm could never execute. - Use a captured format identifier in the chunk-size assertion in format/mod.rs so the assertion's argument shares a line with its always-executed condition. Mutants fixed with new or rewritten tests: - compression.rs: boxed Compressing::flush delegation. - limits.rs: RATIO_FLOOR_BYTES pinned to a literal `32_768`. - pool.rs: round trip and capacity bound coverage for decompressor and zstd pooling (previously only "disables recycling" and "poisoned pool" were tested). - zstd/mod.rs: WindowLog::MAX pinned to an independently computed expected value. - brotli/codec.rs, flate/codec.rs, zstd/codec.rs: mode mapping, remaining_output delegation to FormatLimits, Drop returning engines to the pool, and the flush completion guard in step(). Final results: - cargo coverage-gate --package compressors: 100.0%, OK. - cargo mutants -p compressors --no-shuffle --jobs 6: 421 mutants tested, 291 caught, 113 unviable, 17 timeouts, 0 missed. No new coverage exclusions or mutants::skip attributes were added; every gap was closed with a test or a structural refactor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cd05f9b-fab4-477e-bec1-5dd8aca5034a
…rmats Reworks the crate's public surface so that what is common to every format lives in one place, and only what is genuinely format-specific stays in the format modules. * `CompressorBuilder<T = ()>` and `DecompressorBuilder<T = ()>` replace the five per-format builders and the runtime-format ones. The type parameter names the format: `()` has not chosen one and gains a `build_gzip`-style method per enabled format plus `build_format(Format, ..)` returning a boxed operation, while `CompressorBuilder<Brotli>` gains brotli's own settings and a `build` returning the concrete compressor. Each format module keeps its own marker type, setters and `build`, so no shared code enumerates formats. * Builds that can fail now say so. Brotli and zstd validate their configuration as they apply it, so their `build` returns the new `BuildError` instead of deferring the failure to the first `pull`. * `Compressor` and `Decompressor` expose only `builder` and `new`; the operations moved onto `Compression`, `Compressing` and `Decompressing`, which now live in the `core` module along with the byte counters. * `Resources` bundles the memory provider and engine recycling that every operation needs, and is what the public APIs accept instead of a memory provider and a pool separately. Recycling is on by default, so `Pool` is now an implementation detail reached through `Resources::enable_pooling`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
… traits `Compressing` and `Decompressing` existed to carry one method each, which made every signature choose between naming a direction and naming the contract. `Compression` now carries both directions on its own: * `flush` moves onto `Compression` with a default that does nothing, which is the truth for decompression: its output is already produced as soon as the input allows, so there is nothing buffered to release early. Compressors override it. * `take_remainder` is gone, and with it the idea that a decompressor hands back input it did not use. All pushed input is consumed, so `TrailingData::Preserve` becomes `TrailingData::Ignore`: a single-stream decoder still stops at the end of its stream, it simply does not offer the bytes after it. * The runtime builders now produce `Box<dyn Compression<Mode = Compress>>` and `Box<dyn Compression<Mode = Decompress>>` rather than the direction traits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
…ions
The one-shot conveniences were provided methods on `Compression`, which meant
importing the trait to compress a buffer and reading `x.compress(input)` as
though the compressor were the thing being compressed. They are now plain
functions at the crate root:
compressors::compress(input, gzip::Compressor::new(resources))?
compressors::decompress(input, decompressor)?
Each takes the operation generically, so a concrete compressor stays statically
dispatched and unboxed, while a boxed one from `build_format` still fits. The
direction is part of the bound, so handing `compress` a decompressor does not
compile.
`process`, the loop both of them wrap, is now a `pub(crate)` free function
rather than a trait method: nothing outside the crate needed it once the two
directions had names of their own.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
`format` was a public module holding one public item, so every mention of a runtime format read `compressors::format::Format`. The enum is now `compressors::Format`, and the module that defines it is private, along with the `build_format` methods that have to know every format by name. The generator macros move out of it to `crate::macros`, where they no longer look like part of the runtime-format story. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
`gzip::CompressorBuilder` and friends were aliases for `CompressorBuilder<Gzip>`, which gave every builder two names and made the format modules look like they owned a builder type they do not. The shared type is the only name now; a format module contributes its marker, its own settings and its `build`, and nothing else. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
The bounds belong to the decompressor that enforces them, and the name now says so, matching the builder that carries them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
`Output` is what one step of the [`Compression`] contract reports, so it belongs with the trait rather than in a module of its own, and is reached the same way: `compressors::core::Output`, not `compressors::Output`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
The trait exists so an API can name an operation -- `impl Compression<Mode = Compress>` accepts any compressor and no decompressor. Driving one is this crate's business, so `push`, `pull`, `end_input`, `flush` and the byte counters are now `#[doc(hidden)]`, and the trait documentation says plainly that they are internal and can change: callers reach for `compress`, `decompress` or `CompressionStream`. Also repairs the intra-doc links that the recent moves left dangling -- the per-format builder aliases, `Pool`, `Output` and the private `builder` module -- so the documentation builds without warnings again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
`gzip` was on by default, so a dependent that wanted only brotli still compiled flate2 unless it remembered `default-features = false`. Nothing is on now: a dependent names the formats it actually speaks, and a build that names none still gets the contract, the builders and `Resources`. The crate documentation illustrates itself with gzip, so its examples grow the hidden `#[cfg(feature = "gzip")]` shims that let a doctest compile either way, and the intra-doc links that need a format follow the workspace pattern of being checked only in a build that has one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
The crate documentation taught the `Compression` trait: the Streaming section was a hand-written push/pull loop, and Choosing a format explained boxed trait objects. Neither is what a caller should reach for, and both contradict the trait's own documentation, which now says its methods are internal. Streaming is `CompressionStream`, choosing a format is `Format`, and both examples draw their memory from the resources they compress with, which is the shape to copy. Security said the same thing three times and repeated calibration that `DecompressorLimits` documents properly. It now says what the exposure is, what to set for untrusted input, and where to read the detail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
✅ Version increments look sufficient
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #722 +/- ##
=========================================
Coverage 100.0% 100.0%
=========================================
Files 587 605 +18
Lines 63007 64398 +1391
=========================================
+ Hits 63007 64398 +1391
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…e gap Two CI failures, both from this branch. `anvil-fmt` checks with the pinned nightly rustfmt, which honours `format_code_in_doc_comments`; a stable `cargo fmt` silently drops that option, so the code inside doc examples was never formatted locally. Reformatted with the same toolchain CI uses. Coverage sat at 99.7% against a 100% gate, on nine lines this branch introduced: the default `flush` -- which only a decompressor reaches, and nothing called -- and the byte counters a boxed operation forwards. Both are now covered by tests worth having: that flushing a decompressor is a no-op rather than an error or an end of stream, and that boxing an operation does not lose its counters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
The tokio_stream example drove its synthetic upstream with tokio::time::interval directly. A tick::PeriodicTimer over a tick::Clock does the same thing while keeping the example honest about how time should be reached in this workspace: a test can drive the clock instantly instead of waiting on the runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
`Drop` moved the engine into the pool unconditionally, so a pool that could not keep it -- disabled, poisoned, or already at capacity -- freed it inside `Drop::drop`, while the value being destroyed was still borrowed. Borrow the engine instead and take it only when it will be stored, leaving the rest to ordinary drop glue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
Miri cannot run either of the crate's native compression engines. `zstd-safe` binds the native zstd library, and Miri cannot call foreign functions at all; `flate2`'s `zlib-rs` backend trips Stacked Borrows whenever a deflate or inflate stream is dropped, an open upstream soundness bug (trifectatechfoundation/zlib-rs#491) with no released fix. Only the brotli path would survive, which does not justify gating every other format's tests on `cfg(miri)`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
|
🔄 [AspBot] ## Automated multi-facet review — PR #722 ( This PR was reviewed across build, correctness, complexity, consolidation, idiomaticity, documentation, security, and performance facets. Overall this is a well-engineered, defensively-written, and unusually well-documented crate. The typestate builders, sealed traits, canonical error type, Overall assessment: REQUEST CHANGES — one High-severity, safe-by-default hardening item; everything else is Medium/Low polish.🔴 HighH1 — Decompression is effectively UNBOUNDED BY DEFAULT for every format (CWE-409/770/400)
🟠 Medium
🟡 Low (defense-in-depth / polish)
✅ Verified cleanNo memory-safety bug, no exploitable panic, no integer-overflow bug. All 4 Build/clippy/test status
Review performed by an automated multi-agent review team. Line numbers reference head |
… soundness Addresses an automated multi-facet review, plus three rounds of follow-up review that corrected the first two attempts at the main finding. Decompression was effectively unbounded by default: brotli declared no bounds at all, and no format bounded total output or concatenated stream count. Ratio bounds alone cannot separate a bomb from legitimate highly-compressible data. The bounds belong to the APIs that accumulate, not to every decompressor. `Pump` counts output for its whole life and never resets, so a cap in `FormatLimits` would have capped total bytes ever produced rather than bytes buffered -- breaking the crate's central promise that a stream of any length passes through in bounded memory. Instead a single `DecompressorLimits::for_buffered_output` fills the bounds a caller left unset, and only the entry points that buffer a whole result apply it: each format's `decompress` and `decompress_with_limits`, and the same pair on `Format`. Explicit values and explicit removals survive untouched, so overriding one bound can no longer silently drop the others. Driving a decompressor directly, or through `CompressionStream`, still carries only the format's ratio bound. `Codec` is now an unsafe trait. Its reported output count is load-bearing -- the engine declares exactly that many bytes of uninitialized capacity initialized -- so the obligation now sits on implementors where the compiler can see it, rather than in a doc comment. Zstd writes through `zstd_safe::WriteBuf` instead of zero-filling the output chunk before every step and transmuting it. That removes a memset of up to 64 KiB per step and one of the two copies of the unsafe `initialize` helper. A truncated later member now reports `unexpected_end_of_stream` rather than `corrupt_data`. Reaching that branch means the codec wants input that is not coming; whether an earlier member completed says nothing about it, and data the codec knows to be malformed already fails through its own error path. Also removes the write-only `Pump::done_reported` field. Testing: every test now runs in under a second, down from a worst case of 16.5s, by building large fixtures cheaply rather than compressing megabytes. Every drain loop is bounded, so a test that would spin now fails instead of hanging -- which also lets mutation testing reach a verdict. The handful of mutations that remove termination outright are marked skipped with their reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
Miri cannot run either of the crate's compression engines: `zstd-safe` binds the native zstd library and Miri cannot call foreign functions, while `flate2`'s `zlib-rs` backend trips Stacked Borrows whenever a deflate or inflate stream is dropped (trifectatechfoundation/zlib-rs#491). The crate already carries `package.metadata.anvil.miri.exclude`, which the `anvil-miri` recipe honours and which is why the `pr-runtime-analysis` job passes. This job builds its own `cargo miri` command line, so the exclusion has to be spelled out here as well. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2214a6ae-e9a0-4550-bff3-63b47d8c9ed8
There was a problem hiding this comment.
🔵 Needs a closer look
There are correctness issues in the new crate’s fallible-format compress convenience (panic path) and a test that doesn’t actually exercise the custom-memory-provider path it claims to verify.
Review details
Suppressed comments (2)
crates/compressors/src/tests/round_trip.rs:306
- This test intends to verify that a caller-supplied memory provider is used, but it never uses the
memoryit constructs: it callsgzip::compress/...decompresswithResources::default(), which uses the crate's global resources.
Create a Resources from memory and pass it through so the test actually exercises the custom-provider path.
crates/compressors/src/macros.rs:149
- In the fallible-compressor macro branch,
compress(...)callsCompressor::new(resources), which uses anexpect(...)internally. That makes this convenience function capable of panicking on a build-time rejection (e.g., if an upstream engine version changes what it accepts), even though the signature advertises error reporting viaResult.
Prefer building via the builder and propagating BuildError (which already converts into Error) so this function never panics for configuration rejection.
pub fn compress(input: impl $crate::InputData, resources: &$crate::Resources) -> Result<BytesView> {
let input = $crate::InputData::into_view(input, resources);
$crate::compress(input, Compressor::new(resources))
}
- Files reviewed: 37/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
The seal held against *implementing* `Compression` but not against calling
its supertrait methods. Trait-object method resolution treats supertrait
methods as inherent candidates, needing neither an import nor the supertrait
to be nameable, so any downstream crate could unsize a concrete compressor
and drive the crate-private mechanics:
let mut b: Box<dyn Compression<Mode = Compress>> = Box::new(gzip::Compressor::new(&resources));
b.push(view)?; b.end_input();
b.pull()?; // -> Data(BytesView { len: 27 })
That made `core/mod.rs`'s claim that the mechanics "can change freely"
untrue, and neither cargo-public-api nor cargo-semver-checks would have
flagged a break, because the supertrait is nominally crate-private.
The runtime-format half of this closed earlier when `build_format` started
returning a concrete `format::Compressor`. Adding `Sized` closes the rest:
no `dyn Compression` can be formed at all. Verified from an external probe
crate -- the unsizing above is now E0038, while boxing a concrete compressor,
writing `impl Compression<Mode = Compress>`, and runtime format selection all
still work.
`total_in` and `total_out` become unreachable downstream, which is not a loss:
they were only ever reachable through this hole.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
… setters Every other fluent setter in the crate uses the bare setting name -- level, limits, output_chunk_size, multi_stream, quality, max_window_log -- so the with_max_* family was a second convention for an identical builder shape. The with_ prefix marked neither a conversion nor a state transition. with_max_ratio -> max_ratio with_max_output_len -> max_output_len with_max_streams -> max_streams The explicit unbounded variants are renamed with it, since without_ is meaningless once its with_ counterpart is gone: without_max_ratio -> unbounded_ratio without_max_output_len -> unbounded_output_len without_max_streams -> unbounded_streams Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
The method sets how many idle engines are retained; pooling is already on after Resources::new. So "enable" described neither the common non-zero use, which adjusts capacity, nor the prominently documented enable_pooling(0), which reads as enabling the thing it disables. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
BrotliCompress::new returned Result<Self, BuildError> for a rejection the backend cannot produce here. brotli's set_parameter refuses only an already-initialized encoder or an unrecognized parameter; the state is created two statements earlier and all three identifiers are recognized. Quality, WindowSize and Mode validate on construction and the portable Level maps into 0..=11, and an existing test walks the entire expressible configuration space. So every caller configuring brotli was handling an error that could not occur, and the crate carried both build paths plus an unreachable error helper. The parameters are still checked, but as an assertion: a refusal now means the encoder's contract changed under us, not that the caller configured something invalid. zstd stays fallible -- its native library genuinely validates what it is given -- so BuildError remains, with its gate and its doctest moved off brotli. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
| feature = "zlib", | ||
| feature = "zstd" | ||
| ))] | ||
| pub use level::Level; |
There was a problem hiding this comment.
🤖: The no-format build no longer exports Level, even though its public builders still expose methods that require it. Restore the unconditional pub use level::Level, or gate every public API that names Level consistently.
When pub use format::Format was removed, its #[cfg(...)] remained and now applies to the next item, Level. A downstream crate using the advertised featureless contract can therefore name CompressorBuilder, but cannot supply the public type required by .level(...).
| /// Compresses one complete byte sequence that is already in memory. | ||
| /// | ||
| /// Takes any compressor: a concrete one such as [`gzip::Compressor`], or a | ||
| /// boxed one whose format was chosen at runtime. The direction is part of the bound, so a |
There was a problem hiding this comment.
🤖: Non-blocking — The compress docs still promise a boxed runtime-selected compressor, but that type no longer implements Compression. Name the concrete format::Compressor returned by build_format instead, and make the same correction in the generated compressor docs in src/macros.rs.
The runtime-format redesign removed impl Compression for Box<dyn Compression> and made Compression: Sized, so readers following the boxed guidance reach a value that cannot be passed to this helper.
There was a problem hiding this comment.
🔵 Needs a closer look
A newly added test claims to verify use of a custom memory provider but currently uses Resources::default() (ignoring the custom provider), so it does not validate its stated behavior.
Review details
Suppressed comments (1)
crates/compressors/src/tests/round_trip.rs:307
- This test claims to validate that a custom
MemorySharedprovider is used for output, but it still callsgzip::compress/gzip::decompresswith&Resources::default(), so thememorycreated here is unused (and theview(..)input is also allocated from an unrelated throwaway pool). Build aResourcesfrommemoryand use it for both calls (and pass a slice so input is allocated from that same provider too).
- Files reviewed: 37/40 changed files
- Comments generated: 0 new
- Review effort level: Lite
The crate's cross-cutting decisions were spread across the README, rustdoc, backend modules, benchmarks and tests, so a maintainer changing one layer had to reconstruct its relationship to the others. Two guides now carry the decisions that no single API item can: - docs/design.md: user-visible policy -- format selection and the raw-DEFLATE vs HTTP-deflate split, what is uniform across formats and what deliberately is not, the retained-output rule that shapes decompression bounding, stream framing defaults, resources and recycling, and why Compression needs both a private supertrait and a Sized bound to be sealed. - docs/implementation.md: the mechanisms -- the pump state machine and the push/pull outcomes, the unsafe initialized-output contract every backend adapter must honour and how each family satisfies it, the two runaway guards, engine pooling with the reasons each engine is in or out, async driving rules, runtime-format dispatch, and the test-build superset convention. Both link to rustdoc and existing workspace docs rather than restating them, to keep the synchronization cost proportionate. Linked from the crate docs, so they also appear in the generated README; docs/**/*.md is already in the packaging allowlist, and these are markdown only, so no LFS concern. Also fixes a duplicated summary line on the Codec trait that had been there since 05ec5ee. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of concrete correctness/documentation issues in new code (a test that doesn't exercise what it claims, and a broken intra-doc link) that should be fixed before approval.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
crates/compressors/src/zstd/codec.rs:28
- This intra-doc link points to
crate::zstd::CompressorBuilder, but the zstd module doesn't define aCompressorBuildertype alias. This will render as a broken rustdoc link; link directly to the specializedcrate::CompressorBuilder<crate::zstd::Zstd>method instead.
crates/compressors/src/tests/round_trip.rs:306
- This test claims to validate that a custom
MemorySharedprovider is used, but the compression/decompression calls use&Resources::default()instead of resources built from theGlobalPoolcreated in the test. As written, it doesn't exercise the custom-provider path it describes.
crates/compressors/src/zstd/codec.rs:350 - The test hard-codes zstd's level range as
1..=22, which can become inaccurate as the bundled zstd library changes. Usingzstd_safe::min_c_level()/max_c_level()makes this assertion validate the real engine contract.
- Files reviewed: 39/42 changed files
- Comments generated: 0 new
- Review effort level: Lite
| //! # Fallible builds | ||
| //! | ||
| //! Most engines take their configuration without validating it, so their builders cannot fail. | ||
| //! Brotli and zstd validate as they apply it, so theirs return a [`BuildError`][crate::BuildError]. |
There was a problem hiding this comment.
🤖: Non-blocking — The fallibility summary still says Brotli builders return BuildError, but this push made Brotli compressor construction infallible. Update this section and the matching summary in src/tests/format_contract.rs to identify zstd as the only fallible format.
BrotliCompress::new now returns Self, the Brotli macro invocation selects compressor_build = infallible, and the public BuildError documentation already reflects the new contract. Leaving the generator documentation unchanged gives maintainers the wrong rule for choosing the build macro.
| | brotli | initializes the slice first, because its encoder takes `&mut [u8]` | | ||
|
|
||
| Brotli's zero-fill is a real cost that the other two do not pay, so it is done | ||
| with a bulk `fill` rather than per element. `UninitOutput::filled_until` clamps |
There was a problem hiding this comment.
🤖: Non-blocking — This soundness guide attributes the crate-wide over-report defense to the zstd-only UninitOutput::filled_until helper, and says its clamp rejects an engine report. Describe Pump::pull's produced > provided_output check as the guard that rejects over-reports before BytesBuf::advance.
filled_until only bounds the initialized prefix that zstd can read back; it neither exists for the flate and Brotli adapters nor rejects a count. The engine-independent check in Pump is what keeps an excessive produced value from marking uninitialized output as initialized for every backend.
|
|
||
| Two consumption models are offered and they differ in what the *caller* retains: | ||
|
|
||
| - Driving a compressor directly yields one bounded chunk at a time, so a consumer |
There was a problem hiding this comment.
🤖: Non-blocking — This guide presents direct compressor driving as a public consumption model, but downstream callers cannot invoke push, pull, or end_input because those methods live on the crate-private CompressionInternal trait. Describe CompressionStream as the public incremental model instead, including its futures-stream feature requirement.
As written, readers are directed toward an API the crate intentionally sealed in this same push; only the whole-buffer helpers and CompressionStream are available to callers.
cargo-mutants on this branch's diff left three mutants alive in format.rs, which would fail the pr-mutants job: replace <impl CompressionInternal for Compressor>::total_out -> u64 with 1 replace <impl CompressionInternal for Decompressor>::total_in -> u64 with 1 replace <impl CompressionInternal for Decompressor>::flush with Ok(()) The two counters were asserted with > 0, which a mutant returning 1 also satisfies. They now assert the exact byte counts the operation actually moved. The flush mutant was equivalent rather than a test gap: CompressionInternal supplies a default flush of Ok(()) because decompression has nothing to flush, and no format overrides it, so the runtime-format forwarder could never differ from the default. Removed the forwarder instead of writing a test that cannot distinguish anything. The compressor's flush is real and is now pinned by asserting that a flush after end of input is refused, which the previous call site -- landing on a state that accepts a flush -- could not. Re-running cargo-mutants over every total_in/total_out/flush mutant in format.rs: 14 tested, 14 caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
The six adapter types are the boundary between the native engines and the shared unsafe Codec contract, but a maintainer had to combine constructors, Codec impls, stream-end hooks and Drop impls to learn when native state is reset or recycled, and which fields span one stream rather than one operation. Those are exactly the invariants that concatenated-stream correctness and the initialized-output boundary depend on. Each type now says what it owns and for how long: which fields last the whole operation and return to the pool on drop, which are fixed policy from the builder, and which span a single stream and drive the deferred reset. Also records why FlateDecompress has no pool for gzip and why the zstd compressor pool is unkeyed, at the declarations where those facts matter. Adds 'unkeyed' to .spelling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of correctness/API-consistency issues in the new code (notably a test that doesn’t actually exercise custom resources and a fallible-format compress convenience that can still panic) that should be fixed before approval.
Review details
Suppressed comments (2)
crates/compressors/src/tests/round_trip.rs:307
- This test claims to validate that a custom
MemorySharedprovider is used for output, but it builds/usesResources::default()(the global resources) for bothcompressanddecompress, so thememorycreated above is not actually exercised by the codec output path.
crates/compressors/src/macros.rs:149 - In the fallible format case, this
compressconvenience returnsResultand documents errors, but it callsCompressor::new(resources)which can panic if the engine rejects the default configuration. Building via the fallible builder keeps the API consistent (no panic) and letsBuildErrorconvert intoError.
pub fn compress(input: impl $crate::InputData, resources: &$crate::Resources) -> Result<BytesView> {
let input = $crate::InputData::into_view(input, resources);
$crate::compress(input, Compressor::new(resources))
}
- Files reviewed: 39/42 changed files
- Comments generated: 0 new
- Review effort level: Lite
The crate root carried expect(rustdoc::broken_intra_doc_links) for every build without both gzip and futures-stream. It silenced the nine genuinely feature-conditional links it existed for, but it silenced everything else too, so a stale or misspelled link anywhere in the public API would not have been caught in those configurations. Each of the nine now uses a code-formatted name where the target is not guaranteed to exist, with the enabling feature stated nearby. The suppression is gone, so rustdoc under -D warnings now checks intra-doc links in every configuration rather than only when both features happen to be on. Verified against nine configurations -- no features, each format alone, futures-stream alone, deflate+gzip, and all features -- all clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🔵 Needs a closer look
A newly added test (a_custom_memory_provider_is_used_for_output) does not actually use the custom memory provider it claims to validate, so it needs to be corrected before approval.
Review details
Suppressed comments (1)
crates/compressors/src/tests/round_trip.rs:307
- This test claims to validate use of a custom
MemorySharedprovider, but it never uses thememoryit creates: bothcompressanddecompressare run withResources::default()(the global resources). As written, it doesn't actually exercise the intended behavior.
- Files reviewed: 39/42 changed files
- Comments generated: 0 new
- Review effort level: Lite
body() built its own Clock::new_tokio(), so the comment claiming a test could drive the timer instantly was not true -- neither a test nor scripts/run-examples.rs could substitute a controlled clock, and the automated examples check spent seconds waiting on wall time and on runtime scheduling. body() now takes the clock, and main picks one: a ClockControl with auto-advance under IS_TESTING (which run-examples.rs sets), Clock::new_tokio otherwise. The requested period also moves from 50 microseconds, which PeriodicTimer clamps away, to the 1 millisecond it was actually getting. Measured on the same binary, identical output both ways: simulated clock: 0.068s real clock: 3.215s ClockControl needs tick's test-util feature, added to the dev-dependency only, so nothing downstream is affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🔵 Needs a closer look
It contains at least one behavior-test that does not actually exercise the intended custom-memory path and a macro-generated API that can panic on a fallible default build instead of returning an error.
Review details
Suppressed comments (2)
crates/compressors/src/macros.rs:149
- For fallible formats (e.g. zstd), this whole-buffer
compressconvenience can panic because it callsCompressor::new(resources), which uses.expect(...)if the engine rejects even the default configuration. Since this function already returnsResult<BytesView>, it should propagate a build rejection as an error (leveraging the existingFrom<BuildError> for Error) instead of panicking.
pub fn compress(input: impl $crate::InputData, resources: &$crate::Resources) -> Result<BytesView> {
let input = $crate::InputData::into_view(input, resources);
$crate::compress(input, Compressor::new(resources))
}
crates/compressors/src/tests/round_trip.rs:306
- This test claims to validate that a custom
MemorySharedprovider is used for output, but it builds compressors withResources::default()(global resources) and also constructs input viaview(...)which allocates from its own throwaway pool. As written, the custommemoryprovider isn’t actually exercised by the compressor/decompressor under test.
- Files reviewed: 39/42 changed files
- Comments generated: 0 new
- Review effort level: Lite
Adds
compressors, a streaming compression crate forbytesbufbyte sequences.Five formats, each behind a cargo feature of its own:
deflate,zlib,gzip,brotliandzstd. None is enabled by default, so a build that speaks only brotli never compilesflate2.Native
bytesbufintegrationInput is read segment by segment straight out of a
BytesView, and output is written into the uninitialized spare capacity of aBytesBuf. A view is a chain of segments, so nothing is flattened into a contiguous buffer on the way in and nothing is copied out of a scratch buffer on the way back. Every allocation comes from the caller's own memory provider.The whole-buffer conveniences take anything implementing the sealed
InputDatatrait, so a caller with a plain slice does not have to build a view first --gzip::compress(b"hello", resources)andgzip::compress(view, resources)are both accepted, and an existing view is forwarded without a copy.Resource pooling
Resourcescarries what a codec draws on -- a memory provider and recycled engine state -- and is what every API takes instead of the two separately.Building a compressor allocates and initializes a substantial amount of state; on a small message that setup can cost as much as the compression itself. Recycling it is therefore on by default, so a service compressing many small bodies spends its budget compressing rather than getting ready to.
Resources::global()shares one set process-wide,enable_pooling(n)sizes or disables it, and recycling is transparent: it applies to the engines that benefit and quietly skips the rest.Building a codec, then using it
Each format module's
compress/decompressis the whole-buffer convenience. When a setting matters, build the codec through its builder and hand it to the crate-levelcompress, which takes any compressor whatever built it:The same compressor can instead be driven incrementally, or handed to
CompressionStream; building it is the same either way. Committing to a format also unlocks that format's own settings, and the formats whose engines validate their configuration -- brotli and zstd -- report that frombuildrather than deferring it to the first chunk:Streamed compression and decompression
A codec is a state machine, not a one-shot transform, so a stream of any length moves through it with a bounded working set -- one pending input view and one output chunk, however many gigabytes pass through. Behind the
futures-streamfeature,CompressionStreampresents that as afutures_core::Stream, turning any stream of byte sequences into its compressed or decompressed counterpart.Runtime format selection
The
formatmodule is where a format that is only known at runtime lives, and it has the same shape as every compile-time format module: aCompressor, aDecompressor, andcompress/decompress/decompress_with_limits, with theFormatthreaded through.CompressorBuilder::build_formatproduces one when the level or the chunk size matters. It returns the module's ownCompressor-- a concrete type holding the chosen format internally -- rather than a boxed trait object, so the runtime-format path is not a second-class citizen and the mechanics that drive a codec stay out of the public API.Bounded decompression
Every one of these formats can expand its input by orders of magnitude. Nothing in the crate accumulates, so the exposure is in what a caller buffers:
DecompressorLimitsdocuments what each format bounds by default, why a ratio alone is not protection, and what to set for untrusted input.Each bound takes a non-zero type, so "allow nothing" is not expressible by accident.
Shape of the API
CompressorBuilder<T>/DecompressorBuilder<T>carry every setting that means the same thing in every format. The type parameter names the format:<()>has not chosen one and gains abuild_gzip-style method per enabled format plusbuild_format(Format, ..);<Brotli>gains brotli's quality, window and content mode.buildreturns aBuildErrorrather than deferring the failure to the first chunk.compress/decompressat the crate root take any operation, statically dispatched.core::Compressionis the contract the formats share, so an API can name an operation:impl Compression<Mode = Compress>accepts any compressor and no decompressor. How a codec is actually driven -- push, pull, end of input -- lives on a crate-private supertrait, so it is not public API.ErrorandBuildErrorboth implementrecoverable::Recovery, so a caller with a uniform retry policy can classify either. A truncated stream reportsUnknownrather thanRetry: re-running the same decode is deterministic, so whether asking again helps belongs to whoever owns the byte source.Error::otherwraps a foreign failure and detects its recovery from anio::Erroranywhere in the cause chain;Error::other_with_recoverytakes the classification when the caller knows better.Resources, and theformatmodule's types -- there is simply noFormatvariant to hand them.Testing
--all-features.