Skip to content

--hot: pick up an imported file that is deleted and then recreated - #33073

Closed
robobun wants to merge 10 commits into
mainfrom
farm/c9035eb1/hot-recreated-import-unwatched
Closed

robobun wants to merge 10 commits into
mainfrom
farm/c9035eb1/hot-recreated-import-unwatched

Conversation

@robobun

@robobun robobun commented Jun 29, 2026 •

Copy link
Copy Markdown
Collaborator

Repro

entry.ts imports ./dep.ts and serves its value with Bun.serve.

bun --hot entry.ts
curl :PORT                              # "A"
rm dep.ts                               # one tolerated reload:
                                        #   error: Cannot find module './dep.ts'
printf 'export default "B"' > dep.ts
curl :PORT                              # still "A"  <- the recreated file is invisible
printf 'export default "C"' > dep.ts
curl :PORT                              # still "A": later edits are invisible too

Touching any other watched file recovers, because that reload re-imports dep.ts and re-adds its watch. Until then --hot silently serves the stale module with no error and no log line. Reproduces on 1.4.0 and on current main, on every platform.

Cause

Deleting a watched file evicts its entry from the watchlist (on_file_update in src/jsc/hot_reloader.rs). After that, nothing can notice the recreated path:

  • On POSIX the per-inode watch died with the inode, and the later CREATE on the parent directory is handled by scanning the changed name against the watchlist, which no longer contains the file.
  • On Windows, ReadDirectoryChangesW events are matched against the watchlist by path, so the Added/Modified for the recreated file only matches the parent-directory item, whose Windows arm was just bust_dir_cache + continue.

Either way no reload is scheduled and nothing re-adds the watch. The entrypoint already has a dedicated recovery for exactly this (MainFile.is_waiting_for_dir_change on kqueue, the main.dir_hash == current_hash block on inotify), and the existing hot.test.ts "deleted and rewritten" test only covers the entrypoint. Imported files had no equivalent.

One non-obvious detail: on Linux the eviction for an rm does not come from the file's own IN_DELETE_SELF. Bun keeps the source file descriptor open, which pins the unlinked inode, so that event is deferred until flush_evictions closes the fd, by which point the watch descriptor is already gone from the watchlist snapshot and the event is dropped. The eviction that actually happens comes from the parent directory's IN_DELETE being matched against the watchlist. On macOS kqueue's NOTE_DELETE, and on Windows FILE_ACTION_REMOVED, both fire at unlink time, so the File arm does see the delete there.

Fix

NewHotReloader gets a deleted_watched_files map (absolute path to watch hash):

  • remember_deleted_watch(path, hash) records an eviction, but only when the path is actually gone from disk (a rename-into-place also reports the target as removed, and in that case the already-enqueued reload re-watches it).
  • It is called from both eviction sites: the File arm's DELETE (kqueue, Windows, and Linux watches added without a cached fd) and the directory arm's watchlist scan (the path rm actually takes on Linux for an imported file).
  • reenqueue_recreated_files(dir_path, watchlist_hashes, task) is called from both platform directory arms. For a recorded path directly under the directory, it enqueues a reload (and drops the record) only when all of these hold, each of which is load-bearing:
    1. the path exists on disk again,
    2. it is not already back in the watchlist (otherwise an earlier reload already re-imported it), and
    3. no reload is already queued (pending_count == 0) or buffered by this very batch (task.count == 0). Either one reads the live filesystem when it runs and re-adds everything that resolves, so the record just survives to the next directory event. This is the same coalescing invariant Task::run uses, and the task.count half came out of review.

Gate 2's position matters: it runs after the existence filter. watchlist_hashes is the batch-entry snapshot and evictions are only flushed after the event loop, so a hash recorded earlier in the same batch still appears watched. Checking membership first would permanently drop a just-made record; on macOS, kqueue does not sort events by watchlist index (unlike inotify and the Windows watcher), so a single rm can deliver the file's NOTE_DELETE before the parent directory's NOTE_WRITE in one batch and hit exactly that. Review caught this.

The enqueued reload's re-import re-adds the per-file watch. This is the entrypoint's recovery mechanism, generalized to every watched file, on every platform.

