Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Watcher.zig
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,11 @@ pub const WatchEvent = struct {
write: bool = false,
move_to: bool = false,
create: bool = false,
_padding: u2 = 0,
/// True when the event concerns a directory (inotify IN_ISDIR).
/// Used by recursive fs.watch to know when to register a new
/// inotify watch on a newly-created subdirectory.
is_dir: bool = false,
_padding: u1 = 0,

pub fn merge(before: Op, after: Op) Op {
return .{
Expand All @@ -190,6 +194,7 @@ pub const WatchEvent = struct {
.rename = before.rename or after.rename,
.move_to = before.move_to or after.move_to,
.create = before.create or after.create,
.is_dir = before.is_dir or after.is_dir,
};
}

Expand Down
247 changes: 244 additions & 3 deletions src/bun.js/node/path_watcher.zig
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,46 @@ pub const PathWatcherManager = struct {
fn _fdFromAbsolutePathZ(
this: *PathWatcherManager,
path: [:0]const u8,
) bun.sys.Maybe(PathInfo) {
return this._fdFromAbsolutePathZImpl(path, .allow_file);
}

/// Directory-only variant of `_fdFromAbsolutePathZ`. Returns `ENOTDIR`
/// (without creating a `PathInfo` entry) when the target is a regular
/// file — the caller never gets back an `is_file` `PathInfo` and therefore
/// never has to release a fresh refs=1 entry via `_decrementPathRef`,
/// which would acquire `manager.mutex` then `Watcher.mutex` (via
/// `main_watcher.remove` on hash) and deadlock against the watcher
/// thread's `Watcher.mutex → manager.mutex` ordering in `onFileUpdate`.
///
/// Used by `NewSubdirTask` because a merged inotify event may OR-in
/// `IN_ISDIR` from a sibling name in the same batch (e.g. `mkdir sub;
/// touch file.txt`), causing the `file.txt` name to be queued as a
/// "new subdirectory" candidate; this helper lets the task reject
/// non-directories cleanly without ever taking ownership of an fd.
fn _dirFdFromAbsolutePathZ(
this: *PathWatcherManager,
path: [:0]const u8,
) bun.sys.Maybe(PathInfo) {
return this._fdFromAbsolutePathZImpl(path, .dir_only);
}

fn _fdFromAbsolutePathZImpl(
this: *PathWatcherManager,
path: [:0]const u8,
comptime mode: enum { allow_file, dir_only },
) bun.sys.Maybe(PathInfo) {
this.mutex.lock();
defer this.mutex.unlock();

if (this.file_paths.getEntry(path)) |entry| {
var info = entry.value_ptr;
if (mode == .dir_only and info.is_file) {
return .{ .err = .{
.errno = @truncate(@intFromEnum(bun.sys.E.NOTDIR)),
.syscall = .open,
} };
}
Comment thread
robobun marked this conversation as resolved.
info.refs += 1;
return .{ .result = info.* };
}
Expand All @@ -73,7 +107,7 @@ pub const PathWatcherManager = struct {
.windows => bun.sys.openDirAtWindowsA(bun.FD.cwd(), path, .{ .iterable = true, .read_only = true }),
}) {
.err => |e| {
if (e.errno == @intFromEnum(bun.sys.E.NOTDIR)) {
if (mode == .allow_file and e.errno == @intFromEnum(bun.sys.E.NOTDIR)) {
const file = switch (bun.sys.open(path, 0, 0)) {
.err => |file_err| return .{ .err = file_err.withPath(path) },
.result => |r| r,
Expand Down Expand Up @@ -159,6 +193,15 @@ pub const PathWatcherManager = struct {

const timestamp = std.time.milliTimestamp();

// Subdirectories created or moved into a watched tree during this
// batch. Registering a new inotify watch touches manager.mutex and
// Watcher.mutex, both of which are held (transitively) right now —
// onFileUpdate runs inside INotifyWatcher.watchLoopCycle with
// Watcher.mutex held. Defer registration to a WorkPool task that
// runs after both locks are released.
var new_subdirs: NewSubdirTask.List = .{};
defer if (new_subdirs.count > 0) NewSubdirTask.schedule(this, new_subdirs);

this.mutex.lock();
defer this.mutex.unlock();

Expand Down Expand Up @@ -256,9 +299,32 @@ pub const PathWatcherManager = struct {
}
}

// IN_ISDIR is set on the event mask when the event concerns
// a subdirectory. Pair with IN_CREATE/IN_MOVED_TO to detect
// a newly-created subdirectory that recursive watchers need
// to start watching.
const is_new_subdir = Environment.isLinux and event.op.is_dir and (event.op.create or event.op.move_to);
Comment thread
robobun marked this conversation as resolved.

for (affected) |changed_name_| {
const changed_name: []const u8 = bun.asByteSlice(changed_name_.?);
if (changed_name.len == 0 or changed_name[0] == '~' or changed_name[0] == '.') continue;
if (changed_name.len == 0) continue;

// Pre-existing: suppress editor swap/backup files (`.foo.swp`,
// `~foo`) from user-visible events. We still have to *register*
// dotfile-named subdirectories for recursive watchers below
// though, otherwise `.next/`, `.nuxt/`, `.cache/` created at
// runtime would be blind spots (inconsistent with the initial
// scan in `DirectoryRegisterTask.processWatcher`, which has no
// such filter).
const skip_emit = changed_name[0] == '~' or changed_name[0] == '.';

// Fast path: the common noisy case (every `.foo.swp` write, every
// `~backup` save) never needs to compute `path_slice` or iterate
// watchers because neither the emit nor the subdir-register gate
// will fire. `is_new_subdir` is loop-invariant, so this branch
// only short-circuits events that would have produced no
// observable effect.
if (skip_emit and !is_new_subdir) continue;

const file_path_without_trailing_slash = std.mem.trimRight(u8, file_path, std.fs.path.sep_str);

Comment thread
robobun marked this conversation as resolved.
Expand Down Expand Up @@ -302,7 +368,26 @@ pub const PathWatcherManager = struct {
continue;
}

watcher.emit(event_type.toEvent(path), hash, timestamp, false);
// A new subdirectory appeared under a recursive watcher's tree.
// Queue a task that will open it and register a fresh inotify
// watch — without this, events for files created inside the new
// subdirectory would never fire (#29677). Ownership of the
// refPendingDirectory ref transfers into NewSubdirTask; `add` is
// infallible (aborts on OOM) so there is no rollback path that
// would re-enter `manager.mutex` via `unrefPendingDirectory →
// PathWatcher.deinit → unregisterWatcher`.
//
// Runs even when `skip_emit` is true so dot-prefixed build
// output dirs (`.next`, `.nuxt`, `.cache`) are registered —
// matches `processWatcher`'s unconditional initial-scan
// registration.
if (is_new_subdir and watcher.recursive and watcher.refPendingDirectory()) {
new_subdirs.add(watcher, path_slice);
}
Comment thread
robobun marked this conversation as resolved.

if (!skip_emit) {
watcher.emit(event_type.toEvent(path), hash, timestamp, false);
}
}
}
}
Expand Down Expand Up @@ -555,6 +640,162 @@ pub const PathWatcherManager = struct {
}
};

/// WorkPool task that registers inotify watches for subdirectories that
/// appeared inside a recursive `fs.watch()` tree after the watcher started.
///
/// Created inside `onFileUpdate` (which runs holding Watcher.mutex and
/// takes manager.mutex). Actual registration — `_fdFromAbsolutePathZ`,
/// `main_watcher.addDirectory` — takes both of those mutexes, so it must
/// run on a WorkPool thread after `onFileUpdate` returns and both locks
/// are released.
pub const NewSubdirTask = struct {
manager: *PathWatcherManager,
entries: List,
task: jsc.WorkPoolTask = .{ .callback = callback },

/// Small inline list that holds a handful of (watcher, absolute path)
/// pairs. `onFileUpdate` can produce a burst of these when a directory
/// tree is moved in, but typical batches have 0–1.
pub const List = struct {
/// Heap-allocated entries. Null when count == 0 to avoid
/// allocating on the common empty path.
items: ?[*]Entry = null,
count: u32 = 0,
capacity: u32 = 0,

pub const Entry = struct {
watcher: *PathWatcher,
path: [:0]u8,
};

/// Append. Small allocations — follow the file's `bun.handleOom`
/// convention (see `_fdFromAbsolutePathZImpl`) and abort on OOM
/// rather than propagating a rollback path that would need to
/// `unrefPendingDirectory()` inside `manager.mutex`, re-entering
/// the mutex via `PathWatcher.deinit → unregisterWatcher` if the
/// watcher was closed in the race window.
pub fn add(this: *List, watcher: *PathWatcher, abs_path: []const u8) void {
if (this.count == this.capacity) {
const new_cap: u32 = if (this.capacity == 0) 4 else this.capacity * 2;
if (this.items) |p| {
const old_slice = p[0..this.capacity];
const resized = bun.handleOom(bun.default_allocator.realloc(old_slice, new_cap));
this.items = resized.ptr;
} else {
const fresh = bun.handleOom(bun.default_allocator.alloc(Entry, new_cap));
this.items = fresh.ptr;
}
this.capacity = new_cap;
}
const dup = bun.handleOom(bun.default_allocator.dupeZ(u8, abs_path));
this.items.?[this.count] = .{ .watcher = watcher, .path = dup };
this.count += 1;
}

pub fn slice(this: *const List) []Entry {
return if (this.items) |p| p[0..this.count] else &[_]Entry{};
}

pub fn deinit(this: *List) void {
if (this.items) |p| {
bun.default_allocator.free(p[0..this.capacity]);
this.items = null;
this.count = 0;
this.capacity = 0;
}
}
};

pub fn schedule(manager: *PathWatcherManager, entries: List) void {
// Keep the manager alive until the task runs. If refPendingTask
// fails (manager is shutting down), skip registration but still
// release the per-watcher refs we took in onFileUpdate.
if (!manager.refPendingTask()) {
for (entries.slice()) |entry| {
entry.watcher.unrefPendingDirectory();
bun.default_allocator.free(entry.path);
}
var mut_entries = entries;
mut_entries.deinit();
return;
}
const task = bun.handleOom(bun.default_allocator.create(NewSubdirTask));
task.* = .{ .manager = manager, .entries = entries };
jsc.WorkPool.schedule(&task.task);
}

fn callback(task: *jsc.WorkPoolTask) void {
const self: *NewSubdirTask = @fieldParentPtr("task", task);
defer {
var entries = self.entries;
entries.deinit();
self.manager.unrefPendingTask();
bun.default_allocator.destroy(self);
}
for (self.entries.slice()) |entry| {
defer {
entry.watcher.unrefPendingDirectory();
bun.default_allocator.free(entry.path);
}
// Watcher may have been closed between queueing and now.
if (entry.watcher.isClosed()) continue;

// Resolve the absolute path to a directory PathInfo. Uses the
// directory-only helper so a sibling-triggered false positive
// (e.g. `mkdir sub; touch file.txt` merges into one event with
// `is_dir` OR'd over both names) fails with NOTDIR instead of
// opening the regular file and creating a refs=1 PathInfo
// that would need a lock-inverting cleanup — see
// `_dirFdFromAbsolutePathZ` for the full deadlock trace.
//
// Other benign cases that land here: the subdirectory was
// deleted, moved, or symlinked to a non-directory between
// IN_CREATE and the WorkPool pickup. All skip cleanly.
const path_info = switch (self.manager._dirFdFromAbsolutePathZ(entry.path)) {
.result => |p| p,
.err => |err| {
log("[watch] _registerNewSubdirectory({s}) lookup: {f}", .{ entry.path, err });
continue;
},
};

// Track this subdirectory against the watcher so its path
// reference is released when the watcher is unregistered.
// handleOom on append — the rollback path would need to
// clean up the refs=1 entry from `manager.file_paths` while
// skipping `main_watcher.remove` (Watcher.mutex would
// AB/BA-deadlock against the watcher thread's
// Watcher→manager ordering). Aborting on OOM matches the
// rest of the file's `bun.handleOom` convention.
{
entry.watcher.mutex.lock();
defer entry.watcher.mutex.unlock();
bun.handleOom(entry.watcher.file_paths.append(bun.default_allocator, path_info.path));
}

switch (self.manager._addDirectory(entry.watcher, path_info)) {
.err => |err| {
log("[watch] _registerNewSubdirectory({s}) addDirectory: {f}", .{ entry.path, err });
// Surface the failure to JS via 'error' — matches
// `DirectoryRegisterTask.run`'s initial-scan behavior.
// By this point `O_DIRECTORY` already succeeded in
// `_dirFdFromAbsolutePathZ`, so a failure here is a
// real watch-registration problem — canonically
// `inotify_add_watch` hitting `fs.inotify.max_user_watches`
// (ENOSPC) — not a benign race, and the user should
// see it on their 'error' listener. Staying silent
// is the "why isn't my watcher working" gotcha.
entry.watcher.emit(.{ .@"error" = err }, 0, std.time.milliTimestamp(), false);
entry.watcher.flush();
// Leave the ref + file_paths entry in place; the watcher's
Comment thread
robobun marked this conversation as resolved.
// deinit path walks file_paths and decrements refs correctly.
},
.result => {},
}
Comment thread
robobun marked this conversation as resolved.
}
}
};

// this should only be called if thread pool is not null
fn _addDirectory(this: *PathWatcherManager, watcher: *PathWatcher, path: PathInfo) bun.sys.Maybe(void) {
const fd = path.fd;
Expand Down
6 changes: 6 additions & 0 deletions src/watcher/INotifyWatcher.zig
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,12 @@ pub fn watchEventFromInotifyEvent(event: *align(1) const INotifyWatcher.Event, i
.move_to = (event.mask & IN.MOVED_TO) > 0,
.write = (event.mask & IN.MODIFY) > 0,
.create = (event.mask & IN.CREATE) > 0,
// IN_ISDIR is a kernel-set flag in the event mask (not one you
// pass to inotify_add_watch). It is set when the event concerns
// a subdirectory of the watched directory. Recursive fs.watch
// needs this to know when a newly-created entry is itself a
// directory that should get its own inotify watch.
.is_dir = (event.mask & IN.ISDIR) > 0,
},
.index = index,
};
Expand Down
6 changes: 5 additions & 1 deletion test/cli/install/bun-install-lifecycle-scripts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3022,7 +3022,11 @@ for (const forceWaiterThread of isLinux ? [false, true] : [false]) {
expect(await proc.exited).toBe(0);
assertManifestsPopulated(join(packageDir, ".bun-cache"), verdaccio.registryUrl());

expect(proc.resourceUsage()?.cpuTime.total).toBeLessThan(750_000);
// Mirror the Windows-slack multiplier used by the sibling 'bun pm trust'
// assertion below. Windows CI runners routinely overshoot 750ms of CPU
// during `bun install` of a 1s-sleep preinstall script (observed
// 781ms–937ms on buildkite Windows 2019 runners).
expect(proc.resourceUsage()?.cpuTime.total).toBeLessThan(750_000 * (isWindows ? 5 : 1));
});

// https://github.com/oven-sh/bun/issues/11252
Expand Down
3 changes: 3 additions & 0 deletions test/expectations.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ test/js/bun/spawn/spawn-maxbuf.test.ts [ FLAKY ]
[ ASAN ] test/js/third_party/next-auth/next-auth.test.ts [ CRASH ]
[ ASAN ] test/js/node/watch/fs.watch.test.ts [ CRASH ]

# Tests that expect clean stderr but hit JSC assertions enabled in ASAN builds
[ ASAN ] test/integration/build-prefetch/prefetch.test.ts [ FAIL ] # @assert(!dependency.isAsync) in ModuleLoader.js:544 — unrelated to prefetch, async-module linking bug

# Tests failed due to ASAN: SEGV on unknown address
[ ASAN ] test/integration/next-pages/test/dev-server.test.ts [ CRASH ]

Expand Down
Loading
Loading