Repository navigation
install: let libarchive gunzip buffered tarballs instead of inflating into memory - #36541
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
Beyond the inline findings, I confirmed BufferReadStream::open_read() does register archive_read_support_filter_gzip (src/libarchive/lib.rs:944), so handing compressed bytes straight to Archiver::extract_to_dir is sound; and time_started_for_verbose_logs is still consumed by the later "Extracted to" log so it isn't dead after the first verbose block dropped its use.
Extended reasoning...
The core Rust change is correct: libarchive's buffered reader already registers the gzip filter, so passing the raw .tgz works, and the from_ne_bytes→from_le_bytes fix matches RFC 1952. The 2.25 GiB-to-disk test cost flagged inline is the item that warrants a maintainer decision before merge; the cap removal itself is a design call that will be visible to whoever looks at that.
773e9c0 to
c0eafc6
Compare
c0eafc6 to
2f74de4
Compare
… into memory The buffered extract path (local file: tarballs, and the HTTP fallback when streaming is disabled) used to decompress the entire .tgz into a Vec<u8> before handing it to libarchive. That buffer was capped at 2 GiB, so any tarball whose decompressed tar exceeded the cap failed with a bare 'ZlibError', and a 1.9 GiB package cost ~2.3 GB RSS just for the intermediate buffer. libarchive already has a gzip filter wired up (BufferReadStream::open_read registers read_support_filter_gzip, backed by the same zlib-ng bun already links), so pass the compressed bytes straight through and let it gunzip block-by-block. The libdeflate fast path is kept for tarballs whose gzip ISIZE trailer reports under 64 MB, which is the common case for npm packages.
The previous revision extracted 2.25 GiB to disk on every run. The only remaining code boundary is the 64 MB gzip-ISIZE cutoff that gates the libdeflate fast path, so 256 MiB exercises the identical libarchive path. Asserting the child's maxRSS stays under 2x the payload proves the decompressed tar is never materialised in memory (unfixed builds peak at ~780 MB release / ~1 GB ASAN; fixed builds stay at baseline).
2f74de4 to
40e75dc
Compare
…op stale test comment
40e75dc to
755febc
Compare
There was a problem hiding this comment.
All prior review feedback is addressed and the current pass found nothing new, but this rewrites the buffered tarball decompression path in bun install and removes the 2 GiB MAX_DECOMPRESSED_TARBALL_SIZE guard — worth a maintainer's eyes on the design call rather than an auto-approve.
What was reviewed:
- Confirmed
BufferReadStream::open_readregistersarchive_read_support_filter_gzip(src/libarchive/lib.rs:944), so passing compressed bytes through is sound. - The libdeflate fast path is behaviour-preserving for ISIZE < 64 MB; on failure it now falls through to libarchive instead of the removed zlib pre-inflate.
maxRSSunit handling in the test checked againstspawn_process.rs— Windows normalisesPeakWorkingSetSize / 1024, so theisMacOS ? 1 : 1024branch is correct.- Dead-code cleanup (
Error::Zlib,bun_zlibdep,max_output_sizevisibility) and the two siblingfrom_ne_bytes → from_le_bytessites are all applied.
Extended reasoning...
Overview
The PR changes ExtractTarball::extract (src/install/extract_tarball.rs) to stop pre-inflating gzipped tarballs into a Vec<u8> via ZlibReaderArrayList and instead hand the compressed bytes directly to Archiver::extract_to_dir, relying on libarchive's built-in gzip filter to gunzip block-by-block. The libdeflate one-shot fast path for small tarballs (ISIZE < 64 MB) is preserved. Dead code orphaned by the removal is deleted (Error::Zlib, the bun_zlib workspace dep, Cargo.lock entry), ZlibReaderArrayList::max_output_size is narrowed to pub(crate), and the from_ne_bytes → from_le_bytes ISIZE fix is applied at the two sibling sites in InternalState.rs and BunObject.rs. The oversized-tarball test is rewritten to install a 256 MiB local .tgz and assert child maxRSS < 2 * PAYLOAD_SIZE.
Security risks
The change removes MAX_DECOMPRESSED_TARBALL_SIZE = 2 GiB. I believe this is net-neutral to net-positive: the cap bounded an in-memory intermediate buffer that no longer exists, disk output was never capped (libarchive writes entry-by-entry), and the streaming HTTP path — the default for registry packages — already goes through libarchive gunzip without this cap. The buffered path applies to file: tarballs (local, user-controlled) and the BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL fallback. Still, deleting an explicit resource bound in a package manager is the kind of judgment I'd rather a maintainer confirm than sign off on autonomously.
Level of scrutiny
High. ExtractTarball::extract is on the critical path for every bun install that touches a local or buffered tarball, and ~60 lines of its decompression logic are rewritten. This is not a mechanical change; it's a deliberate architectural swap of one decompressor for another with a user-visible behaviour change (previously-failing >2 GiB local tarballs now install).
Other factors
Four prior review rounds on this PR raised test cost (2.25 GiB → 256 MiB + maxRSS assertion), missing gzip error wiring, orphaned dead code, sibling from_ne_bytes sites, a stale comment, and a dropped stderr destructure — all addressed and marked resolved. I verified open_read() at src/libarchive/lib.rs:944 does call read_support_filter_gzip(), and that the test's Windows maxRSS unit assumption matches spawn_process.rs:138 (PeakWorkingSetSize / 1024). The verbose-install log line loses its per-decompress timing (decompressed X in Y → X tgz file); the extraction-elapsed line further down still prints. No CODEOWNERS entry covers these files. The bug hunter found nothing this run.
|
Updated 5:59 AM PT - Jul 31st, 2026
❌ @robobun, your commit 755febc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36541That installs a local version of the PR into your bun-36541 --bun |
|
CI on build 86226: 193 lanes passed, 1 failed. The one hard failure is |
Package extraction now hands compressed bytes to libarchive for tarballs whose gzip ISIZE trailer reports over 64 MB, so the file_buffer.len() bound is no longer always the decompressed length there; the comment now describes the invariant without naming callers. Test comments updated to match (libarchive fails to open the input, not a separate zlib reader).
|
Heads up: the |
Package extraction now hands compressed bytes to libarchive for tarballs whose gzip ISIZE trailer reports over 64 MB, so the file_buffer.len() bound is no longer always the decompressed length there; the comment now describes the invariant without naming callers. Test comments updated to match (libarchive fails to open the input, not a separate zlib reader).
Repro
On 1.4.0-canary and main:
Cause
ExtractTarball::extract()(the buffered path used for everyfile:tarball and for the HTTP fallback when streaming is disabled) first decompresses the entire gzip stream into aVec<u8>viaZlibReaderArrayList, capped atMAX_DECOMPRESSED_TARBALL_SIZE = 2 GiB, then hands the decompressed tar toArchiver::extract_to_dir. Hitting the cap surfaces as a bareZlibError. Below the cap, peak RSS is roughly the decompressed size plus growth-doubling slack: a 256 MiB package costs ~780 MB RSS just for the intermediate buffer.libarchive already supports reading gzipped input directly:
BufferReadStream::open_readregistersarchive_read_support_filter_gzip, backed by the same vendored zlib-ng the removed pre-decompress used.Fix
Hand the compressed
.tgzbytes straight toArchiver::extract_to_dirand let libarchive gunzip block-by-block. The 2 GiB cap and intermediate buffer are gone.The libdeflate fast path is kept for tarballs whose gzip ISIZE trailer reports under 64 MB (unchanged threshold), so small npm packages keep the faster decoder. The trailer is now read as little-endian per RFC 1952 (was native-endian); the same one-token change is applied at the two sibling sites in
src/http/InternalState.rsandsrc/runtime/api/BunObject.rs.Dead code removed with the pre-decompress:
bun_install::Error::Zliband thebun_zlibworkspace dependency inbun_install;ZlibReaderArrayList::max_output_sizenarrowed topub(crate)now its external writer is gone.Verification
buffered extract does not hold the decompressed local tarball in memoryreplaces the previousbuffered extract rejects ...test (which asserted the cap). It installs a 256 MiB local tarball and asserts the child'smaxRSSstays under2 * PAYLOAD_SIZE. On the released bun it peaks at ~780 MB release / ~1 GB debug+ASAN and fails; with this change it stays at baseline (~40 MB release / ~240 MB debug+ASAN) and passes.Also ran
bun-install-tarball-integrity.test.ts(16 pass) andbun-install-registry.test.ts(229 pass) to confirm the common paths are unchanged.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-streaming-extract.test.ts