hot_reloader: take entries_mutex for watcher-thread directory cache probes - #37276
hot_reloader: take entries_mutex for watcher-thread directory cache probes#37276robobun wants to merge 6 commits into
Conversation
…robes The watcher thread probed the process-global directory-entry cache and walked cached DirEntry.data maps with no lock, racing the in-place stale-generation rewrite (entries_at_locked, read_directory) that frees the old map's bucket allocation under entries_mutex. A walk of freed buckets can load a garbage *mut Entry and fault, the mechanism proven by the ASAN trace in #34271 and locked on the resolver side by #34271 and #37274; the watcher-side probes were the remaining unlocked reader. Probe under entries_mutex and carry cache-owned *mut DirEntry (tombstones included) instead of *mut EntriesOption, so the value read is serialized with same-key rewrites and a cached read error can no longer be unwrapped as a listing. The extracted pointers are EntryStore/cache-owned (process lifetime), so only the map walks are critical sections. Lock order matches the existing watcher-thread bust_dir_cache: Watcher.mutex, then entries_mutex; nothing takes them in reverse.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe hot reloader now retains stable cached directory entries and synchronizes directory-change lookups with ChangesHot-reloader cache synchronization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/cli/hot/hot.test.ts`:
- Line 887: Update the timeout expression in the debug test configuration to
replace the Infinity branch with a large finite millisecond budget, while
preserving the existing 60-second non-debug timeout. Ensure hung child processes
still fail with a timeout instead of running indefinitely.
- Around line 836-878: Drain proc.stderr concurrently with proc.stdout before
entering the churn loop, retaining the collected stderr text for later
assertions. Replace the deferred proc.stderr.text() call in the final
Promise.all with the already-started reader’s result, so child stderr output
cannot block process exit while preserving the existing stdout and exit
handling.
🪄 Autofix
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: e91178f0-818d-434d-9b02-8ebb617a623e
📒 Files selected for processing (2)
src/jsc/hot_reloader.rstest/cli/hot/hot.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:51 AM PT - Aug 9th, 2026
❌ @autofix-ci[bot], your commit dfaaf0b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37276That installs a local version of the PR into your bun-37276 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after 8dbd592 and didn't find any bugs — the stderr-drain and debug-timeout concerns from the previous pass are addressed. Because this is a cross-thread memory-safety fix with raw-pointer lifetime and lock-order claims (and the comment-cop flags on those doc comments are still open), a human look would still be worthwhile.
What was reviewed:
probe_entries_cache/ the innerdatawalk takeentries_mutex, matching the resolver-side reader pattern (resolver.rs:3403,:3763) and the same-pathbust_entries_cachecall, so the statedWatcher.mutex→entries_mutexorder holds.- The
tombstonestype change to*mut DirEntry— a cached read error now falls back to the tombstone instead of reaching the panickingentries()accessor. - Test: stderr now drains from spawn,
readyunblocks on early exit, churn loop is bounded by child exit, and the assertion covers{stdout, stderr, exitCode, signalCode}so a signal death fails visibly.
Extended reasoning...
Overview
The PR closes the last unlocked reader of the process-global directory-entry cache: on_file_update on the watcher thread. It replaces two unsynchronized reads (rfs.entries.get(file_path) and dir_ent.entries().get(changed_name)) with lookups under entries_mutex, and narrows the tombstones map value type from *mut EntriesOption to *mut DirEntry so the error variant can no longer reach EntriesOption::entries() (which panics on Err). A new stress test in hot.test.ts drives the exact race shape (watcher-thread directory events vs. FileSystemRouter.reload() + concurrent Bun.build()).
Security risks
None user-facing. This is an internal concurrency fix; no new inputs are parsed and no trust boundary changes. The relevant risk is memory safety (UAF on freed hash-map buckets), which the change tightens rather than loosens.
Level of scrutiny
High. The change adds unsafe derefs of raw *mut DirEntry / NonNull<Entry> and asserts non-local invariants: that DirEntry allocations are cache-owned leaked boxes (process lifetime), that Entry slots are EntryStore-owned and never freed, and that the Watcher.mutex → entries_mutex lock order is one-way. I spot-checked the resolver side: the same lock is taken around DirEntry.data reads at resolver.rs:3400-3403 and several entry().kind() sites, and bust_dir_cache (called from the same on_file_update body while Watcher.mutex is held) reaches bust_entries_cache, which establishes the same lock order. That is consistent with the PR's claims, but a maintainer who owns this cache should confirm the EntryStore lifetime claim and that no path acquires the two locks in reverse.
Other factors
- The three
comment-copgithub-actions flags on the new doc comments (lines ~413, ~822, ~1210) are unresolved. They are style flags on SAFETY-adjacent comments explaining lock order and pointer lifetime; whether to trim them is a judgment call for a human. - The PR description candidly states the test cannot prove fail-before (10/10 pass on unfixed debug+ASAN); its value is ASAN coverage of the code path going forward. That is reasonable for a sub-microsecond race, but it means the fix's correctness rests on the mechanism argument rather than a red→green test.
- My prior inline finding (undrained stderr) and CodeRabbit's
Infinity-timeout note were both addressed in 8dbd592.
…e stress test to ASAN builds The new test plants an EACCES error in the directory-entry cache for a watched directory (chmod 000 plus a failed resolve, dropping to nobody via runuser when root) and then lands one directory event on it. Before the fix the watcher thread fed the error slot to EntriesOption::entries, which panics (EntriesOption::entries on non-Entries variant) and aborts the process; deterministic on Linux, verified 5/5 red on the unfixed build and 5/5 green on the fixed one. The reload/build stress test also drives the resolver-side lookup races that #37274 and #34411 fix, which segfault the fixture on weakly ordered release lanes (ubuntu 25.04 aarch64, build 91033, sigsegv at 0x1E, 41ms in). Gate it to ASAN builds, where the watcher-side use-after-free it guards against is detectable and those lanes are x64.
| const stdoutDone = (async () => { | ||
| const decoder = new TextDecoder(); | ||
| for await (const chunk of proc.stdout) { | ||
| stdout += decoder.decode(chunk, { stream: true }); | ||
| poke(); | ||
| } | ||
| })(); | ||
| const stderrDone = proc.stderr.text(); | ||
| // A crash resolves every pending wait so the assertions below report |
There was a problem hiding this comment.
🟡 The exited.then hook drains waiters once and never fires again, and waitFor doesn't check whether the process is dead before pushing — so a waitFor call whose continuation runs after the exit hook (e.g. child crashes before printing err-cached:true, exit hook resolves the line-1022 waiter, its continuation then calls waitFor(countReady >= 2)) pushes a resolver nothing will ever settle, and the test sits for the full 30s/120s timeout instead of reporting the {stdout, stderr, exitCode, signalCode} diff. Latch a dead flag in the .then and have waitFor short-circuit (resolve immediately) when it's set.
Extended reasoning...
What the bug is
In the new "cached listing is a read error" test, the crash-guard on process exit is one-shot:
const exited = proc.exited.then(code => {
for (const w of waiters.splice(0)) w.resolve();
return code;
});This resolves whatever waiters exist at the moment proc.exited settles, and never fires again. Meanwhile waitFor (line 1012–1016) checks only test(stdout) before pushing:
const waitFor = (test) =>
new Promise<void>(resolve => {
if (test(stdout)) return resolve();
waiters.push({ test, resolve });
});It does not check whether the process has already exited. Any waitFor() call made after the exit hook has drained pushes a resolver that nothing can reach: the child is dead (stdout is at EOF, so poke() never fires again from the for await loop), and exited.then will not re-run.
The code path that triggers it
Step-by-step, for a child that crashes while handling the err stdin command (e.g. inside Bun.resolveSync, or on chmodSync), before it prints err-cached:true:
- Line 1022 —
await waitFor(s => s.includes("err-cached:true\n"))pushes a waiter intowaiters. - The child aborts.
proc.exitedresolves → the.thencallback runswaiters.splice(0)and callsw.resolve()on the pending err-cached waiter, then returnscode. w.resolve()schedules a microtask that resumes theawaitat line 1022.- The continuation runs synchronously:
writeFileSync(join(root, "pages", "p1.tsx"), …)(no effect — child is dead), thenwaitFor(s => countReady(s) >= 2)at line 1029. countReady(stdout)is1(only the firstready\nwas ever printed), so the predicate fails and a new{test, resolve}is pushed ontowaiters.- Nothing ever resolves it: the stdout
for awaitloop has exited (or will exit without anotherpoke()— EOF), andexited.thenhas already run and won't re-run. - The test blocks on that promise until the per-test timeout fires: 30s on release, 120s on debug.
The same window exists between await proc.stdin.flush() at line 1021 and the waitFor at line 1022, if the exit signal is processed before the flush continuation runs.
Why existing safeguards don't prevent it
The exit hook resolves-then-forgets rather than latching state. The comment at line 1006–1007 says "A crash resolves every pending wait so the assertions below report the death instead of hanging" — which is true for waiters that are pending at exit time, but not for waiters registered by the continuations of those very resolves. waitFor consults only stdout, never proc.exitCode/proc.killed/a dead latch.
Note: the test's primary regression target — the watcher panic on the cached-error probe — fires after err-cached:true prints, so in that specific case the countReady >= 2 waiter is already pending when exit fires and is correctly resolved. This gap bites for crashes at other points (during Bun.resolveSync, chmodSync failure, or the resolver-side races the PR description itself names as adjacent).
Impact
On regression, the failure mode degrades from an actionable assertion diff ({stdout: "ready\n", stderr: "<panic backtrace>", exitCode: null, signalCode: "SIGABRT"} vs the expected object) to a bare Timed out after 30000ms / 120000ms with no diagnostic. The test still fails — the timeout is finite — so this is not a hang or a false pass; it is a diagnostic-quality regression that also wastes 30–120s of CI wall-clock per hit.
REVIEW.md → Tests reviewers reject: "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise".
How to fix
Latch a flag in the exit hook and have waitFor short-circuit on it:
let dead = false;
const exited = proc.exited.then(code => {
dead = true;
for (const w of waiters.splice(0)) w.resolve();
return code;
});
const waitFor = (test: (s: string) => boolean) =>
new Promise<void>(resolve => {
if (dead || test(stdout)) return resolve();
waiters.push({ test, resolve });
});Resolving (rather than rejecting) is fine here because the final expect({stdout, stderr, exitCode, signalCode}).toEqual(…) will then report the crash immediately.
Severity
nit — only reachable when the child crashes (i.e., on regression), bounded by the finite per-test timeout, and does not affect the passing case. But it turns a would-be actionable failure into a 30–120s blind timeout, and the fix is one flag.
|
CI status: everything this diff touches is green on all lanes (test/cli/hot/hot.test.ts passes everywhere, including the new deterministic fail-before test; the ASAN-gated stress test runs on the x64-asan lane). The two remaining red files, node-http2.test.js and require-cache.test.ts on the darwin 14 x64 lane, are untouched by this diff and fail independently of it; both are reported for main-break triage. |
1 participant
Part of the race class behind #37266 (
panic: Segmentation fault at address 0x25in the filesystem_router reload/build fixture). #34271 and #37274 locked the resolver-side readers of the directory-entry cache; the watcher thread insrc/jsc/hot_reloader.rswas the remaining unlocked reader, noted in #37274 as left for a separate change.Cause
On a directory event,
on_file_update(watcher thread) performed unsynchronized reads of the process-global directory-entry cache:rfs.entries.get(file_path)probed the outer map and the code later read the slot'sEntriesOptionvalue bytes. Same-key re-caches rewrite those bytes underentries_mutex(read_directory,read_directory_error), so the variant read could tear, and anErrslot reachedEntriesOption::entries(), which panics on theErrvariant.dir_ent.entries().get(changed_name)walked the cachedDirEntry.datahash map with no lock.entries_at_locked/read_directoryrewrite that map in place underentries_mutex(data.clear()plus*e_ptr = new_entry), freeing the old bucket allocation. A walk of freed buckets can load a garbage*mut Entryand fault at a small absolute address. This is the mechanism the ASAN trace in resolver: take entries_mutex in entries_at before the in-place DirEntry rewrite #34271 proved (heap-use-after-free inStringHashMap<*mut Entry>whiledrop_in_place<DirEntry>ran inentries_aton another thread), with the watcher as the reader this time.A stale SAFETY comment claimed this code runs single-threaded on the JS thread.
on_file_updateruns on the watcher thread while JS and bundler threads resolve concurrently (theTask::pending_countdoc in the same file states exactly that).Fix
Probe under
entries_mutex, and carry*mut DirEntryinstead of*mut EntriesOption(thetombstonesmap included):probe_entries_cachetakes the lock, matches the variant, and extracts theDirEntryaddress. A cached read error now falls back to the tombstoned listing instead of panicking later.datawalk only, then carries the found*mut Entryout. The extracted pointers are cache- and EntryStore-owned leaked allocations (process lifetime), so they stay valid after unlock. This is the snapshot pattern already used byFileSystemRouter::bust_dir_cache_recursive,RouteLoader::load, and the bake framework-router scan.Lock order is unchanged: the platform watcher holds
Watcher.mutexaroundon_file_update, andWatcher.mutexthenentries_mutexis already the established order there (bust_dir_cacheon the same path takes it). Nothing takes the two in reverse; the one resolver-to-watcher call (the directory auto-watch inload_as_file) runs after the dir lookup helpers have releasedentries_mutex.The adjacent lock-free
ent.abs_pathread has its own torn-read window; #34411 covers it and is complementary.Tests
Two tests in
test/cli/hot/hot.test.ts.Deterministic (fail-before provable): "a directory event for a dir whose cached listing is a read error does not kill the process". The fixture plants an EACCES readdir error in the directory-entry cache for a watched directory (chmod 000 plus a failed resolve, dropping to
nobodyvia runuser when run as root, following theresolve.test.tsprecedent), then the test lands a single directory event on it by touching the watched page. Before the fix the watcher thread fed the error slot toEntriesOption::entries(), which panics withand aborts the process. Verified 5/5 red on an unfixed debug build and 5/5 green with the fix. The panic requires inotify-style per-name walks, so the red half is Linux-specific (the gate's platform); on macOS the test covers the probe and passes on both builds.
Stress (ASAN only): "directory events race reload() and Bun.build() rewriting the same cached listing" churns the watched directory from outside while
FileSystemRouter.reload()and four concurrentBun.build()calls rewrite the cached listing in place (the #34271 fixture shape). Instrumented counters show roughly 2900 outer probes and 5400 inner map walks per run on the watcher thread. The use-after-free window itself is sub-microsecond and did not fire in 16 unfixed ASAN probe runs (including single-CPU pinning), so this test is regression coverage: under ASAN a reintroduced unlocked walk is a detectable heap-use-after-free. It is gated to ASAN builds because the same reload/build mix drives the resolver-side lookup races that #37274 and #34411 fix, which segfault the fixture on weakly ordered release lanes. This PR's first CI run demonstrated exactly that: sigsegv at address 0x1E, 41ms in, ubuntu 25.04 aarch64 release (build 91033), on a sha that has this PR's watcher-side locks but not #37274's resolver-side ones.Verification
bun bd test test/cli/hot/hot.test.ts: 14 pass (including both new tests)bun bd test test/cli/watch/watch.test.ts: 8 passbun bd test test/js/bun/util/filesystem_router.test.ts: 33 passbun run rust:check-all: 10/10 targetscargo clippy -p bun_jsccleanno test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts