Repository navigation
Conversation
|
Updated 1:20 AM PT - Oct 26th, 2026
❌ @autofix-ci[bot], your commit f90e283 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46960That installs a local version of the PR into your bun-46960 --bun |
|
Status: closed. The self-review found that this fix is at the wrong layer. The closing comment has the reason and the evidence. How I reproduced the problem on main (179664e, Linux x64):
The same happens with no watcher: in one process, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// [`remove`](Self::remove) that also returns the hash of the key and what the key mapped | ||
| /// to: a slot, `NOT_FOUND` or `UNASSIGNED`. A slot stays allocated, so | ||
| /// [`publish`](Self::publish) can map the hash to it again. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Unmaps every key whose slot satisfies `pred` and passes its hash and slot to `unpublished`. | ||
| /// A key that maps to `NOT_FOUND` or `UNASSIGNED` has no slot and stays. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Maps `hash` to a slot that [`unpublish`](Self::unpublish) or | ||
| /// [`unpublish_where`](Self::unpublish_where) returned. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // the resolver mutex. The one field that changes later is `entries`, which is | ||
| // atomic: see `ListingIndex`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `DirInfo::entries`: where the listing of the directory is in `RealFS::entries`. A bust drops | ||
| /// the listing. A re-read that finds the directory as it was keeps the slot, because its | ||
| /// subdirectories hold the index of the slot, and stores the index of the new listing here. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether a re-read of a busted directory (`self`) left it as `before`, the slot it had when | ||
| /// it was busted. A subdirectory copies the pointers and `InsideNodeModules` when it is | ||
| /// filled, and reads the rest through `parent` and `enclosing_browser_scope` later. So when | ||
| /// this is true the directory keeps `before`, and what was filled below it still holds. Every | ||
| /// field is named here so that a new one does not compile until it has an answer. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A slot that a bust took out of the index. A lookup of its key settles it: see | ||
| /// `Resolver::dir_info_cached_miss`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// True: this directory was busted. It is read again and compared with the slot. | ||
| /// False: a directory above it was. The slot is good while its parent has the slot it had. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// By the hash of the key, as in the index. Every access holds `RealFS::entries_mutex`, which | ||
| /// also keeps a fill from writing the slots that are read here. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Takes the directory `key` out of the index, with every directory below it: each one holds | ||
| /// what it took from the directory above it when it was filled. Returns whether `key` was in | ||
| /// the index, as `BSSMapInner::remove` does. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The slot that a bust took out of the index for the key with this hash, if one did. The | ||
| /// caller publishes the slot again or leaves it out for good. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Leaves every parked directory below `dir` out of the index for good: `dir` is not what it | ||
| /// was when they were filled. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `SourceFile::hash` of the file when the configurations that extend it read it. `None`: | ||
| /// two of them read different bytes, so one of them is not what the file has now. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// By the hash of the key of the directory that has the file. A re-read of a busted directory | ||
| /// leaves its slot as it is when its own configuration did not change. A change to one of these | ||
| /// files is what tells it that a directory below it is still not to be published as it was. | ||
| /// Every access holds `RealFS::entries_mutex`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Records what a tsconfig.json that was just loaded read from `path`, a file it extends. | ||
| /// `dir_hash` is the key hash of the directory of `path`, and `hash` is `SourceFile::hash`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// The caller holds `entries_mutex`: `entries` is the process-global | ||
| /// BSSMap singleton, and the callers of `Resolver::bust_dir_cache` | ||
| /// (transpiler / hot-reloader / VM) reach it without `RESOLVER_MUTEX`. | ||
| /// `bust_dir_cache` evicts the directory from its own cache under the | ||
| /// same lock. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether this parse read the dependency groups (`IncludeDependencies::Main` or `Local`). | ||
| /// A parse that did not cannot stand in for one that has to. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The path and the bytes of the package.json of the directory `input_path`. Apart from | ||
| /// [`parse_contents`](Self::parse_contents) so that a re-read of the directory can compare the | ||
| /// bytes with those of the parse it already has. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// The cached directories below the path leave the index too: each one holds what it took | ||
| /// from the directory above it. The next lookup of one puts it back as it is, unless a | ||
| /// directory between it and the path, or the path, is different when it is read again. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A slot that this function filled has no parent. One that `dir_info_cached` | ||
| // filled is not what this function fills: it stays out of the index. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A bust of a directory above this one took it out of the index, and it took | ||
| // nothing from that directory. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `use_shared_buffer = false` above, so `entry_contents` is | ||
| // `Contents::Owned`/`Empty`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `TSConfigJSON` owns `Box<[u8]>` copies of every field, so the source | ||
| // bytes are dead once `parse` returns and can be dropped with the | ||
| // local `Source`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `before_bust`: the parse that the directory had when it was busted. The re-read keeps it | ||
| /// when the file has the same bytes, so that the directory compares equal to its slot | ||
| /// (`DirInfo::is_same_after_reread`) and the parse is not made and kept a second time. The | ||
| /// options of the resolver that made it stay, as they do for a directory that is not busted. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A bust of a directory above this one took it out of the index. What is | ||
| // above it now has the slots it had then, so this one is as it was filled. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `put`; resolver mutex held. The pointer is rooted at the singleton, | ||
| // so the map reborrows inside `fill_dir_info` cannot pop it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Fills a slot for a directory that is not in the index, and puts it in the index. | ||
| /// | ||
| /// `before_bust` is the slot that the directory had when `bust_dir_cache` took it out of the | ||
| /// index. The directories below it hold the index of that slot and what they took from it. | ||
| /// So when the directory is as that slot has it, that slot goes back with the new listing, | ||
| /// and they are still right. When it is not, it gets a new slot, and those that the bust | ||
| /// took out with it stay out: the next lookup of one reads it again. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `before_bust`: the slot that the directory had when it was busted. Its package.json and | ||
| /// tsconfig.json parses are kept when the files are as they were. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // It is for the resolvers that read it. What is wrong with it is not an error of | ||
| // one that reads another in its place. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether each file that `config` was read from is as it was then, and the first one is | ||
| /// `path`: a re-read of the directory then keeps `config`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Whether a file in the directory with this key hash, which the tsconfig.json of another | ||
| /// directory extends, is not what that one read. The directories below this one that a bust | ||
| /// parked are not published again then. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The files that `Resolver::load_tsconfig` read for this configuration: the file of the | ||
| /// directory, then each file that the one before it extends. A re-read of the directory | ||
| /// keeps the configuration while each file hashes to the same value. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Closing this PR. The self-review found that the fix is at the wrong layer, and my checks of its main points agree (Linux x64). The bug is real. This prints const fs = require("fs"), os = require("os"), path = require("path");
const dir = fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), "stale-"));
const write = (file, text) => {
fs.mkdirSync(path.dirname(path.join(dir, file)), { recursive: true });
fs.writeFileSync(path.join(dir, file), text);
};
const imports = target => JSON.stringify({ name: "app", imports: { "#lib": target } });
const probe = `module.exports = () => require("path").basename(require.resolve("#lib"));`;
write("package.json", imports("./a.js"));
write("a.js", "");
write("b.js", "");
write("probe.js", probe);
write("src/probe.js", probe);
const seen = () => ["", "src"].map(sub => require(path.join(dir, sub, "probe.js"))()).join(" ");
console.log(seen());
write("package.json", imports("./b.js"));
try {
require(path.join(dir, "not-there.js")); // a require that fails drops the cached directory
} catch {}
console.log(seen());Why this PR is the wrong fix
Recommended instead
A maintainer decides first
|
Problem
Resolver::bust_dir_cachedrops a directory, the cached directories below it keep the oldpackage.jsonandtsconfig.json. Underbun --hot, an edit oftsconfig.jsonpathsreaches a root file and not a file insrc/.dir_info_uncached(src/resolver/resolver.rs) copies the parent's pointers and slot index into each child. The bust removed one key.Fix
DirInfo::is_same_after_reread).package.jsonbytes andtsconfig.jsonhashes keep the old parse, so an unchanged directory compares equal.src/and passes here (Notes). Self-review: this is the wrong layer. Closed, see the closing comment.Background
DirInfois the resolver's cached record of one directory. A failedrequire(), a--hotdirectory event and the dev server bust it.entries, now atomic.package.jsonagain per--hotsave. Weighed a generation stamp: it adds instructions to every cache hit.Downsides
Notes
Who busts.
bun --hotand--watchon each directory event, the dev server (watched directories, a deleted file, a failed relative import), the runtime after a failedrequire()orimport(),Bun.buildfor an entry point that does not exist, andFileSystemRouter.reload(). So the bug needs no watcher:require("/app/not-there.js")drops/app.What a directory takes from the one above it. The nearest
package.json(forimports,"type",sideEffects, thebrowsermap, auto-install), the nearesttsconfig.json(paths,baseUrl, jsx), its real path, and the index of its parent's slot, which thenode_moduleswalk reads later. All of them were stale below a busted directory.Tests. Each file fails on a debug build with
src/from main and passes with this branch.test/cli/hot/hot.test.ts:tsconfig.jsonpathsandpackage.jsonimportsunderbun --hot.test/bundler/resolver/cache-runtime.test.ts: in one process, with a failedrequire()as the bust. A changed, created and deletedpackage.json, a changedtsconfig.json, a newnode_modulesfolder, a changed file that atsconfig.jsonbelow extends, two busts in both orders, and a bust that changes nothing.test/bundler/resolver/cache-invalidation.test.ts:Bun.build,importsand"type".test/bake/dev/bundle.test.ts: the dev server, after a file next to thepackage.jsonis deleted.test/js/bun/util/filesystem_router.test.ts:reload()and a directory that has no route.test/js/bun/resolve/tsconfig-extends-leak.test.ts(debug builds): 20 busts with no change parsetsconfig.jsononce, open the root again each time, and open no directory below it again.Also run on the debug build, all pass:
test/js/bun/resolve/resolve.test.ts,test/bundler/resolver/cache-node-compat.test.ts, all oftest/cli/hot/hot.test.ts,test/cli/watch/watch.test.ts,test/bake/dev/bundle.test.ts,test/bake/dev/hot.test.ts,test/js/bun/util/filesystem_router.test.ts. Tests ran on Linux x64 only.bun run rust:check-allcompiles the change for all 12 targets. The Windows and kqueue watcher paths were not run.Measurements. Release builds of main 179664e and this branch, Linux x64.
Cache hit, unchanged:
Resolver::dir_info_cached_maybe_log: 770 B and 200 instructions in both builds. 0 instructions differ once addresses are masked.BSSMapInner::get_or_put, both instantiations: 1,208 B and 324 instructions in both. 0 differ.One
bun --hotsave of a root-level file with no config change, 42 cached directories, counted with breakpoints over 10 saves, 3 runs:package.jsonparses kept for the life of the processtsconfig.jsonparsesOne failed
require()from the project root with every directory cached. Instructions for the whole process, bust run minus no-bust run, per bust. The number includes the failedrequire()itself and the re-read of the root.The added work is the scan of the index, and one trip through the cold path for each directory on its next lookup. Wall clock did not resolve it: over 5 interleaved rounds the difference between main and this PR was inside the spread between two runs of main.
2000 failed
require()calls in a loop from the project root. Here the directories below the root stay out of the index, so each later scan is short:Two runs of main differ by about 15%, and by more on the first run after the fixture is written. The remaining 6.5 to 8.8 KB was not investigated.
Size: text 107,582,646 to 107,592,886 B.
size_of::<DirInfo>()stays 104 B (const assert).bust_dir_cache262 to 3,373 B,dir_info_cached_miss6,465 to 7,589 B,dir_info_uncached17,800 to 18,871 B, newfill_dir_info2,652 B.Not measured: syscalls per save, the cost of one real config edit, the syscalls that the check of extended
tsconfig.jsonfiles adds, and the peak size of the table of directories that a bust took out.Still stale after this PR.
package.jsonortsconfig.json. kqueue reports a directory only when an entry is replaced, so an in-place write on macOS raises no bust.tsconfig.jsonthat extends a file outside every busted directory, when that directory is not below the busted one.Question for a maintainer.
DirInfo.entriesis now an atomic that is stored once more when an unchanged directory is read again. Every other field of a slot is still written once. Without it, an unchanged directory needs a new slot on every bust, and the directories below it would hold the index of an orphan.Related. #41534 has the same index scan in another form. #36928 keeps the same parse for unchanged bytes, for every parse. #46911 adds one more field that a directory takes from its parent:
is_same_after_rereadnames every field, so it does not compile until that field has an answer.