Conversation
WalkthroughChangesBlob file metadata now distinguishes explicit slice sizes from live filesystem sizes. Size, existence, timestamps, slicing, stat failure handling, structured-clone state, and regression coverage were updated. Blob live metadata and slicing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:35 AM PT - Jul 29th, 2026
⏳ @robobun, your commit cdead2d is still building in
|
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
Verified each of the bot's suggested issues against this build:
The first four share the root cause fixed here: #28459 is a separate bug: the archive is empty even on a fresh |
|
CI on c932541 (build 69892): the diff is green. All test failures are in the Ready for a maintainer to merge once the darwin lanes clear or are retried. |
…tem state The first stat was memoized on the blob's store forever. exists() guarded on self.size == MAX_SIZE, lastModified on INIT_TIMESTAMP, and resolve_size on seekable.is_none(), so after one call every getter answered from a snapshot. resolve_file_stat also left the cached fields untouched on stat failure, so a file deleted after the first exists() stayed 'true', and a file created after one 'false' answer read as an empty string forever. - resolve_file_stat: reset mode/seekable/max_size/last_modified on stat error, and dedupe the path/fd arms. - get_exists_sync: always re-stat the file store and answer from the fresh mode; stop going through resolve_size (which cached self.size = 0 and made the read path return an empty string). - get_last_modified: always re-stat the file store. - get_size: always re-stat the file store and recompute from the fresh max_size. Add Blob.size_is_explicit so a caller-supplied slice() bound is preserved while a stat-derived cache is refreshed.
The non-seekable arm returned Infinity and the stat-failed arm returned 0
without consulting size_is_explicit, so Bun.file('/dev/null').slice(0, 5).size
regressed from 5 to Infinity. A caller-supplied bound is authoritative on
every arm now.
A sliced Bun.file() must keep its caller-supplied window across structuredClone. The serialize side now emits MAX_SIZE for a stat-derived size (only a slice bound goes on the wire), and the deserialize side sets size_is_explicit when a concrete size arrives, so get_size honours the bound instead of re-statting to the whole file.
c932541 to
ca36103
Compare
|
Rebased onto main (b650f55) and resolved the conflict in One new failure surfaced from a test added on main after this PR was opened ( Verified on this build:
|
Caching self.size = 0 in the stat-failed arm poisoned a later read: after file exists -> .size -> delete -> .size -> recreate -> .text(), do_read_file received max_length = 0 and returned an empty buffer. The write served no purpose (the return value is 0 regardless).
The seekable arm's self.size.set(available) was the sibling of the stat-failed set(0) dropped in the previous commit: after exists -> .size -> truncate -> .size -> grow -> .text(), do_read_file got max_length = <shrunken size> and truncated the read. get_size now never writes self.size for a file store. get_slice resolves the live stat size itself via view_size() instead of reading a cached self.size, so negative-index slice math works on a fresh Bun.file without depending on a prior .size call.
…e size Bun.file(p).slice(-N, Infinity) previously produced a past-EOF offset because get_slice saw the unresolved MAX_SIZE sentinel; the Content-Range test classified that as a bad (empty) range. With get_slice resolving the stat size itself, this is now the spec-correct last-N-bytes view, matching in-memory Blob.slice. Move the case out of badRanges and assert the new behaviour.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/webcore/Blob.rs`:
- Around line 2252-2300: Keep the live re-stat behavior in get_size,
get_last_modified, and get_exists_sync for File-backed blobs so each access
reflects current filesystem state; do not restore cached metadata resolution.
🪄 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: a2939669-6147-4d68-96d2-3af07abe1df7
📒 Files selected for processing (4)
src/jsc/webcore_types.rssrc/runtime/webcore/Blob.rstest/js/bun/http/serve.test.tstest/js/bun/util/bun-file.test.ts
For a slice(0, 100) on a 3-byte file, .size reports 3 (clamped to available) but get_size_for_bindings returned the raw bound. Inline the same available.min(bound) clamp so expect(s).toHaveLength(s.size) holds. Also drop the bare .toThrow() in the matcher test: the throw is incidental setup, not the assertion.
A nested negative-index slice on a slice whose bound exceeds the file (slice(0, 100) on a 3-byte file) resolved against the raw bound instead of the clamped .size. Drop the size_is_explicit guard so view_size goes through the same clamp get_size uses, falling back to self.size.get() when stat has no answer.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/bun-file.test.ts`:
- Around line 305-323: Strengthen the regression test by removing the try/catch
around expect(f).toBeEmpty() so assertion failures propagate, and update the
final expect(s).toHaveLength assertion to use the known expected length 3
instead of s.size. Keep the existing file-read and length checks 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: 416278c7-3a37-44ab-8829-c2951b091209
📒 Files selected for processing (2)
src/runtime/webcore/Blob.rstest/js/bun/util/bun-file.test.ts
There was a problem hiding this comment.
No bugs found this round, but I'd like a human to sign off: this reworks Bun.file's stat-caching semantics (every .size/.lastModified/exists() now re-stats), adds the size_is_explicit field that intentionally conflicts with #33360, and flips a serve.test.ts Content-Range case from 416 to 206. Those are user-visible behavior/design decisions worth a maintainer look.
What was reviewed:
get_size/get_size_for_bindings/view_sizenow sharestat_file_size; checked they agree on explicit slices, past-EOF bounds, missing/non-seekable files.resolve_file_statfailure path resets all four cached fields; path/fd arms deduped.- Structured-clone serialize now writes
MAX_SIZEfor non-explicit file blobs and no longer callsresolve_size(); deserialize setssize_is_explicitsymmetrically. - FormData's remaining
resolve_size()sibling was checked and is fine (writesself.sizeon a temp dupe).
Extended reasoning...
Overview
The PR changes src/runtime/webcore/Blob.rs (get_exists_sync, get_last_modified, get_size, get_size_for_bindings, get_slice, resolve_file_stat, structured-clone serialize/deserialize), adds a size_is_explicit: Cell<bool> field to Blob in src/jsc/webcore_types.rs (plus Default/dupe and one literal-constructor site), introduces two new helpers view_size/stat_file_size, moves one serve.test.ts Content-Range case from bad→good, and adds a 12-test describe to bun-file.test.ts.
Security risks
None identified. The changed code is local stat/size accounting on already-open paths/fds; no new untrusted-input parsing, no auth/crypto surface. stat_file_size clamps offset to file.max_size before subtracting, so no underflow.
Level of scrutiny
High. This is a deliberate semantic change to a core Bun-native API (Bun.file): getters that used to answer from a memoized stat now issue a syscall on every access. The author acknowledged the per-access stat cost as intentional in an earlier thread, but that's a design tradeoff (correctness vs. hot-loop .size cost) a maintainer should ratify. It also changes an observable Bun.serve Content-Range outcome (a previously-416 range now returns 206 with body), and the description explicitly calls out a merge-order conflict with #33360 on the same size_is_explicit field.
Other factors
The PR has been through ~8 rounds of bot review and every raised issue is resolved; the final shape (shared stat_file_size helper, clamped explicit-slice size across all three accessors, view_size used for negative-index resolution) is internally consistent and well-tested. No human reviewer has looked at it yet. Given the user-visible semantics change, the sibling-PR conflict, and the flipped serve.test.ts expectation, deferring rather than auto-approving.
|
CI on b69b413 (build 83827): 192 passed, 1 failed. The one failure is Happy to rebase onto current main if preferred; otherwise ready for review. |
…of 2^52-1 sentinel get_last_modified now checks for the INIT_TIMESTAMP sentinel after resolve_file_stat (set on stat failure) and returns 0 to JS, and the no-store fallthrough returns 0 instead of the sentinel. A missing file is now not 'newer' than every real file. Also adds a test that a file replaced by rename reads the new inode's full contents after a prior size touch (the cached-size clamp this PR already removed).
|
Merged main (59242d6) and folded in the Re-verified on cdead2d:
|
There was a problem hiding this comment.
No further findings from this pass. I'm deferring rather than approving because the PR itself flags two observable behavior changes for maintainer sign-off — per-access stat on .size/.lastModified/exists(), and slice(-N, Infinity) now resolving against the live file size — plus the folded-in lastModified → 0 sentinel change for missing files and in-memory Blobs.
What was reviewed: the stat_file_size/view_size/get_size/get_size_for_bindings accessors now share one clamp and agree on explicit-slice-past-EOF; resolve_file_stat clears cached state on failure so a delete→recreate can't poison later reads; structured-clone serialize no longer stats the source and only wires an explicit slice bound; get_slice's size == 0 early-exit is skipped for file stores and negative indices resolve via view_size. The serve.test.ts badRanges move is the intended consequence of the negative-index fix.
Extended reasoning...
Overview
Rewrites the stat-caching semantics for file-backed Blobs in src/runtime/webcore/Blob.rs and adds a size_is_explicit field on Blob in src/jsc/webcore_types.rs. get_exists_sync, get_size, get_last_modified, get_size_for_bindings, view_size, get_slice, structured-clone serialize/deserialize, and resolve_file_stat are all touched. 17 new tests in bun-file.test.ts; one serve.test.ts Content-Range case moved from badRanges to a positive assertion.
Security risks
None identified. No untrusted-input parsing; the changes route through bun_sys::stat/fstat and adjust how cached results are interpreted.
Level of scrutiny
High. Bun.file() is a core public API and this changes user-observable semantics in three ways the author explicitly calls out for maintainer sign-off: (1) every .size/.lastModified/exists() access now issues a syscall, (2) slice(-N, Infinity) on a file blob changes from empty to last-N-bytes, (3) lastModified on a missing path or in-memory Blob becomes 0 instead of 2**52 - 1. These are the right answers per the W3C slice algorithm and the linked issues, but they are API-behavior decisions, not mechanical fixes.
Other factors
This PR has been through ~9 review rounds; every prior finding (slice-bound clamping in get_size_for_bindings, nested negative-index slice on an explicit bound, the triplicated stat/available block, structured-clone poisoning, toBeEmpty/toHaveLength poisoning) is resolved and covered by a test. Test coverage is thorough across create/delete/truncate/rename/grow, explicit vs unbounded slices, non-seekable and missing files, and both size accessors. It overlaps with #33360 on the size_is_explicit field. Given the explicit "maintainer should sign off" note in the description and the breadth of the semantic change, this warrants a human approval rather than a bot one.
|
CI on cdead2d (build 84825, after merging main and a retry): 194 passed, 2 lanes red.
All Blob-touching suites ( |
### Problem - mordant's `reimplemented_helper` finding baselined for `src/jsc/webcore_types.rs`: `Blob::is_bun_file` and `Blob::needs_to_read_file` had the same signature and the same body (the store is `Some` and its data is `Store::File`), so a change to one would have silently missed the other. - The duplication is inherited, not a porting mistake: `Blob.zig` already had `isBunFile` and `needsToReadFile` with the same body, and neither side has changed since the port (#30412). Nothing relies on them differing. ### Fix - Delete `is_bun_file` and keep `needs_to_read_file`, which is what the ~40 other call sites (and the `blob_needs_to_read_file` hook that `bun_sql_jsc` goes through) already use. - Point the two `is_bun_file` callers at it: `Response.rs` (`getCompleteWebRequestOrResponseBodyValueAsArrayBuffer` returns `undefined` for a file-backed body) and `CryptoHasher.rs` (the synchronous hashers reject `Bun.file()` input). Same predicate, so no behavior change. - Remove the `reimplemented_helper:src/jsc/webcore_types.rs` entry from `mordant-baseline.toml`, and the two stale comments in `webcore/Blob.rs` that named the deleted method. The entry is removed by hand rather than by regenerating the file: a full `bun run rust:mordant:baseline` on Linux also drops two unrelated entries (`always_unwrapped_option` in `PackageInstall.rs`, `narrowed_two_ways` in `node_crypto_binding.rs`) that this PR has no business touching. - No new test: there is no observable behavior to pin (the two predicates were byte-for-byte the same), so no test can distinguish before from after. The `Bun.file()` rejection at the `CryptoHasher` call site is already covered by `bun-cryptohasher.test.ts` ("Bun.file in CryptoHasher is not supported yet"), and the mordant run below is the check for the finding itself. Same shape as #39124, #39116 and #39135 from this batch. - Verified: - `bun run rust:mordant` (full workspace) on this branch: clean, no `target/mordant/over-baseline.txt`. With main's `webcore_types.rs` restored on top of the trimmed baseline it reports `reimplemented_helper ... over the mordant baseline (0 recorded for src/jsc/webcore_types.rs)` and `1 finding(s) over the baseline in bun_jsc`, so the entry was this site and the lint no longer fires. - The `mordant` workflow also runs on this PR since the baseline changed; with the entry removed it fails if the finding is still reported. - `bun bd test test/js/bun/util/bun-cryptohasher.test.ts` (covers the `CryptoHasher` call site: `Bun.file()` input still throws): 402 pass. - `bun bd test` on `test/js/web/fetch/{blob,blob-write,body,blob-file-name-ownership,response}.test.ts`, `test/js/web/structured-clone-blob-file.test.ts` and `test/js/bun/io/bun-write.test.js` (`--timeout 60000`, since several of these spawn a debug+ASAN child and exceed the 5s default): everything passes except `blob.test.ts` "Bun.file(path).slice(start, end) streams only the slice", which fails identically on a clean checkout of main (88a6398) and is being handled separately; it is a `FileReader` streaming bug unrelated to this predicate. - Two open PRs add `is_bun_file()` calls in `Blob.rs` (#32434, #33659); whichever lands after this one needs those calls renamed to `needs_to_read_file()`. That is a compile error, not a silent change. ### Background - `Blob` (`src/jsc/webcore_types.rs`) is a view (offset + size) onto a refcounted `Store`, whose `data` is one of `Bytes` (in memory), `File` (a path or fd; what `Bun.file()` creates) or `S3`. `needs_to_read_file()` asks whether the blob is `File`-backed, i.e. whether its bytes have to be read off disk before anything in memory can look at them; synchronous consumers use it to bail out. - mordant is the advisory Rust lint pack the `rust-lints` workflow runs (`bun run rust:mordant`). `mordant-baseline.toml` holds the per-(lint, file) counts of findings that predate the job, so CI only fails when a count goes up; once a baselined finding is fixed its entry is deleted. Co-authored-by: Alistair Smith <hi@alistair.sh>
|
#39763 is a smaller fix for the failed-stat part of this (the |
|
Closing in favour of #39763. The I ran the tests of this PR against a build of #39763. The
Of the first two, #33360 covers the read side: a read is no longer capped by a size from an earlier stat. A live This branch is also 770 commits behind main and uses |
The first
stata BunFile performs is memoized on the blob's store forever.exists(),.sizeand.lastModifiednever re-stat, so one BunFile answers from a snapshot of a past filesystem state for the life of the object. A file created after one "missing" answer reads as""forever with no error.Repro
Cause
One shared root: the lazy
statresult is memoized on the store the first time any getter needs it and never invalidated.get_exists_sync/get_size/get_last_modifiedguarded on the cached value, andresolve_file_statwas a no-op on stat failure. Case B is the worst:exists()cachedself.size = 0viaresolve_size, and every later.text()passedmax_length = 0to the read path.Fix
resolve_file_statresets the cached stat on failure (mode = 0,seekable = None,max_size = MAX_SIZE,last_modified = INIT_TIMESTAMP).get_exists_sync,get_last_modified,get_size,get_size_for_bindingsalways re-stat file stores via a sharedstat_file_sizehelper and never writeself.size.Blob.size_is_explicitflag (set byslice()) records whenself.sizeis a caller-supplied bound, soget_sizecan clamp a bound past EOF while leaving a whole-file view live.get_sliceskips itssize == 0early exit for file stores, and resolves negative start/end against the live stat size viaview_size()(positive indices and the default end still useself.size.get(), soslice(2)on a growing file stays unbounded).resolve_size()(which cached a stat on the source), and only puts a concrete size on the wire for an explicit slice.Behaviour changes
Two observable changes a maintainer should sign off on:
.size/.lastModified/exists()issue astaton every access for file-backed blobs. This is the fix; hot loops that repeat.sizeshould read it once into a local.Bun.file(p).slice(-N, Infinity)now returns the last N bytes (the W3C slice semantics, matching in-memoryBlob.slice). Previously it saw the unresolvedMAX_SIZEsentinel and produced a past-EOF offset that read empty.serve.test.tsmoves that case frombadRangesto a dedicated "negative start resolves against the live file size" test.Relationship to #33360
Both introduce the same
size_is_explicitfield at the same offset. This PR fixes the getters; #33360 fixes the read path being clamped by a stale stat size. Whichever lands second only conflicts on the field declaration hunk.Verification
test/js/bun/util/bun-file.test.tsgains aBunFile exists()/size/lastModified reflect the current filesystem statedescribe with 13 tests: the three repro cases above, create/delete polling, slice-bound preservation (regular / non-seekable / missing / empty regular file), delete-then-recreate and truncate-then-grow read-poisoning guards,.slice()with no end staying unbounded, negative-index slice against the live size,expect().toHaveLength/toBeEmptynot poisoning, andstructuredClonenot poisoning (source and clone).before / after
Also green:
blob.test.ts,bun-file-exists.test.js,bun-file-read.test.ts,bun-file-fd-read.test.ts,serve.test.tsContent-Range suite,bun-serve-file.test.ts,structured-clone-blob-file.test.ts,FormData.test.ts.bun run rust:check-allpasses on all 10 targets.Fixes #22484
Fixes #4930
Fixes #23902
Fixes #22456
no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts