Delete PathString#31579
Conversation
Replace the packed ptr+len `bun_core::PathString` with the existing byte-slice idioms and remove the type. - Arena / process-lifetime paths (resolver `Entry.abs_path` & `EntryCache.symlink`, router `Route` path columns, test-runner file lists) -> `bun_ptr::Interned`. This deletes the `path_string_static` and `arena_slice` lifetime-widening shims, which only existed because `PathString::slice()` tied the borrow to `&self`. - Bundler `EntryPoint.output_path`, dir_iterator entry name, and the `node::PathOrBuffer` path variant -> `bun_core::RawSlice<u8>` (borrowed, outlives-holder). - `bun_sys`/`bun_io` `PathOrFileDescriptor::Path` -> `&[u8]` (ephemeral borrow, compiler-checked). - Owned sites -> owning types, removing the manual `init_owned`/ `deinit_owned` contract: `Store.Bytes.stored_name` -> `Box<[u8]>` (freed by `Bytes`'s Drop), `PathLike::String` -> owned-or-borrowed `bun_ptr::cow_slice::CowSlice<u8>` (freed by `PathLike`'s Drop, so the blob `Store` no longer frees it explicitly), and the async readdir-recursive task's `root_path`/`basename` -> owned `Box<[u8]>` (drops the leak/reconstruct dance). Net change is behavior-preserving.
|
Status: opened. Deletes
Verified: full Post-merge-with-main verification (781779f): re-verified the branch after the CI: the diff is green; each red has been a different known-flaky/infra lane. clippy + miri + Format + Lint all pass on every run. The rotating Buildkite reds across runs: macOS PTY timeout ( |
The structuredClone ownership test shares blob.test.ts with a pre-existing RSS-threshold leak test that flakes on the debug build (its `isASAN` threshold check keys on the binary being named `bun-asan`, but `bun bd` produces `bun-debug`, so the strict non-ASAN threshold is applied under ASAN's inflated RSS). Isolating the ownership test keeps it runnable without that unrelated flake.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughRemoves the PathString packed type and replaces usages across the repo with RawSlice, Box<[u8]>, CowSlice, Interned, or ZStr depending on ownership/lifetime; updates IO, resolver, router, node fs, blob/store, JSC, bundler, and test infra. ChangesPathString removal and type replacement refactoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/sys/lib.rs`:
- Around line 9225-9231: The function write_file_with_path_buffer is moving
fields out of a shared &WriteFileArgs by destructuring args.data and matching
args.file; make WriteFileData<'a> and PathOrFileDescriptor<'a> implement Copy
(e.g. add #[derive(Clone, Copy)] to their definitions) so they can be moved from
a & reference, or alternatively change the code in write_file_with_path_buffer
to pattern-match by reference (match &args.file and destructure args.data by
reference) to avoid moving out of a borrowed value; update the types or the
match/destructure in write_file_with_path_buffer accordingly, referencing
PathOrFileDescriptor, WriteFileData, WriteFileArgs, args.data and args.file.
🪄 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: 1a0972b2-6d88-4302-aeb8-fa34691afa86
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
src/bun_core/lib.rssrc/bun_core/string/PathString.rssrc/bun_core/string/mod.rssrc/bundler/LinkerGraph.rssrc/bundler/OutputFile.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/io/lib.rssrc/io/openForWriting.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/hot_reloader.rssrc/jsc/node_path.rssrc/jsc/webcore_types.rssrc/patch/lib.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/router/Cargo.tomlsrc/router/lib.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/api/output_file_jsc.rssrc/runtime/api/standalone_graph_jsc.rssrc/runtime/cli/build_command.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/node/dir_iterator.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/builtin/mkdir.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/blob/Store.rssrc/sys/lib.rstest/js/web/fetch/blob.test.ts
💤 Files with no reviewable changes (2)
- src/bun_core/string/PathString.rs
- src/bun_core/string/mod.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The SavedFile::to_js comment still described the old PathString ownership (Store::drop freeing via deinit_owned); update it to match the Saved arm: the owned PathLike::String (a CowSlice) frees itself in PathLike::drop.
|
@robobun fix conflicts |
…pathstring # Conflicts: # src/jsc/node_path.rs
|
Merged latest |
|
Heads up for whoever merges: this is a behavior-preserving refactor — deleting Because there's no behavioral change, there's no regression it "fixes" — the added test ( State: branch is up to date with |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/node_fs.rs (1)
3972-3979:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
bufferbefore coercinglength.This reorders
fs.read()argument validation. A non-TypedArraybuffercan now triggerlengthcoercion first, which changes the thrown error and allows user-definedvalueOf/toStringside effects on the later arg before rejecting the invalid buffer.Proposed fix
let length_float: f64 = if let Some(arg) = arguments.next_eat() { arg.to_number(ctx)? } else { 0.0 }; - let buffer = Buffer::from_js(ctx, buffer_value).ok_or_else(|| { + let buffer = Buffer::from_js(ctx, buffer_value).ok_or_else(|| { ctx.throw_invalid_argument_type_value(b"buffer", b"TypedArray", buffer_value) })?;- let length_float: f64 = if let Some(arg) = arguments.next_eat() { - arg.to_number(ctx)? - } else { - 0.0 - }; let buffer = Buffer::from_js(ctx, buffer_value).ok_or_else(|| { ctx.throw_invalid_argument_type_value(b"buffer", b"TypedArray", buffer_value) })?; + let length_float: f64 = if let Some(arg) = arguments.next_eat() { + arg.to_number(ctx)? + } else { + 0.0 + };🤖 Prompt for 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. In `@src/runtime/node/node_fs.rs` around lines 3972 - 3979, In the fs.read argument handling, validate and convert the buffer before coercing the optional length: call Buffer::from_js(ctx, buffer_value) and handle ctx.throw_invalid_argument_type_value(b"buffer", b"TypedArray", buffer_value) first (the Buffer::from_js call currently assigned to `buffer`), and only then read/coerce the length via arguments.next_eat() and arg.to_number(ctx) into `length_float`; this ensures `buffer_value` is checked prior to any potential side-effecting coercions (avoid calling to_number on a later arg before `Buffer::from_js` succeeds).
🤖 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/web/fetch/blob-file-name-ownership.test.ts`:
- Line 35: The test currently asserts expect(stderr).toBe("") which can fail on
ASAN builds due to a known startup warning; before asserting, filter out any
stderr lines that start with "WARNING: ASAN interferes" (e.g. using the existing
stderr variable: split into lines, filter with line =>
!line.startsWith("WARNING: ASAN interferes"), then rejoin or check the filtered
array) and then assert the filtered stderr is empty using the existing
expect(...).toBe(""); update the assertion that references stderr to use this
filtered result.
---
Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 3972-3979: In the fs.read argument handling, validate and convert
the buffer before coercing the optional length: call Buffer::from_js(ctx,
buffer_value) and handle ctx.throw_invalid_argument_type_value(b"buffer",
b"TypedArray", buffer_value) first (the Buffer::from_js call currently assigned
to `buffer`), and only then read/coerce the length via arguments.next_eat() and
arg.to_number(ctx) into `length_float`; this ensures `buffer_value` is checked
prior to any potential side-effecting coercions (avoid calling to_number on a
later arg before `Buffer::from_js` succeeds).
🪄 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: c0def94e-3232-40b5-aba5-4af7fb152614
📒 Files selected for processing (5)
src/jsc/node_path.rssrc/runtime/api/output_file_jsc.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rstest/js/web/fetch/blob-file-name-ownership.test.ts
The owned-buffer ownership test runs a subprocess and asserts empty stderr. On ASAN debug builds the subprocess emits a startup 'WARNING: ASAN interferes ...' line; filter it out so the assertion still catches a real ASAN double-free report without flaking.
clippy::undocumented-unsafe-blocks requires the // SAFETY: comment on the line immediately preceding the unsafe block. Two detach_lifetime blocks had their comment separated by an intervening let binding.
|
Follow-up for review — the precise ownership invariant, since this is the crux of whether the refactor is safe: On
The refactor moves that same single-free to the type: Net: no behavioral change, hence no regression to reproduce. |
- dir_iterator.rs: IteratorResultWName's doc still called it a 'Fake PathString'; it now mirrors IteratorResult.name (RawSlice<u16> + slice_assume_z), matching the file header. - blob-file-name-ownership test: the File([bytes], name) round-trip only exercises the Bytes.stored_name (Box<[u8]>) path. Added a structuredClone(Bun.file(path)) case so the owned PathLike::String (CowSlice) deserialize arm is covered too, and corrected the comment.
Main deleted PathString (#31579); PathLike::String now carries a CowSlice<u8>, matching the surrounding mkdir_if_not_exists call.
Main deleted PathString (#31579); PathLike::String now carries a CowSlice<u8>, matching the surrounding mkdir_if_not_exists call.
2 participants
What
Deletes the packed-pointer type
bun_core::PathString(a&[u8]crammed into au64/u128) and replaces every use with the byte-slice idioms the codebase already maintains for these roles.Why it's clean to delete
PathStringconflated two distinct roles behind oneCopytype whose ownership was a container-level contract rather than type-enforced:&'static [u8], andinit_owned/deinit_owned), freed by a containing struct'sDrop.The codebase already has purpose-built types for both, and
src/resolver/lib.rseven carried aTODO(port)asking for exactly this onceslice()could return&'static [u8]directly.How each site was replaced
bun_ptr::Interned(aCopy,#[repr(transparent)]&'static [u8]proof type): resolverEntry.abs_path&EntryCache.symlink, routerRoute.{match_name,public_path,abs_path}(stored in SoA columns), and the test-runner file lists (Scanner,Coordinator,runner,ChangedFilesFilter,test_command). This deletes thepath_string_staticandarena_slicelifetime-widening shims — they only existed becausePathString::slice()tied the borrow to&self.'static→bun_core::RawSlice<u8>: bundlerEntryPoint.output_path(AST-lifetime, SoA column),dir_iterator's entryname, andnode::PathOrBuffer::Path.&[u8]/&ZStr:bun_sys/bun_ioPathOrFileDescriptor::Path(compiler-checked lifetimes),RuntimeTranspilerCache::save/from_file_with_cache_file_path(&ZStr, keeping the NUL-termination invariant explicit),patch.init_owned/deinit_ownedfootgun:Store.Bytes.stored_name→Box<[u8]>(freed byBytes'sDrop).PathLike::String→bun_ptr::cow_slice::CowSlice<u8>(owned-or-borrowed; frees its buffer inPathLike'sDrop, so the blobStoreno longer frees it explicitly).PathLike::clonedupes an owned path and shares a borrowed one.root_path/basename→ ownedBox<[u8]>, dropping the previousBox::leak+ manualBox::from_rawreconstruct.Net behavior-preserving; −208 lines overall (deletes
PathString.rs, two shims, and the leak/reconstruct dance).Verification
bun bdandbuild:releaseboth compile and link.test/js/node/fs/fs.test.ts(readdir recursive incl. the owned task buffers),blob.test.ts+workers(namedFile/Blobserialize round-trip — thestored_name+ ownedPathLike::Stringpaths), bundler,filesystem_router, resolver symlink/realpath, transpiler-cache.test/js/web/fetch/blob.test.ts) that round-trips a namedFilethroughstructuredClonein a loop under GC pressure, exercising thestored_name(Box<[u8]>) and ownedPathLike::String(CowSlice) alloc/free paths — a double-free or leak there is a hard crash underbun bd(ASAN). Passes on the ASAN build.Note: this is a behavior-preserving refactor, so the test is coverage for the new ownership model (it passes both before and after), not a fail-before-fix regression.
Footprint tradeoff
PathString's packing only saved space on macOS/BSD (where it was au64); on Linux/Windows it was already au128== fat-pointer width.Interned/RawSlice<u8>are fat pointers (16 bytes), so on macOS/BSD the resolverEntry/EntryCacheand router path columns grow 8 bytes per field. On Linux/Windows there's no footprint change. This is the known price of removing the packing; flagging it in case the per-Entryfootprint on macOS matters.Related PRs (overlap)
abs_pathpath (nowInterned) — may need sequencing.