Skip to content

Blob store: make store::File's path private behind accessors - #43894

Closed
robobun wants to merge 2 commits into
mainfrom
robobun/033c5edf/blob-file-private-path
Closed

robobun wants to merge 2 commits into
mainfrom
robobun/033c5edf/blob-file-private-path

Conversation

@robobun

@robobun robobun commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Behaviour change: none

Problem

Fix

  • The path is a private tagged value: lazy (what Bun.file() makes) or pinned. This PR has no constructor for a pinned store.
  • Consumers use accessors: lazy_pathlike(), lazy_path(), fd(), pinned(), and display_path() for names and error text. get_path and store_path split into path_for_display() and path_for_open().
  • Each consumer keeps its code on the lazy arm. Its pinned arm refuses with EPERM on its existing error channel.
  • Verified: 22 existing test files pass unchanged. bun run rust:check-all passes on 12 targets.

Background

Downsides

Notes

Measurements (release builds of main f385d9a and of #43896 at 8e9fccd, which holds this PR, linux x64; gdb catch syscall because strace is not installed here):

  • size_of (release): store::File 128, store::Data 136, Store 176, ReadFile 448, CopyFile 480, NewSource<FileReader> 512, FileResponseStream 216, Blob 104 B. All 8 are the same as on main. A const assert holds the path enum at the size of PathOrFileDescriptor.
  • Bun.file() syscalls: 10 of 10 traced operations make the same file syscalls as main: text() at 1 B and 64 KiB, a 300000-byte stream drain, fetch bodies of 1 KiB and 64 KiB over http, a Bun.serve handler Response and a static route (GET and HEAD), Bun.write of 1 MiB, a FormData body, Bun.spawn stdin. sendfile and copy_file_range are still taken. The position of socket close and pread64 calls varies from run to run, on main too.
  • allocations (mimalloc totals): 100000 x Bun.file(p) 395.7 K on both builds. 10000 x FormData body 186.9 K on both (5 runs each. One run in 5 reads about 20 K lower, on either build).
  • door audit: 0 uses of the path of store::File outside the store module. 26 calls of the named opt-outs display_path() and path_for_display().
  • rebase surface: Blob, Bun.file: remove unsafe from Blob.rs, read_file.rs, copy_file.rs and Store.rs #40221 conflicts with main in 22 files. This PR adds 0 files to that list (git merge-tree).

Accessor choice per site:

  • display_path() (the named opt-out): inspect, name, SystemError.path text in ReadFile, ReadFileUV, WriteFileWindows, CopyFile, CopyFileWindows, Blob.stat(), the static-route existence check in server_body.rs, and S3 error text.
  • path_for_open(): FileRoute::on and the blob_store_path hook that import() of a blob: URL reads through (jsc_hooks.rs, bundler/options.rs).
  • fd(): every "is this a Bun.file(fd)" test (do_close, is_allowed_to_close, stream de-duplication, mkdirp checks).
  • lazy_pathlike() / lazy_path(): every open, clonefile, uv_fs_copyfile, truncate, chmod, unlink, the FormData and TLS sync reads, Bun.spawn stdio, and shell interpolation.

validate_writable_blob is the one check every write entry point runs (Bun.write destination, writer(), unlink()), so it refuses a pinned file first.

The pinned variant carries #[expect(dead_code)] here, because nothing constructs it yet. #43896 adds the constructor, the compare fields and the verified open, and removes the attribute.

PathOrFileDescriptor::is_path() and is_fd() are gone. Their last callers now ask the store (fd(), lazy_path()).

Not in this PR, because the dead_code lint rejects a field that nothing reads: Lazy::open_file_blob handing back the Stat it computes. #43896 adds it with its reader.

Two redundant assignments in CopyFile::run_async are gone (self.source_fd = ...pathlike.fd() after the same value was set at the top of the function), and an empty if with only a comment.

Existing tests run on the debug build, all pass: bun-file.test.ts, bun-file-read.test.ts, bun-file-fd-read.test.ts, bun-file-exists.test.js, bun-write.test.js, blob.test.ts, blob-write.test.ts, blob-file-name-ownership.test.ts, structured-clone-blob-file.test.ts, streams.test.js, FormData.test.ts, bun-serve-file.test.ts, bun-serve-static.test.ts, bun-serve-routes.test.ts, serve-file-slice-read-error.test.ts, fetch-file-upload.test.ts, tls-bunfile-leak.test.ts, bunshell-file.test.ts, image.test.ts, fs.test.ts, and the Blob and Bun.file tests of spawn.test.ts. In fs.test.ts the test readdirSync(path, {recursive: true}) should work x 100 runs close to its own 10 s limit on a debug build and can exceed it, on any branch.

rust:check-all targets: linux gnu and musl (x64, arm64), android (x64, arm64), darwin (x64, arm64), windows msvc (x64, arm64), freebsd (x64, arm64).

A file-backed Blob store kept its path or descriptor in the public field
`store::File.pathlike`. Every consumer read it and opened the file itself.

The path is now a private tagged value: lazy (what `Bun.file()` makes) or
pinned (a path plus the size and mtime the file must still have). Consumers
use accessors:

- `lazy_pathlike()` / `lazy_path()`: what a reader of a `Bun.file()` opens.
  `None` for a pinned file.
- `fd()`: the descriptor of a `Bun.file(fd)`.
- `display_path()`: for a name, an inspect string, an error message, or a
  stat of the path. Never for a read.
- `pinned()`: the size and the `matches(&Stat)` check of a pinned file.

`Store::get_path` and `Blob::store_path` are split into `path_for_display`
and `path_for_open`, so each caller states what it does with the path.

Nothing constructs a pinned store yet, so no behaviour changes. Each
consumer keeps its code on the lazy arm and gains an arm that refuses a
pinned file with EPERM on its existing error channel.

`resolve_file_stat` is split into `stat_file` and `apply_file_stat`: its
path and descriptor arms were identical.
@robobun

robobun commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Groundwork for #43896, which fixes #43109. This PR changes no behaviour: the existing Blob, Bun.file, Bun.write, stream, FormData, Bun.serve file, fetch upload, spawn and image tests pass unchanged on a debug build of this branch.

Nothing pins a file yet, so the variant carries an expect(dead_code) and
PinnedFile holds the path alone. The compare fields and the constructor
come with their first user. PathOrFileDescriptor::is_path and is_fd lost
their last callers to the accessors.
@robobun

robobun commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: this is now part of #43896.

A review of the stack found that this PR alone adds lines that nothing can run: the pinned variant had no constructor, and every pinned arm was unreachable until #43896. REVIEW.md asks that every added line is live. The private path and its accessors now land together with their first user in #43896, rebased on main.

The accessor set is smaller there. A write or a metadata call reads the path through one accessor and has no pinned arm, so copy_file.rs and write_file.rs differ from main mostly by that rename.

@robobun robobun closed this Sep 25, 2026
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.

2 participants