Skip to content

feat(compressors): add streaming compression crate - #722

Open
martintmk wants to merge 63 commits into
mainfrom
user/martintomka/20260901-add-compressors-crate
Open

feat(compressors): add streaming compression crate#722
martintmk wants to merge 63 commits into
mainfrom
user/martintomka/20260901-add-compressors-crate

Conversation

@martintmk

@martintmk martintmk commented Sep 2, 2026

Copy link
Copy Markdown
Member

Adds compressors, a streaming compression crate for bytesbuf byte sequences.

Five formats, each behind a cargo feature of its own: deflate, zlib, gzip, brotli and zstd. None is enabled by default, so a build that speaks only brotli never compiles flate2.

let resources = Resources::global();

let compressed = gzip::compress(b"hello", resources)?;
assert_eq!(gzip::decompress(compressed, resources)?.to_vec(), b"hello".to_vec());

Native bytesbuf integration

Input is read segment by segment straight out of a BytesView, and output is written into the uninitialized spare capacity of a BytesBuf. 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 InputData trait, so a caller with a plain slice does not have to build a view first -- gzip::compress(b"hello", resources) and gzip::compress(view, resources) are both accepted, and an existing view is forwarded without a copy.

Resource pooling

Resources carries 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/decompress is the whole-buffer convenience. When a setting matters, build the codec through its builder and hand it to the crate-level compress, which takes any compressor whatever built it:

let compressor = gzip::Compressor::builder()
    .level(Level::HIGH)
    .output_chunk_size(chunk(16 * 1024))
    .build(resources);

let body = BytesView::copied_from_slice(b"a response body", resources.memory());
let compressed = compressors::compress(body, compressor)?;

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 from build rather than deferring it to the first chunk:

let compressor = brotli::Compressor::builder()
    .quality(brotli::Quality::new(4).expect("in range"))
    .window_size(brotli::WindowSize::new(18).expect("in range"))
    .mode(brotli::Mode::Text)
    .build(resources)?;

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-stream feature, CompressionStream presents that as a futures_core::Stream, turning any stream of byte sequences into its compressed or decompressed counterpart.

Runtime format selection

The format module is where a format that is only known at runtime lives, and it has the same shape as every compile-time format module: a Compressor, a Decompressor, and compress / decompress / decompress_with_limits, with the Format threaded through.

let format = Format::from_content_encoding(encoding).expect("a supported encoding");

let compressed = format::compress(format, b"negotiated body", resources)?;
let plain = format::decompress(format, compressed, resources)?;

CompressorBuilder::build_format produces one when the level or the chunk size matters. It returns the module's own Compressor -- 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.

let compressor = CompressorBuilder::new()
    .level(Level::FAST)
    .build_format(format, resources)?;

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: DecompressorLimits documents what each format bounds by default, why a ratio alone is not protection, and what to set for untrusted input.

let decompressor = gzip::Decompressor::builder()
    .limits(DecompressorLimits::new().with_max_output_len(NonZeroU64::new(8 * 1024 * 1024).unwrap()))
    .build(resources);

let plain = compressors::decompress(untrusted, decompressor)?;

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 a build_gzip-style method per enabled format plus build_format(Format, ..); <Brotli> gains brotli's quality, window and content mode.
  • Brotli and zstd validate their configuration as they apply it, so their build returns a BuildError rather than deferring the failure to the first chunk.
  • compress / decompress at the crate root take any operation, statically dispatched.
  • core::Compression is 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.
  • Error and BuildError both implement recoverable::Recovery, so a caller with a uniform retry policy can classify either. A truncated stream reports Unknown rather than Retry: re-running the same decode is deterministic, so whether asking again helps belongs to whoever owns the byte source.
  • Error::other wraps a foreign failure and detects its recovery from an io::Error anywhere in the cause chain; Error::other_with_recovery takes the classification when the caller knows better.
  • A build with no format enabled still gets the shared contract, the builders, Resources, and the format module's types -- there is simply no Format variant to hand them.

Testing

  • One contract suite runs every format through the same scenarios, so a format that behaves differently from its siblings fails there rather than surprising a consumer.
  • 100% line coverage, no surviving mutants. Test modules are excluded from the coverage gate, so the figure is production code.
  • Clippy clean across the feature matrix, including a build with no format at all, and doctests pass on a single-format build rather than only under --all-features.

