Repository navigation
Conversation
The constructor replaced `config.dir` with the resolver's path for the directory: the real path when `dir` is or passes through a symlink, else the spelling the resolver cached first. `reload()` evicted, read and named routes against that path. After a link above `dir` moved it still listed the old target, after the old target was deleted it threw for a path the caller never passed, and a path without a trailing separator made it loop forever on an index route. The Zig constructor had the same statement, but it wrote the local `router` after the struct had been copied into the router object (4f41c3f), so it never took effect. The port made it live. Delete the statement. `config.dir` stays the path `Router::init` got.
|
Updated 4:05 PM PT - Oct 16th, 2026
❌ @robobun, your commit 741871a has 1 failures in 🧪 To try this PR locally: bunx bun-pr 45584That installs a local version of the PR into your bun-45584 --bun |
StatusReproduced on the released 1.4.3-canary (367d939) and on a debug build of main 21620e2. With Each case is one of the seven new tests in |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe filesystem router now keeps its configured directory path after loading. New subprocess tests cover reload behavior when symlink targets change or disappear, route retention after resolution failures, and route discovery after files are added. ChangesFilesystem router reload
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that dropping the Box::from(base_dir.leak()) statement leaves base_dir's refcount balanced: claim takes the +1 at src/runtime/api/filesystem_router.rs:317 and finalize releases it at line 671, and leak() only borrowed the bytes for the copy, so the deleted block never touched the count.
Extended reasoning...
The change deletes one post-construction statement in src/runtime/api/filesystem_router.rs so config.dir keeps the caller's normalized path instead of the resolver's realpath, and adds seven subprocess tests in test/js/bun/util/filesystem_router.test.ts. It touches no security-sensitive surface. The inline finding (reload() not refreshing base_dir, so match().src stays relative to the old real path) is why a human should weigh whether that pre-existing gap should land with this PR or as the follow-up the description proposes.
| .router | ||
| .with_mut(|r| r.config.dir = Box::from(base_dir.leak())); | ||
|
|
||
| Ok(fs_router) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing (partial fix): after a release link is repointed and reload() runs, match().src still names the old real path, so asset URLs built from src break. reload() installs the new router at src/runtime/api/filesystem_router.rs:502 but never refreshes base_dir, the prefix get_script_src strips. Fix: reload() should re-derive base_dir from the directory it just read (deref the old +1, claim the new), using a fresh realpath rather than root_dir_info.abs_real_path, which inherits the unbusted parent's stale value. The PR calls this unchanged; the base had stale routes too, so fresh routes with a stale src is a new mixed state.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: dir written through a release link (current -> v1, later current -> v2), a FileSystemRouter constructed before the flip, then reload() and match("/new").src. base_dir is claimed once in the constructor at src/runtime/api/filesystem_router.rs:317 from root_dir_info.abs_real_path (/root/v1/pages). reload() at filesystem_router.rs:453-503 reads config.dir again and replaces the router and arena but leaves this.base_dir untouched. match() hands this.base_dir to MatchedRoute at filesystem_router.rs:616, and get_script_src at filesystem_router.rs:898-913 passes it as dir to get_public_path_with_asset_prefix (src/runtime/api/BunObject.rs:20-62). The route's file_path is now the fresh real path /root/v2/pages/new.tsx, which does not start with /root/v1/pages, so src becomes "../../v2/pages/new.tsx" with no origin, or "/abs:/root/v2/pages/new.tsx" with an origin, instead of "new.tsx" under the assetPrefix. On the base branch reload re-read the stale /root/v1/pages, so src and routes were consistently stale; after this change routes and filePath are fresh but src is not.
Verification: pre-existing (partial fix); acknowledged in diff: the PR description states "reload() does not refresh base_dir" and that bound is accurate per the code. reload() (src/runtime/api/filesystem_router.rs:425-507) installs the new router at lines 502-503 but never touches this.base_dir, so match().src is relative to the first real path. On the base, /new never matched at all. The new tests do not assert src after reload.
Problem
dirwritten through a symlink,reload()lists the old target after the link moves, and throwsUnable to find directory: <old real path>when that target is gone.Bun.resolveSyncfromdirplus a router over a subdirectory does it without a symlink. A debug build aborts:assertion failed: bun_paths::resolve_path::is_sep_any(entry_dir[base_dir.len() - 1]).config.dirwith the resolver's path (src/runtime/api/filesystem_router.rs:330). In 1.3.14 that statement wrote a stack copy and did nothing. The port made it live. No user has reported this.Fix
reload()again evicts, reads and names routes againstdiras written.test/js/bun/util/filesystem_router.test.tsfail on 1.4.3-canary and on main. All 44 in the file pass here.Background
config.diris the directoryreload()reads. The resolver's path is the real path under a symlink, else the spelling cached first.config.dir's length off each entry's directory, so another path gives wrong names.Downsides
reload()is unchanged (35).match().srcstays relative to the first real path after a link moves.Notes
Root cause
bun-v1.3.14,
src/runtime/api/filesystem_router.zig, end ofconstructor:The second line has written a stack copy since 4f41c3f (2022-11-22), so
fs_router.router.config.dirstayed the path the constructor was given. The port (23427db) writes the router object itself:base_dirisabs_real_pathwhen the resolver has one, elseabs_path.reload()usesconfig.dirthree times: to evict the directory cache, to read the directory, and as the prefix the loader slices off.Who can hit it
No user has reported this. It was found by automated testing. A project under a symlinked ancestor that passes its real path as
diris not affected. One of these has to be true, and thenreload()has to be called:diris written through a symlink (current/pages, withcurrenta release link), or the routes directory is itself a symlink.dirfailed earlier in the process, and a router over a subdirectory ofdirwas made.Repro
367d939d9reload()/news/old/newreload()/newUnable to find directory: ROOT/v1/pages/newreload()Unable to find directory: ROOT/current/pages/s/olddirunder or being a symlink, index route,reload()reload()reload(), file added under a symlinkeddir/a /b/a/a /bTests
Each of the seven tests runs one fixture in its own process, because the resolver's directory cache lives as long as the process. The link is made before that process starts. A link made after the resolver has cached its parent directory is not seen as a link, and the bug does not show. Two of the fixtures (
dirthrough a symlink, and the failed resolve) come from #41166.USE_SYSTEM_BUN=1(1.4.3-canary367d939d9): 7 of 7 fail. Four print wrong output or throw, three never return.21620e2be3: 7 of 7 fail. Six abort with the assertion above, one throws forROOT/v1/pages. The other 37 pass.Measurements (Linux x64)
Allocator calls per call, debug build,
ASAN_OPTIONS=atexit=1:print_stats=1,BUN_JSC_useJIT=0. Each number is the count for 600 calls minus the count for 300, divided by 300. Two runs gave identical counts.reload()reload()The statement allocated a copy of the path and freed the one it replaced. Instruction counts and binary size were not measured for this revision. It deletes one statement and adds no code.
Parity with 1.3.14, one case per process: 21 of 21 output lines equal. The cases are those above, a new router and
srcafter a link move, a link that becomes a plain directory, and plain-directory controls (trailing and doubled separators, a relativedirafterchdir, a deleted directory). 1.4.3-canary differs in 12 of the 21.Not changed, as on 1.3.14
reload()does not refreshbase_dir. After a link moves,match().srcis relative to the first real path (../../v2/pages/new.tsx).filePathis right.reload()in between, lists the old target.entry.dir()atconfig.dir.len() - 1. Its debug assertions hold the premise, and they now catch a re-added overwrite.reload()ignores route-parse errors that a constructor throws for (FileSystemRouter: surface route-load errors from reload() like the constructor does #34614).#41166
#41166 rewrote the loader to build route names from the directory walk. It took the slice for the cause. With this change its two tests pass without its loader change. It keeps the overwrite, so
reload()still reads the old link target there. It is still open. Once this PR merges, #41166 is loader hardening with no failing test, and it should not merge before this one.After this change
base_diralways equalsconfig.dirandroute_dirname_lenis always 0 in the loader. Removing them is a possible follow-up. It is not part of this PR.no test proof · iteration 4 · 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