Deduplicate webcore blob, sink, and S3 helpers

[alii]

What this does

Dedupes webcore lifecycle boilerplate with zero intended behavior change:

• impl_file_closer! (Blob.rs): shared FileCloser impl for blob ReadFile/WriteFile. The two hand-written impls were functionally identical; ReadFile gains bun_io::intrusive_io_request! so both recover the parent through IntrusiveIoRequest instead of an open-coded from_field_ptr!. ReadFileUV keeps its own impl (different shape).
• impl_js_sink_forwarders! (Sink.rs, next to impl_js_sink_abi!): the eight 1:1 JsSinkType forwarders shared by ArrayBufferSink, FileSink, FetchRequestBodySink, HTTPServerWritable, and NetworkSink. Per-sink items (finalize, construct, end_from_js, source, done, HAS_*) stay hand-written.
• CopyFile::fallback_read_write: the read_write_fallback call plus error recording appeared three times in do_copy_file_range.
• Blob::set_content_type_from_js and S3File::finish_s3_blob: the write-path options.type override (in do_write and get_writer) and the S3 constructor epilogue (in both construct_s3_file_with_s3_credentials* fns) were each duplicated verbatim.
• S3File::parse_s3_path_or_blob/resolve_s3_blob and S3Client::construct_blob/blob_and_options: shared argument parsing for the S3 static and instance methods. MissingPathError preserves the per-method split between MISSING_ARGS and invalid-argument errors (unlink always reports MISSING_ARGS; the others only when no argument was passed).
• The explicit 14-arm typed-array JSType lists in Blob construction use the existing JSType::is_array_buffer_like() (same 14-type set).

Net -401 lines of src.

History

Originally the top of the foundations/install-cli/jsc-runtime stack split from #31912. Main moved far enough that every file conflicted, so this was re-derived against current main and retargeted at main; it has no code dependency on the other PRs in that stack.

Tests

The consolidated paths had no coverage of their error contracts, so this adds characterization tests pinning them:

• test/js/bun/s3/s3-argument-validation.test.ts: per-method static rejection messages (including stat reusing the "get size" wording), the instance-method ERR_MISSING_ARGS vs ERR_INVALID_ARG_TYPE split, unlink always reporting MISSING_ARGS, and the S3 writer() non-string type rejection
• test/js/web/fetch/blob-write.test.ts: Bun.file().write() options.type contract (non-string throws, valid types lowercased, known types resolved through the mime table, invalid types ignored)
• test/js/web/fetch/blob.test.ts: Blob construction from all 14 ArrayBuffer-like types, as a direct body value and as an array part

Because this is a behavior-preserving refactor, these tests pass identically with and without the src changes by design; that is the equivalence check, not a gap.

Verification

• Line-by-line audit against main for each consolidation: error messages, argument-eating order, drop order, and the unsafe parent-pointer recovery in schedule_close/on_close_io_request are unchanged (the macro's two trampolines are WriteFile's current impl statement for statement, differing only in fully-qualified paths; ReadFile moves from its older &mut *from_field_ptr! form onto that same already-landed shape)
• bun run rust:check-all: all 10 CI target triples pass
• Debug (ASAN) build: the three test files above (102 pass), sink suites plus request-body and server-response stream suites (9161 pass), Bun.write including both copyFileRange-unavailable fallback tests, local S3 suites (38 pass), blob/FormData suites (165 pass). The large-file fallback test needs more than the 5s default on this ASAN box with or without the change.

---

no test proof · iteration 17 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts

[robobun]

^{Updated 6:36 PM PT - Aug 10th, 2026}

✅ @robobun, your commit 706b9f64a453d544e2d0e29bb2cc087241694242 passed in Build #91789! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32024

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

bun-32024 --bun

[alii]

@robobun adopt

[robobun]

Adopted. Re-derived against current main and retargeted at main (see the PR body for the consolidation list and verification). Characterization tests for the consolidated error contracts are included. One clippy fixup (safety-comment placement inside the macro) landed on top of the merge from main; CI running on 706b9f6.

[claude]

I didn't find any bugs, but this is a substantial refactor of core webcore internals (Blob/S3/sink lifecycle, unsafe intrusive-pointer macros, file-copy fallback) across nine Rust files, so it's worth a human pass before merging.

Extended reasoning...

Overview

This PR consolidates duplicated boilerplate across the webcore runtime: a new impl_file_closer! macro replaces the hand-written FileCloser impls for ReadFile/WriteFile; impl_js_sink_forwarders! collapses identical JsSinkType forwarders across four sink types; CopyFile::fallback_read_write extracts a thrice-repeated read/write-loop block; set_content_type_from_js and finish_s3_blob dedupe two byte-identical content-type/S3-epilogue blocks; parse_s3_path_or_blob/resolve_s3_blob/blob_and_options unify S3 argument parsing; and explicit 14-arm JSType match lists are replaced with is_array_buffer_like() (verified to be the identical 14-type set). Net −411 src lines, plus new characterization tests pinning the consolidated error contracts.