martintmk and others added 14 commits September 1, 2026 16:34
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
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Version increments look sufficient

cargo semver-checks compared the 1 crate(s) this PR publishes against their previous version-bump commit in git history. Every version increment is sufficient for the detected API changes.

Crate Baseline Baseline commit This PR Minimum required Status
compressors new crate 0.1.0 0.1.0 ✅ ok

This check is informational and does not block the merge.

View the check run

@martintmk martintmk added the agency-rocket Touched by a rocket skill label Sep 2, 2026
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (1b0f3fe) to head (a3f1881).
⚠️ Report is 2 commits behind head on main.

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     
Flag Coverage Δ
linux 69.3% <100.0%> (?)
linux-arm 68.5% <100.0%> (?)
windows 69.3% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread crates/compressors/examples/tokio_stream.rs Outdated
…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
Comment thread crates/compressors/benches/compressors_codec.rs
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
Comment thread crates/compressors/src/stream.rs Outdated
martintmk and others added 3 commits September 2, 2026 13:43
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
@martintmk

Copy link
Copy Markdown
Member Author

🔄 [AspBot] ## Automated multi-facet review — PR #722 (feat(compressors): add streaming compression crate)

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, MaybeUninit-based zero-copy output path, and saturating limit arithmetic are all high quality.

Overall assessment: REQUEST CHANGES — one High-severity, safe-by-default hardening item; everything else is Medium/Low polish.


🔴 High

H1 — Decompression is effectively UNBOUNDED BY DEFAULT for every format (CWE-409/770/400)
FormatLimits::new(max_ratio, max_output) has no stream-count parameter, and per-format decompressor defaults ship no absolute output cap — brotli defaults to new(None, None) (zero bounds), zstd to ratio-only 250,000×, deflate/gzip to ratio-only 1,100×. decompress() accumulates all output into one BytesBuf, so a small crafted body can expand to multi-GB → OOM DoS on the crate's stated use case (decompressing untrusted Content-Encoding bodies). Ratio-only caps are not real protection (250,000× lets ~1 MiB → ~244 GiB). The enforcement mechanism itself is sound — only the default policy is permissive.

  • Fix (prepared): add a max_streams param to FormatLimits::new and ship conservative safe-by-default caps for every decompressor — 64 MiB absolute output + 1024 streams — with explicit opt-out via with_max_output_len / with_max_streams / UNLIMITED. Values chosen to sit above the largest legitimate default-path test payload (~22.5 MiB) and concatenation count (2–3), so existing tests remain green. Also update the two doc lines that now contradict the bounded defaults (limits.rs:67-68, flate/mod.rs:21).

🟠 Medium

  • Security M1 — zstd decompressor window-log defaults to 128 MiB for untrusted input (zstd/codec.rs:190-194); consider a stricter max_window_log default or document the per-stream cost.
  • Security/Correctness M2/M3 — gzip multi-member: unbounded member count + a fresh new_gzip inflate-state allocation per member (raw/zlib recycle via reset(), only gzip reallocates) → ~1000× alloc/CPU amplification from many tiny empty members. Truncated subsequent member is mislabeled corrupt_data vs unexpected_end_of_stream. Fix via the default max_streams above + reuse inflate state + EOF label.
  • Security M4unsafe { output.advance(produced) } (engine.rs:348) is OOB-sound, but delegates the initialized invariant to the safe Codec::step; mark Codec::step unsafe with a # Safety clause or zero-fill in the driver.
  • Perf H1 (Medium impact) — brotli & zstd zero-fill the whole output chunk (up to 64 KiB) before every engine step even though both backends are write-only; flate proves it's avoidable via *_uninit. The MaybeUninit abstraction is defeated on the hottest path.
  • Perf H2/H3 — empty source chunks enter the full hot codec path (reserve + zero-fill + native step + self-wake); whole-buffer compress/decompress pays streaming-chunking overhead. Perf H4 — the flagship CompressionStream incremental path has zero benchmark coverage.
  • ComplexityPump::pull is a single ~185-line function and the StreamEnd match has 12 guard-heavy arms recomputing max_streams() up to 3×/iteration; extract cohesive helpers.
  • Consolidation — the decompressor limit-delegation trio and stream_ended() logic are byte-identical across all three codecs; the unsafe initialize() helper is copy-pasted verbatim in two files (duplicated unsafe is the riskiest kind).

