Skip to content

zstd: delete ZstdReaderArrayList in favour of StreamingDecoder - #37555

Open
robobun wants to merge 1 commit into
mainfrom
farm/c83f5856/zstd-delete-reader-array-list
Open

robobun wants to merge 1 commit into
mainfrom
farm/c83f5856/zstd-delete-reader-array-list

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What

src/zstd/lib.rs had two streaming decompression loops. The private ZstdReaderArrayList<'a> borrowed the input slice and the output Vec, was handed out as a Box, stored the C handle as a bare *mut ZSTD_DStream, and reused State::End to also mean "the stream has already been freed" (end() freed it and set End; Drop called end() again, relying on that flag). The public StreamingDecoder (already used by bun_http's Decompressor) is the same loop with the stream held as a NonNull and freed once in Drop, and input and output passed per call. decompress_alloc, the only user of the reader (the unknown-content-size and above-16MB branch), now uses StreamingDecoder:

// before
let mut list: Vec<u8> = Vec::new();
let mut reader = ZstdReaderArrayList::init(src, &mut list)?;
reader.read_all(true)?;
drop(reader);
return Ok(list);

// after
let mut output = Vec::new();
let mut decoder = StreamingDecoder::new()?;
decoder.decompress(src, &mut output, true)?;
return Ok(output);

ZstdReaderArrayList (struct, init, init_with_list_allocator, end, read_all, Drop) is deleted, removing 6 unsafe blocks and about 170 lines; the StreamingDecoder doc comment no longer refers to it. 1 call site changes and nothing outside the crate is affected: decompress_alloc, StreamingDecoder and the State re-export keep their signatures.

Why

The crate now has a single owner type for a ZSTD_DStream: StreamingDecoder cannot exist without a live stream and frees it exactly once in Drop, so the "freed" state that ZstdReaderArrayList encoded in its State field is unrepresentable, and there is one streaming loop to read instead of two. StreamingDecoder::decompress does the same work per iteration as the deleted read_all (the same 4096-byte reserve, the same ZSTD_decompressStream call with the same clamped output size, the same ZSTD_initDStream on frame completion for concatenated frames, the same ZstdDecompressionError and ShortRead mapping), so the output Vec, its capacity and the error returned are identical; the only runtime difference is that the reader's Box allocation is gone.

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds. bun bd test test/js/bun/util/zstd.test.ts test/regression/issue/23314/zstd-large-decompression.test.ts test/regression/issue/23314/zstd-large-input.test.ts: 87 pass, 0 fail across 3 files.

decompress_alloc's streaming branch (unknown content size, or a reported size above 16MB) was the only user of the private ZstdReaderArrayList, a Box-allocated reader holding the ZSTD_DStream as a raw pointer and reusing State::End to also mean that the stream had already been freed. The branch now uses the existing StreamingDecoder, which owns the stream as a NonNull and frees it exactly once in Drop; the whole reader (six unsafe blocks) is deleted. StreamingDecoder::decompress performs the same per-iteration work as the deleted read_all: the same 4096-byte reserve, the same ZSTD_decompressStream call, the same re-init on frame completion and the same error mapping, so the only runtime difference is one Box allocation less per call.
@robobun
robobun requested a review from alii August 11, 2026 17:58

@claude claude Bot 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.

LGTM — deletes the crate-private ZstdReaderArrayList and rewires its sole call site in decompress_alloc to the existing StreamingDecoder.

What was reviewed:

  • Line-by-line comparison of read_all vs StreamingDecoder::decompress: same 4096-byte reserve, same max_output_size clamp, same rc == 0 frame-boundary re-init, same ZstdDecompressionError/ShortRead mapping — behavior-identical for the single is_done=true call.
  • Confirmed ZstdReaderArrayList had no other references; decompress_alloc's signature and callers (BunObject.rs, add_completions.rs, node_fallbacks.rs) are unaffected.
  • StreamingDecoder frees the ZSTD_DStream once in Drop (NonNull), so the deleted end()'s early free is now handled by decoder drop at end of scope — no leak on the error path.