Security risks

No new attack surface is introduced — this is pure consolidation of existing logic. That said, the impl_file_closer! macro encapsulates unsafe intrusive-pointer recovery (from_io_request, from_task_ptr, callback_ctx) that was previously open-coded per type, and resolve_s3_blob/construct_s3_file_with_s3_credentials* reshape ownership flow (std::mem::take on credentials, new Box<Blob> wrapping). A subtle mismatch in any of these would surface as UAF or type confusion in file-I/O / S3 hot paths rather than as a logic bug.

Level of scrutiny

This deserves a careful human review. It is a behavior-preserving refactor by intent, but it spans nine production-critical Rust files (Blob construction, file read/write/copy, S3 client and file, four stream sink types), introduces two macros that hide unsafe operations behind a single expansion site, and sits at the top of a four-PR stack. The argument-eating order, drop order, and error-code selection in the S3 helpers are all load-bearing for observable behavior, and while the new characterization tests pin the error contracts, the unsafe lifecycle paths (schedule_close/on_close_io_request) are not directly testable.

Other factors

The bug-hunting system found nothing, the diff audits cleanly (I spot-checked is_array_buffer_like, the S3 next_eat ordering, and the credentials mem::take against finish_s3_blob's field reads), and the added tests are a genuine improvement in coverage. I'm deferring purely on size/criticality grounds rather than any specific concern.

[robobun]

Re the review above: on the one area flagged as not directly testable, the impl_file_closer! schedule_close/on_close_io_request bodies are line-for-line the two hand-written impls they replace (both went through from_io_request/from_task_ptr intrusive recovery and the same close_after_io = false; update() sequence; ReadFile now also uses IntrusiveIoRequest::from_io_request instead of an open-coded from_field_ptr!, which is the same computation). They run on every Bun.file(path).text()/write() close path, so the existing bun-write.test.js and blob read suites exercise them under ASAN end-to-end even though they cannot be unit-tested in isolation.

The mem::take(&mut aws_options.credentials) is safe because finish_s3_blob only reads aws_options.{options, acl, storage_class, request_payer}, never .credentials; those four are all Copy. Agree this warrants a human pass given the surface area.

[alii]

@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 62af7902-6739-4ba5-8f2b-b514c66065eb

📥 Commits

Reviewing files that changed from the base of the PR and between c63dc64 and 706b9f6.

📒 Files selected for processing (14)

• src/runtime/webcore/ArrayBufferSink.rs
• src/runtime/webcore/Blob.rs
• src/runtime/webcore/FileSink.rs
• src/runtime/webcore/S3Client.rs
• src/runtime/webcore/S3File.rs
• src/runtime/webcore/Sink.rs
• src/runtime/webcore/blob/copy_file.rs
• src/runtime/webcore/blob/read_file.rs
• src/runtime/webcore/blob/write_file.rs
• src/runtime/webcore/fetch/FetchRequestBodySink.rs
• src/runtime/webcore/streams.rs
• test/js/bun/s3/s3-argument-validation.test.ts
• test/js/web/fetch/blob-write.test.ts
• test/js/web/fetch/blob.test.ts

---

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

[robobun]

@alii mergeable again, retargeted at main.

Nothing here had landed on main independently (every consolidation target was still duplicated), so I kept it. Main had moved enough that all nine files conflicted, so rather than resolve hunk by hunk I re-derived each consolidation against current main; the one substantive difference from the old diff is that the sink macro now also covers FetchRequestBodySink, which did not exist when this was written, and lives in Sink.rs next to impl_js_sink_abi!. Details and the verification run are in the PR body. Squashed to one commit with you as co-author.

[claude]

I reviewed this PR and the bug hunting system found no issues. Given the surface area — nine src files including unsafe intrusive-pointer recovery in impl_file_closer! and a fresh re-derivation against ~780 commits of main drift — a human pass is still worthwhile.

What was reviewed:

• is_array_buffer_like() is exactly the 14-type set removed from Blob.rs; next_eat() is side-effect-free so the options-eat/FD-check reordering in resolve_s3_blob is not observable.
• finish_s3_blob reads only the four Copy fields of aws_options after mem::take(&mut credentials); fallback_read_write re-reads self.{source,destination}_fd/destination_file_store which are unchanged across do_copy_file_range.
• MissingPathError preserves the per-method MISSING_ARGS vs INVALID_ARG_TYPE split; impl_js_sink_forwarders! bodies match every removed hand-written forward.

Extended reasoning...

Overview

Behavior-preserving deduplication across webcore blob/sink/S3 lifecycle code, net -411 src lines. Introduces two macros (impl_file_closer!, impl_js_sink_forwarders!), extracts CopyFile::fallback_read_write, set_content_type_from_js, finish_s3_blob, and shared S3 argument-parsing helpers (parse_s3_path_or_blob/resolve_s3_blob/blob_and_options). Replaces four explicit 14-variant JSType match arms with is_array_buffer_like(). Adds characterization tests pinning the consolidated error contracts.

Security risks

None identified. No auth/crypto/permission surfaces touched. The S3 credential handling change (mem::take instead of move) is a local ownership reshaping — the taken-from struct's .credentials field is never read afterward.

Level of scrutiny

High. impl_file_closer! contains unsafe intrusive parent-pointer recovery (from_io_request/from_task_ptr) that runs on every Bun.file().text()/write() close path; the S3 helpers reorder argument consumption relative to error checks; and the whole diff was just re-derived against ~780 commits of main drift without a completed CI run yet on the new head. Each individual consolidation reads as equivalent on line-by-line comparison, but the combination of unsafe code, breadth (5 sinks, 6 S3 methods, 2 file-closer impls), and fresh rebase makes this the kind of refactor a maintainer should sign off on.

Other factors

The prior robobun comment (June 24) already agreed this warrants a human pass. The new characterization tests are useful for pinning error-message contracts but by design pass on both trees, so they do not independently prove equivalence of the unsafe paths — those are covered only by the existing end-to-end blob/write suites under ASAN.

[robobun]

For whoever does the human pass, the quickest way to cover the one area the review singles out (impl_file_closer!): compare the macro's schedule_close and on_close_io_request against the impl FileCloser for WriteFile block this PR deletes from write_file.rs. They are the same statements in the same order (raw parent pointer from from_io_request, (fd, poll) read through it, CloseAction with ctx: this.cast(); then from_task_ptr, clear close_after_io, update), with paths spelled out for the macro. That shape is what #36571 already landed for WriteFile; the only thing that changes for ReadFile is moving off its older &mut *from_field_ptr! form onto it.

On this head the paths were exercised end to end under ASAN via the Bun.write suite (including both copy fallback tests), the blob read suites, and the sink and stream suites listed in the PR body. CI on the new head is most of the way through with no failures so far.

[claude]

I reviewed this PR and didn't find any bugs. Given the surface area — unsafe intrusive-pointer recovery in impl_file_closer! on the ReadFile/WriteFile close path, sink trait dispatch across five sink types, and S3 credential/argument-parsing lifecycle — a human pass is still worthwhile (as the thread already notes).

What was reviewed:

• JSType::is_array_buffer_like() matches the exact 14-type set it replaces.
• resolve_s3_blob hoists args.next_eat() above the Fd check for presign/unlink; next_eat is a pure index advance so the reorder is not observable.
• mem::take(&mut aws_options.credentials) — finish_s3_blob reads only the four Copy fields, never .credentials.
• impl_file_closer! is statement-for-statement the deleted WriteFile impl; ReadFile moves from from_field_ptr! to the equivalent IntrusiveIoRequest::from_io_request.

Extended reasoning...

Overview

This PR deduplicates ~400 lines of webcore boilerplate across 11 Rust source files (Blob, S3Client, S3File, five sink types, blob read/write/copy) plus three test files. The consolidations are: an impl_file_closer! macro replacing hand-written FileCloser impls for ReadFile/WriteFile; an impl_js_sink_forwarders! macro for eight identical JsSinkType trait forwarders across five sinks; shared S3 argument-parsing helpers (parse_s3_path_or_blob/resolve_s3_blob/blob_and_options/construct_blob); a shared S3 constructor epilogue (finish_s3_blob); a shared blob options.type setter; a CopyFile::fallback_read_write helper; and replacing four 14-arm JSType match lists with is_array_buffer_like(). Characterization tests pin the error-message and error-code contracts of the consolidated paths.

Security risks

None identified. The S3 credential handling change (mem::take on aws_options.credentials before passing &aws_options to finish_s3_blob) was checked: the epilogue reads only options/acl/storage_class/request_payer, all Copy, and never the taken credentials field. No new user-input parsing surface is introduced; the argument-validation helpers preserve the existing per-method ERR_MISSING_ARGS vs ERR_INVALID_ARG_TYPE split (verified against the new s3-argument-validation.test.ts).

Level of scrutiny

High. The impl_file_closer! macro emits unsafe intrusive-pointer recovery (from_io_request/from_task_ptr) that runs on every Bun.file(path).text()/.write() close path — a memory-safety-critical hot path. The sink forwarder macro affects dispatch for five production sinks (ArrayBuffer, File, HTTP response, Network/S3, fetch request body). While each individual consolidation is mechanical and my spot-checks found the transformations behavior-preserving (including the next_eat() reordering in resolve_s3_blob, which is pure), the breadth and the unsafe-block content put this outside what I'd approve without a human confirming the impl_file_closer! equivalence in particular.

Other factors

The PR thread already flags this for a human pass (robobun's 2026-06-24 comment: "Agree this warrants a human pass given the surface area"). CI on the current head was reported green, rust:check-all passes across all 10 targets, and the added characterization tests pin the S3/blob error contracts. The bug-hunting system found no issues. I verified is_array_buffer_like() is exactly the same 14-type set being replaced, and that ArgumentsSlice::next_eat runs no user JS so its hoist above the Fd-rejection check is unobservable.