🟡 Low (defense-in-depth / polish)

  • stream.rs source is not fused (possible re-poll-after-None panic on caller-supplied Compression); self-wake spin can peg CPU on a perpetually-ready empty source.
  • Pooled engine buffers are not zeroized between uses (in-memory remanence; reset-on-checkout already prevents any functional cross-message leak).
  • zstd is the only C parser on the untrusted path — add a cargo audit/cargo deny CI gate.
  • output_chunk_size has no upper bound; dead done_reported field; brotli collapses NeedsMoreInput/NeedsMoreOutput into one state; crate-root module named core forces ::core:: disambiguation and inconsistent Result spellings; a handful of near-duplicate per-format unit tests could share a harness.

✅ Verified clean

No memory-safety bug, no exploitable panic, no integer-overflow bug. All 4 unsafe blocks proven sound (the task-flagged flate/codec.rs:233 from_raw_parts is test-only and sound). FFI return codes checked; no content-size trust / no huge-alloc bomb pre-allocation; saturating integer math; no PII in error messages; fail-closed config validation; zero-copy input/output paths; no O(n²) append; dependencies current and advisory-clean (flate2 → zlib-rs only, avoiding the C-zlib CVE class). Idiomaticity is exemplary.

Build/clippy/test status

⚠️ Not verified by compiler. The review environment's egress to index.crates.io / static.crates.io was blocked, so cargo build/clippy/test could not fetch dependencies. Source was reviewed at head 17f82b1 via the GitHub API; the H1 fix was implemented on the real tree and parse-checked with rustfmt (clean), and verified complete/non-regressing by static inspection (all 12 FormatLimits::new sites accounted for, enforcement plumbing confirmed end-to-end, existing test payloads confirmed under the new caps). Please run cargo build/test/clippy -p compressors in an environment with crates.io access to confirm.


Review performed by an automated multi-agent review team. Line numbers reference head 17f82b1.

… 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
Comment thread crates/compressors/src/lib.rs Outdated
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
Comment thread crates/compressors/src/error.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 memory it constructs: it calls gzip::compress/...decompress with Resources::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(...) calls Compressor::new(resources), which uses an expect(...) 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 via Result.

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

martintmk and others added 4 commits September 3, 2026 19:05
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
Copilot AI review requested due to automatic review settings September 3, 2026 17:06
feature = "zlib",
feature = "zstd"
))]
pub use level::Level;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 MemoryShared provider is used for output, but it still calls gzip::compress/gzip::decompress with &Resources::default(), so the memory created here is unused (and the view(..) input is also allocated from an unrelated throwaway pool). Build a Resources from memory and 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
Copilot AI review requested due to automatic review settings September 3, 2026 17:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 a CompressorBuilder type alias. This will render as a broken rustdoc link; link directly to the specialized crate::CompressorBuilder<crate::zstd::Zstd> method instead.

crates/compressors/src/tests/round_trip.rs:306

  • This test claims to validate that a custom MemoryShared provider is used, but the compression/decompression calls use &Resources::default() instead of resources built from the GlobalPool created 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. Using zstd_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].

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖: 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.

martintmk and others added 2 commits September 3, 2026 20:17
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
Copilot AI review requested due to automatic review settings September 3, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 MemoryShared provider is used for output, but it builds/uses Resources::default() (the global resources) for both compress and decompress, so the memory created above is not actually exercised by the codec output path.
    crates/compressors/src/macros.rs:149
  • In the fallible format case, this compress convenience returns Result and documents errors, but it calls Compressor::new(resources) which can panic if the engine rejects the default configuration. Building via the fallible builder keeps the API consistent (no panic) and lets BuildError convert into Error.
        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
Copilot AI review requested due to automatic review settings September 3, 2026 18:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 MemoryShared provider, but it never uses the memory it creates: both compress and decompress are run with Resources::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
Copilot AI review requested due to automatic review settings September 3, 2026 18:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 compress convenience can panic because it calls Compressor::new(resources), which uses .expect(...) if the engine rejects even the default configuration. Since this function already returns Result<BytesView>, it should propagate a build rejection as an error (leveraging the existing From<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 MemoryShared provider is used for output, but it builds compressors with Resources::default() (global resources) and also constructs input via view(...) which allocates from its own throwaway pool. As written, the custom memory provider isn’t actually exercised by the compressor/decompressor under test.
  • Files reviewed: 39/42 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agency-rocket Touched by a rocket skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants