fix(watcher): handle vim atomic save race on macOS#23566
Conversation
On macOS, vim's atomic save sequence causes a race condition with kqueue: 1. Old file gets NOTE_RENAME (inode deleted by vim) 2. Hot reloader triggers reload immediately 3. New file hasn't been renamed into place yet → ENOENT error 4. Vim completes rename (a.js~ → a.js) 5. Directory gets NOTE_WRITE but file already removed from watchlist This is macOS-specific because kqueue watches file descriptors/inodes, not paths. When vim deletes the old inode, the fd becomes stale. On Linux, inotify watches paths and receives IN.MOVED_TO (not IN.MOVE_SELF), so files aren't removed from the watchlist during atomic saves. Fix: When the entrypoint receives NOTE_RENAME on macOS, defer the reload until the parent directory receives NOTE_WRITE. Then verify the file exists via faccessat() before triggering reload. This only applies to the entrypoint since dependencies have enough buffering time during import graph traversal. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 10:01 PM PT - Oct 14th, 2025
❌ @Jarred-Sumner, your commit d7532e1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 23566That installs a local version of the PR into your bun-23566 --bun |
WalkthroughAdds slow-path file watching and a macOS kqueue helper to the Watcher; integrates main-entry watching into the hot reloader and VM; changes hot-reload APIs to accept an optional entry_path and updates call sites; adds PathName.findExtname; tweaks faccessat path handling; updates CLAUDE.md import guidance. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
Comment |
WalkthroughAdds macOS watcher helpers and a slow path for path-based file watching. Extends hot reloader to track the entry file, accept optional entry_path, and integrate watcher updates via VirtualMachine. Updates call sites to new signatures. Introduces PathName.findExtname and adjusts sys.faccessat path handling. Updates conventions doc. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/CLAUDE.md(1 hunks)src/Watcher.zig(3 hunks)src/bun.js.zig(3 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/hot_reloader.zig(11 hunks)src/bundler/bundle_v2.zig(1 hunks)src/cli/test_command.zig(1 hunks)src/fs.zig(1 hunks)src/sys.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/fs.zigsrc/bundler/bundle_v2.zigsrc/bun.js/VirtualMachine.zigsrc/sys.zigsrc/Watcher.zigsrc/bun.js.zigsrc/cli/test_command.zigsrc/bun.js/hot_reloader.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/fs.zigsrc/bundler/bundle_v2.zigsrc/bun.js/VirtualMachine.zigsrc/sys.zigsrc/Watcher.zigsrc/bun.js.zigsrc/cli/test_command.zigsrc/bun.js/hot_reloader.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zigsrc/bun.js/hot_reloader.zig
🧠 Learnings (3)
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/CLAUDE.md
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Place import directives at the bottom of Zig files
Applied to files:
src/CLAUDE.md
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/CLAUDE.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/CLAUDE.md (1)
10-11: LGTM! Documentation updates align with coding guidelines.The updated conventions correctly reflect the auto-formatter behavior and clarify the preferred import syntax. These changes are consistent with the retrieved learnings.
Based on learnings.
src/cli/test_command.zig (1)
1521-1522: LGTM! Correct adaptation to updated API signature.Passing
nullfor theentry_pathparameter is appropriate in the test command context, where multiple test files are executed without a single designated entrypoint.src/fs.zig (1)
1565-1568: LGTM! Clean and efficient implementation.The method correctly extracts the file extension including the leading dot. The implementation is straightforward and handles the common cases well:
- Files with extensions: returns ".ext"
- Files without extensions: returns ""
- Files with multiple dots: returns the last extension
Note: For hidden files starting with a dot (e.g., ".gitignore"), this returns the entire filename. This is consistent with the documented behavior of "finding the last dot and returning from there to the end."
src/bundler/bundle_v2.zig (1)
938-938: LGTM! API update aligns with hot reload enhancement.The addition of
nullas the second parameter correctly reflects the updatedenableHotModuleReloadingsignature. At this initialization point, entry points have not been enqueued yet (they are added later viaenqueueEntryPoints), so passingnullis appropriate. The watcher will receive the actual entry point path when files are added to the watch list.
| if (std.fs.path.dirname(file)) |dir| { | ||
| bun.assert(bun.isSliceInBuffer(dir, file)); | ||
| bun.assert(file.len > dir.len + 1); | ||
| main.dir = file[0 .. dir.len + 1]; | ||
| main.dir_hash = Watcher.getHash(dir); | ||
| } |
There was a problem hiding this comment.
Fix directory hash mismatch so the macOS rename slow-path ever fires.
MainFile.init hashes the dirname without its trailing separator, but Watcher.appendFileMaybeLock records directory hashes on dirWithTrailingSlash(). Because of the mismatch, this.main.dir_hash == current_hash never becomes true, so the NOTE_WRITE from the parent directory is ignored, is_waiting_for_dir_change stays stuck, and the entry file is removed from the watchlist without ever scheduling a reload. After the first vim-style atomic save the hot-reload loop stalls permanently. Please hash the exact slice stored in main.dir (with its separator) so the comparison matches and the reload resumes.
- main.dir_hash = Watcher.getHash(dir);
+ main.dir_hash = if (main.dir.len > 0) Watcher.getHash(main.dir) else 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (std.fs.path.dirname(file)) |dir| { | |
| bun.assert(bun.isSliceInBuffer(dir, file)); | |
| bun.assert(file.len > dir.len + 1); | |
| main.dir = file[0 .. dir.len + 1]; | |
| main.dir_hash = Watcher.getHash(dir); | |
| } | |
| if (std.fs.path.dirname(file)) |dir| { | |
| bun.assert(bun.isSliceInBuffer(dir, file)); | |
| bun.assert(file.len > dir.len + 1); | |
| main.dir = file[0 .. dir.len + 1]; | |
| main.dir_hash = if (main.dir.len > 0) Watcher.getHash(main.dir) else 0; | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/hot_reloader.zig around lines 148 to 153, MainFile.init currently
computes main.dir_hash from the dirname slice without its trailing separator but
stores main.dir as the dirname with the separator; update the logic so the hash
is computed over the exact slice stored in main.dir (i.e., include the trailing
separator) so the stored main.dir_hash matches the Watcher records; ensure you
still assert the slice is in the buffer and lengths are consistent before
assigning main.dir and then call Watcher.getHash on the same slice with the
separator.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/sys.zig (1)
3185-3186: Critical: Infinite recursion due to lost sentinel on pointerThis issue was already flagged in previous review comments. Passing
&pathloses the sentinel type information, causing the!has_sentinelbranch to execute recursively until stack overflow.Apply this diff to fix:
- return faccessat(dir_fd, &path); + return faccessat(dir_fd, path);Based on previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/CLAUDE.md(1 hunks)src/Watcher.zig(3 hunks)src/bun.js.zig(3 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/hot_reloader.zig(11 hunks)src/bundler/bundle_v2.zig(1 hunks)src/cli/test_command.zig(1 hunks)src/fs.zig(1 hunks)src/sys.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sys.zigsrc/bun.js.zigsrc/fs.zigsrc/bun.js/VirtualMachine.zigsrc/Watcher.zigsrc/cli/test_command.zigsrc/bundler/bundle_v2.zigsrc/bun.js/hot_reloader.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sys.zigsrc/bun.js.zigsrc/fs.zigsrc/bun.js/VirtualMachine.zigsrc/Watcher.zigsrc/cli/test_command.zigsrc/bundler/bundle_v2.zigsrc/bun.js/hot_reloader.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zigsrc/bun.js/hot_reloader.zig
🧠 Learnings (4)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/VirtualMachine.zig
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/CLAUDE.md
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Place import directives at the bottom of Zig files
Applied to files:
src/CLAUDE.md
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/CLAUDE.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (10)
src/bun.js.zig (3)
314-316: LGTM: pass entry_path to HMR enablersSignature change is applied correctly.
331-332: LGTM: ensure main is added to watcher before tickingAdds robustness for initial failures.
391-409: Ensure VM handler is null-safeThe repeated calls are fine, but reportExceptionInHotReloadedModuleIfNeeded must handle pending_internal_promise == null to avoid a crash.
Also applies to: 400-400, 407-407
src/Watcher.zig (1)
386-387: LGTM: reuse kqueue registration helper for filesReduces duplication.
src/bun.js/hot_reloader.zig (6)
28-37: LGTM: expose slow-path add by pathWrapper is minimal and correct.
121-157: LGTM: MainFile tracks dir/hash and macOS rename state correctlyComputes dir slice with trailing sep and hashes that exact slice; resolves prior mismatch.
238-252: LGTM: HMR enable accepts entry_path and initializes MainFileInitialization is correct and side-effect free until watcher start.
367-400: LGTM: defer entry reload on macOS NOTE_RENAME for entrypointCorrectly skips immediate reload, sets wait flag, and resumes on subsequent writes.
421-433: LGTM: dir NOTE_WRITE resumes entry reload after atomic savefaccessat against parent dir fd + basename is the right existence check.
481-481: Loader lookup relies on correct ext parsingThis path uses Fs.PathName.findExtname(changed_name). Ensure the ext helper uses basename only (see fs.zig comment) to avoid misclassification when directory names contain dots.
Also applies to: 537-539
- Fix faccessat recursion by passing sentinel slice directly - Add null-check for pending_internal_promise to prevent crash - Skip watcher add when main path is empty - Fix findExtname to operate on basename only (avoid mis-parsing paths with dots in directories) - Cast udata consistently in addFileDescriptorToKQueueWithoutChecks - Fix duplicate-watch race in addFileByPathSlow by using addFile which rechecks under lock - Preserve fast-path check to avoid opening file descriptor when already watching 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
On macOS, std.c.faccessat expects [*:0]const u8 (pointer to sentinel), not [1023:0]u8 (array). Pass &path to properly coerce the array to a pointer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Watcher.zig(3 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/fs.zig(1 hunks)src/sys.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/VirtualMachine.zigsrc/Watcher.zigsrc/fs.zigsrc/sys.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/VirtualMachine.zigsrc/Watcher.zigsrc/fs.zigsrc/sys.zig
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/VirtualMachine.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (5)
src/sys.zig (1)
3185-3186: Past recursion issue resolved—code is correct.The infinite-recursion bug (passing
&pathinstead ofpath) identified in the previous review has been fixed. The current implementation correctly passes the sentinel slice itself, so the recursive call seesstd.meta.sentinel([:0]u8) == 0and skips re-normalization.src/fs.zig (1)
1565-1575: Past directory-dot parsing issue resolved—logic is correct.The implementation now properly extracts the basename before searching for the extension dot, preventing false matches on directory components like
dir.ext/file. Thedot > 0check correctly excludes dotfiles (e.g.,.gitignore→""), aligning withNodeJSPathName.initbehavior.src/bun.js/VirtualMachine.zig (2)
680-695: Past review concern addressed: null-check added.The null-check at lines 681-685 correctly addresses the previous review comment about potential null-deref. The function now safely handles both cases (promise present or absent) and ensures the main file is added to the watcher in both branches, which aligns with the hot-reload use case.
696-702: Past review concern addressed: empty-path guard added.The guard at line 699 correctly addresses the previous review comment about avoiding operations on empty paths. The implementation properly checks watcher state, validates the path, and invokes the slow-path watcher registration with the appropriate loader.
src/Watcher.zig (1)
315-347: Past review concern addressed: udata cast to usize.The cast at line 331 (
@as(usize, @intCast(watchlist_id))) correctly addresses the previous review comment about consistent udata casting. The kqueue event registration is properly configured for macOS vnode monitoring.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Watcher.zig(3 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/fs.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/VirtualMachine.zigsrc/fs.zigsrc/Watcher.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/VirtualMachine.zigsrc/fs.zigsrc/Watcher.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
robobun
force-pushed
the
jarred/darwin-watcher
branch
from
bcfeb49 to
9ba2489
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between bcfeb49337b06f669dcc5448d4c681c60bc1eac7 and 9ba248986a6bade2e25ca8a8d6a9ea362f714f6e.
📒 Files selected for processing (1)
src/Watcher.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/Watcher.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/Watcher.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (1)
src/Watcher.zig (1)
386-386: LGTM! Clean refactor.Consolidating the kqueue registration into the helper function improves maintainability.
When two threads race through addFileByPathSlow, the second thread can open a file descriptor after the fast-path check but before calling addFile. If the first thread already added the file, addFile returns success without consuming the fd (on macOS without atomic_file_watcher), causing a leak. Fix: Keep the fast-path check to avoid opening files unnecessarily, but after addFile returns success, check if the stored fd differs from our opened fd. If so, close our fd since it wasn't used. Also prevent double-close when file is removed concurrently: only close if the entry still exists in the watchlist. If it was removed, flushEvictions already closed it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
jarred/darwin-watcher
branch
from
9ba2489 to
150dc37
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 9ba248986a6bade2e25ca8a8d6a9ea362f714f6e and 150dc37.
📒 Files selected for processing (1)
src/Watcher.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/Watcher.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/Watcher.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (1)
src/Watcher.zig (1)
386-386: LGTM: Clean refactoring.The inline kqueue registration code has been properly extracted to the new helper function.
…d improve race condition comments Add comprehensive doc comment for addFileDescriptorToKQueueWithoutChecks explaining: - Function purpose (kqueue registration on macOS) - Preconditions callers must satisfy - Silent error handling behavior Expand race condition comments in addFileByPathSlow to document all three scenarios: 1. Entry removed after fd stored (don't close - already closed by flushEvictions) 2. Entry exists with different fd (close our unused fd to prevent leak) 3. Entry exists with same fd (don't close - our fd is stored) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Watcher.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/Watcher.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/Watcher.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/Watcher.zig (2)
315-356: LGTM: kqueue registration is correct.The documentation is comprehensive, and the implementation correctly registers a vnode event with kqueue. Past review concerns about udata casting and documentation have been properly addressed.
Note: This function is macOS-specific but exposed as a public API on all platforms. While the function name clearly indicates its platform-specific nature (includes "KQueue"), and it's only called from macOS code paths in practice (line 395), consider whether to add a compile-time assertion or conditional compilation if cross-platform compilation with incorrect usage is a concern.
658-681: LGTM: fd lifecycle management is correct.The race condition handling correctly addresses all three scenarios identified in past reviews:
- Entry removed after our fd was stored → don't close (prevents double-close)
- Entry exists with different fd → close our unused fd (prevents leak)
- Entry exists with same fd → don't close (our fd is now stored)
The conditional check at line 676 (
maybe_idx != null and stored_fd.native() != fd.native()) properly distinguishes between these cases.
Add comprehensive doc comment for addFileByPathSlow explaining: - Function purpose (lazy watch for files outside import graph) - Platform behavior (opens O_EVTONLY fd on macOS) - Thread-safety guarantees - Return value semantics Add validation to reject empty file_path before computing hash or opening file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
| { | ||
| this.mutex.lock(); | ||
| const already_watched = this.indexOf(hash) != null; | ||
| this.mutex.unlock(); | ||
|
|
||
| if (already_watched) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this a nested block?
| if (promise_opt == null) { | ||
| this.addMainToWatcherIfNeeded(); | ||
| return; | ||
| } | ||
| var promise = promise_opt.?; |
There was a problem hiding this comment.
edit: see followup suggestion; this can be even simpler
| if (promise_opt == null) { | |
| this.addMainToWatcherIfNeeded(); | |
| return; | |
| } | |
| var promise = promise_opt.?; | |
| var promise = promise_opt orelse { | |
| this.addMainToWatcherIfNeeded(); | |
| return; | |
| }; |
| }; | ||
| } | ||
|
|
||
| pub inline fn addFileByPathSlow( |
There was a problem hiding this comment.
Are we sure this function needs to be semantically inlined?
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
Co-authored-by: taylor.fish <contact@taylor.fish>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Watcher.zig(3 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/hot_reloader.zig(11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/hot_reloader.zigsrc/Watcher.zigsrc/bun.js/VirtualMachine.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/hot_reloader.zigsrc/bun.js/VirtualMachine.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/hot_reloader.zigsrc/Watcher.zigsrc/bun.js/VirtualMachine.zig
🔇 Additional comments (5)
src/Watcher.zig (2)
315-355: LGTM! Well-documented public API addition.The kqueue registration helper is properly documented with clear preconditions and behavior notes. The implementation correctly sets up the kevent structure with appropriate flags and stores the watchlist_id in udata.
Based on learnings from past review comments, this implementation correctly addresses the type casting concern.
639-698: LGTM! Thread-safe slow-path watching with proper race handling.The implementation correctly:
- Validates input (empty path check)
- Uses locking to prevent races when checking if already watched
- Opens file descriptor only on macOS with O_EVTONLY flag
- Handles three race scenarios properly:
- Entry removed (maybe_idx == null): fd was stored then closed by flushEvictions → don't close
- Entry exists with different fd: another thread added entry, addFile didn't use our fd → close ours
- Entry exists with same fd: our fd was stored → don't close
Based on learnings from past review comments, the race condition handling and documentation were refined in previous commits.
src/bun.js/hot_reloader.zig (2)
121-157: LGTM! Correct directory hash computation.The
MainFile.initimplementation correctly addresses the hash mismatch issue flagged in past reviews:
- Stores
main.dirwith the trailing separator (line 151)- Computes
main.dir_hashfrom the exact slice stored inmain.dir(line 152)This ensures the hash matches what
Watcher.appendFileMaybeLockrecords viadirWithTrailingSlash(), so the NOTE_WRITE event from the parent directory will be properly detected.Based on learnings from past review comments, this correctly fixes the directory hash mismatch.
367-432: LGTM! Correct implementation of vim atomic save workaround.The event handling logic correctly implements the race condition fix:
File events (lines 367-396):
- On NOTE_RENAME for entrypoint: set
is_waiting_for_dir_changeflag and skip reload- On NOTE_WRITE for entrypoint after rename: clear flag and proceed with reload (file is back)
Directory events (lines 421-432):
3. On NOTE_WRITE for parent directory: verify file exists withfaccessat(), clear flag, and trigger reloadThe dual-path approach (file write or directory write) correctly handles both the vim atomic save case and simpler file modification cases.
Based on learnings from past review comments, the comments accurately describe the vim save process.
src/bun.js/VirtualMachine.zig (1)
680-696: LGTM! Clean hot-reload exception handling with main file watching.Both functions are well-implemented:
reportExceptionInHotReloadedModuleIfNeeded(lines 680-688):
- Uses
deferto ensureaddMainToWatcherIfNeeded()is always called- Uses
orelse returnfor clean early exit when no pending promise- Properly handles rejected promises and marks them as handled
addMainToWatcherIfNeeded(lines 690-696):
- Guards against empty path before calling the slow path watcher
- Correctly resolves the loader from file extension
Based on learnings from past review comments, these implementations incorporate the suggested simplifications using defer and orelse.
| pub inline fn addFileByPathSlow( | ||
| this: ImportWatcher, | ||
| file_path: string, | ||
| loader: options.Loader, | ||
| ) bool { | ||
| return switch (this) { | ||
| inline .hot, .watch => |w| w.addFileByPathSlow(file_path, loader), | ||
| else => true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing the inline keyword.
The function is a simple delegation that switches on a union type. While inlining may provide minor performance benefits, the inline keyword forces semantic inlining which prevents the compiler from making optimization decisions. Unless profiling shows this is a hot path where inlining provides measurable benefit, consider removing inline and let the compiler decide.
As noted in past review comments, this was previously questioned by taylordotfish.
🤖 Prompt for AI Agents
In src/bun.js/hot_reloader.zig around lines 28 to 37, the function is declared
as "pub inline fn addFileByPathSlow" which forces semantic inlining; remove the
inline keyword so the compiler can decide whether to inline (change the
signature to "pub fn addFileByPathSlow(...)") and then rebuild and run tests to
ensure no performance regressions or regressions in behavior.
3 participants
Summary
Fixes a race condition on macOS where editing the entrypoint with vim's atomic save causes "Module not found" errors during hot reload.
Root Cause
On macOS, kqueue watches file descriptors/inodes, not paths. Vim's atomic save sequence:
a.jstoa.js~→ kqueue reportsNOTE_RENAMEon watched fdENOENTerrora.js, and writes file contents into itNOTE_WRITEbut file already removed from watchlistThis is macOS-specific because:
IN.MOVED_TO(notIN.MOVE_SELF), so files stay in watchlistSolution
When the entrypoint receives
NOTE_RENAMEon macOS:is_waiting_for_dir_changeflagNOTE_WRITEeventfaccessat()to verify file existsThis only applies to the entrypoint because dependencies have buffering time during import graph traversal.
Test Plan
Manual testing with vim on macOS:
bun --hot entrypoint.js:w)🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com