None of the gates are defensive extras; each has a verified justification.

  • The existence check inside remember_deleted_watch is what makes atomic saves safe. On macOS, renameSync(tmp, path) delivers NOTE_DELETE on the replaced inode's still-open fd, so an unconditional record there is a false positive that the next directory event would consume into a gratuitous second reload per save. This PR's first revision had exactly that, and it cut should work with sourcemap generation's throughput on the darwin 26 aarch64 CI lane to 4 of 50 cycles inside the timeout (build 66766).
  • Gate 2's placement after the dirname and existence filters was caught in review. watchlist_hashes is the batch-entry snapshot and evictions are only flushed after the event loop, so a hash recorded earlier in the same batch still appears watched. Placed first, the check would permanently drop a just-made record; kqueue delivers events in kernel order (unlike the inotify and Windows watchers, which sort by index), so a single rm can deliver the file's NOTE_DELETE before its parent directory's NOTE_WRITE in one batch and hit exactly that.
  • Gate 3's task.count half, also from review, covers a reload this batch has already buffered but not yet enqueued (Task::enqueue runs from a scopeguard at batch end), so pending_count alone cannot see it.

This also replaces a hand-rolled access(F_OK) block (and the identical inline copy the entrypoint recovery used) with the existing cross-platform bun_sys::exists helper.

Verification

New test in test/cli/hot/hot.test.ts: should hot reload an imported file after it is deleted and recreated. It deletes the import, waits for the Cannot find module reload on stderr, recreates the file, and asserts the server picks up the new value, then asserts a subsequent edit is picked up too (proving the watch was re-armed, not just reloaded once).

  • on an unfixed build the test fails with Expected: "B", Received: "A"; with the fix it passes in about 1s
  • the first revision left the fix #[cfg(not(windows))] and CI reproduced the Windows bug with that exact failure on all three Windows test lanes, which doubles as the fail-before proof for the Windows half
  • bun bd test test/cli/hot/ (all 16, including both sourcemap tests and should not hot reload when a random file is written, the negative test that guards the "path actually gone" gate), test/cli/watch/, and test/cli/test/test-changed.test.ts all pass
  • bun run rust:check-all is clean across all 10 targets
  • the new test passes on every lane of every CI build since 16dc8ec. The one hot.test.ts failure that keeps appearing on the Windows lanes is should work with sourcemap generation, which is a pre-existing flake, not this PR's: the identical at hot-runner-root.js:1:12 signature is on build 66799 for farm/9a007f8a/subtlecrypto-exception-checks, whose diff against main touches none of src/jsc/hot_reloader.rs, src/watcher/, or test/cli/hot/. Details in the CI status comment below.

Deleting an imported file destroys the inode its per-file watch is attached
to and evicts its watchlist entry. The later CREATE on the parent directory
was only handled for files still in the watchlist, so the recreated import
was never reloaded and never re-watched: --hot kept serving the old module
until some other watched file changed. Only the entrypoint had a recovery
path for this (MainFile / is_waiting_for_dir_change).

On Linux the eviction for an rm does not even come from the file's own
IN_DELETE_SELF: Bun keeps the source fd open, which pins the unlinked inode
and defers that event until flush_evictions closes the fd, by which point
the watch descriptor is gone from the snapshot. The eviction that actually
happens is the directory arm's watchlist scan. kqueue delivers NOTE_DELETE
on the still-open fd at unlink time, so the File arm does see it there.

Record the absolute path at both eviction sites when the path no longer
exists on disk, and on a later event for the parent directory enqueue a
reload for any recorded path that exists again; the re-import re-adds the
per-file watch on the new inode. Also dedupes the inline access(F_OK)
block used by the entrypoint recovery into a path_exists helper.
@robobun

robobun commented Jun 29, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:13 PM PT - Jun 29th, 2026

❌ @robobun, your commit fc26d87 has 3 failures in Build #66851 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33073

That installs a local version of the PR into your bun-33073 executable, so you can run:

bun-33073 --bun

@coderabbitai

coderabbitai Bot commented Jun 29, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Updates hot reload so deleted imported files are remembered and reloaded when recreated, adds an integration test for that flow, and adjusts Markdown formatting in two docs files.

Changes

Hot Reload Delete/Recreate Recovery

