resolver: strip the trailing slash before busting the dir cache for a specifier - #39211
resolver: strip the trailing slash before busting the dir cache for a specifier#39211robobun wants to merge 5 commits into
Conversation
… specifier bust_dir_cache_from_specifier passed the joined path (or an absolute specifier) straight to bust_dir_cache. A specifier ending in a slash keeps that slash through the join, so the key failed the cache key assertion and aborted the dev server and bun build --watch in builds with assertions enabled; release builds silently missed the cache. Normalize the key the same way the resolver's lookup sites do, so the bust hits the entries the failed resolution populated.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe resolver now shares trailing-separator cache invalidation for absolute paths. Development-server and watch-mode tests cover unresolved relative and absolute imports, continued process activity, and successful rebuilds after source changes. ChangesTrailing-separator resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix and tests pushed (latest d693207), waiting on CI. Reproduced on a debug build of main (2f941ed) with Tests: Current shape (after self-review): the trailing separator is stripped inside Related: #39203 touches the same function for a different failure (specifier longer than the path buffer); the two changes are independent. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix applies the same without_trailing_slash_windows_path normalization the assertion's own doc comment names and that the other cache-key sites in this file already use, and both branches now share one helper. The inline nit is pre-existing behavior on pathological doubled-separator specifiers, not a regression.
Extended reasoning...
Overview
Normalizes the cache keys bust_dir_cache_from_specifier derives from a user-written import specifier by stripping the trailing separator before calling bust_dir_cache. The absolute and relative branches are folded into a new bust_dir_cache_and_parent helper (net ~12 lines in src/resolver/resolver.rs). Two tests: a bun build --watch test in test/bundler/cli.test.ts covering both relative and absolute trailing-slash specifiers via test.each, and a dev-server test in test/bake/dev/bundle.test.ts. Both assert the resolution error is reported and that the process survives to rebuild after the entry is fixed.
Security risks
None. This is path normalization for an internal directory-cache key; the input is an import specifier that already goes through the resolver, and the change only strips a trailing separator before a map lookup/removal.
Level of scrutiny
Low-to-medium. The fix is a one-line normalization at the layer that owns the invariant, using strings::without_trailing_slash_windows_path — the exact helper assert_valid_cache_key's doc comment names and the same one used at the three other cache-key sites in this file (resolver.rs:3332, :4248, :5806). The refactor into bust_dir_cache_and_parent is behavior-preserving for inputs without a trailing slash (identical call sequence) and correcting for inputs with one. On Windows it also fixes which parent gets busted (dirname of C:\x\missing\ was returning the same directory), which the PR description calls out.
Other factors
- Tests await observable conditions (
outputUntilreads the stream until the needle appears and throws with accumulated output on early end; the dev test uses the harness'sexpectReload/expectMessage), and verify the watcher/server survives by triggering and asserting a subsequent rebuild — not just "no panic in output". - The single finding is a Nit about pre-existing pathological inputs (
/tmp//missing,C://) that panicked identically before this PR; it's a same-class hardening suggestion, explicitly non-blocking. - Checked that the first
outputUntil(proc.stderr, ...)while stdout is unread cannot deadlock: the failing build writes nothing to stdout, so the pipe buffer stays empty until the second write triggers the rebuild. - No CODEOWNERS for these paths; PR description notes the independent overlap with #39203 in the same function.
dirname of an absolute specifier with a doubled separator (/a//hello) is /a/, which is not a valid cache key either. Mirrors load_as_file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Addressed the review: 3d4e505 also normalizes the parent key (covers absolute specifiers with a doubled separator, now a third row in the cli.test.ts block) and 7689109 trims the helper's doc comment. The Windows drive-root quirk in the shared helper is out of scope here and being handled separately; see the PR description. |
There was a problem hiding this comment.
LGTM — the follow-up commits addressed both prior review threads (parent key now normalized via without_trailing_slash_windows_path, doc comment trimmed).
What was reviewed:
bust_dir_cache_and_parentnormalizes both keys the same way as the sibling site atresolver.rs:5808; drive roots and/are preserved by the helper.- The doubled-separator absolute case from the earlier review is now covered and has a
test.eachrow; the remaining WindowsC:\\\\case lives in the shared helper and is scoped out with rationale. - Tests await observable output (no sleeps), use
await usingfor the watch process, and verify the watcher survives and rebuilds.
Extended reasoning...
Overview
The PR fixes a debug/ASAN-only assertion abort in Resolver::bust_dir_cache_from_specifier when a bundled file has an unresolvable import whose specifier ends in (or, for absolute paths, contains a doubled) path separator. The fix extracts a shared helper bust_dir_cache_and_parent used by both the absolute and relative branches, and passes both the path key and its dirname result through strings::without_trailing_slash_windows_path — the same normalization every other cache-key site in this file already applies. Net Rust change is ~10 lines. Tests are added to both entry points that reach this code: bun build --watch (three specifier shapes via test.each) and the dev server (test/bake/dev/bundle.test.ts, mirroring the neighboring "importing a file before it is created" test).
Security risks
None. This is cache-key string normalization on the failed-resolution recovery path; no new inputs are trusted, no filesystem writes are gated on it, and the assertion it satisfies is a debug-only internal invariant check.
Level of scrutiny
Low-to-moderate. The Rust change is small, mechanical, and converges on the exact pattern already used at resolver.rs:5808 (load_as_file's dirname handling). The only behavioral change is that keys previously passed with a trailing separator are now stripped — which is what the cache expects, so in release builds this turns silent cache misses into hits. Nothing else calls the new helper.
Other factors
My earlier review flagged two same-class edge cases. The author addressed the actionable one (POSIX doubled-separator dirname) in 3d4e505 exactly as suggested, added a test row for it, and documented why the second (Windows C:\\\\ from root_len + 1 in the shared string_paths.rs helper) belongs in a separate change against that helper — I agree with that scoping. The comment-cop feedback on the doc comment was also addressed (7689109). Both review threads are resolved. The tests follow harness conventions (tempDir, bunEnv, await using, test.each, awaiting stream output rather than sleeping) and each asserts the process stays alive and rebuilds after the entry is fixed, so they can fail for the right reason on debug/ASAN lanes while passing on release.
Every lookup and populate site strips the trailing separator before asserting; make the bust side do the same so no caller has to.
|
Restructured in d693207 after a self-review pass: the strip now lives inside bust_dir_cache (the lookup sites read_directory_with_iterator, dir_info_for_resolution and dir_info_cached already strip before asserting; the bust side was the one that did not), and bust_dir_cache_from_specifier only keeps the strip that has to happen before the dirname for Windows. The description was also corrected: in release builds on POSIX the maps already trim trailing separators when hashing (BSSMapInner::key_hash), so this is an assertion-build fix there, with a Windows-only parent-directory correction on top. Tests are unchanged. |
There was a problem hiding this comment.
LGTM — my earlier review feedback (normalize the parent key too) was addressed, and the follow-up commit moves the strip into bust_dir_cache itself so every caller is covered.
What was reviewed:
- Traced the doubled-separator absolute case (
/tmp//missing) throughbust_dir_cache_and_parent→dirname_platformreturns/tmp/, now stripped insidebust_dir_cachebefore the assertion. - Checked all other
bust_dir_cachecall sites (hot_reloader, dev_server, filesystem_router, transpiler, VirtualMachine) — they already wrap withwithout_trailing_slash_windows_path, so the added strip is idempotent for them and newly coversincremental_graph.rs:1562anddev_server/mod.rs:461. - Tests await observable output (
outputUntilpolls the pipe, throws with the crash report if the process dies), and the dev-server test mirrors the neighboring "importing a file before it is created" case.
Extended reasoning...
Overview
Fixes a debug/ASAN-only assertion abort in Resolver::bust_dir_cache_from_specifier when a bundled file has an unresolvable import whose specifier ends in a slash (./missing/, /abs/dir/) or, for absolute specifiers, contains a doubled separator. The fix is ~15 lines in src/resolver/resolver.rs: both branches of bust_dir_cache_from_specifier now route through a shared bust_dir_cache_and_parent helper that strips the trailing separator before taking the dirname, and bust_dir_cache itself now strips before asserting. Three test.each rows in test/bundler/cli.test.ts cover bun build --watch, and one devTest in test/bake/dev/bundle.test.ts covers the dev server.
Security risks
None. This is path-string normalization for an internal cache-key lookup; no auth, no external I/O, no data exposure surface. The input is a user-written import specifier that was already being processed — the change only affects which key is used to remove an entry from an in-memory map.
Level of scrutiny
Low-medium. The mechanism is well-understood (the assertion doc comment names without_trailing_slash_windows_path as the canonical normalizer, and this PR applies exactly that). The change is additive normalization at the same layer as the assertion it satisfies; without_trailing_slash_windows_path is idempotent, so existing callers that already normalize are unaffected. The two callers that previously did not normalize (incremental_graph.rs and dev_server/mod.rs) now get it for free, which is strictly safer.
Other factors
- I previously flagged two residual edge cases in the same class. Case 1 (POSIX doubled internal separator) is now covered by normalizing inside
bust_dir_cache. Case 2 (WindowsC:\\\\drive-root off-by-one in the shared helper) is explicitly and reasonably deferred to a separate fix insrc/paths/string_paths.rs. - The comment-cop flag on the long doc comment was addressed in 7689109, and d693207 went further by moving the normalization into
bust_dir_cache(fixing the code rather than commenting the workaround). - CI passed on 7689109; d693207 on top is a one-line relocation of the strip call to a strictly-earlier point in the same call chain.
- Tests follow harness conventions:
tempDir,bunEnv,await using proc, poll for observable output rather than sleep, and theoutputUntilhelper surfaces the crash report in the failure message so a regression is diagnosable from CI output alone.
There was a problem hiding this comment.
LGTM — the restructure in d693207 (stripping inside bust_dir_cache itself, mirroring the lookup sites) addresses the doubled-separator case I flagged earlier, and the third test.each row covers it. The remaining Windows C:// edge case is correctly deferred to the shared helper.
What was reviewed:
- Traced
/tmp//missing→dirname_platform→"/tmp/"through the newbust_dir_cachestrip — assertion now passes. - Checked that moving the strip into
bust_dir_cacheis safe for existing callers (they already pre-strip; now redundant, not wrong). - Tests:
outputUntilawaits the actual stream output and errors with the crash report if the process dies; no sleeps;describe.concurrentfor the three subprocess variants.
Extended reasoning...
Overview
~15 net Rust lines in src/resolver/resolver.rs: bust_dir_cache now calls strings::without_trailing_slash_windows_path before assert_valid_cache_key (matching read_directory_with_iterator, dir_info_for_resolution, and dir_info_cached), and bust_dir_cache_from_specifier factors its two branches into a shared bust_dir_cache_and_parent helper that strips before the dirname (needed on Windows where dirname of a\\hello\\ is a\\hello, not a). Two test additions: a three-row test.each in test/bundler/cli.test.ts for bun build --watch with trailing-slash / doubled-separator specifiers, and a dev-server variant in test/bake/dev/bundle.test.ts.
Prior review resolution
My earlier review flagged two same-class edge cases. Case 1 (POSIX /tmp//missing → dirname returns /tmp/ with trailing slash) is now handled because the strip lives inside bust_dir_cache — I re-traced the path and the "/tmp/" result of dirname_platform is stripped to "/tmp" before the assertion. The third test row exercises exactly this. Case 2 (Windows C:// → root_len + 1 off-by-one in without_trailing_slash_windows_path) is a bug in the shared helper at src/paths/string_paths.rs used at 20+ sites; the PR description and author reply both defer it to a separate fix, which I'd already agreed was the right scope. The comment-cop paragraph-comment flag was addressed by trimming to one line. All inline threads are resolved.
Security risks
None. This is normalization of an internal cache-invalidation key derived from a user-written import specifier. On POSIX release builds nothing observable changes (the maps already hash with trailing separators trimmed via REMOVE_TRAILING_SLASHES); on debug/ASAN builds an assertion abort becomes a normal resolution error; on Windows release builds the correct parent directory is now busted for a trailing-slash specifier. No filesystem writes, no path traversal surface — the key is only used to remove entries from two in-memory maps.
Level of scrutiny
Low-to-medium. The Rust change is small, uses the exact helper the assertion's own doc comment names, and moves the fix to the layer that owns the invariant (bust_dir_cache) rather than one call site — REVIEW.md's "fix the whole class" guidance. Existing callers that already strip become harmlessly redundant. No CODEOWNERS entry covers src/resolver/.
Other factors
Tests follow harness conventions: tempDir, bunEnv/bunExe(), no sleeps — outputUntil polls the stream for the actual output and throws (with the accumulated crash report as the message) if the process dies before the needle appears, so the failure mode is diagnosable. describe.concurrent keeps the three subprocess variants parallel. The dev-server test mirrors the neighboring "importing a file before it is created" test with only the specifier changed. The PR description is thorough and correctly characterizes the per-build-type effect after the author's self-correction.
1 participant
Problem
bun build --watchabort when a bundled file has an unresolvable import whose specifier ends in a slash, such asimport "./missing/"orimport "/abs/dir/":error: Could not resolve: "./missing/", with the watcher or dev server staying up.Resolver::bust_dir_cache_from_specifier(src/resolver/resolver.rs:2466), which the bundler calls on everyModuleNotFoundwhile a watcher or dev server is attached, passed its keys tobust_dir_cacheas is.join_abspreserves a trailing separator, so./missing/became the key/tmp/repro/missing/; the absolute branch passed the user's specifier verbatim; and an absolute specifier with a doubled separator (/tmp//missing) has the dirname/tmp/.bust_dir_cacheasserted on each of these without normalizing first.bust_dir_cachewas the only one of the resolver's cache key sites that asserted without normalizing:read_directory_with_iterator(src/resolver/lib.rs:1220),dir_info_for_resolution(resolver.rs:3331) anddir_info_cached(resolver.rs:4247) all strip the trailing separator and then assert, so every other caller ofbust_dir_cache(hot reloader, VM resolve retry, dev server, FileSystemRouter, transpiler) open-codes the strip at its call site, and this caller was the one that did not.Fix
bust_dir_cachenow strips the trailing separator itself (strings::without_trailing_slash_windows_path, the helper the assertion's doc comment names) before asserting, the same shape as the three lookup sites above. That covers every caller, present and future, instead of adding one more call-site strip; the existing call-site strips elsewhere become redundant and are left alone here.bust_dir_cache_from_specifierroutes both branches through one helper that strips before taking the dirname. The strip beforedirnameis what matters on Windows:dirnameofC:\x\missing\isC:\x\missing(the directory itself), so the parentC:\x, whose listing is what changes whenmissinggets created, was never busted there. POSIXdirnamealready skips a trailing separator.BSSMapInner::key_hash, instantiated withREMOVE_TRAILING_SLASHESin src/resolver/dir_info.rs:398 and src/resolver/lib.rs:943), so these busts already hit. On Windows release builds the parent directory is now the one busted for a trailing-slash specifier.test/bundler/cli.test.ts, "bun build --watch" block, three specifiers (relative with a trailing slash, absolute with a trailing slash, absolute with a doubled separator): each fails on a debug build without the src change (stderr ends with the panic above) and passes with it. Each test then rewrites the entry point and checks that the watcher rebuilds.test/bake/dev/bundle.test.ts, "importing a directory with a trailing slash before it is created": the dev server process died before serving the error overlay without the fix, passes with it. The test then fixes the import and expects a reload, to show the server survived.test/bundler/cli.test.ts,test/bake/dev/bundle.test.ts,test/js/bun/util/filesystem_router.test.tsandtest/cli/hot/watch.test.ts(otherbust_dir_cachecallers) pass on the debug build;cargo clippy -p bun_resolverandcargo fmtare clean.without_trailing_slash_windows_pathdoes not strip a drive root followed by an extra separator (C:\\, fromroot_len + 1in src/paths/string_paths.rs), so on Windows a literalimport "C://"still reaches the assertion. That is a fix in the shared helper.bust_dir_cache_from_specifier(specifier longer than the path buffer). The two fixes are independent; whichever lands second needs a trivial rebase of the few lines after the join.Background
DirInfoper directory (dir_cache) plus the directory listing it read (fs.entries), both keyed by the directory's absolute path.assert_valid_cache_keydocuments the key convention (native separators, no trailing separator except for the roots/andC:\) and is compiled in whenever Rust debug assertions are on, which includes the release-asan CI build (scripts/build/config.ts setsassertions = debug || asan). The maps themselves additionally trim trailing native separators when hashing, so the convention is enforced at the sites that build keys rather than relied on by the maps.bun build --watch, or the dev server),bundle_v2callsbust_dir_cache_from_specifierto drop the cached state for the path the import would have resolved to and for its parent (a newhello.tsshows up in the parent's listing, a newhello/index.tsin the path itself), and retries the resolution once if anything was cached. That is what lets a build pick up a file created after an earlier build cached its absence; the dev server additionally watches the parent directory (DirectoryWatchStore) so the rebuild is triggered when the file appears.Earlier revisions of this PR
The first revision stripped the separator inside
bust_dir_cache_from_specifieronly (then also its dirname, after review pointed at/a//b), and described the release-build effect as a silent cache miss; that was wrong on POSIX because the maps trim trailing separators when hashing (see above). Review of that revision suggested normalizing insidebust_dir_cacheinstead, which is the current shape. The tests are unchanged across revisions.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/cli.test.ts