Repository navigation
Conversation
resolve_size set the size of an S3-backed blob to 0. An S3 object has no size before the download, and a slice already has its final window. Every reader maps a size other than MAX_SIZE to a Range header, so a resolved S3 blob requested `Range: bytes=N-N` and read one byte. This broke the body stream of a Request or Response made from an S3 file (.body, textStream(), HTMLRewriter, WebAssembly.compileStreaming), and every read that came after Bun.inspect(request), expect().toHaveLength(), structuredClone() or a FormData body. The S3 arm of resolve_size and of resolved_size now keeps the size.
Body::Value::size feeds only the `Request (size)` and `Response (size)` header of Bun.inspect. An S3 body printed `0 KB` because resolve_size had set its size to 0. With the size left unknown, get_size_for_bindings returns u64::MAX, which the size formatter prints as `0.0 ZB`. Map the unknown size of an S3 blob to 0, the value a Locked body with no size hint reports. The header of an S3 body does not change, and no other body is affected.
|
Status: closed. #46653 replaces this PR. How to reproduce. A local import { S3Client } from "bun";
const payload = Buffer.alloc(100, "0123456789").toString();
const ranges = [];
using server = Bun.serve({
port: 0,
fetch(req) {
const range = req.headers.get("range");
ranges.push(range);
const m = /^bytes=(\d+)-(\d+)$/.exec(range ?? "");
return m ? new Response(payload.slice(+m[1], +m[2] + 1), { status: 206 }) : new Response(payload);
},
});
const s3 = new S3Client({ endpoint: server.url.href, accessKeyId: "x", secretAccessKey: "y", bucket: "b" });
const request = new Request("http://example.com/", { method: "POST", body: s3.file("key") });
console.log((await Bun.readableStreamToText(request.body)).length); // 1 on 1.4.2 and main, 100 with this PR
console.log(ranges); // [ "bytes=0-0" ] on 1.4.2 and main, [ null ] with this PRThe 16 new tests in |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughS3 Blob size resolution now preserves offsets and sizes. ChangesS3 Blob size and body reads
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to S3-backed body reads retain the intended full-object behavior. No actionable issue remains before merging after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:31 PM PT - Sep 24th, 2026
✅ @robobun, your commit e0df286e495429b6bb5e3df053d1ba8fbc1ca461 passed in 🧪 To try this PR locally: bunx bun-pr 43934That installs a local version of the PR into your bun-43934 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/webcore/Blob.rs— Users who structuredClone or postMessage an S3 file now get a DataClone error on every call, where the base returned a Blob on the second and later calls. The serializer at src/runtime/webcore/Blob.rs:728-736 tags an S3 store as SerializeTag::Bytes and then writes the u64 size at Blob.rs:753 before resolve_size; the Bytes deserializer at Blob.rs:3807 reads the low 32 bits of that size as a byte length, and MAX_SIZE yields 0xFFFFFFFF, which read_slice rejects with TooSmall. Fix: give S3 stores their own SerializeTag (or reuse File with the S3 path record) so serialize and deserialize agree, and add a round-trip test for structuredClone(s3file).Why this was flagged
Trigger:
structuredClone(s3file)orworker.postMessage(s3file); on_structured_clone_serialize at src/runtime/webcore/Blob.rs:728-736 selects SerializeTag::Bytes for an S3 store, writes the blob size at Blob.rs:753, and then Store::serialize (blob/Store.rs:199) writes the S3 path record. on_structured_clone_deserialize at Blob.rs:3806-3808 reads a u32 bytes_len from the first four bytes of the size. With MAX_SIZE = (1<<52)-1 the low u32 is 0xFFFFFFFF, so read_slice fails with Error::TooSmall and the call throws. On the base the first call throws the same way, but resolve_size (Blob.rs:754) then set the size to 0 so every later clone parsed a zero-length Bytes record and returned a Blob; after this change the size stays MAX_SIZE and every call throws. The new test at test/js/bun/s3/s3.test.ts:2248-2250 swallows this with an empty catch, so nothing asserts what structuredClone returns. Remedy: serialize an S3 store with a tag the deserializer understands, and assert the clone result in the test.Verification: pre-existing. Trigger:
structuredClone(s3file)/postMessage(s3file)on any S3-backed Blob. Mechanism verified: /home/claude/bun/src/runtime/webcore/Blob.rs:728-736 tags every non-File store (includingstore::Data::S3) asSerializeTag::Bytes; the else-branch at :753 then writesself.size.get()as a u64 BEFOREself.resolve_size()(:754) andstore.serialize(:755, the S3 arm in… -
🟣
src/runtime/webcore/Blob.rs— Anyone who sends a FormData containing an S3 file as a Request or Response body uploads an empty part, and the file contents are silently dropped. The S3 arm of the multipart serializer at src/runtime/webcore/Blob.rs:3677 is a TODO that writes no bytes after resolve_size at Blob.rs:3668, and nothing reports the omission to the caller. Fix: either read the S3 object into the part (async download like the File arm) or reject the body with an error naming the unsupported S3 entry, so a multipart upload never signals success with missing data.Why this was flagged
Trigger:
form.append("file", s3client.file(key))thennew Response(form).arrayBuffer()orfetch(url, { body: form }); the FormData entry reaches the multipart serializer at src/runtime/webcore/Blob.rs:3668 which calls resolve_size and then matches store::Data::S3 at Blob.rs:3677, an empty arm. Before this change the size was set to 0 and the part was empty; after it the size stays MAX_SIZE and the part is still empty, so the base and the change both produce a multipart body whose S3 part has headers and no payload, with no error. The new test at test/js/bun/s3/s3.test.ts:2258-2260 wraps this call intry {} catch {}and only asserts that the entry can still be read afterwards, so the silent drop is never asserted against. The dismissal treats the arm as out of scope, but the PR touches the size handling that feeds this arm and leaves a successful-looking upload with missing content. Remedy: fail loudly for an S3 entry until the async download is implemented.Verification: pre-existing. Trigger: a FormData holding an S3 blob (
form.append("file", s3client.file(key))) is used as a Request/Response body and serialized. Mechanism verified at /home/claude/bun/src/runtime/webcore/Blob.rs:3666-3679:on_entrywrites the part headers, callsblob.resolve_size()when size is MAX_SIZE (line 3668), then matchesstore::Data::S3(_) => { // TODO: s3 ... }(lines…
Both test files had a copy of the server that plays the S3 endpoint. Move it to test/js/bun/s3/s3-local-endpoint.ts. The matcher assertions name the message of the length comparison, so a throw for another reason does not pass. The bytes() reader is compared with the payload, not with its length.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reply to the two findings outside the diff. Both are present on 1.4.2 and on main, and this PR does not change them.
The inline comments have a reply in each thread. Three are done in 1394f42. |
There was a problem hiding this comment.
Re-reviewed after the two follow-up commits (shared s3-local-endpoint.ts, specific toThrow messages, bytes() now compared by content) and found no new bugs; a maintainer's look is still worthwhile because resolve_size is a shared helper whose S3 semantics this changes, and the same lines are touched by several open PRs named in the description.
What was reviewed:
- Every
resolve_size/resolved_sizecaller (serializer,get_exists_sync,get_size_for_bindings,get_size, FormData multipart,Body::to_readable_stream/to_text_readable_stream, HEAD response inRequestContext,wasm_streaming) for a consumer that relied on the S3 size being 0 — each already mapsMAX_SIZEto "unknown" (Nonerange infrom_blob_copy_ref,content-length: 0on HEAD,NaNinget_size). Body::Value::sizechange is scoped tois_s3(); other bodies' inspect header is unchanged.- New tests: hermetic local
Bun.serve({ port: 0 }),usingdisposal, exact byte and Range-header assertions, and they exercise the changed arm via.body,textStream(),clone(),Bun.inspect,structuredClone, FormData and slices.
Extended reasoning...
The diff is a three-line runtime change in src/runtime/webcore/Blob.rs (S3 arms of resolve_size and resolved_size keep the size instead of setting 0) and src/runtime/webcore/Body.rs (u64::MAX for an S3 blob maps to 0 in the inspect header), plus a new shared test helper and ~230 lines of tests in test/js/bun/s3/ and test/js/web/fetch/. It touches no security-sensitive surface (no auth, parsing of untrusted input, or credentials beyond placeholder test values). Since the prior review, commits 1394f42 and e0df286 addressed three of the six earlier nits; the remaining ones were marked optional or pre-existing and the author replied to each. Deferring rather than approving because the change alters observable behaviour of a shared Blob helper (matchers on S3 files now throw, structuredClone/FormData paths no longer collapse the slice window) and conflicts with open PRs #43897, #43910 and #36296 on the same lines, which is a landing-order decision for a maintainer; no CODEOWNERS entry covers the changed files.
|
#38364 now contains the four commits of this PR, merged in without a change to the source. It needs them: it lets an empty slice of an S3 file through as an empty FormData part, and that is safe only when nothing else writes size 0 onto an S3 blob. Two small edits to the tests came with the merge:
If #38364 lands first, this PR has nothing left to add. If this one lands first, #38364 keeps only its own part. |
|
#46653 replaces this PR, so I am closing this one. It makes the same two changes to Not carried over:
If a maintainer prefers this version, reopen it. |
Related to #41593
Problem
console.log(request),fetch(request)uploads 1 byte of an S3 file body and gets a 200.request.bodyandrequest.textStream()read 1 byte. Each sendsRange: bytes=0-0.resolve_size(src/runtime/webcore/Blob.rs:2145) sets the size to 0. Each S3 reader turns a known size into a Range header.s3file.text(),request.text()andfetch(url, { body: s3file })are correct.Fix
resolve_sizeand ofresolved_sizekeeps the size. No caller changes.Body::Value::sizemaps the unknown size of an S3 blob to 0, so the inspect header staysRequest (0 KB).test/js/web/fetch/body.test.tsandtest/js/bun/s3/s3.test.ts, 15 fail on main.Background
Blobis anoffsetand asizeover a store: bytes, a file, or an S3 key.MAX_SIZEmeans unknown, andresolve_sizereplaces it.resolve_sizeonce set 0 for every store with no size. fix(blob): don't set size to 0 for non-seekable files in resolveSize() #27852 took pipes out. Bun.file: a failed stat leaves the size unknown #39763 is open for a failed stat.Bun.inspect, the matchers,structuredCloneand FormData would still set the size to 0.Downsides
expect(s3file).toHaveLength(0)and.toBeEmpty()passed, and broke the file. They now fail, as for a missingBun.file().Range: bytes=N-Nand reads one byte #43929).new Response(s3file.slice())redirects to the whole object (new Response(s3file.slice(a, b))redirects to the whole object #43930).CryptoHashersee an empty file (APIs that read a Blob synchronously read an S3 file as empty #43931).Notes
Repro. A local server plays the S3 endpoint. The object has 100 bytes.
new Request(url, { body: s3file }).bodybytes=0-0s3file.slice(10, 30)bytes=10-10bytes=10-29Bun.inspect(request), thenfetch(request)s3file.stream()Paths that read one byte before.
.body,textStream(),clone(),new Request(request),HTMLRewriter.transform(),WebAssembly.compileStreaming(), and each read afterBun.inspect(),expect().toHaveLength(),expect().toBeEmpty(),structuredClone()or a FormData body. AResponsereaches these paths throughfetch()of ablob:URL.new Response(s3file)is a redirect and does not.Cost for callers that do not use S3. Release builds of the merge base 4227e46 and of this PR, same toolchain.
.text: 80,829,819 B before and after (llvm-size).Bun.file()bodies: 1000 before and after (gdbcatch syscall).to_readable_stream(.body)to_readable_stream(.body)to_text_readable_stream(.textStream())to_text_readable_stream(.textStream())from_blob_copy_reffrom_blob_copy_refBody::Value::size(Bun.inspect)Body::Value::size(Bun.inspect)Body::Value::size(Bun.inspect)The S3 row includes its one callee,
resolve_size(19 before, 18 after).perf,valgrindandstraceare not installed on the machine that ran this. A count of whole calls, callees included, varies between runs of one binary, so it is not used.Lines left alone on purpose. The File arm of
resolve_sizestill sets the size to 0 whenstatfails (Blob.rs:2143, and:2186inresolved_size). That 0 is the documented.sizeof a missingBun.file(). #39763 changes it.The
resolved_sizearm. Its one caller,ByteBlobLoader::setup, runs only for a bytes store, so no test reaches this arm today. It changes withresolve_sizebecause #43897 makesresolve_sizeapplyresolved_size. After that, this arm is the rule for S3 and the new tests cover it.The
Body::Value::sizeline. On the first commit alone, the twoBun.inspect()tests fail withRequest (0.0 ZB). The mapping is for an S3 blob only, so the header of each other body is the same as on main.Other open PRs on these lines.
resolve_size. Both keep(offset, 0)for S3 inresolved_size. The conflict with this PR is one hunk. Under Blob: resolve_size applies resolved_size #43897, a rebase that keeps(offset, 0)restores the behaviour of main, and the new tests fail.get_size_for_bindingstakes the length fromresolved_size, which is 0 for S3. This PR reports an unknown length toexpect(). One test here pins that:expect(() => expect(file).toHaveLength(0)).toThrow("Received length").Body::Value::size. The second one to land needs a rebase of that line.resolve_sizefor an S3 entry of a FormData. Throw DataCloneError when structured-cloning an S3File #37905 changes structured clone of an S3 file. The new tests wrap those two calls intry/catch, so they pass in each landing order.Other visible changes.
not.toHaveLength(n)andnot.toBeEmpty()pass for it. A missingBun.file()does the same today.expect():not.toHaveLength()andnot.toBeEmpty()pass for aBlobwith no known size #43961 tracks it.structuredClone()of an S3 file gives the same result on each call. Before, the second call gave an emptyBlob, because the first call had set the size to 0. Throw DataCloneError when structured-cloning an S3File #37905 changes what the call does.An open-ended slice as a body. A body made from
s3file.slice(95)now sendsbytes=95-4503599627370494.s3file.slice(95).text(),.stream()andrequest.text()send that header on main too. Before, the body stream sentbytes=95-95and read 1 byte. #43938 tracks the header of an open-ended slice and of a negative index.The inspect header of a file body. Not changed. A slice of a file and a missing file print
0.0 ZB, on main too. #43939 tracks it.No user report. A search of the issues for an S3 body stream,
bytes=0-0andresolve_sizefound no report. #16398 and #27272 are closed reports about slices that were not empty.Suites run with a debug build.
body.test.tsands3.test.ts: 811 pass, 290 skip (286 of them ins3.test.tsneed S3 credentials or Docker).response.test.ts,wasm-streaming.test.ts,blob.test.ts,FormData.test.ts,FormData-multipart-serialization.test.ts,structured-clone-blob-file.test.ts,inspect.test.js,expect.test.js,html-rewriter.test.jsand five S3 suites that use a local server: 1082 pass. In that runhandle stack overflowinresponse.test.tstimed out once, at 5.5 s against a limit of 5 s. Alone it takes 4.8 s on the debug build and passes. It takes 60 ms on a release build of main and of this PR. It uses aResponsewith no body.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file