Layer / File(s) Summary
deleted_watched_files field and recovery helpers
src/jsc/hot_reloader.rs
Adds deleted_watched_files to NewHotReloader, initializes it in init and enable_hot_module_reloading, and defines remember_deleted_watch and reenqueue_recreated_files.
Delete and directory event recovery
src/jsc/hot_reloader.rs
Records deleted watches during file delete and rename handling, re-enqueues recreated files during directory events, uses bun_sys::exists in entrypoint recovery, and tracks removed imports during inode replacement.
Integration test for recreated imported dependency
test/cli/hot/hot.test.ts
Adds a bun --hot test that deletes an imported dependency, waits for the failure signal, recreates it with new return values, and polls the HTTP endpoint until each update is served.

Docs Formatting Fixes

Layer / File(s) Summary
base64 guide and web-apis table formatting
docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx
Wraps the base64 example in an explicit ts code fence and reflows the Web API support table without changing its content.

Possibly related PRs

  • oven-sh/bun#33040: Touches the same base64 guide and Web API support table files with related documentation edits.

Suggested reviewers

  • dylan-conway
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: hot reload now handles an imported file that is deleted and recreated.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description is detailed and covers the change and verification, though it uses custom sections instead of the repository template.

Comment @coderabbitai help to get the list of available commands.

@mintlify

mintlify Bot commented Jun 29, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bun 🟢 Ready View Preview Jun 29, 2026, 1:33 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Comment thread test/cli/hot/hot.test.ts
The test added in the previous commit failed on every Windows CI lane with
the same symptom it fixes on POSIX (served the stale value after the
recreate), because both the record and consume sides were cfg(not(windows)).
Windows has the same bug: the Removed event evicts the file's watchlist
entry, and a later Added/Modified for the recreated path only matches the
parent directory item, whose Windows arm was bust_dir_cache + continue.

Drop the cfg gates, move the directory-arm consume into a shared
reenqueue_recreated_files method called from both platform arms, and fold
the "is the path actually gone" check into remember_deleted_watch so a
rename-into-place (which also reports Removed for the target on Windows)
is never recorded.

