Conversation
|
Status: rebased onto current main in 74492b5 (squashed). The test file now covers both directions of the race: main-thread Reproduce the failure on a release build: Stock bun 1.4.0 fails both tests. The second one reports 125+ torn paths in 2000 iterations, for example Verify the fix: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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:
WalkthroughThe change removes the resolver’s cached top-level directory. Process CWD updates now publish normalized, interned paths through ChangesProcess CWD propagation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, background, verification steps, test results, and known test limitations. It does not use the template headings exactly, but it provides the required information in equivalent sections. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/VirtualMachine.rs (1)
4639-4662: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
FileSystem::set_top_level_dir's doc comment no longer matches this call site.
FileSystem::set_top_level_dir(src/resolver/lib.rs) documents: "Takes&mut self— callers hold&'static mut FileSystemfrominstance(); only called during single-threaded CLI init."set_process_cwdnow calls it from liveprocess.chdir(), which can run at any point during execution while resolver worker threads concurrently read the sameFileSystemsingleton.Update that doc comment to reflect the new invariant (protected only for the
top_level_dirfield via thebun_coreRwLock, not for the whole struct), so a future reader does not assume the old single-threaded-only contract still holds and reintroduce an unsynchronized mutation elsewhere inFileSystem.As per coding guidelines: "Comments should contain only durable non-obvious information such as invariants, ownership, lifetime, safety rationale."
🤖 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/jsc/VirtualMachine.rs` around lines 4639 - 4662, Update the doc comment for FileSystem::set_top_level_dir to remove the obsolete single-threaded initialization and &'static mut FileSystem assumptions. Document that runtime callers such as set_process_cwd may invoke it while resolver workers read the singleton, and that synchronization protects only the top_level_dir field through the bun_core RwLock rather than the entire FileSystem.Source: Coding guidelines
src/resolver/lib.rs (1)
169-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the public
FileSystem::top_level_dirfield and migrate direct callers totop_level_dir().
bun_bundler::bun_fsre-exportsbun_resolver::fs, and many callers still read the field directly.set_top_level_dirupdates both the field andbun_core, whiletop_level_dir()reads onlybun_core. Direct reads can bypass synchronization and observe inconsistent slice metadata.🤖 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/resolver/lib.rs` around lines 169 - 175, Remove the public FileSystem::top_level_dir field and update every direct caller, including bun_bundler::bun_fs consumers, to use the top_level_dir() accessor. Preserve set_top_level_dir’s synchronization with bun_core and ensure callers no longer access the stored slice directly.
🤖 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/jsc/VirtualMachine.rs`:
- Around line 4654-4657: Update the trailing-separator logic in the
current-directory path handling to check len < buf.len() before writing
buf[len]. When no capacity remains, return a catchable path-length error or
apply the resolver’s existing truncation/rejection behavior, preserving normal
separator insertion when space is available and avoiding a panic for
user-reachable process.chdir() calls.
In `@src/resolver/resolver.rs`:
- Line 1347: Replace the `Fs::FileSystem::instance().top_level_dir()` call at
this location and the other two corresponding call sites with
`self.fs_ref().top_level_dir()`, preserving the existing control flow and
read-only behavior.
---
Outside diff comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 4639-4662: Update the doc comment for
FileSystem::set_top_level_dir to remove the obsolete single-threaded
initialization and &'static mut FileSystem assumptions. Document that runtime
callers such as set_process_cwd may invoke it while resolver workers read the
singleton, and that synchronization protects only the top_level_dir field
through the bun_core RwLock rather than the entire FileSystem.
In `@src/resolver/lib.rs`:
- Around line 169-175: Remove the public FileSystem::top_level_dir field and
update every direct caller, including bun_bundler::bun_fs consumers, to use the
top_level_dir() accessor. Preserve set_top_level_dir’s synchronization with
bun_core and ensure callers no longer access the stored slice directly.
🪄 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: 10f1d0fc-7867-4dd8-9e15-ec0bef90b59e
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/resolver/lib.rssrc/resolver/resolver.rstest/js/node/worker_threads/worker-chdir-resolve-race.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/node/worker_threads/worker-chdir-resolve-race.test.ts (2)
56-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve subprocess failure details before parsing stdout.
If the child aborts before
console.log,JSON.parse(stdout.trim())throws before Line 64 checksexitCode. This can hide the assert or ASAN failure that the test is intended to expose.Assert
{ stdout, stderr, exitCode }together before parsing. Then keep the structurednerrassertion.Proposed assertion order
- expect(stderr).toBe(""); - const out = JSON.parse(stdout.trim()); + const normalized = stdout.trim(); + expect({ stdout: normalized, stderr, exitCode }).toEqual({ + stdout: expect.stringContaining('"nerr":0'), + stderr: "", + exitCode: 0, + }); + const out = JSON.parse(normalized); // Both d1 and d2 contain mod.mjs, so every import must succeed: any failure // here observed a torn cwd. The pre-fix failure mode shows an interior NUL // ("\u0000") in the rejected module path. expect(out).toEqual({ nerr: 0, errs: [] }); - expect(exitCode).toBe(0);Based on learnings, crash-oriented Worker subprocess tests should report stdout, stderr, and process status together. The PR objective also states that the unfixed build can abort on assert builds.
🤖 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 `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts` around lines 56 - 64, Update the subprocess result handling around Promise.all so stdout, stderr, and exitCode are asserted together before calling JSON.parse, preserving failure details when the child aborts early. Keep the existing structured out.nerr/errs assertion after parsing, and retain the successful exit-code expectation.Source: Learnings
28-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake Worker failure handling reject and clean up the churn loop.
"error"and unexpected"exit"events resolve a failure object instead of rejecting. Ifnew Worker(...)throws,Promise.allrejects before Line 41, sochurncontinues to callprocess.chdir()until the test timeout.Wrap Worker creation and result collection in
try/finally. Setstop = trueand awaitchurnin thefinallyblock. Reject unexpected Worker events, and terminate every created Worker during cleanup. Keep a settled guard so the expected exit after"message"is not reported twice.As per coding guidelines, test failure events must reject and test-owned asynchronous resources must be cleaned up on every exit path.
🤖 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 `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts` around lines 28 - 41, Update the Worker creation and result collection around the churn loop to use try/finally, ensuring stop is set and churn is awaited on every exit path. Make Worker error and unexpected exit events reject rather than resolve failure objects, while preserving a settled guard so the expected exit after a message is ignored; track every created Worker and terminate them during cleanup, including when new Worker throws.Source: Coding guidelines
🤖 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/bundler/bundle_v2.rs`:
- Around line 6196-6201: Update the import-path handling around specifier_to_use
to call Fs::FileSystem::instance().top_level_dir() once, store that result in a
local variable before the prefix check, and reuse the same variable for both
starts_with and slicing. Preserve the existing diagnostic behavior while
ensuring the prefix and slice offset use an identical directory value.
In `@src/bundler/linker.rs`:
- Around line 720-721: Update the hash-key calculation around the
top_level_dir() calls to read fs.top_level_dir() once into a local value, then
use that same value for both starts_with and the slice length. Preserve the
existing prefix-check behavior while preventing inconsistent directory values
between operations.
In `@src/jsc/RuntimeTranspilerCache.rs`:
- Line 711: Update the `RuntimeTranspilerCache` code at the `top_level_dir()`
call to use the shared `FileSystem::get()` accessor instead of
`FileSystem::instance()`, preserving the existing top-level directory retrieval
behavior.
In `@src/runtime/api/filesystem_router.rs`:
- Line 166: In the worker-resolution path, replace the mutable singleton access
in the call to top_level_dir with the read-only Fs::FileSystem::get() accessor,
matching the usage near line 950; leave the top_level_dir read and surrounding
logic unchanged.
In `@src/runtime/cli/publish_command.rs`:
- Line 147: Update the read-only FileSystem accesses near the publish command
logic, including the call at line 147 and the corresponding access near line
654, to use FileSystem::get() instead of FileSystem::instance(). Preserve the
existing top_level_dir() behavior while obtaining the shared reference for
concurrent resolver readers.
In `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts`:
- Line 65: Remove the per-test timeout argument from the test call in
worker-chdir-resolve-race.test.ts, ending the call with `});`. Keep the test
behavior unchanged and rely on the build-aware runner’s timeout.
---
Outside diff comments:
In `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts`:
- Around line 56-64: Update the subprocess result handling around Promise.all so
stdout, stderr, and exitCode are asserted together before calling JSON.parse,
preserving failure details when the child aborts early. Keep the existing
structured out.nerr/errs assertion after parsing, and retain the successful
exit-code expectation.
- Around line 28-41: Update the Worker creation and result collection around the
churn loop to use try/finally, ensuring stop is set and churn is awaited on
every exit path. Make Worker error and unexpected exit events reject rather than
resolve failure objects, while preserving a settled guard so the expected exit
after a message is ignored; track every created Worker and terminate them during
cleanup, including when new Worker throws.
🪄 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: f4e0d931-e6d6-420f-81ed-d6d55a9e6ca8
📒 Files selected for processing (49)
src/bundler/HTMLImportManifest.rssrc/bundler/HTMLScanner.rssrc/bundler/LinkerContext.rssrc/bundler/bundle_v2.rssrc/bundler/linker.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/install/PackageManager/install_with_manager.rssrc/install/lockfile.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/VirtualMachine.rssrc/jsc/hot_reloader.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/cron.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/bake_body.rssrc/runtime/bake/mod.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/init_command.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/open.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test/ChangedFilesFilter.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_stat_watcher.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/node_process.rssrc/runtime/server/RequestContext.rssrc/runtime/server/server_body.rssrc/runtime/webcore/fetch.rstest/js/node/worker_threads/worker-chdir-resolve-race.test.ts
There was a problem hiding this comment.
No bugs found, but this is a concurrency fix to process-global state (FileSystem field removal, new intern_cwd cache, RwLock-backed cwd publication) touching 49 files including the resolver and VirtualMachine::set_process_cwd — worth a human look.
What was reviewed:
set_process_cwdrewrite: local-buffer build + trailing-SEP append +DirnameStoreintern + RwLock publish; rollback path still reads the guarded accessor.intern_cwdstaticMutex<Vec>dedup — bounded by distinct cwds visited;DirnameStore::appendis the process-lifetime interner so'staticholds.- Field-removal migration: ~45
.top_level_dir→.top_level_dir()sites are compiler-enforced; double-read TOCTOU sites (bundle_v2, linker) were hoisted to a single local per earlier review. - Test: both dirs contain
mod.mjsso any import failure proves a torn cwd;error/exitevents wired to resolve, subprocess pipes drained concurrently.
Extended reasoning...
Overview
Fixes a data race where process.chdir() on the main thread rewrote the shared FileSystem singleton's top_level_dir_buf in place in two steps and re-sliced the public top_level_dir: &'static [u8] field with a plain fat-pointer store, while Worker VMs read that field unsynchronized during relative-import resolution. The fix removes the mutable buffer and public field entirely: FileSystem::top_level_dir() now delegates to the pre-existing bun_core::top_level_dir() (RwLock-guarded), set_process_cwd builds the new cwd locally and interns it into the process-lifetime DirnameStore via a content-deduplicated static cache, and ~45 call sites are mechanically migrated from field to method (compiler-enforced since the field is gone). init_with_force now seeds bun_core::set_top_level_dir(cwd) so the guarded value is correct from startup.
Security risks
None identified. This is an internal concurrency fix; no user-controlled input reaches new parsing or allocation logic. The intern_cwd cache grows with distinct cwds visited over process lifetime, which is user-controllable via repeated process.chdir() to distinct paths, but each entry is a small interned slice and the dedup prevents unbounded growth from oscillation between the same directories (the common case the test exercises).
Level of scrutiny
High. This touches process-global state read on every relative module resolution, across the main VM and all Worker VMs, and the failure mode on the unfixed build is a debug-assert abort / release-build torn path. The mechanical call-site migration is low-risk (type-checked), but the core set_process_cwd rewrite, the new static Mutex<Vec> interning cache, and the FileSystem struct field removal are design changes a maintainer should sign off on.
Other factors
The bug hunting system found no issues. All prior CodeRabbit findings were either addressed (double-read hoisting in bundle_v2.rs/linker.rs) or reasonably scoped out (pre-existing instance() vs get() accessor choice, orthogonal to this fix). My earlier nit on the stale Scanner.rs SAFETY comment was addressed in 2742bb3. The new stress test follows harness conventions (tempDir, bunEnv, concurrent pipe drain, error events wired to resolve) and its 30s timeout matches the sibling worker-transfer-terminate-stress.test.ts pattern. rust:check-all reportedly passes on all 10 targets.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/node/worker_threads/worker-chdir-resolve-race.test.ts (3)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass a copied environment to the child process.
Use
env: { ...bunEnv }instead of passing the shared harness object directly. This keeps the subprocess environment local and supports future child-specific overrides without mutating shared test state.As per coding guidelines, subprocess tests should spread
bunEnv.Proposed fix
- env: bunEnv, + env: { ...bunEnv },🤖 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 `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts` around lines 49 - 55, Update the Bun.spawn invocation in the worker-chdir-resolve-race test to pass a copied environment using the existing bunEnv values, rather than the shared bunEnv object directly; preserve the current subprocess settings and command while allowing future child-specific overrides.Source: Coding guidelines
56-64: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve subprocess failure diagnostics.
JSON.parse(stdout.trim())runs beforeexitCodeis asserted. If the assert-build regression aborts before writing JSON, the parser error can hide the subprocess outcome. Parse defensively and assert the parsed output,stderr, andexitCodeas one combined result.As per coding guidelines, subprocess tests must drain stdout, stderr, and process exit concurrently and assert a combined result. Based on learnings, Worker crash tests should keep the complete subprocess outcome together.
Proposed fix
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); - const out = JSON.parse(stdout.trim()); + let out: unknown; + try { + out = JSON.parse(stdout.trim()); + } catch { + out = undefined; + } - expect(out).toEqual({ nerr: 0, errs: [] }); - expect(exitCode).toBe(0); + expect({ out, stderr, exitCode }).toEqual({ + out: { nerr: 0, errs: [] }, + stderr: "", + exitCode: 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 `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts` around lines 56 - 64, Update the subprocess result handling around Promise.all so stdout, stderr, and exitCode are drained concurrently and asserted as one combined outcome; parse stdout defensively only after confirming the subprocess completed successfully, while preserving diagnostics from stderr and nonzero exit codes when JSON output is missing or invalid.Sources: Coding guidelines, Learnings
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the successful-import count.
The Worker reports
ok, but the parent drops it and only aggregatesnerr. A Worker that performs fewer than 2,000 iterations without throwing can still make this test pass. Aggregateokand assert4_000for the two Workers.As per coding guidelines, test assertions must check the strongest invariant.
Proposed fix
console.log(JSON.stringify({ + ok: results.reduce((a, r) => a + r.ok, 0), nerr: results.reduce((a, r) => a + r.nerr, 0), errs: results.flatMap(r => r.errs).slice(0, 3), })); @@ - expect(out).toEqual({ nerr: 0, errs: [] }); + expect(out).toEqual({ ok: 4_000, nerr: 0, errs: [] });Also applies to: 60-63
🤖 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 `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts` around lines 42 - 45, Update the result aggregation around the worker result summary to include the successful-import count from each Worker, then assert that the combined count is exactly 4,000 for the two Workers. Preserve the existing error aggregation and assertions while ensuring a Worker that completes fewer iterations cannot satisfy the test.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/worker_threads/worker-chdir-resolve-race.test.ts`:
- Around line 49-55: Update the Bun.spawn invocation in the
worker-chdir-resolve-race test to pass a copied environment using the existing
bunEnv values, rather than the shared bunEnv object directly; preserve the
current subprocess settings and command while allowing future child-specific
overrides.
- Around line 56-64: Update the subprocess result handling around Promise.all so
stdout, stderr, and exitCode are drained concurrently and asserted as one
combined outcome; parse stdout defensively only after confirming the subprocess
completed successfully, while preserving diagnostics from stderr and nonzero
exit codes when JSON output is missing or invalid.
- Around line 42-45: Update the result aggregation around the worker result
summary to include the successful-import count from each Worker, then assert
that the combined count is exactly 4,000 for the two Workers. Preserve the
existing error aggregation and assertions while ensuring a Worker that completes
fewer iterations cannot satisfy the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3429c99-c329-440e-8c83-6654d44fcc1f
📒 Files selected for processing (1)
test/js/node/worker_threads/worker-chdir-resolve-race.test.ts
There was a problem hiding this comment.
All six of my earlier findings (unsynchronized field readers, unbounded DirnameStore intern leak, double-read TOCTOU in bundle_v2/linker, stale Scanner SAFETY comment, unwired worker exit handler) are now addressed, and the bug-hunt pass found nothing new. Deferring only because this is a cross-thread synchronization change to the process-global cwd that every Worker resolve reads — worth a human glance at the ~50 substantive lines in VirtualMachine::set_process_cwd/intern_cwd and resolver/lib.rs; the other ~45 files are the mechanical .top_level_dir → .top_level_dir() migration the field removal forces.
Also checked: bun_core::top_level_dir() is the RwLock-guarded &'static [u8] (Global.rs:31-44) so the fat-pointer publish is atomic; intern_cwd's linear-scan Vec cache is bounded by distinct-directory count; the len < buf.len() guard covers the trailing-SEP write; init_with_force seeds the RwLock so pre-chdir reads no longer see b".".
Extended reasoning...
Overview
Fixes a data race where process.chdir() on the main thread mutated the shared FileSystem singleton's top_level_dir_buf in place while Worker VMs read fs.top_level_dir during relative-import resolution, producing torn paths with interior NUL bytes (release) or ZStr::as_cstr panics (debug/ASAN). The fix: (1) set_process_cwd builds the new cwd in a local buffer, interns it via a content-deduplicated Mutex<Vec> cache into DirnameStore, and publishes through the RwLock-guarded bun_core::set_top_level_dir; (2) the pub top_level_dir field and top_level_dir_buf are removed from FileSystem, with top_level_dir() delegating to bun_core::top_level_dir(); (3) all ~90 direct-field readers across 45 files are migrated to the method so the compiler enforces no unsynchronized fat-pointer reads remain. A Worker stress test reproduces the tear on the unfixed build.
Security risks
None. No auth, crypto, network, or untrusted-input parsing surface. The change narrows a pre-existing user-reachable panic/crash into correct behavior.
Level of scrutiny
High — this is a thread-safety fix on process-global state that every module resolution reads, reachable from user JS via process.chdir() + new Worker(). That said, the substantive change is ~50 lines across VirtualMachine.rs and resolver/lib.rs; the remaining ~45 files are a compiler-forced mechanical rename (removing the pub field means every .top_level_dir read fails to compile until switched to .top_level_dir()).
Other factors
I've reviewed this across three prior rounds. Both 🔴 blockers I raised (the field/store still being unsynchronized after the first revision; the unbounded per-chdir intern leak) and all four 🟡 nits (double-read TOCTOU in bundle_v2/linker, stale Scanner SAFETY comment, unwired worker exit, per-test timeout) were addressed — the timeout nit was declined with the sibling-stress-test precedent, which is fine. CodeRabbit's separate bounds-check and instance()-vs-get() threads are all resolved or withdrawn. rust:check-all passes on all 10 targets and the Node compat tests still pass per the description. Given the criticality of the resolver path I'd rather a maintainer confirm the intern_cwd static-in-fn + DirnameStore approach is the shape they want long-term, but I have no correctness concerns.
|
Two more data points for this fix, from a separate sighting of the same root cause (
The branch currently conflicts with main and needs a rebase. |
e626918 to
74492b5
Compare
|
Updated 9:35 PM PT - Aug 27th, 2026
❌ @robobun, your commit 8b677d6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36584That installs a local version of the PR into your bun-36584 --bun |
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and the bug hunting system found no issues this round; all seven of my prior inline findings across earlier revisions have been addressed (field removal, dedup cache, hoisted double-reads, Scanner SAFETY comment, worker readiness gate in the second test). Because the core change replaces the resolver singleton's cwd storage model — read on every module resolution across the main VM, workers, and bundler threads — a maintainer look at the intern_cwd bound and the per-read RwLock cost would still be worthwhile.
What was reviewed:
set_process_cwdnow interns intoDirnameStoreand publishes via thebun_coreRwLock; the in-placetop_level_dir_bufand unsynchronized field are gone.- The ~43 mechanical
.top_level_dir→.top_level_dir()sites, plus the two hoisted TOCTOU sites inbundle_v2.rs/linker.rsand the borrowck-driven hoists inhot_reloader.rs/options.rs. - Both regression tests: worker
error/exitwiring, the readinesspostMessagegate before the read loop, and that neither test can pass vacuously.
Extended reasoning...
Overview
This PR fixes a data race where process.chdir() overwrote the resolver FileSystem singleton's top_level_dir_buf in place while other threads (Workers, bundler threads, node:fs) held &'static slices into it, producing torn paths with interior NULs. The fix: (1) remove the mutable buffer and unsynchronized field from FileSystem; (2) have top_level_dir() delegate to the existing bun_core::top_level_dir() RwLock; (3) have set_process_cwd intern the new cwd into the append-only DirnameStore via a deduplicating Mutex<Vec<&'static [u8]>> cache so repeated chdir between the same directories interns each once. ~43 of the 48 files are the mechanical field→method migration; the substantive change is ~50 lines across VirtualMachine.rs and resolver/lib.rs, plus a 144-line test file exercising both race directions.
Security risks
None identified. This is an internal thread-safety fix; no user-controlled input reaches the new intern_cwd path beyond what already reached chdir. The dedup cache is bounded by the number of distinct directories the process chdirs into, which is user-controllable but no worse than the pre-existing DirnameStore growth from module resolution.
Level of scrutiny
High. The resolver FileSystem singleton's cwd is read on every relative-path resolution across the main VM, every Worker, the bundler thread pool, Bun.spawn, fs.watch, Bun.which, and the test runner. Swapping its storage model from a fixed buffer to RwLock-guarded interned slices changes the cost profile of a hot path (one RwLock read per top_level_dir() call) and introduces a process-lifetime allocation per distinct cwd. The fix is correct as far as I can verify — published slices are genuinely immutable, DirnameStore is documented as append-only/mutex-protected, and the RwLock already existed in bun_core — but the hot-path and growth-bound tradeoffs are maintainer judgment calls.
Other factors
All prior review feedback is resolved: my six earlier inline findings (unsynchronized field still present, unbounded interning, unhandled worker exit, two TOCTOU double-reads, stale Scanner SAFETY comment) and the most recent one (second test's vacuous-pass window) were each fixed in follow-up commits, the last being 43c36f9 which gates the read loop on per-worker postMessage readiness with onerror rejection. CodeRabbit's instance()→get() suggestions were correctly scoped out as pre-existing. The PR description is unusually thorough (repros for both directions, panic-site enumeration, cross-target rust:check-all). The open policy question the author flags — whether Web Workers should be allowed to call process.chdir() at all — is orthogonal to this fix but worth a maintainer's note.
There was a problem hiding this comment.
I reviewed this PR again after 43c36f9 addressed the last inline nit — no bugs found. Given it changes the synchronization model of the process-global resolver cwd (removing top_level_dir/top_level_dir_buf and routing ~45 readers through the bun_core RwLock + DirnameStore intern), a human sign-off on the design would still be worthwhile.
What was reviewed:
set_process_cwd/intern_cwd: the published slice is immutable, the trailing-separator append is bounds-checked, and the dedup cache bounds growth to distinct cwds.FileSystem::initseedsbun_core::set_top_level_dir, so early readers see the initial cwd;set_top_level_dirnow takes&selfand only writes the RwLock.- Double-read sites in
bundle_v2.rs/linker.rsare hoisted;Scanner::top_level_dirno longer derefsself.fsand its stale SAFETY comment moved tofilename_store. - Both tests now wire worker
error/messageso a worker load failure or slow startup can't produce a vacuous pass.
Extended reasoning...
Overview
The PR fixes a cross-thread data race in the resolver's cached cwd: VirtualMachine::set_process_cwd previously overwrote FileSystem.top_level_dir_buf in place and re-sliced top_level_dir in two unsynchronized steps, while ~90 sites (resolver, bundler threads, Worker VMs, node:fs, Bun.which, path.resolve, Bun.pathToFileURL) held &'static slices into that buffer. The fix removes the field and buffer entirely, interns each new cwd into the process-lifetime DirnameStore (deduplicated via a small Mutex<Vec> cache), and publishes it through the existing bun_core::top_level_dir() RwLock. FileSystem::top_level_dir() becomes a thin read of that lock. Roughly 45 files are the mechanical .top_level_dir → .top_level_dir() migration; the substantive changes are in src/jsc/VirtualMachine.rs and src/resolver/lib.rs.
Security risks
None identified. The change tightens a memory-safety invariant (no torn slices with interior NULs reaching open_dir_absolute_z / resolver cache keys). No new user-controlled input is parsed; the interned cwds come from getcwd().
Level of scrutiny
High. This is a synchronization change to a process-global singleton on a hot path — the resolver's cwd is read on every relative-path resolution, by the bundler's worker threads, and by every Worker VM. The fix is architecturally sound (immutable interned slice + RwLock-guarded (ptr, len) publish, matching what bun_core::top_level_dir() already did), but the blast radius (48 files, module resolution, bundling, fs.watch, Bun.build) and the threading model change put it outside the "simple/mechanical" bar for auto-approval.
Other factors
Five prior rounds of inline feedback from this review (and CodeRabbit's) have all been addressed and resolved: the bundle_v2.rs/linker.rs double-reads were hoisted; the stale Scanner.rs SAFETY comment was fixed by dropping the raw-pointer deref; the second test now gates its read loop on worker readiness with onerror wired. Two regression tests cover both directions of the race (main-thread chdir vs. Worker resolve, and Worker chdir vs. main-thread cwd readers), and the PR description documents additional deterministic and CI-observed reproductions. rust:check-all and clippy --workspace are reported clean. The only change since my last inline comment is the test fix I requested (43c36f9) plus a CI retrigger (9826eb4). No human reviewer has weighed in yet.
9826eb4 to
2e791b3
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; all prior inline findings (the Scanner.rs stale SAFETY comment, the bundle_v2.rs/linker.rs double-reads, and the second test's worker-readiness precondition) have been addressed. Because this reworks how the process-global FileSystem cwd is published and read across ~45 call sites — a cross-thread memory-safety fix in the resolver/VM core — a human look would still be worthwhile.
Checked: intern_cwd correctly routes through the mutex-protected DirnameStore so published slices are immutable and process-lifetime; FileSystem::top_level_dir() now delegates to the RwLock-guarded bun_core::top_level_dir() so (ptr, len) can't tear; set_top_level_dir dropping to &self is sound since it only writes through the RwLock. The options.rs hoist of top_level_dir before &mut fs.fs avoids the borrow conflict the old field access sidestepped.
Extended reasoning...
Overview
This PR fixes a data race where process.chdir() overwrote the resolver FileSystem singleton's top_level_dir_buf in place while other threads (Workers, bundler threads) held &'static slices into it, producing torn paths with interior NULs. The fix: (1) VirtualMachine::set_process_cwd now interns the new cwd into the append-only DirnameStore via a deduplicating intern_cwd cache, so a published slice is never mutated; (2) the top_level_dir field and top_level_dir_buf are removed from FileSystem, and top_level_dir() reads through the existing RwLock-guarded bun_core::top_level_dir(); (3) ~45 files mechanically migrate .top_level_dir → .top_level_dir(). Two double-read sites (bundle_v2.rs, linker.rs) were hoisted so a single resolve uses one cwd snapshot. A new two-direction stress test covers Worker-imports-while-main-chdirs and main-reads-while-Worker-chdirs.
Security risks
None identified. This is a correctness/memory-safety fix; no new attack surface, auth, or untrusted-input parsing is introduced. The intern_cwd cache is bounded by the number of distinct directories the process chdirs into.
Level of scrutiny
High. The change touches the process-global resolver singleton and the VM's cwd handling — infrastructure every module resolution, fs.realpath, path.resolve, Bun.which, Bun.build, and Worker startup reads. It is a concurrency fix, and the correctness argument depends on DirnameStore being append-only/process-lifetime and bun_core::top_level_dir() being RwLock-guarded — both hold, but a maintainer should confirm the interning approach and the removal of top_level_dir_buf don't interact badly with any in-flight work.
Other factors
The PR has been through several review rounds: CodeRabbit's double-read findings were fixed; its instance()-vs-get() suggestions were correctly deferred as pre-existing/out-of-scope; my prior findings (stale Scanner.rs SAFETY comment now that the method call autorefs the whole struct, and the second test's vacuous-pass window before workers start flipping) were both fixed. The test now awaits a postMessage handshake from each flip worker before the read loop and wires onerror to reject. rust:check-all and clippy are reported clean. Given the breadth (48 files) and that this is core-path concurrency code with no human review yet, deferring rather than auto-approving.
2e791b3 to
6945bd5
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues on the current revision — all earlier findings (field removal, intern dedup, double-read hoists, Scanner SAFETY comment, worker-readiness gating in both tests) have been addressed. Because this is a thread-safety change to the process-global FileSystem cwd cache read from ~90 sites across the resolver, bundler threads, Worker VMs and node:fs, spanning 48 files, a human sign-off is still worthwhile.
What was reviewed:
set_process_cwd/intern_cwd: dedup cache bounds interning to distinct cwds; published slice is DirnameStore-backed and never mutated.FileSystem::top_level_dir()now delegates to the RwLock-guardedbun_core::top_level_dir();initseeds it; the two check-then-slice sites inbundle_v2.rs/linker.rsare hoisted to a single read.- Both regression tests wire error/exit and gate the read loop on worker readiness so they cannot pass vacuously.
Extended reasoning...
Overview
This PR fixes a data race in process.chdir(): the resolver's process-global FileSystem singleton previously overwrote its top_level_dir_buf in place while other threads (Worker VMs, bundler threads, node:fs, path.resolve, Bun.which) held &'static slices into it, producing torn paths with interior NULs and assert-build panics. The fix removes the mutable buffer entirely: set_process_cwd now interns each cwd into the append-only DirnameStore via a small Mutex<Vec<&'static [u8]>> dedup cache, and FileSystem::top_level_dir() delegates to the existing RwLock-guarded bun_core::top_level_dir(). About 45 files receive the mechanical .top_level_dir → .top_level_dir() migration; two double-read sites are hoisted; Scanner::top_level_dir() drops its now-unnecessary raw-pointer deref. A new two-direction Worker-vs-main-thread stress test lands under test/js/node/worker_threads/.
Security risks
None specific to this change. The affected surface is internal path-resolution infrastructure; no auth, crypto, or untrusted-input parsing is touched. The pre-existing bug was itself a memory-safety hazard (torn slice with interior NUL passed to C APIs, assert-build panics from resolver/bundler threads); the fix closes it.
Level of scrutiny
High. This is a concurrency fix to a process-global singleton on the module-resolution hot path, read from roughly 90 sites including the bundler thread, Worker VMs, node:fs, and the DevServer watcher. The mechanical migration is straightforward, but the core change — swapping an in-place buffer rewrite for interned-slice publication under an RwLock, plus the new intern_cwd dedup cache — is exactly the class of change where a subtle ordering or lifetime mistake surfaces only under load. The 48-file span alone puts it outside the "simple, mechanical, or obvious" bucket.
Other factors
The PR has iterated well: earlier automated findings (the field remaining public alongside the accessor, unbounded per-call interning, TOCTOU on adjacent top_level_dir() reads, stale SAFETY comment in Scanner.rs, and vacuous-pass windows in both regression tests) have all been addressed in follow-up commits, and CodeRabbit's parallel findings were either fixed or correctly scoped out as pre-existing instance()-vs-get() cleanup. The PR description documents deterministic single-threaded repros (--isolate --preload) and the debug-runner itBundled abort as additional evidence for the same root cause. rust:check-all and clippy are reported clean. Given the scope and the criticality of the resolver cwd cache, I'm deferring rather than approving so a maintainer can confirm the publication/interning approach and the FileSystem::init → bun_core::set_top_level_dir seeding order.
6945bd5 to
7550523
Compare
There was a problem hiding this comment.
Re-reviewed after a0c5be3 — everything I raised earlier is now in the code, and I found nothing new. Given this is a 48-file thread-safety change to process-global state, a maintainer sign-off is still worthwhile.
What was reviewed:
- Confirmed the
top_level_dirfield andtop_level_dir_bufare gone fromFileSystem; grep shows no remaining resolver-FileSystemfield reads (the surviving.top_level_dirhits are the unrelatedMiniEventLoop/bun_pathsstructs). intern_cwddedups against aMutex<Vec>so growth is bounded to distinct dirs; the double-read TOCTOU sites inbundle_v2.rsandlinker.rsare hoisted;Scanner.rsno longer carries a stale SAFETY comment.- The second test now awaits a per-worker
"flipping"message and wiresonerrorbefore entering the read loop, so it can no longer pass vacuously.
Extended reasoning...
Overview
This PR fixes a data race in process.chdir() where the resolver's process-global FileSystem singleton stored its cached cwd as a &'static [u8] slice into a mutable PathBuffer that set_process_cwd overwrote in place with no synchronization. Worker threads (or bundler threads) reading the slice concurrently could observe torn (ptr, len) pairs or mid-write bytes, producing paths with interior NULs and aborting assert builds. The fix removes the field and its backing buffer entirely, routes top_level_dir() through the existing bun_core::TOP_LEVEL_DIR RwLock<&'static [u8]>, and has set_process_cwd intern each new cwd into the process-lifetime DirnameStore (deduplicated via a small Mutex<Vec> cache) before publishing it. Roughly 45 mechanical .top_level_dir → .top_level_dir() conversions follow across bundler/install/jsc/runtime, plus a two-test regression file.
Security risks
None identified. The change tightens synchronization around a process-global path cache; it does not touch auth, crypto, network input parsing, or trust boundaries. The interned strings come from getcwd() (kernel-provided), and the dedup cache is bounded by distinct-directory count, so there is no user-controllable unbounded allocation.
Level of scrutiny
High. This is a concurrency fix to shared mutable state read from ~90 sites across the resolver, bundler worker threads, Worker VMs, and node:fs. Over prior review rounds I flagged (and the author fixed): the field was still public and directly read at dozens of sites; interning leaked unboundedly per chdir; two double-read sites in bundle_v2.rs/linker.rs could TOCTOU-panic on a slice index; a stale SAFETY comment in Scanner.rs; and both tests had unwired failure paths / a vacuous-pass window. This run confirmed each of those is now in the code — the field is deleted (compiler-enforced), intern_cwd dedups, the double reads are hoisted to locals, Scanner::top_level_dir calls bun_core::top_level_dir() directly with no unsafe, and the second test awaits a worker readiness handshake with onerror wired. A grep for \.top_level_dir\b[^(] and top_level_dir_buf across src/ found no remaining resolver-FileSystem field accesses.
Other factors
The one nit I previously left standing (the 60_000 per-test timeout) is unchanged; I noted at the time it matches sibling stress tests in the same directory and is not blocking, so I am not repeating it. No third-party CHANGES_REQUESTED reviews are outstanding; the coderabbit threads are resolved by a non-author. The change is well-tested (two direction-specific race tests plus the listed suite runs), but 48 files of process-global concurrency plumbing is beyond what I would auto-approve without a maintainer look — hence deferring rather than approving.
|
CI state on 8b677d6 (build 107260): the only red job is darwin x64. Its failure is the pre-existing |
…tion never sees a torn path `VirtualMachine::set_process_cwd` rewrote the resolver `FileSystem` singleton's `top_level_dir_buf` in place and re-sliced `top_level_dir` after each step, with no synchronization. Any other thread that read the cached cwd at that moment (a Worker resolving a relative import, the bundler thread, or the main thread while a Web Worker calls chdir) saw a mix of the old and new directory names, often with an interior NUL. Intern each new cwd into the `DirnameStore` instead, deduplicated, so a published slice is never mutated. Remove the plain `top_level_dir` field and route every reader through `FileSystem::top_level_dir()`, which reads the RwLock-guarded `bun_core::top_level_dir()`, so the (ptr, len) pair cannot tear either. Seed that lock at `FileSystem::init`. Tests cover both directions: main-thread chdir against Worker import() resolution, and Web Worker chdir against main-thread realpathSync, pathToFileURL, path.resolve and Bun.which.
Each flip worker posts a message after its first chdir batch. The main thread waits for all four before the read loop, and a worker load failure rejects the wait. This removes the vacuous-pass window where the synchronous loop finished before any worker started to chdir.
7550523 to
8b677d6
Compare
|
One more trigger for the same root cause, for the record. A debug build of current main (d508ad8) aborts with // race.ts: run from a directory that contains invalid.css (any CSS).
// `long` and `short` are two existing directories of different lengths,
// both containing invalid.css.
const long = "/tmp/chdir-race/" + "a".repeat(93);
const short = "/tmp/chdir-race/b";
process.chdir(short);
let done = false;
async function buildLoop() {
while (!done) {
try { await Bun.build({ entrypoints: ["invalid.css"] }); } catch {}
}
}
const loops = [buildLoop(), buildLoop()];
const deadline = Date.now() + 30_000;
while (Date.now() < deadline) {
for (let i = 0; i < 200; i++) { process.chdir(long); process.chdir(short); }
await new Promise(r => setImmediate(r));
}
done = true;
await Promise.all(loops);Three runs of 40 s each aborted, through three different readers of the torn
This is also how |
This branch has not been deployed
2 participants
Problem
process.chdir()on one thread corrupts the cached cwd every other thread reads. A Worker import fails withCannot find module '/tmp/.../d1\0/mod.mjs'. While a Web Worker calls chdir, the main thread getsfs.realpathSync(".")=.../yy(a different directory) andBun.pathToFileURL("rel")=.../x/%00yyyy/rel.path.resolve,Bun.which,fs.watchandBun.buildtear the same way. Assert builds abort:panic: ZStr::as_cstr: interior NUL would truncate the C view.VirtualMachine::set_process_cwd(src/jsc/VirtualMachine.rs) overwrote the singleton'stop_level_dir_bufin place, with no lock, while other threads held&'staticslices into it. Thetop_level_dirfield was a plain 16-byte store read by about 90 sites.Fix
set_process_cwdinterns the new cwd intoDirnameStorethrough a deduplicated cache (intern_cwd). A published slice is never mutated.top_level_dirfield andtop_level_dir_bufare removed.FileSystem::top_level_dir()reads the RwLock-guardedbun_core::top_level_dir(). Every reader goes through the method, so the (ptr, len) pair cannot tear.FileSystem::initseeds the lock.test/js/node/worker_threads/worker-chdir-resolve-race.test.ts, both directions (stock bun fails 2 of 2). Also Node'stest-process-chdir*.js,test/cli/test/isolation.test.ts,test/js/node/worker_threads/.Background
FileSystem(src/resolver/lib.rs) is a process-global singleton.top_level_diris its cached cwd. The resolver, bundler threads, Worker VMs andnode:fsjoin relative paths against it.DirnameStoreis an append-only, mutex-protected string arena for the whole process, so its slices are&'static.bun_core::top_level_dir()(src/bun_core/Global.rs) already kept the cwd behind aRwLock<&'static [u8]>because a split ptr/len could tear. The resolver kept its own unsynchronized copy.Notes
Repro for the Worker direction: the main thread flips
process.chdir()between two sibling dirs while 2 Workers loopawait import("./mod.mjs?i=" + i). Both dirs containmod.mjs, so any rejection proves a torn cwd.Repro for the main-thread direction: 4 Web Workers flip
process.chdir()betweenxandyyyyyyyyyyyyyyyy(different lengths) while the main thread, which never calls chdir, readsfs.realpathSync("."),Bun.pathToFileURL("rel"),path.resolve("f")andBun.which("tool", { PATH: "rel" }). Stock 1.4.0 produces 125+ torn values in 2000 iterations, for example.../yy,.../y/f,.../x\0/f,.../x//rel/tool,file:///.../x%00yyyyyyyyyyyyyy/rel. 1.3.14 behaves the same, so this is not a regression.Other panic sites seen on assert builds, all the same root cause:
open_dir_absolute_z <- Resolver::dir_info_cached_miss(worker startup, main-threadimport(), theBun.buildbundle thread),path_watcher::watch <- NodeFS::watch,NodeFS::realpath_inner, andInternal Assertion Failure: Invalid cache key "/.../dA/\0BBBB/"fromResolver::assert_valid_cache_keyon the bundle thread. The debug test runner itself hit the bundle-thread abort while runningtest/bundler/esbuild/default.test.ts, becauseitBundledcallsprocess.chdiraround everyBun.build().intern_cwdkeeps aMutex<Vec<&'static [u8]>>of interned cwds and does a linear scan. Growth is bounded by the number of distinct directories a process chdirs into. Repeated chdir between the same directories interns each once.The mechanical migration (
.top_level_dirto.top_level_dir()) touches about 45 files. Two double-read sites inbundle_v2.rsandlinker.rswere hoisted to a single local so one resolve uses one cwd.rust:check-allpassed on all targets on the first version of this change.cargo clippy --workspaceis clean on the rebase onto current main.Other suites run on the rebased debug build:
test/js/node/process/process.test.js(one pre-existing failure:$USERunset in the container),test/js/bun/spawn/spawn-renamed-cwd.test.ts,test/js/bun/util/which.test.ts,test/js/bun/glob/scan.test.ts(recursive-scan cases exceed their 30 s budget on a debug build regardless of this change).Whether a Web Worker should be allowed to call
process.chdir()at all (node:worker_threads workers throwERR_WORKER_UNSUPPORTED_OPERATION) is a separate policy question. This change makes the cache safe regardless of which thread writes it.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/worker_threads/worker-chdir-resolve-race.test.ts