Skip to content

Blob: resolve_size applies resolved_size - #43897

Open
robobun wants to merge 1 commit into
mainfrom
robobun/c897cd79/blob-resolve-size-one-impl
Open

robobun wants to merge 1 commit into
mainfrom
robobun/c897cd79/blob-resolve-size-one-impl

Conversation

@robobun

@robobun robobun commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Blob::resolve_size and Blob::resolved_size (src/runtime/webcore/Blob.rs) hold the same size rules twice. One writes offset and size onto the blob. One returns the pair.
  • A rule changed in one body only makes ByteBlobLoader::setup disagree with the nine resolve_size callers. A follow-up changes the rules for file stores. It must change them in one place.

Fix

  • resolve_size takes the pair from resolved_size and stores it. resolved_size keeps the rules.
  • Correct because the two bodies agree in every arm (table in Notes). Both stat a file store under the same condition.
  • The Stacked Borrows comment and the FIFO comment move to resolved_size, where that code now lives.
  • Verified: blob.test.ts, body.test.ts, body-clone.test.ts, wasm-streaming.test.ts and structured-clone-blob-file.test.ts cover this code and pass. More in Notes.

Behaviour change: none

Background

  • A Blob is a view (offset, size) onto a store: memory, a file, or an S3 object. size is MAX_SIZE while unknown. Resolving fills it in from the store. For a file that is one stat, cached on the store.
  • resolve_size serves .size, exists(), body streams, FormData, HEAD Content-Length and structured clone. resolved_size exists because ByteBlobLoader::setup resolves a blob that it cannot mutate.
  • Considered editing both bodies in the follow-up. That keeps the drift risk.

Downsides

  • None found. Checked: every arm returns what it stored before. The same stat runs under the same condition. No call gains an allocation or a syscall. Arms that stored nothing now store the values already there.
Notes

The two bodies, arm by arm. "stores" is what resolve_size wrote before this PR. "returns" is what resolved_size returns.

store resolve_size stores resolved_size returns
none size = 0 (offset, 0)
bytes offset = min(len, offset), size = window_size(size, len - offset) the same pair
file, stat known offset = min(st_size, offset), size = window_size(size, st_size - offset) the same pair
file, not seekable (pipe, FIFO) nothing (offset, size)
file, stat failed size = 0 (offset, 0)
S3 size = 0 (offset, 0)

Suites run with a debug build:

  • blob.test.ts, body.test.ts, body-clone.test.ts, wasm-streaming.test.ts, structured-clone-blob-file.test.ts: 1044 pass, 0 fail.
  • fetch.test.ts, bun-file-exists.test.js, bun-file.test.ts, bun-file-read.test.ts, bun-serve-file.test.ts, FormData.test.ts: 663 pass and 21 fail, with this change and with src/ at main. The set of failing test names is identical in both runs. The 21 need IPv6, the public internet, a non-root user, or more than 5 s under ASAN, and the test machine has none of these.

This PR is the first of a stack of two. #43910 is the second, and its base is this branch. It stops resolve_size from copying a cached stat size onto a file blob, which is the cause of #4930 and #23902, and it lets each file reader stop at the size from its own fstat.


no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check

resolve_size and resolved_size held the same size rules twice. One wrote
the offset and size onto the blob, the other returned the pair. The two
bodies agree in every arm, and both stat a file store under the same
condition.

resolve_size now takes the pair from resolved_size and stores it. The
Stacked Borrows comment and the FIFO comment move to resolved_size,
because the code they describe lives there now.

No caller can observe a difference.
@robobun

robobun commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:35 AM PT - Sep 24th, 2026

✅ @robobun, your commit a49a8256e50c09179a7425cec1f18ca19e4f985e passed in Build #120321! 🎉


🧪   To try this PR locally:

bunx bun-pr 43897

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

bun-43897 --bun

@robobun

robobun commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. This PR is a refactor with no behaviour change, and it is the first of a stack of three.

How it was checked:

  • resolve_size and resolved_size agree in every arm. The table is in the Notes block of the description.
  • blob.test.ts, body.test.ts, body-clone.test.ts, wasm-streaming.test.ts and structured-clone-blob-file.test.ts pass with a debug build (1044 tests).
  • fetch.test.ts, bun-file-exists.test.js, bun-file.test.ts, bun-file-read.test.ts, bun-serve-file.test.ts and FormData.test.ts give 663 pass and 21 fail with this change, and the same 21 with src/ at main. Those 21 need IPv6, the public internet or a non-root user, which the test machine does not have.

The bug the stack fixes, on canary 1.4.3+367d939d9 (Linux):

const f = Bun.file("/proc/self/status");
f.size;                 // or: await f.exists()
await f.text();         // "" (a fresh Bun.file(p).text() returns about 1500 bytes)

The same cached size makes #4930 write 0 bytes. The next two PRs follow once this one has a green build.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 62a9e04d-c1e7-4204-ba13-80145b24c84a

📥 Commits

Reviewing files that changed from the base of the PR and between 8d36bff and a49a825.

📒 Files selected for processing (1)
  • src/runtime/webcore/Blob.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

Blob::resolve_size now applies the offset and size returned by Blob::resolved_size. The helper clamps byte-backed and seekable-file views to the available store range, preserves MAX_SIZE for non-seekable files with unknown size, and returns zero size for unresolved stores.

Changes

Blob size resolution

Layer / File(s) Summary
Resolve blob view window
src/runtime/webcore/Blob.rs
resolved_size computes the offset and size without mutating the blob. resolve_size applies the returned values. Byte-backed and seekable-file views are clamped to the available range. Non-seekable files with unknown size retain MAX_SIZE.

Suggested reviewers: jarred-sumner, dylan-conway

Merge Risk: ⚪ Minimal · up to a49a8

The Blob size-resolution change preserves the inspected behavior, with no actionable merge risk identified.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains the problem, fix, behavior impact, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sect…
Title check ✅ Passed The title is concise, specific, and accurately summarizes the main change: applying the result of resolved_size in Blob::resolve_size.

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline comment, I also checked the equivalence claim arm by arm in resolved_size (no store, Bytes with and without a known store size, File with stat known / not seekable / stat failed, S3) against the deleted resolve_size body — each arm returns exactly the pair the old code stored, available is computed against the clamped offset as before, and the new unconditional offset write-back only ever stores the value already in the Cell. resolve_file_stat mutates only the store's File, not the blob's offset/size, so applying the pair after the stat is order-safe.

Extended reasoning...

The change touches only src/runtime/webcore/Blob.rs, collapsing the mutating resolve_size into a wrapper over the existing non-mutating resolved_size. It touches no security-sensitive surface. The refactor is behavior-preserving on every store arm, but one inline finding about the moved Stacked-Borrows comment was reported, so a defer note recording the equivalence check is the appropriate output rather than an approval.

Comment thread src/runtime/webcore/Blob.rs

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant