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 583 606 +23
Lines 62930 64533 +1603
=========================================
+ Hits 62930 64533 +1603
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.
🟡 Changes recommended
crates/compressors/src/core/output.rs matches on *self in several predicate/accessor methods, which attempts to move a non-Copy enum out of &self and should be rewritten to match on self by reference.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
crates/compressors/src/core/output.rs:65
as_datausesmatch *self/ref, which attempts to move the enum out of&self. Match onselfdirectly so this method can borrow the containedBytesViewwithout moving the enum.
pub fn as_data(&self) -> Option<&BytesView> {
match *self {
Self::Data(ref data) => Some(data),
_ => None,
}
crates/compressors/src/core/output.rs:72
is_need_inputmatches on*self, which would require moving the enum out of&self. Usematches!(self, ...)to match on the reference instead.
#[must_use]
pub fn is_need_input(&self) -> bool {
matches!(*self, Self::NeedInput)
}
crates/compressors/src/core/output.rs:78
is_progressmatches on*self, which would require moving the enum out of&self. Usematches!(self, ...)to match on the reference instead.
/// Whether the engine made progress and should be pulled again.
#[must_use]
pub fn is_progress(&self) -> bool {
matches!(*self, Self::Progress)
}
crates/compressors/src/core/output.rs:84
is_donematches on*self, which would require moving the enum out of&self. Usematches!(self, ...)to match on the reference instead.
/// Whether the stream has ended.
#[must_use]
pub fn is_done(&self) -> bool {
matches!(*self, Self::Done)
}
- Files reviewed: 40/42 changed files
- Comments generated: 1
- Review effort level: Lite
Three changes that came out of the red `testing` legs and the review of the
previous commit.
**The no-format build.** `--no-default-features` failed under `-D warnings`:
`format::{Compressor,Decompressor}::pull` take a `Destination` that the
`dispatch!` expansion never mentions when no format is enabled, and
`DecompressorLimits::buffered_ceiling` is then reachable from nothing. Both now
carry the same suppression the neighbouring `push` already had.
**One cfg alias instead of five features, 28 times.** The condition "some format
is enabled" was spelled out as a five-feature `any(..)` at every site, nine lines
each, and every future format would have to be added to each copy. A build script
now derives `cfg(any_format)` with `cfg_aliases`, so those sites read
`not(any(test, any_format))`.
Derived rather than declared as an internal feature that each format enables:
Cargo lets anything enable such a feature directly, and `cargo hack
--feature-powerset` does exactly that. That configuration claims a format exists
while none does, which is the inverse of what these sites assume -- it was tried
first and fails with E0004. A derived cfg cannot disagree with the features it is
computed from, and it adds nothing to the powerset. `test` stays at the use site
because a build script runs once per package and cannot answer it per target.
**A 1 MiB output cap in test builds.** Three tests allocated 64 MiB apiece, one of
them brotli-compressing that much, because they size their payload from
`DEFAULT_MAX_OUTPUT_LEN`. Nothing they assert depends on its magnitude. The
constant is now 1 MiB under `cfg(test)`; `SHIPPED_MAX_OUTPUT_LEN` keeps the
documented 64 MiB pinned by the doc-lock test. The suite drops from 34.5s to 1.0s.
Also adds the runtime-format counterpart of the buffering-ceiling test, covering
the case raised on the `format.rs` review thread.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
…conflict) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🟢 Approval recommended
The changes introduce a well-structured new crate with consistent API shape and strong test/benchmark/docs support, and the remaining feedback is limited to maintainability nits.
Review details
Suppressed comments (1)
crates/compressors/src/macros.rs:331
- These docs hard-code the buffering caps ("64 MiB" / "1024 streams") even though the authoritative values live in
limits.rsand are already documented onDecompressorLimits. Duplicating the numbers here risks doc drift if the policy changes; consider referencingDecompressorLimitsinstead of repeating the literals.
- Files reviewed: 41/43 changed files
- Comments generated: 2
- Review effort level: Lite
…utation testing `pr-mutants` reported it as the run's only two timeouts. The default answers `None`, so a mutant answering `Some(0)` or `Some(1)` is not wrong -- it hands the step loop a one-byte budget, and the probe byte keeps the pump producing exactly the right bytes, one step at a time. Every codec that keeps this default, which is every compressor, then runs orders of magnitude slower and the harness records a timeout rather than a verdict. The overriding implementations that had the same problem were excluded in `0128081a`; this is the trait default they inherit from, which that pass missed because a longer timeout still caught it locally. It no longer does: the harness derives its timeout from the suite's own runtime, and lowering the test-build output cap took that from 34.5s to 1.0s, so the budget is now the 20s floor. Brotli's and zstd's overrides stay in scope -- CI caught those inside 20s, so only what actually times out is excluded. 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 introduces a large new crate with unsafe-adjacent buffer/codec boundaries and a broad public API surface that warrants final human review despite only minor fixable nits found.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/compressors/src/tests/round_trip.rs:51
- Step cap is described as "within {MAX_STEPS} steps" but the assertion fails at exactly MAX_STEPS (it currently allows only MAX_STEPS - 1 iterations). This is a minor off-by-one that can make a non-spinning test fail earlier than the constant and docs imply.
crates/compressors/src/pool.rs:237 - The doc comment mentions a zstd compressor built for a
level, buttake_zstd_compressoris intentionally unkeyed and takes no level parameter. Updating the comment avoids confusion about how pooling works for zstd.
- Files reviewed: 41/43 changed files
- Comments generated: 0 new
- Review effort level: Lite
Martin Taillefer (geeknoid)
left a comment
There was a problem hiding this comment.
Five static passes — security, correctness, testing, performance, conformance — scoped to the diff. Nothing was executed: no build, test, coverage, mutation, or Miri run, so everything below is by argument from the source.
Two findings, both inline. The crate-level decompress retrofits the 64 MiB output ceiling but not the 1024-stream cap, so it is not at the parity with the per-format conveniences that its own Security table implies. And the two Level tests assert best <= fast, which equality satisfies — a codec that ignored the configured level would pass them.
What I attacked and could not break. The unsafe initialized-output contract holds at all four production sites: brotli bulk-fills before the MaybeUninit → &mut [u8] cast, zstd's filled_until now asserts in every build, and Pump independently rejects produced > provided_output before advance (engine.rs:472) — defence in depth rather than trust in the codec. The one-probe-byte overshoot is exactly one byte and never reaches the caller; remaining is recomputed per step so it cannot go stale; chunk_size - output.len() cannot underflow, since a full buffer always returns early. Per-format ratio defaults match the documented table. Pooling rests on one uniform decision — reset on checkout, not on return — which is what makes a failed or mid-stream engine harmless to recycle, and gzip decompressors are correctly excluded from it. The truncation-reports-Unknown claim has oracles at both layers. Optional deps are all mirrored as non-optional dev-dependencies, so default = [] does not mean CI tests nothing.
Open questions, each with the experiment rather than a verdict:
- The Miri exclusion is documented and I am not objecting to it, but it leaves the uninit-output contract as the crate's least-checked invariant. Brotli is pure Rust and would run — a brotli-only
cfg(miri)pass over the pump would cover the one cast that most needs it. limits.rs:315never exercisesoutput == input * ratio, so a>→>=mutant there may survive. A two-line boundary test settles it.- Only gzip has an independent decode vector. Decoding one crate-produced zstd or brotli stream with an external tool would retire the doubt for the rest.
| # Licensed under the MIT License. | ||
|
|
||
| [package] | ||
| name = "compressors" |
There was a problem hiding this comment.
I think this name is not appropriate as this crate in fact contains no compressors.
Consider bytesbuf_compression instead
There was a problem hiding this comment.
I think this name is not appropriate as this crate in fact contains no compressors.
Can you elaborate? We have a bunch of processors each under crate::<format>::Processor. While I am not opposed to bytesbuf_compression I would prefer one-word crate name if possible. (compressors is free)
…t ceiling `compressors::decompress` retrofitted the 64 MiB output ceiling onto an already-built decompressor but not the 1024-stream cap, so `decompress(untrusted, gzip::Decompressor::new(&res))` accepted unboundedly many concatenated members. That is precisely the case the stream cap exists for: many tiny members each pay a full engine setup while producing almost no output, so no output bound ever trips. Reported by @geeknoid. The two bounds now travel as one `BufferedFallbacks` value rather than two accessors, because travelling separately is how the gap happened -- the earlier change added the output half and silently kept no stream bound. `Pump` applies the stream fallback by narrowing `max_streams`, the same way it narrows `remaining_output`; whichever of the engine's own bound and the fallback is tighter decides, and neither applies to `Destination::Stream`. No matching check lands in `check_limits`: the narrowed gate stops the pump at the bound and refuses the *next* stream before it starts, so the count cannot run past it the way output can via the probe byte. Pinned end to end -- 1025 empty gzip members through the crate-level `decompress` are refused, and the four-row fallback table is asserted for the stream bound as it already was for output. Verified the test fails without the fix. 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 small but concrete correctness/maintainability issues in new test/helper code (stored in PR comments) that should be addressed before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/compressors/src/pool.rs:442
- Inside this
#[cfg(test)]module, the#[cfg(all(not(test), ...))]wrapper()definition can never be compiled, so the test always usesWrapper::Rawand never exercises the zlib reset path in a zlib-only feature build. Consider making wrapper selection depend on feature flags instead ofnot(test)so zlib-only configurations actually coverWrapper::Zlib.
crates/compressors/src/tests/round_trip.rs:50 StepGuard::stepincrements the counter and then assertsself.0 < MAX_STEPS, which means the maximum permitted steps is actuallyMAX_STEPS - 1while the panic message claims{MAX_STEPS}. This off-by-one makes the guard stricter than documented and can cause a failure exactly at the configured limit.
- Files reviewed: 45/47 changed files
- Comments generated: 0 new
- Review effort level: Lite
Adds `docs/SECURITY.md`, the threat model the crate is written against: what makes compressed input untrusted and why decompression does not upgrade content trust, which resource each budget bounds and which it does not, which defaults apply to which consumption mode, what the caller still owns, and how the claims are verified. Two pairings it states outright, because each is a case where the obvious bound does not fire: a ratio cannot bound output in a format with no structural expansion ceiling, and an output cap cannot bound stream count, because many tiny members cost engine setup while producing almost no output. Also records what is deliberately out of scope -- authenticity, compression side channels of the CRIME/BREACH family, upstream backend defects -- and the one known gap, that there is no fuzz target yet. The guides are now reached through a `documentation` module rather than a list of links in the crate root, so the crate docs point at one place and the prose stays as Markdown that reads on GitHub too. Addresses the security-model review thread. 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 introduces a large new crate with a broad public API surface and soundness-sensitive codec boundary, which warrants final human review.
Review details
- Files reviewed: 47/49 changed files
- Comments generated: 0 new
- Review effort level: Lite
…on surface The deterministic suite covers chosen cuts, chunk sizes and corruption offsets. What it cannot cover is their product: a member boundary landing on a `BytesView` span boundary, on output exhaustion, on an exact limit and on trailing bytes at once. This campaign explores that space. One target for every backend, since CI charges per target and what is interesting is the interaction between this crate's framing and *some* engine rather than any one of them. It generates a format, a payload, a corruption (truncate, bit flip, append, concatenate), a span layout, an output chunk size, a trailing-data and multi-stream policy, and a bound placed at or beside the real output size. Asserted: decompression never panics and always terminates, an unmutated stream round-trips, and a bound below the real output is refused with a limit error. Deliberately not asserted is that decompression *succeeds* -- malformed input is supposed to fail. Everything goes through the public API. The push/pull state machine is sealed, so this reaches it the way callers do, which is also the surface an attacker reaches: the operation ordering a fuzz-only entry point would expose is chosen by trusted calling code, never by input bytes. That avoids widening the public surface, or adding a feature, purely for the harness. Every generated dimension is capped -- 4 KiB payloads, 16 spans, 4 concatenated copies -- and a 1 MiB output ceiling is applied to every run underneath whatever the scenario asked for, so a generated expansion bomb cannot spend the campaign budget on one input. The first run found a bug, in the harness rather than the crate: a `JustUnder` bound on a one-byte payload clamps back to one byte, because a bound must be non-zero, so decoding correctly succeeded where the assertion expected refusal. The guard now requires two bytes for that case to be meaningful. No workflow or justfile changes: `tests/bolero_*.rs` is auto-discovered, and the existing `fuzz-testing` job gives each target a 60s libfuzzer run on Linux. 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 introduces a large new, security-sensitive crate (including unsafe codec boundary code and many new APIs), so it warrants final human review despite only minor actionable feedback.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/compressors/src/engine.rs:499
- The invalid byte-counts check is shared by both compression and decompression codecs, but the error message says "compression engine". If a decompressor backend misreports counts, this will be misleading during diagnostics; consider using a direction-neutral message (e.g., "codec" or "backend engine").
- Files reviewed: 48/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
…erify The gzip abandonment regression dirtied an engine in one `Resources` and checked the recovery through another. Each `Resources` owns its own pool, so the engine under test was never the engine that was dirtied -- the assertion compared a fresh engine against a fresh baseline and would have held however broken the reset was. The test name claimed reuse coverage the test did not have. Every test that asserts a specific engine history now uses a private pool holding exactly one idle engine, for both the setup and the verification. Capacity one means the engine a drop returns is the engine the next build receives; a private pool means no concurrently running test can take it in between. Verified the fix rather than assuming it: with `Pool::take_compressor`'s `reset()` removed, the repaired test now fails, and the recovered stream is missing the gzip magic entirely because it is mid-stream continuation data. Before the fix that mutation could not have been caught here. The shared pool stays where engine identity is not what is being asserted, and for the concurrency scenario, which is about a handle being shared rather than about which engine any one request gets. Addresses the pooling-determinism review thread. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
Several pooling tests were named as though they exercised a recycled engine -- `an_engine_abandoned_mid_stream_is_cleaned_before_reuse`, `levels_never_share_engines`, `pool_capacity_bounds_retention_without_changing_output`. For brotli, which recycles nothing, and for a gzip decompressor, which is deliberately never recycled, that claim is not true: no state is returned, so no reset could leak. The property they actually assert is universal, and is the one worth asserting: reuse is invisible. Renaming them to say so makes the claim honest without tying the suite to which engines happen to pool today. That coupling is what is being avoided -- gating these on the current pooling matrix would mean editing the contract every time an engine gains or loses a reset, when the property holds either way and the tests keep passing unchanged. The section comment now records that reasoning, and points at `Pool` for the exact mechanics: retention, keying, capacity and poisoning are tested there, while what lives in the contract is the caller-visible behaviour. No test bodies changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e45d226a-3e10-419e-a316-a16a849db56a
There was a problem hiding this comment.
🟢 Approval recommended
The changes introduce the new crate with consistent feature gating, documented security/limits model, and comprehensive tests/benchmarks, and no objective issues were found in the reviewed diffs.
Review details
- Files reviewed: 48/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new crate with multiple backends and soundness-sensitive codec boundaries, so it warrants final human review despite only minor nits found.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.spelling:885
.spellingcontains a duplicate entry forzstd's(it already appears earlier in the file). Duplicates add noise and make future dictionary edits harder to review.
- Files reviewed: 48/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new crate with multiple backends and unsafe codec adapters, so it needs final human review despite only minor specific issues found here.
Review details
Suppressed comments (2)
crates/compressors/src/core/output.rs:57
matches!(*self, ...)forces dereferencing (and, ifOutputisCopy, copying) the entireOutputvalue just to test the variant. SinceOutput::Datacarries a largeBytesView(noted above as ~272 bytes), these predicates should match on&selfto avoid an unnecessary copy.
pub fn is_data(&self) -> bool {
matches!(*self, Self::Data(_))
}
crates/compressors/src/core/output.rs:78
- Same as above:
matches!(*self, ...)needlessly dereferences (and potentially copies) the fullOutputvalue. Matching onselfavoids copying a large enum payload when checkingProgress/Done.
pub fn is_progress(&self) -> bool {
matches!(*self, Self::Progress)
}
- Files reviewed: 48/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
| } | ||
|
|
||
| fn run(scenario: &Scenario) { | ||
| let Some(&format) = Format::ALL.get(usize::from(scenario.format) % Format::ALL.len().max(1)) else { |
There was a problem hiding this comment.
🤖: The Linux fuzz job compiles this target with no compressor-format features, so every generated case returns at Format::ALL.get(...) and the advertised campaign performs no decompression. Pass --all-features (or explicit format features) to both Linux cargo bolero list and cargo bolero test invocations.
compressors declares default = []; unlike the non-Linux fallback, the Linux commands in justfiles/extended.just do not enable any features. This is the wiring gap in the earlier fuzz-target thread: with Format::ALL empty, line 123 returns for every scenario, so the required 60-second job can remain green without exercising a backend.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 460b188a-08ea-418c-802a-41d557329876
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large, security-relevant new crate (compression/decompression + unsafe codec boundary) and warrants final human review despite no specific blocking issues found in the provided diffs.
Review details
- Files reviewed: 49/51 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
crates/compressors/src/core/output.rs uses matches!(*self, ...) / match *self on a non-Copy enum, which needs to be corrected to inspect &self without moving.
Review details
Suppressed comments (3)
crates/compressors/src/core/output.rs:57
OutputcontainsBytesView(which is notCopy), so usingmatches!(*self, ...)attempts to move out of&self. Match onselfinstead so the enum is inspected by reference.
pub fn is_data(&self) -> bool {
matches!(*self, Self::Data(_))
}
crates/compressors/src/core/output.rs:65
as_datamatches on*self, which would move theOutputout of&self. Match onselfand bind the payload by reference.
pub fn as_data(&self) -> Option<&BytesView> {
match *self {
Self::Data(ref data) => Some(data),
_ => None,
}
crates/compressors/src/core/output.rs:72
- These predicate helpers also use
matches!(*self, ...), which tries to move out of&selffor a non-Copyenum. Usematches!(self, ...)consistently.
/// Whether the engine needs more input before it can produce more output.
#[must_use]
pub fn is_need_input(&self) -> bool {
matches!(*self, Self::NeedInput)
}
- Files reviewed: 49/51 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. Output buffers come from the caller's own memory provider, and so does any input view the crate has to build.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 an engine 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,with_pool_capacity(n)sizes or disables it, and recycling is transparent: it applies to the engines that benefit and quietly skips the rest.Building a compressor, then using it
Each format module's
compress/decompressis the whole-buffer convenience. When a setting matters, build the compressor through its builder and hand it to the crate-levelcompress, which accepts any implementor ofCompression, however it was constructed: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 zstd, whose native library validates the parameters it is given, reports that frombuildrather than deferring it to the first chunk:Streamed compression and decompression
An engine is a state machine, not a one-shot transform, so a stream of any length moves through it while the output it has buffered but not yet handed back stays bounded by the configured chunk size. Pending input and the engine's own window and tables are additional, and depend on the format. 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 an engine 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. Brotli's construction is infallible: no value its builders can express is one the encoder rejects.compress/decompressat the crate root take any engine, statically dispatched.core::Compressionis the contract the formats share, so an API can name an engine:impl Compression<Mode = Compress>accepts any compressor and no decompressor. How an engine is actually driven -- push, pull, end of input -- lives on a crate-private supertrait, and the trait isSizedso nodyn Compressioncan expose it. Neither is 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.ResourcesimplementsThreadAware. Relocation moves the memory provider, since a NUMA-aware provider will want the destination's memory; the engine pool is a deliberate no-op, because every clone shares it and an idle engine is plain memory with no affinity to where it was built.Documentation
docs/DESIGN.mdrecords the user-visible policies that span several APIs -- format selection, what is uniform across formats and what is not, how decompression is bounded, stream framing, and why the public surface is sealed.docs/IMPLEMENTATION.mdcovers the mechanisms: the pump state machine, the unsafe initialized-output contract every backend adapter honours, engine pooling and its exclusions, and the async driving rules.Testing
--all-features.