Extended reasoning...

Overview

This PR deletes the crate-private ZstdReaderArrayList<'a> struct (~170 lines, 6 unsafe blocks) from src/zstd/lib.rs and switches its only caller — the unknown-size / >16MB branch of decompress_alloc — to the pre-existing public StreamingDecoder. The StreamingDecoder doc comment is trimmed to no longer reference the deleted type. No public API changes: decompress_alloc, StreamingDecoder, and the State re-export keep their signatures.

Security risks

None introduced. The decompression-bomb guard (max_output_size clamping the per-call output window) is present identically in StreamingDecoder::decompress and defaults to usize::MAX in both types, so the decompress_alloc path is unchanged. The deleted type held a raw *mut ZSTD_DStream and used a State::End flag to prevent double-free; StreamingDecoder uses NonNull with a single free in Drop, which is strictly safer.

Level of scrutiny

Low-to-medium. This is a pure consolidation: deleting a private duplicate implementation in favour of an existing one already exercised by bun_http's Decompressor. The two loops are structurally identical — same empty-input handling, same reserve step, same ZSTD_decompressStream invocation with clamped output, same ZSTD_initDStream on rc == 0 for concatenated frames, same error/state transitions. The only per-call differences (total_in on self vs local; end() freeing eagerly vs Drop freeing at scope exit) don't matter for decompress_alloc's single-shot is_done=true usage where the decoder drops immediately after.

Other factors

Grep confirms zero remaining references to ZstdReaderArrayList in the tree. Callers of decompress_alloc (BunObject.rs sync/async paths, add_completions.rs, node_fallbacks.rs) are untouched. The PR reports 87 passing tests across zstd.test.ts and both 23314 large-input regression tests, which exercise exactly this streaming branch. This aligns with the repo's "one implementation, in the right place" and "delete dead code in the same PR that makes it dead" review guidance.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The oversized or unknown-size zstd decompression path now uses StreamingDecoder. The removed ZstdReaderArrayList implementation and its stateful borrowing flow are replaced by a decoder that owns the zstd stream.

Changes

Zstd streaming decompression

Layer / File(s) Summary
StreamingDecoder ownership contract
src/zstd/lib.rs
StreamingDecoder now owns the zstd stream and accepts input and output for each decompression call. ZstdReaderArrayList and its methods were removed.
Oversized decompression integration
src/zstd/lib.rs
decompress_alloc creates StreamingDecoder, decompresses into a new output vector, and returns the result.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the removal of ZstdReaderArrayList and its replacement with StreamingDecoder.
Description check ✅ Passed The description explains the change, rationale, implementation details, and verification results, including clean checks and passing tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/zstd/lib.rs`:
- Around line 312-315: Call output.shrink_to_fit() after
StreamingDecoder::decompress completes and before returning output from this
decompression path, ensuring the buffer passed through output.leak() contains no
spare capacity.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 18a4b1af-4eac-48f9-ad37-1f0fb85edabc

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and 4539471.

📒 Files selected for processing (1)
  • src/zstd/lib.rs

Comment thread src/zstd/lib.rs
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:15 AM PT - Aug 11th, 2026

✅ @robobun, your commit 4539471105e982f7184a65c133d3f0049bb41bea passed in Build #92364! 🎉


🧪   To try this PR locally:

bunx bun-pr 37555

That installs a local version of the PR into your bun-37555 executable, so you can run:

bun-37555 --bun

@robobun

robobun commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up: #39417 edits ZstdReaderArrayList::read_all and decompress_alloc in this file (fallible try_reserve growth, a bounded initial reservation, and mapping zstd's own allocation failures to OutOfMemory). Whichever of the two lands second needs a small rebase; if this one lands second, those changes belong in StreamingDecoder / the new decompress_alloc.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants