resolver: take the per-entry lock when rewriting cached directory entries - #33056
Merged
Conversation
…lock The FileSystemRouter / FrameworkRouter load loops iterated a cached DirEntry's hashmap while another thread (e.g. Bun.build's resolver) could rewrite that map in place, and the lazily-populated Entry stat cache was rewritten from several threads without a lock. - Snapshot the DirEntry's entry pointers under the existing entries_mutex before each load/bust/scan loop (the guard is dropped before the loop so the read_dir_info recursion can re-acquire it). - Take the existing per-entry Entry.mutex (double-checked) inside Entry::kind/Entry::symlink's lazy-stat branch and at every other site that rewrites a cached Entry (Route::parse, the resolver's set_cache_* callers, the hot reloader), matching the lock the directory re-read path already takes; cached reads stay lock-free. - Keep a refreshed DirEntry's interned directory name stable across in-place refreshes, and accept both trailing-slash spellings of a cached directory name in the route loader (the resolver and the router spell the same directory differently, which also tripped a debug assertion when a Bun.build preceded the router on the same directory).
Collaborator
|
Updated 5:26 AM PT - Jun 29th, 2026
❌ @Jarred-Sumner, your commit 75c5471 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33056That installs a local version of the PR into your bun-33056 --bun |
Contributor
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Contributor
WalkthroughSerialization for ChangesPer-entry mutex for filesystem cache rewrites
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
src/router/lib.rs (1)
1024-1029: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winLock before reading
abs_path, not only before writing it.Line 1024 still reads
Entry.abs_pathoutsideEntry.mutex, while Line 1150 now serializes the laterset_abs_pathwrite. Two concurrent route loads can race on this non-atomic field; move the first read under the same guard and double-check there.Suggested shape
- let entry_abs_path = unsafe { &*entry }.abs_path().as_bytes(); - let mut abs_path_str: &[u8] = if entry_abs_path.is_empty() { - b"" - } else { - entry_abs_path - }; + let mut abs_path_str: &[u8] = b""; ... - if abs_path_str.is_empty() { - // The reads of `cache().fd` and the `set_abs_path` write below - // rewrite the cached `Entry`; serialize them on the per-entry - // mutex (the same lock every other `Entry` rewrite path takes). - // SAFETY: see fn-level NOTE — read-only reborrow. - let _entry_guard = unsafe { &*entry }.mutex.lock_guard(); + { + let _entry_guard = unsafe { &*entry }.mutex.lock_guard(); + let entry_abs_path = unsafe { &*entry }.abs_path().as_bytes(); + if !entry_abs_path.is_empty() { + abs_path_str = entry_abs_path; + } else { + // existing fd/open/get_fd_path/set_abs_path block + } + }As per coding guidelines, shared state must use atomics or locks consistently.
Also applies to: 1145-1150
🤖 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/router/lib.rs` around lines 1024 - 1029, `Entry.abs_path` is being read outside the `Entry.mutex`, while `set_abs_path` is already protected, so the access pattern is inconsistent and can race during concurrent route loading. Update the logic in the route-loading path that uses `Entry::abs_path` so the initial read happens under the same mutex guard used by `set_abs_path`, and keep the read/modify/write sequence inside that critical section. Revisit the related `set_abs_path`/lookup flow to ensure both the read and write paths for `abs_path` are synchronized consistently via `Entry.mutex`.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/resolver/fs.rs`:
- Around line 472-497: The lock-free cache reads in Entry::kind and symlink are
racing with cache rewrites protected by Entry.mutex, which can corrupt the
shared EntryCache state. Update these fast paths to read the cache only while
holding the same mutex used by set_cache_fd and set_cache_symlink, or split the
mutable cache fields so each part is independently synchronized. Use the
existing Entry::kind, symlink, and cache accessors as the main touchpoints when
applying the fix.
In `@src/runtime/api/filesystem_router.rs`:
- Around line 389-394: The concurrent reload path in `filesystem_router.rs`
should not call `FileSystem::instance()` just to reach `entries_mutex`, since
that creates a process-global mutable singleton access in a racing code path.
Update the `entry_ptrs` snapshot block to use the existing shared filesystem
accessor already available in this flow, and keep the lock-taking behavior the
same so `dir_ref.get_entries_const()` is still protected consistently by
`entries_mutex`.
In `@test/js/bun/util/filesystem_router.test.ts`:
- Around line 739-742: The explanatory comments in the filesystem router tests
are too long and need to be compressed to fit the 3-line limit. Shorten the
multi-line comments near the subprocess setup in the filesystem router test
while preserving the invariant about the process-global directory-entry cache
being shared between the route-load loop and Bun.build entry-point resolution,
and keep the reference to running in a subprocess so a crash is observable as a
signal. Apply the same cleanup to both comment blocks identified in the test.
- Line 786: Remove the explicit per-test timeout from the test case in
filesystem_router.test.ts so it relies on the runner’s default ASAN-scaled
budget. Update the affected test block near the closing of the suite to
eliminate the 60_000 argument, and keep the rest of the test logic unchanged.
- Around line 783-785: The subprocess assertions in the filesystem router
crash-oriented tests are split, which can hide useful failure diagnostics when
the child exits unexpectedly. Update the assertions around proc.stdout.text(),
proc.stderr.text(), proc.exited, and proc.signalCode so they are checked
together as a single object in one expect call, preserving stdout, stderr,
exitCode, and signalCode in the same diff; apply the same pattern in the other
affected test block as well.
- Line 797: The filesystem router test is only invoking Bun.build() with throw:
false, so a build failure could be silently ignored and the test would still
pass without proving the pre-cached path. Update the test around the Bun.build
call in filesystem_router.test.ts to explicitly assert that the pre-cache build
succeeds, using the existing build result or another success check before
continuing with the router scan.
---
Outside diff comments:
In `@src/router/lib.rs`:
- Around line 1024-1029: `Entry.abs_path` is being read outside the
`Entry.mutex`, while `set_abs_path` is already protected, so the access pattern
is inconsistent and can race during concurrent route loading. Update the logic
in the route-loading path that uses `Entry::abs_path` so the initial read
happens under the same mutex guard used by `set_abs_path`, and keep the
read/modify/write sequence inside that critical section. Revisit the related
`set_abs_path`/lookup flow to ensure both the read and write paths for
`abs_path` are synchronized consistently via `Entry.mutex`.
🪄 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: 7c93c4cb-ce63-4ac5-8eef-37a4323b7f12
📒 Files selected for processing (8)
src/jsc/hot_reloader.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/api/filesystem_router.rssrc/runtime/bake/FrameworkRouter.rstest/js/bun/util/filesystem_router.test.ts
Comment on lines
+472
to
+497
| /// `RealFS` singleton in practice). `resolve_kind` must not re-enter | ||
| /// this entry's `mutex` (it only performs syscalls and string interning). | ||
| // `Entry` lives in the EntryStore BSSMap singleton. The lazy-stat rewrite | ||
| // of `need_stat` / `cache` is serialized on the per-entry `mutex` here | ||
| // (double-checked: the cached fast path stays lock-free). `fs` is `*mut` | ||
| // so the call site does not require a second exclusive `&mut RealFS` | ||
| // borrow while a `&mut Entry` (borrowed out of `RealFS.entries`) is live. | ||
| // Generic over `R: EntryKindResolver` so this block is independent of | ||
| // which `RealFS` copy `fs` points at (see file-top comment). | ||
| pub unsafe fn kind<R: EntryKindResolver>(&self, fs: *mut R, store_fd: bool) -> EntryKind { | ||
| if self.need_stat.get() { | ||
| self.need_stat.set(false); | ||
| // This is technically incorrect, but we are choosing not to handle errors here | ||
| // SAFETY: `fs` points at the process-global RealFS singleton; caller holds | ||
| // `entries_mutex` so the `&mut` is exclusive for the duration of this call. | ||
| match unsafe { &mut *fs }.resolve_kind(self.dir, self.base(), self.cache().fd, store_fd) | ||
| { | ||
| Ok(c) => self.cache.set(c), | ||
| Err(_) => return self.cache().kind, | ||
| let _guard = self.mutex.lock_guard(); | ||
| if self.need_stat.get() { | ||
| self.need_stat.set(false); | ||
| // This is technically incorrect, but we are choosing not to handle errors here | ||
| // SAFETY: `fs` points at the process-global RealFS singleton; `resolve_kind` | ||
| // only does syscalls + string interning, so the short `&mut` cannot alias. | ||
| match unsafe { &mut *fs }.resolve_kind( | ||
| self.dir, | ||
| self.base(), | ||
| self.cache().fd, | ||
| store_fd, | ||
| ) { | ||
| Ok(c) => self.cache.set(c), | ||
| Err(_) => return self.cache().kind, | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# EntryCache layout
ast-grep --lang rust --pattern 'struct EntryCache { $$$ }' src/resolver/fs.rs
ast-grep --lang rust --pattern 'pub struct EntryCache { $$$ }' src/resolver/fs.rs
# Interned representation + as_bytes
fd -e rs . src/ptr 2>/dev/null | head
rg -nP 'struct\s+Interned' --type rust -C4
rg -nP 'impl\b.*\bInterned\b' --type rust -C2 | rg -n 'as_bytes' -C3
# Mutex semantics (does lock_guard imply a full barrier readers observe?)
rg -nP 'fn\s+lock_guard' --type rust -C4Repository: oven-sh/bun
Length of output: 668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant definitions and access patterns.
sed -n '340,560p' src/resolver/fs.rs
printf '\n--- Interned ---\n'
rg -n --context 4 'pub struct Interned|type Interned|enum Interned|impl .*Interned' src -g '!**/target/**'
printf '\n--- lock_guard / mutex impls ---\n'
rg -n --context 4 'fn lock_guard|struct .*Mutex|impl .*lock_guard|lock_guard\(' src -g '!**/target/**'Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '340,560p' src/resolver/fs.rs
printf '\n--- Interned ---\n'
rg -n --context 4 'pub struct Interned|type Interned|enum Interned|impl .*Interned' src -g '!**/target/**'
printf '\n--- lock_guard / mutex impls ---\n'
rg -n --context 4 'fn lock_guard|struct .*Mutex|impl .*lock_guard|lock_guard\(' src -g '!**/target/**'Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow, read-only inspection of kind()/symlink() callers and cache access patterns.
rg -n --context 2 '\.kind\(\s*[^)]*\)|\.symlink\(\s*[^)]*\)|cache\(\)\.(kind|symlink)|set_cache(_kind|_fd|_symlink)?\(' src/resolver src/runtime -g '!**/target/**' | head -n 200
printf '\n--- files mentioning Entry::kind / Entry::symlink ---\n'
rg -n --context 2 'Entry::kind|Entry::symlink|\.kind\(' src -g '!**/target/**' | head -n 200Repository: oven-sh/bun
Length of output: 31387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1120,1185p' src/router/lib.rs
printf '\n---\n'
sed -n '360,430p' src/runtime/api/filesystem_router.rs
printf '\n---\n'
sed -n '500,560p' src/runtime/node/node_fs.rsRepository: oven-sh/bun
Length of output: 11712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --context 3 '\.kind\(' src/resolver/fs.rs src/router/lib.rs src/runtime/api/filesystem_router.rs src/runtime/node/node_fs.rs src/runtime/bake/FrameworkRouter.rs src -g '!**/target/**' | head -n 120
printf '\n--- symlink ---\n'
rg -n --context 3 '\.symlink\(' src/resolver/fs.rs src/router/lib.rs src/runtime/api/filesystem_router.rs src/runtime/node/node_fs.rs src/runtime/bake/FrameworkRouter.rs src -g '!**/target/**' | head -n 120Repository: oven-sh/bun
Length of output: 17081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1788,1825p' src/resolver/resolver.rs
printf '\n---\n'
sed -n '1728,1742p' src/resolver/resolver.rs
printf '\n--- unsafe impl Sync for Entry ---\n'
rg -n --context 3 'unsafe impl Sync for Entry|impl Sync for Entry' src/resolver/fs.rs src/resolver/resolver.rs src -g '!**/target/**'Repository: oven-sh/bun
Length of output: 4761
Guard the cache reads on the same mutex as the rewrites. kind() and symlink() still read self.cache() lock-free, but other paths rewrite the same EntryCache under Entry.mutex (set_cache_fd, set_cache_symlink, etc.). Since EntryCache includes Interned = &'static [u8], a concurrent Cell::set() can race the fast path and tear the slice metadata. Keep these reads under the mutex or split the mutable state into atomic/independently guarded fields.
🤖 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/fs.rs` around lines 472 - 497, The lock-free cache reads in
Entry::kind and symlink are racing with cache rewrites protected by Entry.mutex,
which can corrupt the shared EntryCache state. Update these fast paths to read
the cache only while holding the same mutex used by set_cache_fd and
set_cache_symlink, or split the mutable cache fields so each part is
independently synchronized. Use the existing Entry::kind, symlink, and cache
accessors as the main touchpoints when applying the fix.
Comment on lines
+389
to
+394
| // Snapshot the cached `DirEntry`'s entry pointers under `entries_mutex` | ||
| // (other threads rewrite the map in place under that lock), then drop | ||
| // the guard: `bust_dir_cache` / the recursion below re-acquire it. | ||
| let entry_ptrs: Vec<*mut Fs::Entry> = { | ||
| let _entries_lock = Fs::FileSystem::instance().fs.entries_mutex.lock_guard(); | ||
| match dir_ref.get_entries_const() { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid FileSystem::instance() in this concurrent reload path.
Line 393 materializes the process-global filesystem as &'static mut just to lock entries_mutex, while this path is specifically racing with resolver work on other threads. Use the existing shared accessor instead.
Suggested fix
- let _entries_lock = Fs::FileSystem::instance().fs.entries_mutex.lock_guard();
+ let _entries_lock = vm.fs().fs.entries_mutex.lock_guard();As per coding guidelines, shared state must use atomics or locks consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Snapshot the cached `DirEntry`'s entry pointers under `entries_mutex` | |
| // (other threads rewrite the map in place under that lock), then drop | |
| // the guard: `bust_dir_cache` / the recursion below re-acquire it. | |
| let entry_ptrs: Vec<*mut Fs::Entry> = { | |
| let _entries_lock = Fs::FileSystem::instance().fs.entries_mutex.lock_guard(); | |
| match dir_ref.get_entries_const() { | |
| // Snapshot the cached `DirEntry`'s entry pointers under `entries_mutex` | |
| // (other threads rewrite the map in place under that lock), then drop | |
| // the guard: `bust_dir_cache` / the recursion below re-acquire it. | |
| let entry_ptrs: Vec<*mut Fs::Entry> = { | |
| let _entries_lock = vm.fs().fs.entries_mutex.lock_guard(); | |
| match dir_ref.get_entries_const() { |
🤖 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/api/filesystem_router.rs` around lines 389 - 394, The concurrent
reload path in `filesystem_router.rs` should not call `FileSystem::instance()`
just to reach `entries_mutex`, since that creates a process-global mutable
singleton access in a racing code path. Update the `entry_ptrs` snapshot block
to use the existing shared filesystem accessor already available in this flow,
and keep the lock-taking behavior the same so `dir_ref.get_entries_const()` is
still protected consistently by `entries_mutex`.
Source: Coding guidelines
Comment on lines
+739
to
+742
| // The router's route-load loop and Bun.build's entry-point resolution (which | ||
| // runs on the bundler thread) share the process-global directory-entry cache. | ||
| // Run in a subprocess so a crash is observable as a signal instead of taking | ||
| // down the test runner. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep these comments within the 3-line limit.
Both new explanatory comments are 4 lines. Please compress them without losing the invariant being tested. As per coding guidelines, “Keep code comments to 3 lines max.”
Also applies to: 789-792
🤖 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/bun/util/filesystem_router.test.ts` around lines 739 - 742, The
explanatory comments in the filesystem router tests are too long and need to be
compressed to fit the 3-line limit. Shorten the multi-line comments near the
subprocess setup in the filesystem router test while preserving the invariant
about the process-global directory-entry cache being shared between the
route-load loop and Bun.build entry-point resolution, and keep the reference to
running in a subprocess so a crash is observable as a signal. Apply the same
cleanup to both comment blocks identified in the test.
Source: Coding guidelines
Comment on lines
+783
to
+785
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); | ||
| expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert subprocess results as one object for crash diagnostics.
These regressions are crash/UAF-oriented; if the child dies before printing stdout, the current first assertion hides stderr/signal details. Keep all fields in one diff. Based on learnings, crash-mode subprocess tests should assert stdout, stderr, signal, and exit code together.
Suggested assertion shape
- expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true");
- expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null });
+ expect({
+ stdout: normalizeBunSnapshot(stdout, String(dir)),
+ stderr,
+ exitCode,
+ signalCode: proc.signalCode,
+ }).toEqual({
+ stdout: "matches 50 builds-ok true",
+ stderr: expect.any(String),
+ exitCode: 0,
+ signalCode: null,
+ });- expect(normalizeBunSnapshot(stdout, String(dir))).toBe("/a /b /sub/c /b");
- expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null });
+ expect({
+ stdout: normalizeBunSnapshot(stdout, String(dir)),
+ stderr,
+ exitCode,
+ signalCode: proc.signalCode,
+ }).toEqual({
+ stdout: "/a /b /sub/c /b",
+ stderr: expect.any(String),
+ exitCode: 0,
+ signalCode: null,
+ });Also applies to: 817-819
🤖 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/bun/util/filesystem_router.test.ts` around lines 783 - 785, The
subprocess assertions in the filesystem router crash-oriented tests are split,
which can hide useful failure diagnostics when the child exits unexpectedly.
Update the assertions around proc.stdout.text(), proc.stderr.text(),
proc.exited, and proc.signalCode so they are checked together as a single object
in one expect call, preserving stdout, stderr, exitCode, and signalCode in the
same diff; apply the same pattern in the other affected test block as well.
Source: Learnings
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); | ||
| expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); | ||
| }, 60_000); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the explicit per-test timeout.
This file is under test/js/bun/**, and this is not one of the documented timeout exceptions. Let the runner’s ASAN-scaled timeout budget apply. As per coding guidelines, “No timeouts” in tests; based on learnings, avoid explicit per-test timeouts in Bun/CLI tests unless the file has a documented exception.
Suggested fix
-}, 60_000);
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| }, 60_000); | |
| }); |
🤖 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/bun/util/filesystem_router.test.ts` at line 786, Remove the explicit
per-test timeout from the test case in filesystem_router.test.ts so it relies on
the runner’s default ASAN-scaled budget. Update the affected test block near the
closing of the suite to eliminate the 60_000 argument, and keep the rest of the
test logic unchanged.
Sources: Coding guidelines, Learnings
| "fixture.ts": /* ts */ ` | ||
| import path from "path"; | ||
| const pagesDir = path.join(import.meta.dir, "pages"); | ||
| await Bun.build({ entrypoints: [path.join(pagesDir, "a.tsx")], target: "bun", throw: false }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the pre-cache build actually succeeded.
With throw: false, a failed Bun.build() can be ignored and the router may still pass by scanning an uncached directory, so the test would no longer prove the pre-warmed cache path.
Suggested fix
- await Bun.build({ entrypoints: [path.join(pagesDir, "a.tsx")], target: "bun", throw: false });
+ const build = await Bun.build({ entrypoints: [path.join(pagesDir, "a.tsx")], target: "bun", throw: false });
+ if (!build.success) throw new Error("pre-cache Bun.build() failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await Bun.build({ entrypoints: [path.join(pagesDir, "a.tsx")], target: "bun", throw: false }); | |
| const build = await Bun.build({ entrypoints: [path.join(pagesDir, "a.tsx")], target: "bun", throw: false }); | |
| if (!build.success) throw new Error("pre-cache Bun.build() failed"); |
🤖 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/bun/util/filesystem_router.test.ts` at line 797, The filesystem
router test is only invoking Bun.build() with throw: false, so a build failure
could be silently ignored and the test would still pass without proving the
pre-cached path. Update the test around the Bun.build call in
filesystem_router.test.ts to explicitly assert that the pre-cache build
succeeds, using the existing build result or another success check before
continuing with the router scan.
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 4, 2026
## What
A hardening and robustness pass across the runtime: input validation,
bounds checking, protocol-state handling, and object-lifetime
correctness in ~200 files. It contains 122 individual fixes and ~190 new
tests (171 new `test`/`it` blocks, several parameterized, across 79
existing test files). No new API is introduced; every behavioral change
below has a test unless explicitly noted, and each is aligned with Node,
the relevant RFC/spec, or the upstream reference implementation.
## Potentially breaking / behavior-visible changes
Read this section first. Everything else in the PR preserves behavior
for valid inputs.
- **`Bun.serve` `request.url` is only synthesized from a structurally
valid `Host`.** For every HTTP/1.x request (`Bun.serve` and node-compat
servers alike), a `Host` value that is empty or contains bytes outside
`uri-host [":" port]` (RFC 3986 authority: alphanumerics, `.-:_~%[]` and
sub-delims) is never used as the authority of the synthesized
`request.url`; `request.url` falls back to the request target (e.g.
`/path`) and the request is still served. No request is rejected on the
basis of the `Host` field value, and a valid `Host` still round-trips
into `request.url` exactly. Why: `request.url` should never carry an
authority that cannot come back out of `new URL()`.
- **`fetch` rejects request lines it cannot legally serialize.** A URL
path/host (or, for proxied requests, the full href) containing a control
character, space, or DEL now fails with `InvalidURL` before any bytes
are written (RFC 9112 request-line grammar; normal `fetch()` input is
percent-encoded by the URL parser and unaffected). Also: a redirect
whose `Location` resolves to a non-http(s) scheme now fails with
`UnsupportedRedirectProtocol` (Fetch spec, matches undici); a `101`
arriving on the pre-tunnel leg of a proxied request is treated as an
unrequested upgrade; connections whose identity was accepted by a
per-request `checkServerIdentity` callback are never entered into or
taken from the keep-alive pool.
- **An own `__proto__` key from data files and macros is printed as a
computed key.** The `json`, `jsonc`, `json5`, `toml`, `yaml`, and
CSS-module loaders — and objects returned from Bun macros — now emit
`["__proto__"]: ...` so importing such data yields an own `__proto__`
property (like `JSON.parse`) instead of a prototype assignment. Who
notices: only code importing data with a `__proto__` key; matches
esbuild's JSON loader and Node semantics.
- **`node:url` legacy `url.parse` lookup tables no longer inherit from
`Object.prototype`** (both `node:url` and the browser fallback), the
hostless/slashed lookups use the lowercased protocol, and `url.parse(s,
true).query` is a null-prototype object (empty query included). All
three match Node's `lib/url.js` exactly. Who notices: code doing
`query.hasOwnProperty(...)` or parsing schemes named like `toString:`.
- **WebSocket client: missing negotiated subprotocol fails the
handshake.** Per RFC 6455 §4.1, if `new WebSocket(url, ["a"])` requested
subprotocols and the server's 101 omits `Sec-WebSocket-Protocol`, the
connection now closes with 1002 instead of opening with `ws.protocol ===
""`. Matches browsers, `ws`, and undici. Connections that request no
subprotocol are unaffected.
- **HTTP/2 (client and server) enforces RFC 9113 message framing.**
Trailer blocks must carry END_STREAM and no pseudo-headers;
`content-length` must be `1*DIGIT`, non-duplicated, and equal to the
DATA actually received (CONNECT exempt) — violations get
RST_STREAM(PROTOCOL_ERROR) instead of being delivered. With
`maxSessionMemory` exceeded, new peer streams are refused with
REFUSED_STREAM (retryable), and reset streams promptly release their
native state — Node/nghttp2 parity throughout. The all-streams teardown
helper now throws a `TypeError` for a non-numeric error code instead of
coercing it per stream.
- **`node:http2` HTTP/1 fallback (`allowHTTP1`) frames responses like
Node.** Header-name matching is case-insensitive; HEAD and
close-delimited responses don't get an auto `Transfer-Encoding:
chunked`/terminating chunk; `writeHead` now throws
`ERR_HTTP_INVALID_STATUS_CODE` / `ERR_INVALID_CHAR` like Node's
`ServerResponse`. Re-entrant `sendTrailers()` raises
`ERR_HTTP2_TRAILERS_ALREADY_SENT` in the same order Node does.
- **`node:http(s)` proxy `CONNECT` endpoint is validated with
`validateHeaderValue` in release builds** (previously a debug-only
assertion), so an invalid host/port surfaces as the same error Node
throws.
- **Glob: walking through a self-referential directory symlink
completes.** With `followSymlinks`, a link that resolves to one of its
own live ancestors is descended exactly once (like `find -L`, glibc
`fts`, node-glob); sibling/cousin links to the same target are still all
visited. One pre-existing test changed: it previously asserted the walk
failed with `ENAMETOOLONG` after the path grew past the limit; it now
asserts the scan completes.
- **Resolver: an `exports`/`imports` target whose expansion would exceed
the OS path limit is a normal resolution error** (`Invalid module
specifier` / `Invalid package target`, as Node models it) instead of a
hard failure.
- **Shell:** template arrays nested deeper than 100 levels throw a clear
error instead of recursing without bound; an interpolated string equal
to `if`/`then`/`elif`/`else`/`fi` is treated as data, never as a
reserved word (POSIX: reserved words are only recognized literally);
`$.escape` now quotes strings containing tab, CR, or `?` (word
delimiters / glob metacharacters).
- **`bun pack` / `bun publish` include/exclude matches npm-packlist.**
With a `"files"` field, the non-overridable defaults (`.git`, `.npmrc`,
`node_modules`, lockfiles) are now applied inside the `files` traversal
too; conversely `.hg` moved to the *overridable* default-ignore list, so
`"files"` can re-include it — exactly npm's split.
- **`bun upgrade` verifies the downloaded artifact against the digest
the GitHub Releases API reports** for that asset, and fails with a
retryable error on mismatch. If the API reports no (or an unrecognized)
digest, behavior is unchanged.
- **install:** an `integrity` string carrying several space-separated
digests (legal SSRI) is now parsed correctly and verified against the
strongest algorithm present (see Deviations); a stored `bun.lockb` with
a non-0/1 byte in a boolean slot fails validation instead of being
reinterpreted; lifecycle scripts for *registry* packages always come
from the installed `package.json` (never from lockfile bytes), matching
what Bun writes and what npm does; isolated installs apply the same
name/alias shape validation as hoisted installs; bin links reached
through a subdirectory get the same resolved-containment check the
dotted forms already had (npm only links files inside the package
folder).
- **`node:fs`:** `mode` arguments are no longer masked to `0o777`
(setuid/setgid/sticky pass through to the syscall, like Node);
`copyFile`/`cp` create the destination with the source's permission bits
(libuv parity); on Windows, `cp` copies directory junctions/symlinks via
the unprivileged-create + junction-fallback helpers and rewrites
`\\?\UNC\` targets to `\\server\share` form (libuv parity), so copying a
tree with junctions works without elevation; on macOS the
`clonefile`/`openat` paths use `NOFOLLOW` so the copy matches the
`lstat` classification (`dereference:false`).
- **Web plumbing observable from JS:** record conversion (`new
Headers(obj)`, fetch init, `URLSearchParams`, …) snapshots the key list
once and re-resolves keys mutated by a converter, exactly as Web IDL
specifies (deleted keys skipped, replaced values re-read) — released
Bun/Node/WebKit order preserved; `TextDecoder.decode` over a
`SharedArrayBuffer` or resizable buffer view snapshots the bytes first;
consuming a `Blob`/`Response` body no longer empties *other* objects
sharing the same byte store (transfer only when sole owner); deeply
nested serialized arrays in `structuredClone` data hit the same
recursion cap objects already had; the SIMD `decodeURIComponent` fast
path decodes non-ASCII input as UTF-8 (with U+FFFD for ill-formed
sequences) instead of throwing/garbling.
- **N-API / V8 API:** `napi_create_arraybuffer` returns zeroed memory
(Node contract); `napi_get_typedarray_info`/`napi_get_dataview_info`
report the view's real `byte_offset`; `v8::String::Utf8Length` returns
the exact byte count `WriteUtf8` will produce for ill-formed UTF-16;
`v8::Number::New` canonicalizes NaN payloads.
- **Dev-only endpoints check `Host`/`Origin`:** the inspector (`bun
--inspect`) HTTP/WebSocket endpoint applies its Host/Origin checks
before the `/json`* discovery routes and rejects non-matching DNS-name
`Host` values with 400 (Node inspector semantics); the bake/dev-server
internal routes require an allowed Host and same-origin for the
error-report/sourcemap endpoints; internal HMR pub/sub topics are
namespaced so user `publish`/`subscribe` topic strings can never collide
with them.
- **markdown:** reference-link expansion is charged against md4c's exact
output budget (`16 × min(input, 64 KiB)` scale); once exhausted, further
references degrade to literal bracketed text — no error — exactly as
md4c does.
- **Misc small behavior corrections:** `checkPrime` validates the
candidate before the options (Node's order); HKDF rejects non-secret
`KeyObject`s with `ERR_CRYPTO_INVALID_KEY_OBJECT_TYPE` (current Node);
Ed25519 sign/verify with a wrong-length key errors/returns false; X25519
JWK import honors `kty`/`crv`/`use`/`key_ops`/`ext`; SPKAC helpers
return false/empty for empty or whitespace-only input (Node);
`CookieMap.delete` of a `__Host-`/`__Secure-` cookie emits `Secure` so
browsers accept the expiry; postgres `escapeIdentifier` rejects embedded
NUL (`pg` parity); valkey/redis pending commands reject with "Connection
closed" on disconnect and out-of-band push frames never consume an
unrelated command's promise (RESP3); semver strings consisting only of
`v`/`=`/whitespace parse as `*` (node-semver); `Bun.wrapAnsi` measures
rows whose seam joins grapheme clusters (combining marks/ZWJ/VS16) the
way npm `wrap-ansi` does; bash/zsh completions handle script names
containing `:` and other special characters; the Docker images verify
(not just decode) the release checksum signature.
## Changes by area
- **HTTP server (uWS / `Bun.serve` / `node:http`)** —
`packages/bun-uws/HttpParser.h`, `packages/bun-usockets`,
`src/runtime/webcore/Request.rs`, `src/runtime/server`: the
`request.url` `Host` handling above (URL synthesis in `Request.rs` only
— the HTTP parser's `Host` handling is unchanged and every request is
served); CONNECT requests are framed as an opaque tunnel regardless of
`Transfer-Encoding`/`Content-Length` (RFC 9110 §9.3.6); TLS socket
relocation updates the loop's spill/last-error owner pointers so
bookkeeping never points at a moved socket; the bake-only route
additionally requires an allowed `Host`.
- **fetch / HTTP client / webcore** — `src/http/lib.rs`,
`src/http/ssl_config.rs`,
`src/runtime/webcore/{Request,Blob,TextDecoder}.rs`,
`src/jsc/bindings/webcore/*`,
`src/jsc/bindings/decodeURIComponentSIMD.cpp`, `src/url/lib.rs`,
`src/runtime/api/BunObject.rs`: everything in the highlights, plus:
`write_request` failures propagate their real error instead of being
reported as out-of-memory; partial-header/1xx short reads no longer
re-feed already-consumed bytes to the parser; `SSLConfig` now actually
applies `secureOptions` and the client-renegotiation limit/window it was
already accepting; `URLSearchParams` no longer drops a pair whose value
has a malformed percent sequence (WHATWG: never drop, decode lazily);
`AbortSignal` native listeners deregistered by an earlier abort callback
are not invoked with a stale context; the compression helpers
(`Bun.gzipSync` et al.) read the options object before coercing the
input buffer (so a getter can't invalidate the captured slice) and no
longer register a deallocator for an empty result's dangling sentinel
pointer (an invalid free at GC time under debug allocators).
- **node compat** — `src/runtime/node/*`, `src/js/node/*`,
`src/node-fallbacks/url.js`, `src/jsc/ipc.rs`, `src/runtime/socket`:
everything in the highlights, plus: `fs` path arguments from typed
arrays are always pinned for the call's duration; `BlockList`
structured-clone carries only an opaque per-instance nonce resolved
through a live table (round-trip unchanged); IPC advanced-mode frame
lengths are range-checked before span arithmetic, serialization failures
leave no partial frame in the send queue, and a received fd is closed if
its message fails to parse; TLS-upgrade `initialData` is copied to an
owned buffer before use; `node:wasi` interprets rights bitfields as
unsigned u64 (per the ABI) and no longer reports success for a
`path_open` that threw internally; the inspector/debugger endpoint
changes above.
- **HTTP/2 & WebSocket client** —
`src/runtime/api/bun/h2/connection.rs`, `h2_frame_parser.rs`,
`src/http_jsc/*`: everything in the highlights, plus: 1xx interim
responses are not misclassified as trailers; refused streams still
HPACK-decode the discarded block (RFC 9113 §4.3) and advance
`last_stream_id` so pipelined RST_STREAMs don't become connection
errors; the frame parser no longer holds an exclusive stream reference
across calls back into JS (`options`/header getters, `toString`
coercions) — engine-side stream eviction is deferred until the dispatch
unwinds; the trailers failure path still ends the stream with
FRAME_SIZE_ERROR + graceful GOAWAY so in-flight streams stay retryable;
the deflate plumbing gains a per-VM slot (no behavior change yet).
- **install / pack / bunx / upgrade / create** — `src/install/*`,
`src/runtime/cli/{pack,create,upgrade}_command.rs`, `src/semver`,
`packages/bun-release`, `packages/bun-vscode`: everything in the
highlights, plus: SSRI option suffixes (`?...`) are stripped from digest
payloads; a GitHub dependency whose resolved ref would not form a single
well-formed folder name is refused with a clear error (real refs/SHAs
always pass); the trusted-dependency lookup moved off the extraction
worker thread (no user-visible change); `bun create`'s `package.json`
rewrite uses the CLI arena so the parsed AST outlives its uses; the npm
installer package validates archive entry paths stay inside the
destination; the VS Code lockfile preview escapes interpolated text and
the debug adapter's session id comes from `crypto.randomBytes`.
- **resolver / bundler / parsers / macros / sourcemap / markdown** —
`src/resolver`, `src/bundler`, `src/parsers/{json,json5,yaml}.rs`,
`src/ast/e.rs`, `src/js_parser/lexer.rs`, `src/js_parser_jsc/Macro.rs`,
`src/jsc/RuntimeTranspilerStore.rs`, `src/jsc/bindings/BunPlugin.cpp`,
`src/sourcemap`, `src/md`, `src/paths`, `src/standalone_graph`: the
`__proto__`, resolver-limit, and markdown items above, plus: a
native-plugin `onLoad` source buffer now has exactly one owner (its free
callback was registered twice); `onResolve` callback lists are
snapshotted (GC-visible) before user callbacks run, so a callback
registering more plugins can't perturb the in-progress dispatch;
barrel-import scheduling copies its alias seeds instead of holding
references into a map the BFS mutates; embedded bytecode caches are
handed to `ResolvedSource` as a genuinely owned allocation; the lazy
sourcemap decompression cache became `OnceLock`-based (shared-reference
safe); the lexer's SIMD long-string fast path advances past scanned
bytes (removes a quadratic re-scan on unterminated literals);
`is_parent_or_equal` uses a true prefix check instead of substring
containment.
- **shell / glob / CLI / wrapAnsi** — `src/shell_parser`,
`src/runtime/shell`, `src/glob/GlobWalker.rs`,
`completions/bun.{bash,zsh}`, `src/jsc/bindings/wrapAnsi.cpp`: the
highlights above; the glob walker's followed-link tracking is scoped to
the live ancestor chain (DAG revisits still enumerate); `wrapAnsi` also
caches row widths so the seam fix comes with fewer full-row rescans.
- **crypto** —
`src/jsc/bindings/{ncrypto.cpp,node/crypto/*,webcrypto/*}`: the
highlights above, plus: the sign/verify job copies signature bytes out
of the JS view before the async job runs; `ECDH.convertKey` and
`prepareAsymmetricKey` capture buffer spans only after argument
coercions that can run user JS; deserialized `CryptoKey`s re-validate
that the algorithm matches the key class (and an empty key payload is
rejected); an OOM while encoding returns after throwing.
- **sql / valkey / s3** — `src/sql`, `src/sql_jsc`,
`src/js/internal/sql`, `src/runtime/valkey_jsc`, `src/valkey`,
`src/s3_signing`: the highlights above, plus: postgres `CopyData`
payload length is computed per the wire protocol (was one byte short)
and `PortalSuspended`/`Copy*` messages are consumed instead of
desynchronizing the stream; MySQL zero-length
`AuthSwitchRequest`/`LocalInfileRequest` packets are clean protocol
errors instead of a length underflow; prepared-statement caches are
keyed by the full statement name, not a 64-bit hash (a hash hit is
verified by equality); the distributed-transaction name is type-checked;
the S3 region used to synthesize a host must be host-safe and endpoint
parsing is index-safe on odd endpoint strings.
- **JSC bindings / N-API / V8 / sqlite / misc** —
`src/jsc/bindings/{napi.cpp,v8/*,ZigException.cpp,ZigGlobalObject.cpp,CookieMap.cpp,sqlite/JSSQLStatement.cpp}`,
`src/jsc/rare_data.rs`, `src/runtime/webview`: the N-API/V8 items above;
stack-trace population skips out-of-range frame indices instead of
asserting; the native microtask trampoline is hidden from stack traces;
`bun:sqlite` detects a database closed re-entrantly from inside a bind
coercion and throws "Database has closed"; the macOS webview bridge
type-checks the objects a page posts to its internal message handler.
- **dev server / bake / build & CI** — `src/runtime/bake/*`,
`dockerhub/*`, `.github/workflows/update-vendor.yml`: the dev-endpoint
gating and HMR topic namespacing above; the dev-server terminal error
report blanks non-UTF-8 bytes (not just encoded C1); the React SSR
flight-data inliner escapes the fully decoded string instead of
per-chunk (a boundary-split escape character could previously produce a
malformed inline script); Docker images use `gpg --verify`; the
vendor-update workflow passes matrix values through `env:`.
## Deviations / decisions
- **Integrity: strongest-single-digest verification.** When an
`integrity` field carries several digests, Bun verifies the strongest
supported algorithm present; when several digests of that same algorithm
are present, the first is the one verified. npm/ssri accepts a match on
*any* digest of the chosen algorithm — keeping the full set needs
plumbing through the manifest cache/lockfile types and is left for a
follow-up.
- **No response-decompression size cap in `fetch`.** A per-response
decompressed-body limit was implemented and then deliberately removed
from this PR ("Keep fetch response decompression unbounded"): Node
imposes none, and any cap is a behavior change for legitimate large
responses. The net diff has no decompression change.
- **markdown reference expansion degrades instead of erroring**,
matching md4c exactly (see highlights). No error is ever surfaced.
- **Record conversion keeps the specification's per-property order** —
`[[GetOwnProperty]]`/`Get` interleaved with value conversion, so a
`toString` that mutates a sibling property is observed and a deleted one
skipped, the same as released Bun, Node, and WebKit. The property table
is never held across user code.
- Dead code removed because these changes made it unreachable:
`cache::Entry.external_free_function` (plus `Entry::new` and the free
branch of `Entry::deinit`) and `AlreadyBundled::bytecode_slice`; the
`bun_wyhash` dependency of `sql_jsc` is dropped.
- Deeper, behavior-visible sibling work in the same areas was split into
its own PRs so it can be reviewed on its own terms: #33054 (`node:https`
TLS option set), #33055 (websocket dispatch re-entrancy), #33056
(FileSystemRouter/resolver entry locking), #33060 (`child_process`
uid/gid), #33061 (`node:http` server headers/request timeouts). Nothing
from those PRs is claimed here.
- The changes to CI workflow files, shell completion scripts,
Dockerfiles, and the VS Code extension have no automated-test harness; a
few lifetime/ordering corrections have no deterministic observation from
JS (they are covered by the existing suites and the sanitizer jobs) and
are noted as such instead of shipping a non-asserting test.
## How it was verified
- `bun bd` (full debug build) and `cargo check` clean with zero
warnings; `bun run rust:check-all` passes 10/10 targets (the change set
includes Windows- and macOS-gated code); clippy lints raised on the
touched files were addressed.
- ~190 new regression tests in existing test files (171 `test`/`it`
blocks across 79 files, several parameterized). Each was verified to
fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test
<file>` (except the handful noted above with no JS-observable
assertion); the touched suites were run in full to confirm no
regressions.
- HTTP/2 changes were exercised against a dedicated h2 conformance suite
(`test/js/node/http2/h2-conformance.test.ts`) and Node's own http2
tests; the `node:http` Host behavior was checked against Node's
conformance test for accepted host values; the record-conversion
ordering was checked against WebKit/Node observable order.
- rustfmt / clang-format / prettier / oxlint clean over the changed
files.
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Jul 15, 2026
Collaborator
|
A remaining race on this path segfaulted once on alpine x64 in build 73276 (same test this PR added). |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 15, 2026
…ry rewrite (#34271) `test/js/bun/util/filesystem_router.test.ts` went red on alpine x64 in build [73276](https://buildkite.com/bun/bun/builds/73276): the `reload() while Bun.build() resolves the same directory` subprocess segfaulted in `bust_dir_cache_recursive`, inlined from `NonNull::new`. ## Cause `RealFS::entries_at` (`src/resolver/lib.rs`) replaces a cached `DirEntry` in place when the caller's resolver generation is newer than the cached listing's. The replacement at `*e_ptr = new_entry` drops the old `DirEntry`, which drops its `data: StringHashMap<*mut Entry>` and frees the hashmap's bucket allocation. The function's comment says `entries_mutex held by caller`, but that is only true on one of the five paths that reach it: `dir_info_uncached`, when entered from `dir_info_cached_miss`. The other callers (`finalize_result`, `handle_esm_resolution`, `load_index_with_extension`, `Transpiler::run_env_loader`) all reach `entries_at` after `dir_info_cached_maybe_log` has already returned and released both `RESOLVER_MUTEX` and `entries_mutex`. `FileSystemRouter::reload()` and `RouteLoader::load` iterate the same `DirEntry.data` map under `entries_mutex` (the snapshot pattern #33056 introduced for exactly this kind of concurrent rewrite). With `entries_at`'s rewrite unsynchronized, a `Bun.build()` on the bundler thread can drop the map while `reload()` on the JS thread is mid-iteration. The generation mismatch is what makes `entries_at` enter its rewrite branch, so the window only opens once the bundle thread has processed at least one batch (it bumps its own generation after every queue drain); every subsequent `Bun.build()` then re-reads any directory that `reload()` just refreshed to generation 0. ASAN catches it as a heap-use-after-free with the two sides of the race laid out exactly: ``` READ of size 16 (thread T0): #6 HashMap::values #7 StringHashMap<*mut Entry>::values src/collections/array_hash_map.rs:1864 #8 FileSystemRouter::bust_dir_cache_recursive src/runtime/api/filesystem_router.rs:395 #9 FileSystemRouter::bust_dir_cache src/runtime/api/filesystem_router.rs:451 #10 FileSystemRouter::reload src/runtime/api/filesystem_router.rs:476 freed by thread T11 (Bundler): #11 drop_in_place<bun_resolver::fs_full::DirEntry> #12 bun_resolver::fs::RealFS::entries_at src/resolver/lib.rs:1639 #13 DirInfo::get_entries_ref src/resolver/dir_info.rs:266 #14 Resolver::finalize_result src/resolver/resolver.rs:1714 #15 Resolver::resolve_and_auto_install src/resolver/resolver.rs:1485 ... #23 BundleThread::generate_in_new_thread src/bundler/BundleThread.rs:276 previously allocated by thread T0: #17 HashMap::reserve #18 Resolver::dir_info_cached_miss src/resolver/resolver.rs:4591 #19 Resolver::dir_info_cached_maybe_log src/resolver/resolver.rs:4201 #20 Resolver::read_dir_info src/resolver/resolver.rs:4118 #21 FileSystemRouter::reload src/runtime/api/filesystem_router.rs:492 ``` (The use side is sometimes `RouteLoader::load` at `src/router/lib.rs:816` instead; same map, same lock.) This has been the shape of `entries_at` since the Rust port; #33056 narrowed the race by snapshotting under the lock but assumed the rewrite side already held it. ## Fix `entries_at` now takes `entries_mutex` itself, matching `read_directory_with_iterator` which already does. The one call path that reaches it with the lock already held (`dir_info_cached_miss` -> `dir_info_uncached` -> `parent_.get_entries_ref`) routes through a new `entries_at_locked` / `get_entries_ref_locked` pair so the non-recursive mutex is not re-entered. That path is the only one that passes a non-`None` parent to `dir_info_uncached`; the other caller (`dir_info_for_resolution`) passes `None`, so the parent branch containing the accessor never runs there. ## Test The existing concurrency test now awaits one `Bun.build()` first, so the bundle thread's generation is already past zero when the concurrent rounds start, and then runs forty reload/build rounds instead of one. That is the shape that reaches the stale-generation rewrite at all; the original single-round fixture usually completes with every build still on generation 0. The race is scheduling-dependent. Pinning the fixture to a single core reproduces the ASAN use-after-free on roughly 3 in 10 runs against an unpatched debug build and 0 in 15 with this change; with all 16 cores available the unpatched build reproduces at roughly 1 in 30. The assertions are otherwise the same as before, so the test continues to cover the behavior #33056 added. Also ran the full `filesystem_router.test.ts`, `test/bundler/bun-build-api.test.ts` (including the thousands-of-builds test that exercises the generation path heavily), `test/js/bun/resolve/resolve.test.ts`, `test/cli/hot/hot.test.ts`, `test/cli/watch/watch.test.ts`, `test/bake/framework-router.test.ts`, and `bun run rust:check-all` (10/10 targets). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts <!-- robobun:evidence:end -->
This was referenced Jul 24, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The filesystem-router load loops (
Bun.FileSystemRouter,bake'sFrameworkRouter,bust_dir_cache_recursive) iterate the resolver's cachedDirEntryhashmap, andEntry's lazily-populated stat cache (need_stat/cacheCells) is rewritten from several places. Both happened without the locks the rest of the cache uses, so arouter.reload()on the JS thread concurrent withBun.build()(whose entry-point resolution runs on the bundler thread and rewrites the same cachedDirEntryin place underentries_mutex) reliably crashes:Calling
reload()on aBun.FileSystemRouterwhile a concurrentBun.build()re-reads the samedirectory makes the router iterate a cached directory-entry map that the bundler thread is
rewriting in place under its own lock; the route-load loop then reads hashmap buckets that were
just freed and crashes. The new test reproduces this deterministically.
Changes, using only the two locks that already exist:
entries_mutex. Each load/bust/scan loop now copies theDirEntry's*mut Entrylist while holding the existingRealFS.entries_mutex(the same lock the in-place rewrite holds), then drops the guard before the loop body — theread_dir_inforecursion re-acquires it, so the guard is never held across it.Entryslots live in the process-lifetimeEntryStore, so the snapshotted pointers stay valid.Entry.mutexon every rewrite.Entry::kind/Entry::symlinktakeEntry.mutex(double-checked; the cached fast path is untouched) inside the lazy-stat branch, and every otherEntryrewrite site (Route::parse'sset_abs_path, the resolver'sset_cache_fd/set_cache_symlinkcallers, the hot reloader's fd reset) takes the same lock — matching what the directory re-read path (add_entry_with_store) already did. Lock order is alwaysentries_mutex→Entry.mutex; no critical section underEntry.mutexacquiresentries_mutex.DirEntryrefresh now reuses the slot's existing interneddirinstead of re-interning the caller's spelling: the resolver and the router spell the same directory with and without a trailing slash, so the old code rewroteEntry.dir(a 2-word slice) to a different value under unlocked readers. The route loader also now accepts both spellings —Bun.build()followed bynew Bun.FileSystemRouter()on the same directory tripped an out-of-bounds index in a debug assertion 100% deterministically (debug builds only; release behavior was unaffected because both spellings trim to the samepublic_dir).entries_mutexserialized allEntryaccess.Intentionally unchanged, called out for review:
cache(),base(),dir(),abs_path()) stay lock-free per the existing design; the rewrite sites now always write back the same or a freshly-published value, but this is not full read-side synchronization.dir_entry_accessor::DirEntryDirIteralso iterates aDirEntrymap without the lock; its only caller is the single-threaded CLI--filterglob, so it was left alone.Tests
Two new subprocess tests in
test/js/bun/util/filesystem_router.test.ts:reload() while Bun.build() resolves the same directory— fails (segfault, empty stdout) withUSE_SYSTEM_BUN=1, passes with the fix (30/30 repeated runs of the fixture on a debug build).loads routes from a directory already cached by Bun.build()— the deterministic sequential repro of the trailing-slash assertion; it only crashes on debug builds of main (the broken assertion isdebug_assert!-gated), so it passes underUSE_SYSTEM_BUN=1but fails on an unfixedbun bd testrun.Also ran the full
filesystem_router.test.ts,test/bake/framework-router.test.ts,test/bundler/bun-build-api.test.ts,test/cli/hot/hot.test.ts,test/cli/watch/watch.test.ts, andbun run rust:check-all(10/10 targets).