Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
9 changes: 6 additions & 3 deletions src/bun.js/node/node_fs_watcher.zig
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,6 @@
pub fn initJS(this: *FSWatcher, listener: jsc.JSValue) void {
if (this.persistent) {
this.poll_ref.ref(this.ctx);
_ = this.pending_activity_count.fetchAdd(1, .monotonic);
}

const js_this = this.toJS(this.globalThis);
Expand Down Expand Up @@ -575,7 +574,8 @@
this.mutex.lock();
defer this.mutex.unlock();
// JSC eventually will free it
_ = this.pending_activity_count.fetchSub(1, .monotonic);
const prev = this.pending_activity_count.fetchSub(1, .monotonic);
bun.debugAssert(prev > 0);

Check failure on line 578 in src/bun.js/node/node_fs_watcher.zig

View check run for this annotation

Claude / Claude Code Review

New debugAssert underflows on Windows pre-aborted signal path

On Windows, `FSWatchTaskWindows.appendAbort()` enqueues a task without incrementing `pending_activity_count` (unlike the Posix path and `onPathUpdateWindows`), but `FSWatchTaskWindows.run()` still calls `ctx.unrefTask()` at the end. With the persistent `+1` removed, `fs.watch(path, { signal: AbortSignal.abort() })` now ends up calling `unrefTask()` with `prev == 0`, so the new `bun.debugAssert(prev > 0)` fires in debug builds (and release still wraps to `u32::MAX` and leaks). `FSWatchTaskWindows
Comment thread
claude[bot] marked this conversation as resolved.
}

pub fn close(this: *FSWatcher) void {
Expand All @@ -588,7 +588,10 @@

if (js_this != .zero) {
if (FSWatcher.js.listenerGetCached(js_this)) |listener| {
_ = this.refTask();
// `closed` is already true so `refTask()` would return false without
// incrementing; bump the counter directly so the `unrefTask()` below is
// balanced and the count stays > 0 while the close event is emitted.
_ = this.pending_activity_count.fetchAdd(1, .monotonic);
log("emit('close')", .{});
emitJS(listener, this.globalThis, .js_undefined, .close);
this.unrefTask();
Expand Down
54 changes: 54 additions & 0 deletions test/js/node/watch/fs.watch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,60 @@ describe("immediately closing", () => {
});
});

// FSWatcher.close() set `closed = true` before calling refTask(), so refTask() returned
// false without incrementing pending_activity_count and the paired unrefTask() ran anyway.
// For { persistent: false } watchers (count starts at 1), close() did a net -2, wrapping the
// u32 to MAX. hasPendingActivity() then returned true forever, pinning the native FSWatcher
// (and via its cached listener closure, the JS FSWatcher) as a GC root — a permanent leak
// per watcher. Persistent watchers only landed at 0 by accident (start=2, -2).
describe("closed FSWatcher is collectable", () => {
for (const persistent of [false, true]) {
test(`persistent: ${persistent}`, async () => {
using dir = tempDir("fswatch-gc", { "f.txt": "x" });
const watchDir = String(dir);

const fixture = /* js */ `
const fs = require("fs");

let collected = 0;
const registry = new FinalizationRegistry(() => { collected++; });

const ITERS = 64;
(function create() {
for (let i = 0; i < ITERS; i++) {
const w = fs.watch(${JSON.stringify(watchDir)}, { persistent: ${persistent} }, () => {});
registry.register(w);
w.close();
}
})();

(async () => {
for (let i = 0; i < 30 && collected < ITERS; i++) {
Bun.gc(true);
await Bun.sleep(10);
}
console.log(JSON.stringify({ collected, iters: ITERS }));
})();
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
const { collected, iters } = JSON.parse(stdout.trim());
// Before the fix, `collected` is 0 for persistent:false — every watcher leaks.
// After the fix, all of them are collectable; allow a little slack for GC timing.
expect(collected).toBeGreaterThanOrEqual(Math.floor(iters / 2));
expect(exitCode).toBe(0);
});
}
});

// On Windows, if fs.watch() fails after getOrPut() inserts into the internal path->watcher
// map (e.g. uv_fs_event_start fails on a dangling junction, an ACL-protected dir, or a
// directory deleted mid-watch), an errdefer that was silently broken by a !*T -> Maybe(*T)
Expand Down
Loading