Also replaces the hand-rolled path_exists helper with the existing
cross-platform bun_sys::exists.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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`:
- Around line 482-495: The async stdout/stderr reader promises in hot.test.ts
are being left uncaught, so runner.kill() can turn expected stream aborts into
test failures after the assertions complete. Update the stderrDone and
stdoutDone IIFEs (and the same hot-reload reader block referenced by the
duplicate comment) to attach a .catch(() => {}) handler, following the existing
hot-test convention, so aborted reader iterations are swallowed cleanly. Use the
existing runner.stdout, runner.stderr, and Promise.withResolvers flows as the
locations to apply the fix.
🪄 Autofix (Beta)

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: ff83bca6-9f48-4e9d-8f49-5dabada8c307

📥 Commits

Reviewing files that changed from the base of the PR and between fb24aac and 16dc8ec.

📒 Files selected for processing (4)
  • docs/guides/util/base64.mdx
  • docs/runtime/web-apis.mdx
  • src/jsc/hot_reloader.rs
  • test/cli/hot/hot.test.ts

Comment thread test/cli/hot/hot.test.ts Outdated
robobun added 3 commits June 29, 2026 14:23
The stdout/stderr reader loops only accumulate diagnostic output and
resolve the PORT / module-not-found signal promises; every signal they
carry is already covered by those, so a stream torn down by the final
kill must not turn an already-passed run into a failure.
…ready covers

The recreated-path recovery was, on Windows, stacking a second reload
of an unchanged module. The sourcemap-generation hot test's Windows
atomic-write shim does rmSync(root) then renameSync(tmp, root); the
watcher records root inside that window, but the delete's own reload
then runs after the rename, succeeds, and re-adds the watch. Consuming
the now-stale record at a later directory event enqueued a second,
redundant reload, which can land between a rejected module's eval and
the moment its rejection is reported and clobber the in-place
sourcemap (the :1003: frames come out as transpiled :1: coordinates).
This failed the sourcemap-generation test on every Windows CI lane.

Two gates on the re-enqueue, both required:
- drop a recorded path that is already back in the watchlist (an
  earlier reload re-imported it), and
- bail while a reload is already queued and has not started, since it
  reads the live filesystem and covers everything that has reappeared.
  This reuses the same pending_count coalescing invariant Task::run
  relies on; the record survives to the next directory event.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/jsc/hot_reloader.rs (1)

844-850: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shorten the new helper comments to the 3-line limit.

These new comment blocks exceed the repository’s 3-line max; condense the invariant here and move the detailed timeline/rationale to the PR description if needed.

Proposed comment trim
-    /// Record a file watch entry that is being evicted while its path is
-    /// gone from disk. No-op if the path still exists, because then the
-    /// reload being enqueued re-imports and re-watches it. When it is gone
-    /// the reload can't, so nothing would ever re-arm the watch and `--hot`
-    /// would keep serving the stale module; the directory arms of
-    /// `on_file_update` consume this via [`Self::reenqueue_recreated_files`]
-    /// once the path exists again.
+    /// Remember an evicted file watch only while its path is gone from disk.
+    /// Parent-directory events consume this via `reenqueue_recreated_files`
+    /// once the path exists again.
@@
-    /// Directory-arm half of the recreated-import recovery: for every
-    /// [`Self::remember_deleted_watch`]ed path directly under `dir_path` that
-    /// exists on disk again but is **not** back in the watchlist, enqueue a
-    /// reload and forget it; the re-import re-adds the per-file watch.
-    ///
-    /// The watchlist membership check is load-bearing, not an optimization.
-    /// A delete also enqueues its own reload (the `File` arm's `DELETE`
-    /// append), and if the path reappears before that reload runs (e.g. a
-    /// rename immediately follows the unlink), that reload already re-added
-    /// the watch. Appending again here would stack a second reload of an
-    /// unchanged module, which can land between a rejected module's eval and
-    /// its report and clobber its sourcemap. `watchlist_hashes` is the
-    /// caller's snapshot taken on this batch's entry, so it already reflects
-    /// every re-add the JS thread performed since the record was made.
+    /// Enqueue recreated imports under `dir_path` whose per-file watch is
+    /// still missing; duplicate reloads can clobber sourcemap reporting.
@@
-        // A reload is already queued and has not started. It reads the live
-        // filesystem and re-adds every module that resolves, so it already
-        // covers any recorded path that has reappeared by now. Re-check on
-        // the next directory event instead of stacking a second reload.
+        // A queued reload will re-read the filesystem and re-add resolved modules.

As per coding guidelines, “Keep code comments to 3 lines max.”

Also applies to: 858-871, 881-884

🤖 Prompt for 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.

In `@src/jsc/hot_reloader.rs` around lines 844 - 850, The new helper doc comments
in hot_reloader need to be shortened to fit the 3-line comment limit. Condense
the invariant and behavior summary in the affected comment blocks around the
watch eviction helper and related reenqueue logic, and remove the longer
timeline/rationale text while keeping the key symbols like
`reenqueue_recreated_files` and `on_file_update` clear enough to locate the
code.

Source: Coding guidelines

test/cli/hot/hot.test.ts (1)

443-446: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the test rationale comment within 3 lines.

This new 4-line block exceeds the repository comment-length rule; it can be shortened without losing the invariant.

Proposed comment trim
-// Unlike the entrypoint (covered above), an *imported* file has no dedicated
-// recovery path: `rm` destroys the inode its per-file watch is attached to and
-// evicts its watchlist entry, so only the parent-directory watch can notice
-// the recreated path. Before the fix, --hot served the old module forever.
+// Imported files rely on parent-directory recovery after deletion/recreation;
+// before this fix, --hot kept serving the stale module forever.

As per coding guidelines, “Keep code comments to 3 lines max.”

🤖 Prompt for 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.

In `@test/cli/hot/hot.test.ts` around lines 443 - 446, Shorten the explanatory
comment in the hot reload test so it stays within the repository’s 3-line limit
while preserving the same invariant. Update the nearby rationale in the
`hot.test.ts` test block (the imported-file recovery explanation) by removing
redundant detail and keeping only the essential watch/inode behavior summary. Do
not change the test logic or assertions, only trim the comment text.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@src/jsc/hot_reloader.rs`:
- Around line 844-850: The new helper doc comments in hot_reloader need to be
shortened to fit the 3-line comment limit. Condense the invariant and behavior
summary in the affected comment blocks around the watch eviction helper and
related reenqueue logic, and remove the longer timeline/rationale text while
keeping the key symbols like `reenqueue_recreated_files` and `on_file_update`
clear enough to locate the code.

