Add zip support and streaming append to Bun.Archive - #33318
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Updated 8:27 PM PT - Jul 11th, 2026
❌ @robobun, your commit 9c3a670 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33318That installs a local version of the PR into your bun-33318 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ZIP support to ChangesBun.Archive tar/zip and append feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/runtime/archive.mdx`:
- Around line 71-76: Reword the `Bun.file()` entry in `archive.mdx` to remove
the “a chunk at a time” phrasing that implies async streaming; update the
object-form bullet and the nearby explanatory sentence so they read consistently
with synchronous construction. Use the `Bun.file()` and `append()` references to
make it clear that `Bun.file()` reads from disk synchronously in chunks while
the archive constructor runs, and that `append()` is the non-blocking
alternative for large files.
In `@test/js/bun/archive.test.ts`:
- Around line 2012-2019: The test data strings in archive.test.ts are built with
String.prototype.repeat(), which should be replaced with the
Buffer.alloc(...).toString() pattern used elsewhere in this file. Update the
repetitive content creation in the affected Bun.Archive tests (including the
cases around deflate/store and the other repeated-string block) so the same
string values are generated via Buffer.alloc(count, fill).toString() instead of
.repeat(...), keeping the test expectations unchanged.
- Line 1911: Switch the new I/O-heavy describe blocks to concurrent execution by
using describe.concurrent for Bun.file() entries, zip format, archive.append(),
and spilling to disk. The tests already use isolated archive instances and
unique tempDir/Bun.write paths, so update the corresponding describe
declarations in the archive.test.ts suite to run concurrently while keeping the
existing test bodies unchanged.
🪄 Autofix (Beta)
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: 3a904445-ebe5-49d8-a0be-1ed0d5826057
📒 Files selected for processing (11)
docs/runtime/archive.mdxpackages/bun-types/bun.d.tssrc/event_loop/ConcurrentTask.rssrc/libarchive/lib.rssrc/runtime/api.rssrc/runtime/api/Archive.classes.tssrc/runtime/api/Archive.rssrc/runtime/api/archive_builder.rssrc/runtime/dispatch.rssrc/runtime/webcore/Blob.rstest/js/bun/archive.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/api/Archive.rs (1)
373-383: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject embedded NULs before creating the archive entry path.
Line 383 copies unchecked JS bytes into the builder path after only the empty-path guard. Add a NUL-byte check before handing the entry name to the archive builder/libarchive path so
foo\0barcannot be truncated or misinterpreted downstream. As per coding guidelines, “Reject embedded NULs in strings passed to C APIs.”🛡️ Proposed fix
- if path_slice.slice().is_empty() { + let path_bytes = path_slice.slice(); + if path_bytes.is_empty() { return Err(global .throw_invalid_arguments(format_args!("Archive.append: path must not be empty"))); } - let path = ZBox::from_bytes(path_slice.slice()); + if bun_core::strings::contains(path_bytes, b"\0") { + return Err(global.throw_invalid_arguments(format_args!( + "Archive.append: path must not contain NUL bytes" + ))); + } + let path = ZBox::from_bytes(path_bytes);🤖 Prompt for 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. In `@src/runtime/api/Archive.rs` around lines 373 - 383, The Archive.append path handling currently accepts any non-empty string and copies it into the archive entry name without checking for embedded NUL bytes. Update the argument validation in Archive.append, before creating the path with ZBox::from_bytes, to reject any path_slice that contains a NUL byte and return an invalid-arguments error. Keep the existing string and empty-path checks, and ensure the new validation happens in the same code path that builds the archive entry name for libarchive.Source: Coding guidelines
🤖 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/runtime/api/Archive.rs`:
- Around line 853-856: In Archive.write(), the gzip selection logic is treating
an explicit compress override the same as a missing value, so the archive-level
setting can win even when the caller passed a format. Update the logic around
options.compress.gzip() and archive.options.compress so explicit compress values
like "store" or "deflate" are preserved, and only fall back to the archive
default when compress was not provided at all.
---
Outside diff comments:
In `@src/runtime/api/Archive.rs`:
- Around line 373-383: The Archive.append path handling currently accepts any
non-empty string and copies it into the archive entry name without checking for
embedded NUL bytes. Update the argument validation in Archive.append, before
creating the path with ZBox::from_bytes, to reject any path_slice that contains
a NUL byte and return an invalid-arguments error. Keep the existing string and
empty-path checks, and ensure the new validation happens in the same code path
that builds the archive entry name for libarchive.
🪄 Autofix (Beta)
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: 4c4c76bb-c89b-497f-baa7-8bcc2ec8dd4e
📒 Files selected for processing (1)
src/runtime/api/Archive.rs
|
Thanks, both review findings were real and are fixed in c9c62b1. Embedded NULs in entry paths. Confirmed before fixing: libarchive takes entry names as NUL-terminated C strings, so the name was silently truncated. The object form had the same hole (it predates this PR), so both entry points now go through one validator that rejects empty names and NULs and quotes the offending path. Explicit The three nits ( On the scope warning, and the issue bots. This is not a Of the three issues the bot suggested, I verified two and dropped one:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/api/Archive.rs (1)
285-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle store-less Blobs before the object fallback. Empty/detached Blobs reach this branch with
store == None, so they fall through tois_object()and get packed as an empty object instead of going through the Blob/byte-input path.🤖 Prompt for 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. In `@src/runtime/api/Archive.rs` around lines 285 - 306, Handle store-less Blob inputs before the plain object fallback in Archive::new’s data_arg branching. The current blob_from_js path only returns early when blob.store.get().as_ref() exists, so detached/empty Blobs fall through to data_arg.is_object() and are treated as empty objects. Update the Blob/Archive handling in this function to recognize Blob inputs even when no store is present and route them through the Blob/byte-input path instead of the object packing branch.
🤖 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 `@test/js/bun/archive.test.ts`:
- Around line 1046-1062: The current Archive write test in archive.test.ts
covers inheritance and fully overridden options, but it misses the key
regression case where write() is called with format changed to zip while
compress is omitted. Add a case around Bun.Archive.write using the existing
gzip-compressed source archive and override only format via the write options,
then assert the output does not follow the source archive’s gzip setting; this
will verify the intended behavior of write(), Bun.Archive, and the
compress/format option precedence.
---
Outside diff comments:
In `@src/runtime/api/Archive.rs`:
- Around line 285-306: Handle store-less Blob inputs before the plain object
fallback in Archive::new’s data_arg branching. The current blob_from_js path
only returns early when blob.store.get().as_ref() exists, so detached/empty
Blobs fall through to data_arg.is_object() and are treated as empty objects.
Update the Blob/Archive handling in this function to recognize Blob inputs even
when no store is present and route them through the Blob/byte-input path instead
of the object packing branch.
🪄 Autofix (Beta)
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: a1c09f56-fc53-4b3f-bd7f-d884c5fda394
📒 Files selected for processing (3)
docs/runtime/archive.mdxsrc/runtime/api/Archive.rstest/js/bun/archive.test.ts
|
Good catches across the board. All six are fixed in 8514c01 and c1085bf, and one of them was a real bug with teeth. Abandoning a writer mid-entry wrote out the whole declared entry size
Measured on a 600 MB file truncated out from under the read loop: reproconst fs = require("node:fs");
const io = () => Number(/^write_bytes:\s*(\d+)/m.exec(fs.readFileSync("/proc/self/io", "utf8"))[1]) / 1024 / 1024;
const big = "/tmp/big.bin";
await Bun.write(big, Buffer.alloc(600 * 1024 * 1024, 7));
const a = new Bun.Archive(undefined, { maxMemory: 0 });
const before = io();
const p = a.append("x.bin", Bun.file(big)).catch(e => e.message);
await Bun.sleep(2);
fs.truncateSync(big, 0);
console.log("rejected:", await p);
console.log(`disk bytes written during the FAILED append: ${Math.round(io() - before)}MB`);A The test covers the path (a source whose read fails after the header was written) rather than the byte count, because triggering a large declared size needs a file that shrinks between the A failed append's rejection now arrives before the cascadeThis one was user-visible through the pattern the docs recommend: await Promise.all(paths.map(path => archive.append(path, Bun.file(path))));
The rest
One suggestion I did not take
That assertion would fail, and should. 141 tests pass under ASAN, clippy is clean, and all 10 target triples compile. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/api/archive_builder.rs (2)
81-88: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t discard the spill path when unlink-on-create fails.
Line 83 ignores
unlinkfailure, then Line 86 clearspath, so a failed POSIX unlink leaves the temp archive file on disk with noDropcleanup. Return the unlink error or keep the path for deferred cleanup. As per coding guidelines, "Never swallow a failure or signal success on one."Suggested fix
#[cfg(unix)] { - let _ = bun_sys::unlink(path.as_zstr()); + bun_sys::unlink(path.as_zstr())?; return Ok(SpillFile { file: Some(file), path: ZBox::from_bytes(b""), len: 0,🤖 Prompt for 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. In `@src/runtime/api/archive_builder.rs` around lines 81 - 88, The SpillFile creation path in archive_builder::SpillFile should not ignore a failed bun_sys::unlink or clear the path afterward, because that leaves the temp archive orphaned with no cleanup. Update the #[cfg(unix)] branch in the SpillFile constructor logic to either return the unlink error immediately or preserve the original path so Drop can remove it later, and keep the behavior consistent with the existing file/path fields.Source: Coding guidelines
471-475: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMark the writer closed only after
write_close()succeeds.If
write_close()fails, the builder is being abandoned, butDropskipswrite_fail()becauseclosedis already true. Keep it unclosed on failure so the Drop path poisons libarchive before freeing.Suggested fix
if !self.closed { - self.closed = true; if self.archive().write_close() != lib::Result::Ok { return Err(self.fail("ArchiveCloseError")); } + self.closed = true; }🤖 Prompt for 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. In `@src/runtime/api/archive_builder.rs` around lines 471 - 475, In the archive closing path in ArchiveBuilder’s close/write_close logic, do not set self.closed to true before calling write_close(). If write_close() returns an error, leave the builder marked open so Drop can still run write_fail() and poison libarchive before cleanup. Update the close flow in the ArchiveBuilder method that calls archive().write_close() so closed is only set after a successful write_close() result.
🤖 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/runtime/api/Archive.rs`:
- Around line 285-294: The Blob handling in Archive::serialize currently clones
only the backing store via StoreRef::clone, which loses the Blob’s offset and
size and can serialize the parent bytes instead of the sliced view. Update the
blob path in Archive::serialize to preserve the visible range by materializing
the Blob’s current slice/view when it does not span the full store, while
keeping the existing zero-copy fast path only for full-store Blobs. Use
blob_from_js, blob.store, and the Archive serialization branch to locate the
fix.
---
Outside diff comments:
In `@src/runtime/api/archive_builder.rs`:
- Around line 81-88: The SpillFile creation path in archive_builder::SpillFile
should not ignore a failed bun_sys::unlink or clear the path afterward, because
that leaves the temp archive orphaned with no cleanup. Update the #[cfg(unix)]
branch in the SpillFile constructor logic to either return the unlink error
immediately or preserve the original path so Drop can remove it later, and keep
the behavior consistent with the existing file/path fields.
- Around line 471-475: In the archive closing path in ArchiveBuilder’s
close/write_close logic, do not set self.closed to true before calling
write_close(). If write_close() returns an error, leave the builder marked open
so Drop can still run write_fail() and poison libarchive before cleanup. Update
the close flow in the ArchiveBuilder method that calls archive().write_close()
so closed is only set after a successful write_close() result.
🪄 Autofix (Beta)
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: f128e5bd-311c-46eb-9b02-3da1abd70137
📒 Files selected for processing (3)
src/runtime/api/Archive.rssrc/runtime/api/archive_builder.rstest/js/bun/archive.test.ts
|
All three fixed in 17fca9b. The Blob one was a real data bug, and it predates this PR. A sliced Blob archived its parent's bytes. Both call sites now go through one helper: it keeps the zero-copy store ref when the Blob spans its whole store, and copies the window otherwise. File- and S3-backed stores have no bytes to window into, so they pass through as before. That fix also settled the empty-Blob case. An empty Blob has no store at all, so it is now read as empty archive data rather than being mistaken for an empty object literal. Reading its entries fails with The unlink-at-create. Good point, but I kept the path rather than failing the whole archive: if the unlink somehow fails, the spill file is still usable, so
Verification unchanged: 144 tests pass under ASAN, clippy clean, all 10 target triples compile, no fd or temp-file leaks across 20 abandoned spilled archives, and the 600 MB padding repro still reports single-digit MB. On the recurring out of scope flag: this is a feature request (zip plus streaming append for |
|
Rebased onto CI on build 72006 (complete)283 jobs passed. Three lanes red, none of them this diff.
A worker MessagePort test tripping a JSC exception-scope assertion. This diff touches no worker, MessagePort, structured-clone, or
Test-environment flake on that box, nothing to do with this diff.
Network flake fetching test vendor repos, zero tests executed. Local verification after the rebaseThis PR already spent its one re-trigger on an earlier flake, so I am not pushing another commit to re-roll. A re-run of the three lanes (or the worker_threads main-break fix landing) clears it. |
|
Both fixed in 303de6e. The first one is a silent data-corruption bug in new code, and it reproduces.
|
There was a problem hiding this comment.
All prior review feedback (mine and CodeRabbit's) has been addressed and I found nothing new on this pass, but this adds new public API surface (append(), format, maxMemory) plus ~700 lines of native code with cross-thread builder ownership, libarchive C callbacks, and spill-to-disk mmap — a maintainer should sign off on the API shape and the memory/threading design.
Extended reasoning...
Overview
This PR adds three user-facing features to Bun.Archive: zip container output (format: "zip" with "deflate"/"store" compression), a streaming append(path, data) method that keeps a libarchive writer open across async work-pool tasks, and a spill-to-disk sink (maxMemory) with mmap read-back on POSIX. It also fixes two pre-existing bugs (#28459 Bun.file() writing 0-byte entries, and a null-pointer slice on stored-zip extraction). The core changes are a rewritten src/runtime/api/Archive.rs (state machine Building/Done/Failed, append FIFO, a new AppendTask wired through dispatch.rs), a new 523-line src/runtime/api/archive_builder.rs (Builder, SpillSink, SpillFile with libarchive write callbacks), and additions to src/libarchive/lib.rs (zip reader/writer bindings, write_fail, write_set_bytes_in_last_block). Docs, .d.ts, and ~420 lines of new tests round it out.
Security risks
Archive extraction is inherently security-sensitive (path traversal, symlink escapes, zip bombs). This PR does not touch the existing extraction path-safety logic — the zip reader is enabled via a new ExtractOptions.zip flag (off for bun install, on for Bun.Archive), and extraction reuses the existing extract_to_dir guards. The new write side validates entry paths for empty/embedded-NUL. The spill file is created O_EXCL | 0o600 in the system temp dir and unlinked immediately on POSIX. The SpillFile::mmap uses MAP_PRIVATE on a file the process itself just wrote. I don't see a new attack surface beyond what Bun.Archive already exposed, but "we now read zip via libarchive" is worth a maintainer's glance given libarchive's CVE history for format-specific readers.
Level of scrutiny
High. This is a substantial feature PR, not a bugfix:
- New public API surface —
append(),format,maxMemory, and changed constructor semantics (optional first arg, archive stays open until first read). These are design decisions a maintainer should ratify. - Native memory/threading —
Builderis moved between the JS thread and a work-pool thread via anOptionslot on theArchive, with a hand-writtenunsafe impl Sendjustified by unique ownership.AppendContextholds ajsc::Strongand has its ownunsafe impl Send(JSC handle only touched on the JS thread).SpillSinkis heap-allocated and its raw pointer is handed to libarchive asclient_data;Builder::Dropfrees the archive before the sink to keep the write callback valid duringarchive_write_free's flush. - Behaviour change — an
Archivebuilt from an object no longer materializes its bytes in the constructor;blob_store()closes the writer lazily on first read. This changes when work happens for existing callers.
Other factors
The PR has been through several review rounds. I left seven inline findings across three passes; all were fixed with verified reproductions (notably the archive_write_data INT_MAX clamp, the failed-append rejection ordering, and the write_fail poisoning to avoid 600 MB of NUL-padding on abandonment). CodeRabbit's two substantive findings (compress-override inheritance, sliced-Blob store window) were also fixed. Test coverage is thorough (~40 new tests including round-trips, stored-zip extraction, spill equivalence, error-path poisoning, Promise.all rejection ordering, NUL-in-path). CI on the latest build is green on x64-asan; the one hard failure is a pipeline-wide darwin artifact-download timeout unrelated to this diff. The author flagged an overlap with #28460 and offered to drop the second commit if that lands first — a maintainer should decide which approach to take.
|
Heads up, the design is changing: So instead of const archive = new Bun.Archive(undefined, { format: "zip" });
const done = Bun.write("out.zip", archive.stream()); // or new Response(archive.stream())
for (const path of paths) {
await archive.append(path, Bun.file(path)); // resolves when the consumer is ready for more
}
await archive.end();
await done;The producer is throttled by whoever is reading, so memory stays bounded by the stream's queue rather than by the archive's size, and it composes with Implementation-wise this hangs off Please hold off reviewing Thanks @claude for the review pass. Nothing to action on it, and I agree the API shape wants a maintainer's eye, which is exactly what prompted this change. |
|
So the PR as it stands is: zip format, What comes next
const archive = new Bun.Archive(undefined, { format: "zip" });
const done = Bun.write("out.zip", archive.stream()); // or new Response(archive.stream())
for (const path of paths) {
await archive.append(path, Bun.file(path)); // resolves when the consumer is ready for more
}
await archive.end();
await done;The mechanism is settled. The one real design point: a libarchive write callback runs on the work-pool thread, and One question before I build itThis PR is already ~1700 lines across 10 commits, and
I am happy either way and will default to (b) if I do not hear otherwise. Flagging it only because (a) gives a reviewer a much smaller surface for the native-memory and threading parts, which is where the real risk sits. |
|
const archive = new Bun.Archive(undefined, { format: "zip" });
const done = Bun.write("out.zip", new Response(archive.stream()));
for (const path of paths) {
await archive.append(path, Bun.file(path)); // parks when the consumer falls behind
}
archive.end();
await done;Reading the stream is what lets the next
How it worksThe one structural constraint: libarchive's write callback runs on a work-pool thread, and
An archive with no stream attached is unchanged: the whole entry is written in one step and the bytes accumulate for One thing worth knowing
VerificationEight new tests cover the tar and zip round-trips, the parking behaviour under a stalled reader, a cancelled consumer rejecting a parked append, a streamed archive having nothing left to read, and the |
|
Both 🟡 findings checked. One was real and is fixed, one turned out to be normal stream semantics, and the important one was a claim in my own docs that I could not back up. The docs were claiming backpressure that does not existThe All 80 MB of entries go through and uSockets holds what the client has not taken. The response is still produced incrementally, which is most of the value, but peak memory there is bounded by the archive rather than by the high-water mark. The docs, the Wiring
|
|
This round's finding did not reproduce, so The claim was that a failed I built the case where the consumer is provably parked outside Then a matrix: small entry, big entry, drained and not, 20 and 50 barrier turns, sync and async loop bodies. Five shapes, all propagated the error. The native The asymmetry in the code is real, though, and worth recording: What the commit does add is the sibling test for the branch that was genuinely uncovered. The existing failed-append test consumes with It is load-bearing rather than decorative: neuter I left the docs claim as it stands, since on every path I can actually construct, a failed 154 pass, 0 fail under debug + ASAN. |
|
Follow-up on the last round, because the review pushed back with a falsifiable prediction and it is worth recording how it came out. The objection was fair: my It does not. The error leaves through the Pending branch even with the consumer demonstrably parked in an 25 iterations of each shape, to rule out a lucky schedule: So the docs claim stays as written. Softening "a failed The Both test variants are load-bearing: neuter |
Bun.Archive could only write tar, and an archive had to be built entirely in memory before any of it could be read, so packing anything larger than RAM was impossible. Two additions: `format: "zip"` writes zip instead of tar, with per-entry deflate (the default, levels 1-9) or `compress: "store"`. Reading auto-detects zip alongside tar and tar.gz, so extract(), files(), and the glob filters all work on zip input without an option. `archive.append(path, data)` streams one more entry into an archive that has not been read yet. The archive from `new Archive(object)` (or from no argument at all) now keeps its libarchive writer open until the first read instead of closing it in the constructor. Appends are serialized through a FIFO on the work pool, so Promise.all() over a list of files still produces a deterministic archive. Passing a Bun.file() reads it from disk a chunk at a time, never materializing it. The writer's output buffers in memory up to `maxMemory` bytes (64 MB by default) and spills to a temporary file past that. On POSIX the spill file is unlinked the moment it is created, so a crash cannot leave it behind, and it is mapped copy-on-write when the archive is finally read, which keeps extract(), files(), Bun.write(), and Archive.write() streaming off the page cache rather than allocating a second copy. Building a 400 MB zip out of files on disk now costs ~47 MB of RSS. Also fixes a latent null-pointer `slice::from_raw_parts` in `lib::Archive::next()`: libarchive's stored-zip reader signals end-of-entry with ARCHIVE_OK and a null data block, which tar never produced. And the zip writer no longer zero-pads its output up to libarchive's 10240-byte default block, which tar wants and zip does not; an empty zip is 22 bytes again instead of 10 KB.
… files
new Bun.Archive({ "a.txt": Bun.file(p) }) wrote a 0-byte entry: the
constructor read the blob with shared_view(), which is empty for a
file-backed store. It now goes through the same streaming path append()
uses, so a Bun.file() entry is copied off disk a chunk at a time and a
sliced one honours its window. A missing file now throws instead of
silently producing an empty entry, and an S3 file says to read its bytes
first rather than archiving nothing.
Two things review caught.
libarchive takes entry names as NUL-terminated C strings, so an embedded
NUL silently truncated the name: new Archive({"a\0b.txt": x}) produced an
entry called "a". Both entry paths (the object form and append()) now go
through one validator that rejects empty names and NULs, quoting the
offending path.
Archive.write(path, archive, options) inherited the archive's gzip setting
whenever options.compress had no gzip post-filter, which is true for both
"store" and "deflate" as well as for an omitted option. An explicit
compress now wins, and only an omitted one inherits.
Also applies the repo's test conventions to the new tests: Buffer.alloc()
over String#repeat, and describe.concurrent for the I/O-heavy blocks.
Abandoning a libarchive writer mid-entry runs a normal close through
archive_write_free, and tar's format_finish_entry pads the entry out to
the size its header declared (archive_write_set_format_pax.c's
__archive_write_nulls(remaining + padding)), pushing every one of those
bytes through the client write callback.
So a failed append of a large file wrote the whole declared size as NULs
into a sink whose output was about to be thrown away, synchronously on the
JS thread. Measured on a 600 MB file truncated out from under the read
loop:
before: 600 MB written to a throwaway spill file, JS blocked 494ms
after: 4 MB (the bytes that were actually read), JS blocked 94ms
A Builder being dropped without having been closed now poisons the writer
with archive_write_fail first, so archive_write_free skips the close. The
writer's resources are still released: _archive_write_free frees the format
data, the filters, and its buffers regardless of state.
The test covers the path (a source whose read fails after the header was
written) rather than the byte count, since triggering a large declared size
needs a file that shrinks between fstat and the read loop.
Also from review: open a streamed entry's source with O_CLOEXEC, matching
SpillFile::create and bun_sys::open_file_read_only, and collapse the two
copies of the BuildError match into one, so the "file changed size" message
no longer names append() when the constructor reached it.
When an append fails, every append queued behind it is rejected with "Archive.append() failed earlier". Those rejections were delivered first, so `Promise.all(paths.map(p => archive.append(p, Bun.file(p))))` — the pattern the docs recommend — rejected with the generic cascade message and hid the ENOENT that actually caused it. `TaskContext` gains an `after_settled` hook that `AsyncTask` calls once the task's own promise has settled; the append context drains the queue there. Also from review: - Read `compress` from the options object once. Reading it twice let a getter return different values to `compress_given` and `compress`, which is exactly the desync the `compress_given` flag exists to prevent. - Take the tmpname scratch buffer from `bun_paths::path_buffer_pool` rather than putting a `PathBuffer` (~96 KB on Windows) on the stack. - Treat a store-less Blob as archive data rather than letting it fall through to the object form and be packed as an empty object literal.
Bun.Archive.write(path, archive, { format: "zip" }) cannot change bytes
that are already packed. Only compress still applies there, and omitting it
inherits the archive's own setting.
new Bun.Archive(blob.slice(0, 512)) shared the Blob's store, which carries every byte of the parent, so the archive was the parent's 10240 bytes rather than the 512 the slice sees. Archive.write(path, slicedBlob) had the same hole. Both now go through one helper that keeps the zero-copy store ref for a Blob spanning its whole store, and copies the window otherwise. An empty Blob has no store at all, so it now reads as empty archive data instead of being mistaken for an empty object literal, matching what an empty Uint8Array already did. Also from review: - Keep the spill file's path if the unlink-at-create fails, so Drop still removes it rather than orphaning it. - Mark the writer closed only once write_close() succeeded, so a failed close still reaches Drop's write_fail().
archive_write_data clamps its length argument to INT_MAX and returns the
count it actually took (archive_write.c: `if (s > max_write) s = max_write`),
so a single call cannot be assumed to consume the whole slice. write_data
only checked for a negative return, so an in-memory entry over 2 GiB wrote
INT_MAX bytes and then finish_entry NUL-padded the rest out to the size the
header declared. The archive was structurally valid and extracted without
error, with the tail silently replaced by zeros.
Reachable from append() with a Blob, Uint8Array, or string, and from the
object form. add_file was already safe because it writes in 256 KB chunks.
entry of INT_MAX + 1024 bytes, round-tripped through files():
before: tail 1024 bytes are NUL
after: tail marker intact
No test: the trigger needs a single >2 GiB in-memory JS value, so exercising
it costs ~2.2 GB of RAM and as much disk.
Also skip the "fails mid-read" test on macOS. It relies on fstat(dir).st_size
being nonzero so the read loop runs, and POSIX leaves a directory's st_size
undefined.
283 of 287 jobs passed on 303de6e. The one hard failure is test/js/bun/terminal/terminal.test.ts timing out on darwin 14 x64, a PTY test this diff does not touch; it passes locally in 7s and also failed on an unrelated branch (claude/fs-watch-cfrunloop-shutdown-race, build 68279).
Paging the archive out to a temporary file is the wrong answer to an archive that outgrows memory. The right one is to produce it chunk by chunk and let the consumer say when it is ready for more, which a following commit adds as `archive.stream()`. So `maxMemory`, `SpillFile`, the disk half of the sink, the POSIX mmap read-back and the Windows read-back all go. The sink is a plain growing buffer again, and a closed builder hands its bytes straight to a Blob store.
Removing the spill sink left prose behind that the code no longer honours. `append()` still reads a `Bun.file()` a chunk at a time, so the entry's bytes are never buffered whole, but the archive it is building still accumulates in memory, so "a file larger than available memory" and "bounds memory" were overstatements. Two Rust comments still said the builder's `Drop` removes its spill file, which no longer exists; it poisons and frees the libarchive writer.
The replacement for the spill sink. `stream()` hands the archive's bytes to a
`ReadableStream` as they are produced, and reading it is what lets the next
`append()` make progress: once the stream's queue passes its high-water mark an
in-flight append parks, and the consumer's drain puts it back on the work pool.
const archive = new Bun.Archive(undefined, { format: "zip" });
const done = Bun.write("out.zip", new Response(archive.stream()));
for (const path of paths) await archive.append(path, Bun.file(path));
archive.end();
await done;
Streaming 800 MB through it costs 37 MB of RSS.
The one structural constraint is that libarchive's write callback runs on a
work-pool thread while `on_data` touches JS, so the bytes cannot go straight
from the writer into the stream. An entry is therefore written a chunk at a
time: `Builder` grows `begin_entry`/`write_body`/`end_entry` next to the
all-at-once helpers, and `AppendContext` carries the cursor and the open file
handle across chunks. `TaskContext::step_from_js` lets the context say
Reschedule, Parked, or Settle; everything else keeps settling on the first
pass, as before. Parking holds the task rather than blocking a pool thread, so
a slow consumer cannot starve the pool.
An archive with no stream attached is unchanged: the whole entry is written in
one step and the bytes accumulate for `bytes()` / `blob()` / `extract()`.
A streamed archive's bytes went to the consumer, so reading it afterwards
throws, and a cancelled consumer rejects the parked append rather than hanging
it.
A failed append set State::Failed and rejected its own promise, but never told the stream. The consumer's read stayed parked on bytes that would never come, so the pattern the docs recommend hung forever on any ENOENT, with no way out: end() on a failed archive throws without closing the stream either. The failure path now errors the stream with the same value it rejects the append with, so `new Response(archive.stream())` rejects with the ENOENT instead of hanging. Two more from the same review: - A cancelled consumer only settled an append that was already parked. One still on the work pool kept writing chunks into a stream nobody reads, then resolved. `step_from_js` now notices the cancellation itself, so cancelling at any point rejects the append. That also removes the race the cancel test was papering over with a sleep. - `compress: "gzip"` is a post-filter over the finished tar, and nothing post-filters what goes to the stream, so a streamed gzip archive was a raw uncompressed tar with no error. `stream()` rejects it and points at format: "zip", whose compression is per-entry and streams fine. The two backpressure tests no longer wait on wall-clock time: they race the appends against a bounded run of macrotask barriers, and the cancel test is now a `test.each` over 0, 1, and 8 barriers since cancellation is deterministic.
Two ways a stream() consumer could be left waiting forever. stream_is_backed_up() read ByteStream's buffer length as a queue depth. That holds on the pull path, but a buffer_action consumer (the one new Response(stream).bytes() installs) uses the same buffer as an accumulator it never drains. Any entry past the 1MB high-water mark parked the append against a number that only grows, and the only thing that could unpark it was the push that same task would have made. blob_store() matched on state alone, and stream() leaves the state Building until end() runs. So bytes()/blob()/files()/extract() and Bun.write(path, archive) in between closed the builder, handed the caller the bytes, and never signalled the stream. Reading a streaming archive now throws, as the docs already said it did. The docs recommended Bun.write(path, new Response(archive.stream())), which hangs for any stream, archive or not. Use a FileSink pump, which also keeps the memory bound: an 800MB archive costs ~5MB more than a 200MB one. Also assert the specific message on every throw in the new tests.
… when end() fails The docs said a streamed archive served over HTTP was throttled by the socket. It is not. RequestContext attaches ByteStream's pipe and drains its buffer, after which every chunk goes straight to resp.write() and buffer.len() stays 0, so stream_is_backed_up() never trips and uSockets buffers whatever the client has not taken. Measured: a client that reads nothing still lets all 20 appends of an 80MB archive finish. The memory bound holds when the consumer drains chunk by chunk, which is what the file example does. Say that, rather than stating it flat. end()'s streaming branch also set State::Failed and threw without telling the consumer, so a close that failed left the read waiting. Only reachable on OOM, since the sink only fails on a failed try_reserve, but it was the last terminal path not signalling the stream. Abandoning a streaming archive without end() leaves its consumer waiting, same as any ReadableStream whose producer never closes it. Documented as a rule rather than special-cased.
The existing test consumed with new Response(stream).bytes(), which leaves ByteStream through the buffer_action branch. A getReader() consumer leaves through a different one, and it has already taken a chunk when the failure lands, so the error arrives after the stream started producing. Verified load-bearing: neutering error_stream() makes it time out.
A consumer whose loop body awaits is between reads when the append fails, which is the shape worth pinning. It still sees the error: the controller re-pulls after each read resolves, so ByteStream's pending slot is Pending when the Err arrives and it goes out through pending.run() rather than being stashed. Both variants time out if error_stream() is neutered.
Step was inserted between the trait's doc block and the trait, so rustdoc attached 'Context must provide: run / run_from_js / Drop' to the enum and left the trait undocumented. Move Step above the block.
8535a4e to
9c3a670
Compare
Fixes #27077
Fixes #28459
Bun.Archivecould only write tar, and an archive had to be built entirely in memory before any of it could be read, so packing anything bigger than RAM was impossible.format: "zip"Per-entry deflate is the default (levels 1-9);
compress: "store"writes them uncompressed. Reading auto-detects zip alongside tar and tar.gz, soextract(),files(), and the glob filters work on zip input with no option at all. libarchive already compiled its zip reader and writer in, only the bindings were missing.archive.append(path, data)An archive from
new Archive(object)(or from no argument) now keeps its libarchive writer open until its bytes are first read, instead of closing it in the constructor.Appends are serialized through a FIFO onto the work pool, so
Promise.all()over a list of files still produces a deterministic archive. Reading an archive closes it;append()after that, or on anArchivewrapping existing archive data, throws.archive.stream()The bytes come out as a
ReadableStreamwhile the archive is still being written, and the consumer's reads are what let the nextappend()make progress. Once the stream's queue passes its high-water mark, an in-flightappend()parks mid-entry and its work-pool task resumes on the stream's drain. A large archive never has to fit in memory.Streaming a zip straight to an HTTP client, so it sees bytes before the archive is finished:
A slow client does not throttle the
append()s on this path:RequestContextattaches theByteStream's pipe and drains its buffer, after which every chunk goes straight toresp.write()and the buffer length stays 0, sostream_is_backed_up()never trips. Measured with a client that reads nothing: all 20 appends of an 80 MB archive still finish, and uSockets holds what the client has not taken. The response is still produced incrementally; wiringresp.write()'s backpressure back into theByteStreamis aRequestContextchange, not this one. The docs say so rather than claiming a bound that is not there.Memory is flat in the size of the archive. Draining the stream incrementally, under a debug + ASAN build (which has a large fixed floor of its own):
Quadrupling the archive costs 12 MB. That bound is the incremental-drain case. A consumer that asks for the whole thing at once, like
new Response(archive.stream()).bytes(), buffers it all by definition, and theBun.servepath above does not throttle at all; the docs say both.An
append()is split into resumable steps, so a long one yields to the event loop between chunks instead of holding the writer for the whole file. If the consumer cancels, a parkedappend()rejects rather than hanging; if anappend()fails, the consumer's read fails with the same error rather than waiting forever.compress: "gzip"is a post-filter over the finished tar (libdeflate has no streaming API), sostream()throws on a gzip archive and points atformat: "zip", whose compression is per-entry.Bugs this turned up
A latent null-pointer slice.
lib::Archive::next()didslice::from_raw_parts(buff, size)unconditionally onARCHIVE_OK. libarchive's stored-zip reader signals end-of-entry withARCHIVE_OKand*buff = NULL, *size = 0(zip_read_data_none), which tar never produced. Extracting an uncompressed zip aborted:Entries over 2 GiB were silently corrupted.
archive_write_dataclamps its length toINT_MAXand returns the count it actually took, so one call cannot be assumed to consume the whole slice. Writing a singleINT_MAX + 1024byte entry produced an archive whose last 1024 bytes read back as NULs: libarchive padded the body out to the size the header declared. It is now a loop over the return value.Block padding. libarchive zero-pads writes up to its 10240-byte default block, which is what tar wants and what a zip must not have: every zip came out 10 KB minimum with trailing zeros. The zip writer sets
bytes_in_last_block = 1; an empty zip is the canonical 22 bytes again, and tar keeps its padding.Abandoning a writer mid-entry wrote out the whole entry.
archive_write_freeruns a normal close, and closing with a half-written entry makes the format pad that entry to its declared size. A failedappend()of a 600 MB file wrote 600 MB of NULs into the sink before the promise settled, blocking the JS thread for 494 ms.Dropnow callsarchive_write_failfirst, which puts the handle inARCHIVE_STATE_FATALso the close is skipped: 4 MB and 94 ms.An embedded NUL truncated an entry's path.
"a\0b.txt"was archived asa. Rejected up front now, along with the empty path.A sliced
Bun.file()archived its parent's bytes.Bun.file(p).slice(4, 8)ignored the window and wrote the whole file.Bun.file()in the object formThe second commit fixes #28459:
new Bun.Archive({ "a.txt": Bun.file(p) })wrote a 0-byte entry, because the constructor read the blob withshared_view(), which is empty for a file-backed store. It now takes the same streaming pathappend()uses, so the file is copied off disk a chunk at a time rather than read wholly into memory. A missing file throws instead of silently archiving nothing. This overlaps with #28460, which fixes the same issue by reading the file into memory; I went this way becauseappend()needed the streaming path anyway and leaving the object form broken inside this diff would have been strange. Happy to drop the commit if #28460 lands first.Verification
Interop, both directions:
unzip -tand python'szipfileread and verify our output (deflated and stored), and we read and extract theirs.bun install's tarball extraction is untouched, the zip reader is opt-in viaExtractOptions.zip, off there and on forBun.Archive.Two
Bun.writebugs the docs walked intoNeither is this PR's to fix, but both shaped the docs, and the second one looks worth a separate issue. On stock 1.4.0, with no archive code involved:
The first is #31689. The second I have not found an issue for, and it is the pattern an earlier revision of these docs recommended. The docs now pump into a
Bun.file(path).writer()instead, which works and keeps the memory bound.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/archive.test.ts