Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughExports gzip compressor and adds a cross-thread compress-and-write flow for ChangesArchive Compression on Write
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the bug, reproduction, cause, fix, affected destinations, implementation details, tests, verification results, and linked issue. It provides the information requested by the template, although it uses different section headings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d05623-0b37-4eb0-a5ab-0d280a34b404
📒 Files selected for processing (3)
src/runtime/api/Archive.zigsrc/runtime/webcore/Blob.zigtest/js/bun/archive.test.ts
|
Updated 6:14 PM PT - Aug 25th, 2026
❌ @robobun, your commit fb5d0c9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30236That installs a local version of the PR into your bun-30236 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/webcore/Blob.zig`:
- Around line 1456-1472: runFromJS in ArchiveCompressAndWriteTask returns early
when this.vm.isShuttingDown(), but leaves the successful compression result
(this.result.ok) allocated; ensure you free or deinitialize the compressed bytes
before returning on shutdown. Locate runFromJS and, just before the early return
guarded by this.vm.isShuttingDown(), release the memory held in this.result.ok
(call the appropriate deinit/free method for the buffer or blob and clear the
variant) so the defer doesn't need to handle this specific allocation and no
leak occurs.
🪄 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: ddae2aaa-94d1-4b7e-bcf4-b277da6090d1
📒 Files selected for processing (1)
src/runtime/webcore/Blob.zig
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/webcore/Blob.zig`:
- Around line 1446-1449: The current error handling after calling
Archive.compressGzip(this.store.sharedView(), this.level) stores any error into
this.result, which will turn an OutOfMemory into a recoverable JS error; change
the else branch to detect error.OutOfMemory and call bun.handleOom() (or the
appropriate handleOom symbol) to crash immediately, and only set this.result =
.{ .err = err } for non-OOM errors; keep the call site around
Archive.compressGzip and the assignment to this.result unchanged for non-OOM
cases.
🪄 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: edafd3f0-c10b-4672-9ef4-338a647c6858
📒 Files selected for processing (1)
src/runtime/webcore/Blob.zig
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/webcore/Blob.zig (1)
1444-1450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCrash on gzip OOM instead of rejecting
Bun.write().
Archive.compressGzip()can returnerror.OutOfMemory, and this path currently stores it inthis.resultlike any other gzip failure. That turns allocator exhaustion into a recoverable JS rejection on the worker path.🐛 Proposed fix
fn runFromThreadPool(task: *jsc.WorkPoolTask) void { const this: *ArchiveCompressAndWriteTask = `@fieldParentPtr`("work_task", task); - if (Archive.compressGzip(this.store.sharedView(), this.level)) |bytes| { - this.result = .{ .ok = bytes }; - } else |err| { - this.result = .{ .err = err }; - } + const bytes = Archive.compressGzip(this.store.sharedView(), this.level) catch |err| switch (err) { + error.OutOfMemory => bun.outOfMemory(), + else => { + this.result = .{ .err = err }; + this.vm.eventLoop().enqueueTaskConcurrent( + jsc.ConcurrentTask.create(jsc.ManagedTask.New(ArchiveCompressAndWriteTask, runFromJS).init(this)), + ); + return; + }, + }; + this.result = .{ .ok = bytes }; this.vm.eventLoop().enqueueTaskConcurrent( jsc.ConcurrentTask.create(jsc.ManagedTask.New(ArchiveCompressAndWriteTask, runFromJS).init(this)), ); }Based on learnings, "In oven-sh/bun Zig code, follow the established OutOfMemory handling pattern: when you have an exhaustive
switch (err)over the error union, keep usingerror.OutOfMemory => bun.outOfMemory()inside the exhaustive switch arm."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/webcore/Blob.zig` around lines 1444 - 1450, The handler in runFromThreadPool for Archive.compressGzip currently stores all errors (including error.OutOfMemory) in this.result which wrongly turns allocator OOM into a recoverable JS rejection; modify the error arm to distinguish OutOfMemory and call bun.outOfMemory() for error.OutOfMemory, while preserving the existing this.result = .{ .err = err } behavior for other errors (keep references to Archive.compressGzip, ArchiveCompressAndWriteTask, this.result, and bun.outOfMemory to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/runtime/webcore/Blob.zig`:
- Around line 1444-1450: The handler in runFromThreadPool for
Archive.compressGzip currently stores all errors (including error.OutOfMemory)
in this.result which wrongly turns allocator OOM into a recoverable JS
rejection; modify the error arm to distinguish OutOfMemory and call
bun.outOfMemory() for error.OutOfMemory, while preserving the existing
this.result = .{ .err = err } behavior for other errors (keep references to
Archive.compressGzip, ArchiveCompressAndWriteTask, this.result, and
bun.outOfMemory to locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2365873-2417-4952-a234-8d3ce62c3fde
📒 Files selected for processing (1)
src/runtime/webcore/Blob.zig
There was a problem hiding this comment.
No further issues found — all earlier feedback (sync compression, destination_blob detach, shutdown-path free) is addressed. That said, this adds a new ~140-line WorkPool task with promise chaining and several ownership handoffs inside writeFileInternal, so it's worth a human pass over the lifetime/refcount edges before merge.
Extended reasoning...
Overview
This PR fixes #30234: Bun.write(path, archive) ignored the Archive's compress: "gzip" option and wrote the raw tarball. The fix:
src/runtime/api/Archive.zig: makescompressGzippublic (1 line).src/runtime/webcore/Blob.zig: addsArchiveCompressAndWriteTask(~140 lines) that gzips the archive store on the WorkPool, then on the JS thread wraps the result in a byte-backed Blob and resumeswriteFileWithSourceDestination, splicing the inner write promise onto the outer one returned to JS.writeFileInternal's Archive branch now switches onarchive.compress, scheduling the task for.gzipand keeping the old zero-copy store-ref path for.none.test/js/bun/archive.test.ts: three regression tests (gzip magic + byte-equality witharchive.bytes(), level 1 vs 12 sizing,Bun.file()destination).
Security risks
None identified. No new user-controlled parsing, no path handling, no auth/crypto. Inputs are the same Archive store and level already accepted by archive.bytes() / Bun.Archive.write. The task only touches local file/S3 destinations via the existing writeFileWithSourceDestination pipeline.
Level of scrutiny
Moderate-to-high. writeFileInternal is the core of Bun.write() and the new task carries several owned resources across a thread-pool hop: a Store ref, a destination_blob (whose detach is deferred because writeFileWithSourceDestination only struct-copies it), a Strong.Optional for extra_options, a KeepAlive, the outer JSPromise.Strong, and the heap-allocated compressed buffer whose ownership moves into a Blob on success but must be freed on the isShuttingDown() early exit. Each of these has a correct-looking release path in the current diff, but the PR took four rounds of bot-driven fixes (move gzip off the JS thread → bc2cd2d; detach destination on task-creation OOM → 9aec885; free compressed bytes on VM shutdown → 2dd781f) to reach this state, which is a signal that the invariants here are subtle.
Other factors
- All prior inline comments (mine and CodeRabbit's) are resolved; the only commit since the last code review is d5def78, a CI retrigger with no code change.
- The OOM-as-rejection behavior in
runFromThreadPoolmatches the established convention inBlobContext.run/WriteContext.run(confirmed by CodeRabbit), so it's intentionally consistent rather than a regression. - Test coverage for the happy paths is solid (103 pass per the description); the edge cases that warrant a human look — inner/outer promise forwarding when
writeFileWithSourceDestinationreturns a non-promise vs. pending vs. already-settled value, S3 destinations viaextra_options, and Worker-VM shutdown mid-task — aren't exercised by the new tests and are hard to cover automatically.
Given the above, I'm not auto-approving; a maintainer glance at the task's lifetime handling and the promise-splice in runFromJS would be worthwhile.
There was a problem hiding this comment.
No issues found on this revision — all prior feedback is addressed — but this adds ~140 lines of new cross-thread async task machinery (WorkPool → JS hop, promise splicing, manual store/blob/Strong lifecycle) inside writeFileInternal, so I'd like a human to sign off rather than auto-approving.
Extended reasoning...
Overview
This PR fixes Bun.write(path, archive) to honor the compress: "gzip" option on a Bun.Archive instance. It touches three files: src/runtime/api/Archive.zig (1-line visibility change making compressGzip pub), src/runtime/webcore/Blob.zig (~170 added lines: a new ArchiveCompressAndWriteTask struct plus a switch (archive.compress) in writeFileInternal), and test/js/bun/archive.test.ts (3 new regression tests). The new task compresses the archive's store on the WorkPool, then re-enters the JS thread to wrap the result as a byte-backed Blob and hand it to writeFileWithSourceDestination, splicing the inner write promise onto the outer one returned to the caller.
Security risks
None identified. The change does not touch auth, permissions, path resolution, or untrusted-input parsing; it reuses the existing compressGzip (libdeflate) and writeFileWithSourceDestination paths that archive.bytes() / Bun.Archive.write already exercise. No new attack surface is introduced.
Level of scrutiny
Medium-high. writeFileInternal is the core implementation behind Bun.write() — one of the most widely used runtime APIs — and the new code introduces a hand-rolled async task with cross-thread state, a KeepAlive, a JSPromise.Strong, a jsc.Strong.Optional for extra_options, manual Store ref/deref, Blob ownership transfer, and outer/inner promise chaining via outer.resolve(globalThis, inner). The pattern closely mirrors Archive.zig's existing AsyncTask(BlobContext/WriteContext), and the lifecycle looks correct on inspection, but the number of moving parts and the criticality of the call site put it outside the "simple/mechanical" bar for auto-approval.
Other factors
- All four prior review threads (two from me, two from CodeRabbit) are resolved in the current diff: compression is off the JS thread,
destination_blob.detach()runs on the task-creation OOM errdefer, andresult.okis freed on theisShuttingDown()early-return. - The OOM-as-rejection behaviour matches the established convention in
BlobContext.run/WriteContext.runper the resolved discussion. - Three targeted regression tests cover gzip magic + byte-equality with
archive.bytes(), level 1 vs 12 sizing, and theBun.file()destination path; the fullarchive.test.tssuite reportedly passes (103/103). - CI on 2dd781f showed failures; the two most recent commits are explicit CI retriggers ("transient zig compiler panic", "expired build"), suggesting infra flakiness rather than a code problem, but final CI status should be confirmed before merge.
e09cd3a to
ca193c3
Compare
|
CI note for reviewers (updated): this PR's own tests are green; the red
This PR only changes |
4d79a58 to
db4e50f
Compare
There was a problem hiding this comment.
All prior feedback has been addressed and I didn't find new issues, but this adds a new cross-thread task type with an unsafe impl Send (carrying a Blob + JSValue across the WorkPool hop) and reroutes a branch of write_file_internal — worth a human pass on the Send justification and the promise-forwarding in run_from_js.
Extended reasoning...
Overview
This PR fixes #30234: Bun.write(path, archive) ignored the Archive's compress: "gzip" setting and wrote the raw tarball. The fix adds a new ArchiveCompressWriteContext / ArchiveCompressWriteTask that compresses on the WorkPool and then resumes write_file_with_source_destination on the JS thread, plus wiring in ConcurrentTask.rs (new tag), dispatch.rs (new arm + count bump 97→98), a small branch in Blob.rs::write_file_internal, and three regression tests. The PR was re-implemented on the Rust runtime after the Zig sources were removed from main.
Security risks
None identified. The change does not touch auth, crypto, permissions, or untrusted-input parsing; it routes already-validated archive bytes through an existing compression helper and the existing write pipeline. extra_options (S3 credentials) is GC-protected for the duration of the async hop and unprotected in Drop.
Level of scrutiny
Moderate-to-high. While the logical fix is straightforward, the implementation:
- Adds a hand-written
unsafe impl Send for ArchiveCompressWriteContextwhose SAFETY comment assertsBlobisSend(I could not find an explicitimpl Send for Blob; the soundness argument actually rests onrun()only touchingstore/levelon the worker thread, which is true, but a maintainer should confirm the convention). - Threads a
JSValueacross a thread-pool hop with manualprotect()/unprotect()balanced viaDrop. - Modifies
write_file_internal, a core hot path used by everyBun.writecall. - Forwards an inner write promise onto the outer task promise (
PromiseResult::Resolve(new_promise)for the pending case).
These are all reasonable and follow the sibling BlobContext/WriteContext patterns in the same file, but cross-thread + GC + unsafe is exactly where a second set of eyes pays off.
Other factors
All four prior review threads (sync compression on JS thread, destination_blob leak, compressed-buffer leak on shutdown, weak test assertion) have been addressed; the Rust port handles the shutdown-leak case naturally via Vec<u8> Drop. The bug-hunting system found nothing on the current revision. CI redness on Windows is unrelated (napi CRLF + ConPTY flake per the author's note). Test coverage is solid: gzip magic + byte-equality with archive.bytes(), strict level-1-vs-12 size comparison, and a Bun.file() destination case.
|
Re: the
The only remaining |
db4e50f to
c9cc228
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after 961da27 and didn't find any new issues — my earlier inline nit about multi-line comments is now stale (those sites were already trimmed in that commit). Given the new unsafe impl Send carrying a JSValue across the thread-pool hop and the promise-forwarding in run_from_js, a maintainer look would still be worthwhile before merge.
Checked: write_file_with_source_destination clones the source store (Blob.rs:4634), so the stack-local source blob dropping is safe; Blob/StoreRef are both Send (webcore_types.rs:149/1117), matching the SAFETY note; extra_options is protected before scheduling and unprotected in Drop, which runs on the JS thread via AsyncTask::then; the CompressError → &'static str conversion comes from strum::IntoStaticStr.
Extended reasoning...
Overview
The PR routes Bun.write(dest, archive) for gzip-configured Archive instances through a new ArchiveCompressWriteContext async task in src/runtime/api/Archive.rs (~110 new lines): compress on the work pool, then hand a byte-backed Blob to the existing write_file_with_source_destination on the JS thread and forward the inner write promise onto the caller's promise. src/runtime/webcore/Blob.rs gains a small branch in write_file_internal that dispatches to this task when archive.gzip_level() is Some. Three regression tests are added to test/js/bun/archive.test.ts.
Security risks
None identified. The change does not touch parsing of untrusted input, auth, or network paths; it re-uses the existing compress_gzip helper and write_file_with_source_destination pipeline. The archive data being compressed is the caller's own in-memory tarball.
Level of scrutiny
Medium-high. The new code introduces an unsafe impl Send on a struct holding an Option<JSValue> (extra_options), plus protect()/unprotect() bracketing across a thread-pool hop, and non-trivial promise-forwarding. These are exactly the memory-safety patterns REVIEW.md flags as the most-blocked category. The mitigations look correct — the SAFETY comment is accurate (verified Blob and StoreRef are Send; run() reads only store/level), Drop runs on the JS thread via AsyncTask::then taking ctx by value, and the shape mirrors the sibling BlobContext/WriteContext tasks in the same file — but a maintainer should confirm the bun_jsc::Job teardown path drops ctx on the JS thread when a VM shuts down mid-task.
Other factors
This PR has been through several review rounds: earlier findings (sync compression on the JS thread, missing destination_blob detach on error, compressed-buffer leak on shutdown, weak toBeLessThanOrEqual assertion, and comment-cop verbosity flags) were all addressed and resolved. My last inline comment (07:09 UTC today) referenced pre-961da27 line numbers and is stale — every site it named was already trimmed in that commit. The current bug-hunting run found no issues. Tests include fail-before verification against the released binary and cover string-path, Bun.file() destination, and level plumbing.
|
Re: the teardown question — confirmed, and then made moot in 055e641:
Verified after the change: |
There was a problem hiding this comment.
I reviewed this PR and found no bugs; the last commit (586708d) addressed my remaining reject_with_async_stack nit, so all prior feedback is now resolved. Because it adds a new thread-pool → JS-thread JobContext with GC-protected state and Blob ownership carried across the hop, a maintainer sign-off would still be worthwhile.
What was reviewed:
CompressWriteOff: Sendholds via the existingBlob/StoreRefSendimpls;run()touches onlystore/level.Js = (JSPromiseStrong, Option<Protected>)satisfiesJsAffinevia the tuple/Option/Protectedimpls injob.rs;Protectedstays alive through thewrite_file_with_source_destinationcall and unprotects on drop.write_file_with_source_destinationclones the source store (Blob.rs:4634), so the local byte-backedsourcedropping inthen()is safe.- Promise-forwarding matches the
WriteFileWaitFromLockedValueTaskshape; error paths now usereject_with_async_stackconsistently with sibling tasks.
Extended reasoning...
Overview
The PR fixes #30234: Bun.write(dest, archive) ignored the compress: "gzip" option and wrote the raw tarball. It adds ArchiveCompressWriteJob (a direct jsc::JobContext impl) in src/runtime/api/Archive.rs that gzips the store on the work pool and then resumes write_file_with_source_destination on the JS thread with a byte-backed source blob, plus a small branch in write_file_internal (Blob.rs) that routes gzip-configured archives through it. Three regression tests in test/js/bun/archive.test.ts cover gzip magic + byte-for-byte equality with archive.bytes(), level 1 vs 12 strict-size ordering, and the Bun.file() destination variant.
Security risks
None identified. No user-controlled paths or parsing are introduced; the gzip level is already validated in parse_compression_options (1–12). extra_options (S3 credentials) is GC-pinned via Protected for exactly the thread-pool window and released on drop, matching the framework's intended shape.
Level of scrutiny
High — this is native async code with thread-affinity and GC-lifetime concerns, the most-blocked category per REVIEW.md. That said, the implementation now sits entirely on the blessed JobContext/JsAffine/Protected machinery (no unsafe in the diff, no manual protect/unprotect, no hand-rolled Drop), and the promise-forwarding / error-path shape mirrors sibling tasks in the same file. I verified: CompressWriteOff is auto-Send via the existing unsafe impl Send for Blob/StoreRef; the Js tuple is JsAffine via the blanket impls in job.rs; the source blob's store is cloned inside write_file_with_source_destination before the local drops; the compressed Vec<u8> is owned by CompressWriteResult and drops naturally on every path (including VM-teardown, per job.rs's guarantee that both halves drop on the JS thread with the heap alive).
Other factors
This PR has been through several review rounds. My earlier findings — the shutdown-path buffer leak (Zig era), the vacuous toBeLessThanOrEqual level assertion, the multi-line comment narration, and the plain reject on a fresh error — have each been addressed in follow-up commits, the last in 586708d. The comment-cop bot's remaining flags were rebutted (the doc-comment on the JobContext deviation and the two-line ownership note in Blob.rs are factual, not workaround justifications). CI failures noted in the thread are pre-existing flakes unrelated to this path. Given the thread-pool + GC surface, I'm deferring for a maintainer sign-off rather than shadow-approving.
586708d to
ea110fa
Compare
There was a problem hiding this comment.
LGTM — d126b7b addresses the last nit (CompressError enum → e.to_js(global)), so all four archive entry points now produce the same error type/message.
What was reviewed
ArchiveCompressWriteJobuses theJobContextsplit as designed:Js = (JSPromiseStrong, Option<Protected>)satisfiesJsAffinevia the tuple/Option blankets in job.rs;CompressWriteOffis auto-Send(nounsafe).- All three
WriteFileOptionsfields (mkdirp_if_not_exists,mode,extra_options) thread through;extra_optionsis GC-pinned viaProtected::newand dropped on the JS thread per job.rs guarantees. - The Blob.rs early-return moves
destination_blobout viamem::replace; the empty replacement's drop is a no-op, and thesource_blobscopeguard below is never reached. - Error paths match sibling tasks:
reject_with_async_stackfor fresh errors, plainreject(.., Err(e))for pending exceptions.
Extended reasoning...
Overview
Fixes #30234: Bun.write(path, new Bun.Archive(obj, { compress: "gzip" })) wrote raw uncompressed tar. Three files touched: src/runtime/api/Archive.rs (+~110 lines: gzip_level() accessor + ArchiveCompressWriteJob implementing jsc::JobContext + start_archive_compress_write_task), src/runtime/webcore/Blob.rs (+13 lines routing the gzip case through the new task), and three regression tests in test/js/bun/archive.test.ts.
Security risks
None identified. No new parsing of untrusted input; the gzip path reuses the existing compress_gzip helper. No path handling changes. extra_options (S3 credentials) is GC-protected via the framework's RAII Protected type on the Js side of the job, which job.rs guarantees drops on the JS thread with the heap alive.
Level of scrutiny
Medium-high — native runtime code with thread-pool + JSC interop. However: the new code contains zero unsafe blocks; it uses the JobContext/JsAffine/Protected framework exactly as designed (verified (JSPromiseStrong, Option<Protected>) satisfies JsAffine via job.rs:113/109/89/119); CompressWriteOff is auto-Send because Blob/StoreRef already carry the necessary impls and CompressError is a plain unit enum; and the shape mirrors the sibling BlobContext/WriteContext tasks in the same file.
Other factors
This PR has been through four review rounds. Every prior finding is addressed and resolved: (1) the shutdown-path leak of the compressed buffer — now moot under JobContext's drop-on-JS-thread guarantee; (2) the weak toBeLessThanOrEqual level test — now strict toBeLessThan with the 50KB payload matching the sibling test; (3) narrative comments — trimmed, comment-cop threads resolved; (4) plain reject for a fresh error — now reject_with_async_stack; (5) &'static str error flattening — now stores CompressError and calls e.to_js(global) (d126b7b). The bug-hunting system found nothing on this revision. Tests cover gzip magic + byte-for-byte parity with archive.bytes(), level plumbing, and the Bun.file() destination variant.
The Archive -> source-blob branch of write_file_internal cloned the
archive's store as-is, so Bun.write(path, new Bun.Archive(obj,
{ compress: "gzip" })) wrote the raw tarball. Archive.write(...) and
archive.bytes()/blob() already gzip on the thread pool; only the
Bun.write path was broken.
Gzip-configured archives now schedule ArchiveCompressWriteJob (a direct
jsc::JobContext): compress_gzip runs on the work pool, then the JS-side
completion hands the compressed bytes to
write_file_with_source_destination as a byte-backed source blob and
forwards the inner write promise. JS-affine completion state
(extra_options as a Protected) rides the Js side, so the off-thread
half is plain Send data. Local-file, Bun.file, and S3 destinations all
receive the compressed bytes.
Fixes #30234.
Stores the CompressError enum instead of flattening to a static str, so Bun.write(dest, gzipArchive) produces the same error types as the sibling entry points (OutOfMemory routes to create_out_of_memory_error; others reject with the bare tag).
d126b7b to
0d0930c
Compare
The inner calls already return JsResult<()>; bare returns match the sibling AsyncTask::then.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Superseded by #46661, which includes this change: |
What
new Bun.Archive(obj, { compress: "gzip" })+await Bun.write(path, archive)wrote the raw uncompressed tarball to disk, ignoring thecompressoption.Bun.Archive.write(path, obj, { compress: "gzip" })andarchive.bytes()/archive.blob()on the same instance all produce the correct gzip; only theBun.writepath was broken.Reproduction
Before:
out.tar.gzbegan with68 65("he" from "hello.txt") andgzip -t out.tar.gz→not in gzip format.After:
out.tar.gzbegins with1f 8b(gzip magic), byte-for-byte identical toarchive.bytes()/Bun.Archive.write.Cause
In the
Bun.writeimplementation (write_file_internal), the branch that unwraps anArchivesource re-used the archive'sstoredirectly (the uncompressed tarball built in the constructor) and never consulted the archive's compression setting. The staticArchive.writeandarchive.bytes()/blob()routes both compress on the thread pool; only theBun.writeentry point forgot.Fix
src/runtime/api/Archive.rs— newArchiveCompressWriteContext(anAsyncTaskjob): gzips the store on the work pool, then on the JS thread hands the compressed bytes towrite_file_with_source_destinationas a byte-backed source blob and forwards the inner write promise onto the one returned to the caller. Also agzip_level()accessor onArchive.src/runtime/webcore/Blob.rs— theArchivebranch ofwrite_file_internalroutes gzip-configured archives through the new task; uncompressed archives keep the zero-copy store path.This keeps gzip off the JS thread (parity with
archive.bytes()/Archive.write) and still honors local-file,Bun.file(), and S3 destinations (unlike the path-only staticWriteContext).options.extra_options(S3 credentials) is GC-protected across the thread-pool hop and released inDrop.Rebase notes
bun_jsc::Job: the earlierdispatch.rs/ConcurrentTask.rstag wiring became unnecessary and was dropped, and the task now usesAsyncTask::startlike the sibling archive tasks. Net diff is 3 files.Tests
Three regression tests in
test/js/bun/archive.test.ts→describe("Bun.write with Archive"):archive.bytes()+ libarchive round-triplevelplumbing)Bun.file()destinationVerification:
USE_SYSTEM_BUN=1 bun test test/js/bun/archive.test.ts -t "30234|gzip compression level|gzipped archive to a Bun.file"→ 3 fail (bug present)bun bd test test/js/bun/archive.test.ts→ 109 pass, 0 fail (fix works, no regression)Fixes #30234.
no test proof · iteration 14 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/archive.test.ts