In `@test/cli/hot/hot.test.ts`:
- Around line 443-446: Shorten the explanatory comment in the hot reload test so
it stays within the repository’s 3-line limit while preserving the same
invariant. Update the nearby rationale in the `hot.test.ts` test block (the
imported-file recovery explanation) by removing redundant detail and keeping
only the essential watch/inode behavior summary. Do not change the test logic or
assertions, only trim the comment text.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 68d6f674-7a55-43f9-8665-9bd49f8e43e2

📥 Commits

Reviewing files that changed from the base of the PR and between 16dc8ec and 1bb630c.

📒 Files selected for processing (2)
  • src/jsc/hot_reloader.rs
  • test/cli/hot/hot.test.ts

Trim the new helper and field comments down to the invariants. The
timeline and rationale live in the PR description; the reason each
re-enqueue gate exists stays as an inline comment at that gate.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/jsc/hot_reloader.rs:889-893 — This check (added in 1bb630c, after the last CI run on de0a116) drops recovery entries that were recorded earlier in the same batch: watchlist_hashes is the batch-entry snapshot, and remove_at_index only appends to evict_list (the actual swap_remove happens in flush_evictions() after this loop), so a hash that was just remember_deleted_watched and evict-marked is still in the snapshot and return false permanently discards it. On macOS a single rm dep.ts posts NOTE_DELETE(file) + NOTE_WRITE(dir) in one kevent() batch in kernel order — when the file event is processed first, the directory arm immediately drops the just-recorded entry, the later recreate finds deleted_watched_files empty, and --hot serves the stale module forever (the exact bug this PR fixes; the new test will be order-dependent flaky on macOS). Fix: move the watchlist_hashes.contains(hash) check after the dirname/exists filter, where "exists on disk AND in watchlist" genuinely means already re-watched.

    Extended reasoning...

    What the bug is

    Commit 1bb630c (HEAD, post-dating the robobun CI status which is for de0a116) added a watchlist_hashes.contains(hash) guard at the top of the retain closure in reenqueue_recreated_files. The intent — per the doc comment — is: "watchlist_hashes is the caller's snapshot taken on this batch's entry, so it already reflects every re-add the JS thread performed since the record was made." That reasoning is correct only when the record was made in a previous batch. For an entry recorded earlier in the same on_file_update batch, the snapshot reflects the pre-eviction state, not a JS-thread re-add, so the check is a false positive and the entry is permanently dropped from deleted_watched_files.

    The code path that triggers it

    watchlist_hashes is hashes = slice.items_hash() taken at the top of on_file_update. ctx.remove_at_index(...) (Watcher.rs) does not mutate the hash column — it only appends to evict_list[evict_list_i++]. The actual swap_remove happens in flush_evictions(), which runs via the _flush scopeguard after the event loop completes. So any hash evict-marked during this batch is still present in hashes when reenqueue_recreated_files runs later in the same batch.

    The check also runs before the dirname(deleted_path) != dir_no_slash filter, so a Directory event for any watched directory sweeps and drops every same-batch-recorded entry, not just entries under that directory.

    pending_count does not save it: Task::enqueue (which does fetch_add(1)) runs via the current_task scopeguard, which is declared before _flush and so drops after it — both at batch end. Mid-batch with count >= 1, pending_count is still 0.

    Step-by-step proof (macOS / kqueue)

    KEventWatcher::watch_loop_cycle does not sort events — it only merges consecutive same-udata events and preserves kernel order. It also has a 100µs coalescing kevent() window that makes it likely both events from a single rm land in one batch. A single rm dep.ts posts NOTE_DELETE on the file vnode and NOTE_WRITE on the parent directory vnode. When the kernel delivers file-before-directory:

    1. File arm for dep.ts: event.op.contains(DELETE) → ctx.remove_at_index(File, idx, ...) (appends idx to evict_list; hashes[idx] unchanged) → self.remember_deleted_watch(dep_path, dep_hash) (file gone, so recorded) → current_task.append(dep_hash) (count = 1, not yet enqueued).
    2. Directory arm for the parent dir → self.reenqueue_recreated_files(dir, hashes, &mut current_task):
      • deleted_watched_files.is_empty() → false (contains dep).
      • pending_count.load() → 0 (enqueue is end-of-batch).
      • retain closure for dep: watchlist_hashes.contains(&dep_hash) → true (eviction not yet flushed) → return false → entry dropped from the map.
    3. Batch ends: _flush → flush_evictions() swap-removes dep; then current_task guard → enqueue() → JS thread reloads → Cannot find module './dep.ts' → dep is not re-added to the watchlist.
    4. User recreates dep.ts → parent dir NOTE_WRITE → next batch's reenqueue_recreated_files: deleted_watched_files.is_empty() → true → early return. On kqueue, affected_len is only populated on DELETE, so the dir-ent scan also produces nothing. No reload is enqueued and the watch is never re-armed. --hot serves the stale "A" forever — exactly the bug this PR set out to fix.

    The same shape is reachable on Linux/Windows whenever a file's eviction (the inotify directory-arm remember_deleted_watch, or the Windows File arm) is followed in the same batch by a Directory event for a different watched directory — e.g. rm src/dep.ts while a build tool concurrently touches dist/ — because the contains check runs before the dirname filter.

    Why CI didn't catch it

    The robobun status comment is for de0a116; 1bb630c added this check afterward and has no CI run. The new test uses a single directory: on Windows the dir entry sorts to a lower watchlist index than the file (append_file_maybe_lock adds the parent first), so the directory event is processed first and the bug doesn't trigger; on Linux the eviction comes from the directory arm itself so there's no later directory event in the same batch. On macOS it's kernel-order-dependent — the new test will likely manifest as flaky (Expected: "B", Received: "A" when file-before-dir).

    Impact

    Order-dependent regression of the PR's core fix on macOS (single-directory rm), and reachable on all platforms via the multi-directory same-batch path. Once hit, recovery is permanently lost for that import.

    How to fix

    Move the watchlist_hashes.contains(hash) check after the dirname/exists filter:

    self.deleted_watched_files.retain(|deleted_path, hash| {
        let deleted_path: &[u8] = deleted_path;
        if bun_core::dirname(deleted_path) != Some(dir_no_slash)
            || !bun_sys::exists(deleted_path)
        {
            return true;
        }
        // Already re-watched: an earlier reload re-imported it.
        if watchlist_hashes.contains(hash) {
            return false;
        }
        record_changed_path(deleted_path);
        task.append(*hash);
        false
    });

    With this ordering: in the macOS same-batch case the file doesn't exist yet → return true (kept); in the multi-directory case dirname mismatches → return true (kept); and in the genuine cross-batch "JS thread re-added it" case the file does exist on disk AND its hash is in the (post-flush, fresh-batch) snapshot → return false without re-enqueue, which is correct. An alternative is to also consult ctx.evict_list[..evict_list_i] and treat "in evict_list" as not-re-watched, but the reorder is simpler and sufficient.

