Repository navigation
Conversation
…dir cache A directory reached through a symlink is cached under both the symlink path and its real path. bust_dir_cache only invalidated the key it was given, so a resolve of the realpath'd result could hit a stale listing and fail with "Cannot find module ... from ''". Follow the DirInfo.abs_real_path alias and bust it too.
|
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 (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesThe resolver now invalidates directory and filesystem-entry cache entries under a cached realpath alias. A regression test covers resolving a newly created sibling TypeScript file through a symlinked directory. Resolver cache invalidation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Beyond the inline finding, I checked whether holding real_path across dir_cache.remove(path) is a use-after-free — it is not: DirInfo.abs_real_path is &'static [u8] interned in dirname_store, not owned by the removed map entry. Also confirmed without_trailing_slash_windows_path on the alias matches the normalization other dir_cache lookups apply before assert_valid_cache_key.
Extended reasoning...
The fix reads abs_real_path from the cached DirInfo before removing it, then busts that alias key too. The lifetime concern (borrowing a field of an entry about to be removed) is resolved by abs_real_path: &'static [u8] at src/resolver/dir_info.rs:138 — the bytes live in the dirname_store arena, not the map node. The .filter(|real| !real.is_empty() && *real != path) guard means non-symlink directories (empty abs_real_path) do no extra work. The test follows harness conventions (tempDir, bunExe/bunEnv, concurrent pipe drain, stderr/stdout asserted before exitCode, symlink created before spawn with "junction" for Windows). One namespace-alias nit is posted inline; nothing further to add.
|
Updated 6:08 PM PT - Aug 26th, 2026
❌ @robobun, your commit 01a997b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40587That installs a local version of the PR into your bun-40587 --bun |
|
CI status: the new test passes on every lane in both runs (106479, 106492). The remaining failures are unrelated: an rspack integration timeout during bun install (reported separately) and known flaky leak-threshold and GC tests (websocket-server, require-cache on Windows, compression, fetch-leak) that passed alone or on retry. The diff is ready for review. |
There was a problem hiding this comment.
LGTM — the earlier namespace-alias nit is addressed and the follow-up comment trim looks fine.
Checked that abs_real_path is &'static [u8] (arena-backed, dir_info.rs:138) and is captured before dir_cache_mut().remove, so no dangling borrow across the eviction.
Verified the added path is a no-op for non-symlinked dirs (abs_real_path is empty → filter drops it), so the common bust cost is one extra hashmap get.
Test uses module-scope mkdirSync/symlinkSync/join/tempDir already imported in the file, "junction" for Windows, drains pipes concurrently, and asserts stderr/stdout before exit code.
Extended reasoning...
Overview
The PR fixes a resolver dir-cache invalidation gap: when a directory is reached through a symlink, it is cached under both the symlink path and its real path (DirInfo.abs_real_path). bust_dir_cache previously only evicted the key it was handed, leaving the real-path alias stale after a sibling file was created — surfacing as Cannot find module ... from '' on macOS where os.tmpdir() sits behind /var → /private/var. The fix reads abs_real_path off the cached entry before eviction and, when it differs, also busts the entries cache and dir cache for that alias. A new subprocess test in resolve.test.ts reproduces the warm-then-create-sibling sequence through a real/link junction pair.
Security risks
None. This is cache-invalidation bookkeeping inside the module resolver; no untrusted input parsing, no auth/crypto/permissions surface. The only new read is of an already-cached, already-normalized internal path.
Level of scrutiny
Low-to-moderate. The Rust change is ~15 lines and purely additive: it can only bust more cache entries, never fewer, so the worst-case regression is redundant re-reads rather than incorrect resolution. I specifically checked the one memory-shaped concern — using real_path after remove(path) — and confirmed abs_real_path is declared &'static [u8] in dir_info.rs (arena-owned string data, not freed by hashmap removal), and it is captured before any mutation. The without_trailing_slash_windows_path normalization matches how other call sites in this file prepare cache keys, and the file-local strings:: alias is now used per the earlier nit.
Other factors
The test follows repo conventions end to end (harness tempDir with using, bunExe()/bunEnv, await using on the spawn, concurrent pipe drain, output asserted before exit code, "junction" symlink kind for Windows), and all referenced identifiers are already imported at module scope in resolve.test.ts. No CODEOWNERS entry covers src/resolver/. The github-actions inline comments on lines 2450/2453 were followed by a comment-shortening commit, and there are no outstanding CHANGES_REQUESTED reviews.
|
This also fixes #40790 (import() of a newly created sibling .mjs fails when the directory sits behind a symlink, regressed in 1.3.14). I applied this diff on current main and ran the repro script from that issue on linux x64 with TMPDIR pointed through a symlink. It fails on stock bun and passes with the diff. Consider adding "Fixes #40790" to the body so the issue closes on merge. |
Problem
require()orimport()of a newly created sibling.tsfile fails withCannot find module '...' from ''when the directory sits behind a symlink. macOS always hits this becauseos.tmpdir()is behind/var -> /private/var. Fixes require()/import() fails on the first resolution of a second fresh sibling .ts file per process, from '' error #40585.DirInfo.abs_real_path).bust_dir_cache(src/resolver/resolver.rs:2446) only invalidated the key it was given, so the real-path key kept a stale listing taken before the new file existed.Fix
bust_dir_cachenow readsabs_real_pathfrom the cachedDirInfobefore it removes the entry. If the alias differs from the given path, it busts the alias's entries cache and dir cache too.getper bust. A real directory has an emptyabs_real_path, so the common case does no extra work.test/js/bun/resolve/resolve.test.ts(new test, fails on stock bun). Also the fulltest/js/bun/resolve/suite andtest/cli/hot/.Background
require(b.ts)resolves the symlink path, and the retry logic busts the symlink-side key and finds the file. The resolver returns the realpath'd result. The module loader then resolves that result with an empty referrer, which has no retry, and it hits the stale real-path listing./varon macOS reproduces it deterministically and a plain/tmppath on linux does not.Notes
Reproduction on linux needs a stand-in for the macOS
/varsymlink, created before bun starts:mkdir -p /tmp/private-root && ln -s /tmp/private-root /tmp/var-linkThen the issue's verbatim repro script fails on attempt 1 and passes on attempts 2 and 3, matching the report:
Resolver trace (
BUN_DEBUG_RESOLVER=1) for attempt 1:The second-stage resolve has referrer
'', soretry_on_not_foundinVirtualMachine::resolveis false (is_absolute(source_to_use)fails) and no bust-and-retry happens there. Fixing the bust to cover the alias repairs every bust call site (VM retry, hot reloader, dev server, filesystem router) in one place.test/js/bun/resolve/load-same-js-file-a-lot.test.ts("load the same empty JS file 2000 times") times out at 5s in the local debug+ASAN run both with and without this diff. It is a pre-existing debug-build timeout, not related.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/resolve/resolve.test.ts