From 3d75e550b5d6c3085c7e99453113fac787998b1d Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 2 May 2026 15:44:39 +0000 Subject: [PATCH 1/5] fs_events(darwin): drop unlocked fast path on fsevents_default_loop / fsevents_cf / fsevents_cs Darwin.addWatch (path_watcher.zig) calls FSEvents.watch() without holding manager.mutex (released first to keep lock order fsevents -> manager), so two Workers can enter FSEvents.watch() concurrently. The old code read fsevents_default_loop with no lock and no acquire fence; only the else-branch took fsevents_default_loop_mutex. On ARM64 Worker A's store of the pointer can become visible to Worker B before the stores inside FSEventsLoop.init() (this.* = fs_loop), so Worker B would call registerWatcher() on a partially-visible loop and lock a garbage loop.mutex / read a garbage loop.watchers BabyList. CoreFoundation.get() / CoreServices.get() / closeAndWait() had the identical unlocked-fast-path pattern on fsevents_cf / fsevents_cs / fsevents_default_loop. Fix: always take the mutex first - exactly what PathWatcherManager.get() already does for its own default_manager (with the same explanatory comment). These run once per fs.watch() call; the mutex is uncontended after initialization. In watch(), release the init mutex before calling FSEventsWatcher.init() so we never nest fsevents_default_loop_mutex and loop.mutex. Adds a macOS-only regression stress test that spawns N Workers which each call fs.watch() as their very first statement on distinct directories, in a fresh process per iteration so the global starts null every time. --- src/runtime/node/fs_events.zig | 32 +++++-- .../watch/fs.watch.worker-init-race.test.ts | 95 +++++++++++++++++++ 2 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 test/js/node/watch/fs.watch.worker-init-race.test.ts diff --git a/src/runtime/node/fs_events.zig b/src/runtime/node/fs_events.zig index f8f8c13eec78..368d6816ba63 100644 --- a/src/runtime/node/fs_events.zig +++ b/src/runtime/node/fs_events.zig @@ -114,7 +114,8 @@ pub const CoreFoundation = struct { RunLoopDefaultMode: *CFStringRef, pub fn get() CoreFoundation { - if (fsevents_cf) |cf| return cf; + // No unlocked fast path — see `watch()` below for the reasoning. The + // mutex is uncontended after initialization. fsevents_mutex.lock(); defer fsevents_mutex.unlock(); if (fsevents_cf) |cf| return cf; @@ -146,7 +147,8 @@ pub const CoreServices = struct { kFSEventStreamEventIdSinceNow: FSEventStreamEventId = 18446744073709551615, pub fn get() CoreServices { - if (fsevents_cs) |cs| return cs; + // No unlocked fast path — see `watch()` below for the reasoning. The + // mutex is uncontended after initialization. fsevents_mutex.lock(); defer fsevents_mutex.unlock(); if (fsevents_cs) |cs| return cs; @@ -622,16 +624,27 @@ pub const FSEventsWatcher = struct { }; pub fn watch(path: string, recursive: bool, callback: FSEventsWatcher.Callback, updateEnd: FSEventsWatcher.UpdateEndCallback, ctx: ?*anyopaque) !*FSEventsWatcher { - if (fsevents_default_loop) |loop| { - return FSEventsWatcher.init(loop, path, recursive, callback, updateEnd, ctx); - } else { + // No unlocked fast path: `fsevents_default_loop` is a plain global and an + // unsynchronized read here would be textbook broken DCLP. `Darwin.addWatch` + // (path_watcher.zig) calls this WITHOUT holding `manager.mutex`, so two + // Workers can enter concurrently; on ARM64 Worker B could observe the + // non-null pointer before Worker A's stores inside `FSEventsLoop.init()` + // (`this.* = fs_loop`) are visible, and then `registerWatcher()` would lock + // a garbage `loop.mutex` / read a garbage `loop.watchers`. Same pattern + // `PathWatcherManager.get()` already fixed. `watch()` runs once per + // `fs.watch()` call; the mutex is uncontended after initialization. + const loop = loop: { fsevents_default_loop_mutex.lock(); defer fsevents_default_loop_mutex.unlock(); if (fsevents_default_loop == null) { fsevents_default_loop = try FSEventsLoop.init(); } - return FSEventsWatcher.init(fsevents_default_loop.?, path, recursive, callback, updateEnd, ctx); - } + break :loop fsevents_default_loop.?; + }; + // Release `fsevents_default_loop_mutex` before `registerWatcher()` (which + // takes `loop.mutex`) so we never nest the two. `loop` is stable once + // published — only `closeAndWait()` at process exit ever clears it. + return FSEventsWatcher.init(loop, path, recursive, callback, updateEnd, ctx); } pub fn closeAndWait() void { @@ -639,9 +652,10 @@ pub fn closeAndWait() void { return; } + // No unlocked fast path — see `watch()` above. + fsevents_default_loop_mutex.lock(); + defer fsevents_default_loop_mutex.unlock(); if (fsevents_default_loop) |loop| { - fsevents_default_loop_mutex.lock(); - defer fsevents_default_loop_mutex.unlock(); loop.deinit(); fsevents_default_loop = null; } diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts new file mode 100644 index 000000000000..b09e59875d4a --- /dev/null +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -0,0 +1,95 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isMacOS, tempDir } from "harness"; + +// Regression test for broken double-checked locking on `fsevents_default_loop` +// in src/bun.js/node/fs_events.zig. +// +// `FSEvents.watch()` is called from `Darwin.addWatch` (path_watcher.zig) +// WITHOUT holding `manager.mutex` (it's released first to keep lock order +// one-way). Two Workers can therefore enter `FSEvents.watch()` concurrently. +// +// Before the fix the function read `fsevents_default_loop` with no lock and +// no acquire fence; only the else-branch took `fsevents_default_loop_mutex`. +// On ARM64 Worker A's store of the pointer could become visible to Worker B +// before the stores inside `FSEventsLoop.init()` (`this.* = fs_loop`), so +// Worker B would call `registerWatcher()` on a partially-visible loop and +// lock a garbage `loop.mutex` / read a garbage `loop.watchers` BabyList. +// `CoreFoundation.get()` / `CoreServices.get()` had the identical pattern. +// +// This is `path_watcher.zig`'s own `PathWatcherManager.get()` comment applied +// to `fs_events.zig`: drop the unlocked fast path; the mutex is uncontended +// after initialization. +// +// The race requires (a) the very first `fs.watch()` in the process to happen +// on two threads at once and (b) store reordering, so it is low-probability +// even on Apple Silicon. This test spawns a fresh process per iteration so +// the loop is uninitialized each time, and fires several Workers that all +// call `fs.watch()` as their first statement on distinct directories (so +// `PathWatcherManager` dedup doesn't serialize them). +// +// macOS-only: the FSEvents code path doesn't exist on other platforms. +test.skipIf(!isMacOS)( + "FSEvents: concurrent first fs.watch() from Workers does not observe a partially-initialized loop", + async () => { + const WORKERS = 8; + const files: Record = {}; + for (let i = 0; i < WORKERS; i++) files[`d${i}/f.txt`] = "x"; + files["worker.js"] = ` + const fs = require("fs"); + const { parentPort, workerData } = require("worker_threads"); + // First thing this thread does: hit FSEvents.watch() via Darwin.addWatch + // with manager.mutex released. Multiple Workers race here on a fresh + // process so fsevents_default_loop starts null. + const w = fs.watch(workerData.dir, () => {}); + w.close(); + parentPort.postMessage("ok"); + `; + files["main.js"] = ` + const path = require("path"); + const { Worker } = require("worker_threads"); + const root = process.argv[2]; + const N = ${WORKERS}; + let done = 0; + let failed = false; + for (let i = 0; i < N; i++) { + const w = new Worker(path.join(root, "worker.js"), { + workerData: { dir: path.join(root, "d" + i) }, + }); + w.on("message", () => { + if (++done === N && !failed) { + console.log("OK"); + process.exit(0); + } + }); + w.on("error", err => { + failed = true; + console.error("worker error:", err); + process.exit(1); + }); + } + `; + + using dir = tempDir("fsevents-worker-init-race", files); + + // Fresh process each iteration so the FSEvents loop global starts null + // and the DCLP race window exists every time. + for (let i = 0; i < 20; i++) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.js", String(dir)], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + proc.exited, + ]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); + } + }, + 60_000, +); From b267506af83ee4a6748d6dd5cf3d7737410d5e40 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 15:47:15 +0000 Subject: [PATCH 2/5] [autofix.ci] apply automated fixes --- test/js/node/watch/fs.watch.worker-init-race.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts index b09e59875d4a..08c1f7bb7ec6 100644 --- a/test/js/node/watch/fs.watch.worker-init-race.test.ts +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -81,11 +81,7 @@ test.skipIf(!isMacOS)( stdout: "pipe", stderr: "pipe", }); - const [stdout, stderr, exitCode] = await Promise.all([ - proc.stdout.text(), - proc.stderr.text(), - proc.exited, - ]); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stderr).toBe(""); expect(stdout.trim()).toBe("OK"); expect(exitCode).toBe(0); From 91223d955addedfa0892a925293f705f4e2728b2 Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 2 May 2026 15:59:28 +0000 Subject: [PATCH 3/5] test: track worker completion via 'exit' so a silent worker death can't hang the test --- .../watch/fs.watch.worker-init-race.test.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts index 08c1f7bb7ec6..cf17ec465eb0 100644 --- a/test/js/node/watch/fs.watch.worker-init-race.test.ts +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -55,17 +55,24 @@ test.skipIf(!isMacOS)( const w = new Worker(path.join(root, "worker.js"), { workerData: { dir: path.join(root, "d" + i) }, }); - w.on("message", () => { - if (++done === N && !failed) { - console.log("OK"); - process.exit(0); - } - }); w.on("error", err => { failed = true; console.error("worker error:", err); process.exit(1); }); + // Track completion via 'exit' (fires exactly once per worker no matter + // how it ends) so a worker that dies without posting can't hang us. + w.on("exit", code => { + if (code !== 0 && !failed) { + failed = true; + console.error("worker exited with code", code); + process.exit(1); + } + if (++done === N && !failed) { + console.log("OK"); + process.exit(0); + } + }); } `; From f5d25eb6967a17c57d2ee2f692288231f519f2e3 Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 2 May 2026 16:23:26 +0000 Subject: [PATCH 4/5] test: drop vestigial parentPort.postMessage now that completion is tracked via 'exit' --- test/js/node/watch/fs.watch.worker-init-race.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts index cf17ec465eb0..9c7a2c6f3def 100644 --- a/test/js/node/watch/fs.watch.worker-init-race.test.ts +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -36,13 +36,12 @@ test.skipIf(!isMacOS)( for (let i = 0; i < WORKERS; i++) files[`d${i}/f.txt`] = "x"; files["worker.js"] = ` const fs = require("fs"); - const { parentPort, workerData } = require("worker_threads"); + const { workerData } = require("worker_threads"); // First thing this thread does: hit FSEvents.watch() via Darwin.addWatch // with manager.mutex released. Multiple Workers race here on a fresh // process so fsevents_default_loop starts null. const w = fs.watch(workerData.dir, () => {}); w.close(); - parentPort.postMessage("ok"); `; files["main.js"] = ` const path = require("path"); From 9fea19cfd80b377180bc4cfa4a6e753b5201e034 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 4 May 2026 10:50:20 +0000 Subject: [PATCH 5/5] test: update fs_events.zig path in comment after src/ restructure --- test/js/node/watch/fs.watch.worker-init-race.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts index 9c7a2c6f3def..d21b2bef81a0 100644 --- a/test/js/node/watch/fs.watch.worker-init-race.test.ts +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -2,7 +2,7 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, isMacOS, tempDir } from "harness"; // Regression test for broken double-checked locking on `fsevents_default_loop` -// in src/bun.js/node/fs_events.zig. +// in src/runtime/node/fs_events.zig. // // `FSEvents.watch()` is called from `Darwin.addWatch` (path_watcher.zig) // WITHOUT holding `manager.mutex` (it's released first to keep lock order