The watchlist-membership gate ran first, but the hash snapshot is taken
on batch entry and evictions are only flushed after the event loop, so a
path recorded earlier in the same batch still looked watched and its
record was permanently dropped before the filters even ran. kqueue does
not sort events by watchlist index the way inotify and the Windows
watcher do, so on macOS a single rm can deliver the file's NOTE_DELETE
before the parent directory's NOTE_WRITE in one batch and hit exactly
that. The check also ran before the dirname filter, so a directory event
for an unrelated directory could sweep every same-batch record.

Move it after the dirname and existence filters: an absent or
other-directory path is kept for a later event, and a path that exists
and is in the snapshot genuinely means an earlier reload re-added it.
@robobun

robobun commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

The reenqueue_recreated_files ordering finding is correct, and it is the sharpest catch on this PR. I verified every link of it against the source:

  • Watcher::remove_at_index only appends to evict_list; the swap_remove is deferred to flush_evictions, which runs from the _flush scopeguard after the event loop. So the hashes snapshot taken on batch entry still contains a hash that was evict-marked earlier in the same batch.
  • Task::enqueue (and its pending_count.fetch_add) runs from the current_task scopeguard, which is declared before _flush and therefore drops after it. So pending_count is still 0 mid-batch and does not save it either.
  • KEventWatcher::watch_loop_cycle has no sort_unstable_by (unlike the inotify and Windows watchers); it only merges consecutive same-udata events, so on macOS a file's NOTE_DELETE can precede the parent directory's NOTE_WRITE in one batch.

So with the check placed first, that batch would record the path in the File arm, then the directory arm in the same batch would see the not-yet-flushed hash as "watched", permanently drop the record, and the later recreate would find nothing to recover. That would make the new test order-dependent on macOS and could lose the record on any platform via a same-batch directory event for an unrelated directory (the check also preceded the dirname filter).

Fixed in 27699a6 exactly as suggested: the membership check now runs after the dirname and existence filters, so an absent or other-directory path is kept for a later event, and "exists on disk and in the snapshot" genuinely means an earlier reload re-added it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/jsc/hot_reloader.rs`:
- Around line 487-490: Trim the newly added documentation comments in
hot_reloader so each block stays within the repo’s 3-line maximum while
preserving the key invariants. Shorten the comment above
reenqueue_recreated_files and the similar comment blocks around
tombstones-related logic, keeping only the essential facts: absolute-path watch
hash, file-gone/recreated behavior, and watcher-thread-only scope. Make the
wording more compact without changing meaning or removing the implementation
constraints.
🪄 Autofix (Beta)

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: c0ee367c-0f0a-4e95-8b85-c3e4c072e6c4

📥 Commits

Reviewing files that changed from the base of the PR and between 1bb630c and 27699a6.

📒 Files selected for processing (2)
  • src/jsc/hot_reloader.rs
  • test/cli/hot/hot.test.ts

Comment thread src/jsc/hot_reloader.rs Outdated
Comment-only. The one load-bearing fact each carried stays: the
membership check's placement after the existence filter is required
because same-batch evictions are still in the watchlist snapshot.
Comment thread src/jsc/hot_reloader.rs Outdated
…ed a reload

pending_count alone only sees reloads that have already been enqueued;
a reload the current batch's File arm appended is still only buffered in
the Task (Task::enqueue runs from the scopeguard at batch end). Treat
that buffered task the same way: it reads the live filesystem when it
runs, so it already covers every recorded path that has reappeared by
then, and appending here would only stack a duplicate.

This also closes the one residual the gate reordering left open: a path
recorded earlier in the same batch that is recreated externally before
the directory arm runs would pass the existence filter and be falsely
dropped by the stale same-batch hash snapshot.
@robobun

robobun commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

The core of this finding is against 1bb630c, which is two commits behind: the diff quoted above has watchlist_hashes.contains(hash) running first, and the "Why the existing gates don't help" section says the !bun_sys::exists(...) check is never reached because contains short-circuits it. That was exactly the previous review's finding, and 27699a6 already applied its suggested fix: the membership check now runs after the dirname and existence filters. Re-running the Windows step-by-step against the current code: at step 5, b.ts was just removed, so !bun_sys::exists(b.ts) returns true (kept) before contains is reached, and the step 7 recreate's fresh batch (post-flush snapshot, b.ts present on disk and absent from the watchlist) enqueues the recovery.

Two parts of it are new and correct, though, and both are taken:

  1. The swap_remove observation. I had been reasoning that Windows was safe from the same-batch file-before-directory ordering because process_watch_event_batch sorts by index and append_file_maybe_lock always appends the parent directory before the file. You are right that flush_evictions's swap_remove can reorder a file below its parent directory after any prior eviction, so that reasoning was wrong. The reorder in 27699a6 covers it regardless, but I was relying on an invariant that does not hold.

  2. The task.count > 0 gate, adopted in fc26d87. pending_count only sees reloads that have already been enqueued; a reload the current batch's File arm appended is still only buffered in the Task until the scopeguard runs enqueue at batch end. Treating that buffered task as "a reload that will read the live filesystem" makes the invariant complete, and it independently closes the one residual the reorder alone left: a path recorded earlier in the batch and then recreated externally before the directory arm runs would pass the existence filter and be falsely dropped by the stale same-batch snapshot.

With both, I walked each kqueue kernel ordering by hand: file-before-directory in one batch now keeps the record via both !exists() and task.count > 0, the batch's own reload fails since the file is gone, and the later recreate's batch (task.count == 0, post-flush snapshot, path present) enqueues the recovery. Directory-before-file is the trivially safe order. All 16 test/cli/hot tests, the fail-before check, and the 10-target cross-compile are green locally.

@robobun

robobun commented Jun 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status

The diff is green. Every remaining red lane is an unrelated flake, each verified by finding the identical failure on a branch that touches none of this code.

The new test, should hot reload an imported file after it is deleted and recreated, passes on every lane of every build since 16dc8ec (the commit that extended the fix to Windows). Every hot.test.ts result on this PR is 12 pass, 1 fail, and the 1 fail is never the new test.

The failures on the latest build (66851, commit fc26d87):

  1. hot.test.ts > should work with sourcemap generation, Windows lanes only. Two signatures, both pre-existing:
    • invalid string: at ...hot-runner-root.js:1:12 (a non-sourcemapped stack frame). The identical failure, same test, same lane family, is on build 66799 for farm/9a007f8a/subtlecrypto-exception-checks, whose diff against main touches none of src/jsc/hot_reloader.rs, src/watcher/, or test/cli/hot/ (verified with git diff --stat main... on those paths: empty).
    • error: Module not found 'hot-runner-root.js' / ENOENT reading 'hot-runner-root.js'. This is that test's own Windows atomic-write shim: writeHotFileAtomicSync does rmSync(root) then renameSync(tmp, root) (the rmSync exists because renameSync onto an existing target can EPERM on Windows), and the delete-triggered reload, enqueued by the unchanged File-arm DELETE append, sometimes reads root inside that gap. The identical failure is on 66785 (farm/cb2baa7b/define-computed-member-access) and 66784 (claude/spawn-uid-gid-options), neither of which touches the watcher.
  2. test/bake/dev/server-sourcemap.test.ts on 2019 x64-baseline. The bake dev server has its own WatcherContext::on_file_update (src/runtime/bake/dev_server/lifecycle.rs) and never goes through the NewHotReloader this PR changes, so this is unreachable from the diff. The same file also fails on 66841 (farm/71122839/als-unhandled-rejection), on alpine linux.
  3. test/js/node/test/parallel/test-net-connect-memleak.js on alpine 3.23 x64 and 3.23 x64-baseline (the two lanes that showed up after the build finished). The test does globalThis.gc() once and then asserts a dropped net.connect socket was collected; collected comes back false. This PR touches nothing near net or GC (its complete file list is src/jsc/hot_reloader.rs, test/cli/hot/hot.test.ts, and two autofix.ci docs files), and this test is currently failing on those two alpine lanes for everyone: it appears on 10 of the last 70 builds across 9 unrelated branches, for example 66886 (patch-symlink-nofollow), 66884 (test-done-error-hook), 66879 (security-round-11), 66868 (fix-vm-global-freeze), and 66867 (node-http-http2-compat), always on the same two lanes. hot.test.ts itself passed on every Linux lane.
  4. darwin 26 aarch64 - test-bun: buildkite-agent artifact download timed out after 120s; the shard never ran a test. The same lane hit this on the previous build too.
  5. The flaky-retry bucket (recovered): dev-and-prod.test.ts, napi.test.ts, fetch-tls-abortsignal-timeout.test.ts.

One correction to an earlier comment of mine here: I had attributed the :1:12 frames to this PR's re-enqueue stacking a second reload, because they were absent from the one build right after the coalescing gates landed. That was three samples and build 66799 shows the signature without any of this code, so the attribution was wrong: it is a pre-existing flake. The gates themselves stand on their own, verified justifications, which are in the PR description: they eliminate the gratuitous second reload per atomic save that this PR's first revision did introduce on macOS (where it timed out should work with sourcemap generation at 4 of 50 cycles on darwin 26 aarch64 in 66766), and the membership check's placement closes the same-batch false drop found in review.

I have already used the one ci: retrigger re-roll on this PR, so I am stopping here rather than pushing another. Repro, cause, fix, and verification are in the PR description.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant