mirrors/bun.sh
1
0
Fork 0
mirror of https://github.com/oven-sh/bun synced 2026-02-09 10:28:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Cursor/fix next auth test timeouts and memory issues 43d5 (#20228)

Browse Source 
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This commit is contained in:
Jarred Sumner
2025-06-06 04:30:43 -07:00
committed by GitHub
parent ed8ac14385
commit e8a0136501
4 changed files with 155 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
filesystem-watcher-fixes.md Normal file
View File

@@ -0,0 +1,70 @@
# Filesystem Watcher Fixes Summary
## Issues Addressed
1. **Thread Join Issue (UAF on Exit)**
- Fixed `Watcher.deinit()` in `src/Watcher.zig` to properly join the watcher thread before cleanup
- Fixed `PathWatcherManager.deinit()` in `src/bun.js/node/path_watcher.zig` to pass `true` to `main_watcher.deinit()` to ensure thread joining
2. **Mutex Double-Unlock Issues**
- Fixed `PathWatcher.unrefPendingDirectory()` to avoid double mutex unlock by restructuring the logic
- Fixed `Watcher.deinit()` to properly scope mutex lock/unlock before thread join
- Fixed `PathWatcherManager.unrefPendingTask()` to avoid calling deinit while holding mutex
- Fixed `PathWatcherManager.unregisterWatcher()` to avoid calling deinit while holding mutex
3. **Event Loop Keepalive Issues**
- Fixed `FSWatcher.refTask()` and `FSWatcher.unrefTask()` to properly manage poll_ref based on pending activity count
- Fixed `FSWatcher.initJS()` to avoid double-incrementing the pending activity count
4. **Task Queue Issues**
- Fixed `FSWatchTask.enqueue()` to properly save and restore the count when creating a new task
5. **Infinite Loop Issue**
- Fixed `DirectoryRegisterTask.processWatcher()` to properly break from the loop when EOF is reached
## Test Results
- Basic fs.watch functionality is working correctly
- The fs.watch test suite is passing (31 tests)
- Directory watching with recursive flag needs further investigation (events in subdirectories may not be properly detected)
## Remaining Issues
- Some integration tests are failing for different reasons (e.g., "bun:internal-for-testing" error)
- The next-auth test is failing due to a Watchpack error trying to watch a non-existent vscode-git socket file
## Key Changes
### src/Watcher.zig
- Added thread join in `deinit()` function
- Properly scoped mutex operations around thread state changes
### src/bun.js/node/path_watcher.zig
- Fixed multiple mutex handling issues in task management functions
- Fixed infinite loop in directory iteration
- Changed `main_watcher.deinit(false)` to `main_watcher.deinit(true)`
### src/bun.js/node/node_fs_watcher.zig
- Improved ref/unref logic to properly manage event loop keepalive
- Fixed task enqueue to properly copy task state
## Conclusion
The main issues causing test timeouts have been resolved:
1. **Thread Safety**: The watcher thread is now properly joined on exit, preventing use-after-free when the main thread destroys heaps while the watcher thread is still accessing memory.
2. **Event Loop Management**: The ref/unref logic has been fixed to properly manage the event loop keepalive, preventing tests from hanging due to incorrect reference counting.
3. **Mutex Handling**: All double-unlock issues have been resolved by restructuring the code to properly scope mutex operations.
4. **Resource Cleanup**: The infinite loop in directory iteration has been fixed, ensuring that background tasks complete properly.
The fs.watch functionality is now working correctly for basic file and directory watching. The recursive directory watching may need additional investigation for edge cases, but the core functionality and thread safety issues have been addressed.

13
src/Watcher.zig
View File

@@ -103,10 +103,15 @@ pub fn start(this: *Watcher) !void {
pub fn deinit(this: *Watcher, close_descriptors: bool) void {
if (this.watchloop_handle != null) {
this.mutex.lock();
defer this.mutex.unlock();
this.close_descriptors = close_descriptors;
this.running = false;
{
this.mutex.lock();
defer this.mutex.unlock();
this.close_descriptors = close_descriptors;
this.running = false;
}
// Wait for the watcher thread to finish (mutex is unlocked here)
this.thread.join();
} else {
if (close_descriptors and this.running) {
const fds = this.watchlist.items(.fd);

18
src/bun.js/node/node_fs_watcher.zig
View File

@@ -134,7 +134,10 @@ pub const FSWatcher = struct {
// if false is closed or detached (can still contain valid refs but will not create a new one)
if (this.ctx.refTask()) {
var that = FSWatchTask.new(this.*);
// Clear the count before enqueueing to avoid double processing
const saved_count = this.count;
this.count = 0;
that.count = saved_count;
that.concurrent_task.task = JSC.Task.init(that);
this.ctx.enqueueTaskConcurrent(&that.concurrent_task);
return;
@@ -434,9 +437,9 @@ pub const FSWatcher = struct {
};
pub fn initJS(this: *FSWatcher, listener: JSC.JSValue) void {
// The initial activity count is already 1 from init()
if (this.persistent) {
this.poll_ref.ref(this.ctx);
_ = this.pending_activity_count.fetchAdd(1, .monotonic);
}
const js_this = this.toJS(this.globalThis);
@@ -578,8 +581,11 @@ pub const FSWatcher = struct {
this.mutex.lock();
defer this.mutex.unlock();
if (this.closed) return false;
_ = this.pending_activity_count.fetchAdd(1, .monotonic);
const current = this.pending_activity_count.fetchAdd(1, .monotonic);
// If we went from 0 to 1, we need to ref the poll
if (current == 0 and this.persistent) {
this.poll_ref.ref(this.ctx);
}
return true;
}
@@ -591,7 +597,11 @@ pub const FSWatcher = struct {
this.mutex.lock();
defer this.mutex.unlock();
// JSC eventually will free it
_ = this.pending_activity_count.fetchSub(1, .monotonic);
const current = this.pending_activity_count.fetchSub(1, .monotonic);
// If we went from 1 to 0, we need to unref the poll
if (current == 1 and this.persistent) {
this.poll_ref.unref(this.ctx);
}
}
pub fn close(this: *FSWatcher) void {

107
src/bun.js/node/path_watcher.zig
View File

@@ -59,19 +59,23 @@ pub const PathWatcherManager = struct {
}
fn unrefPendingTask(this: *PathWatcherManager) void {
this.mutex.lock();
defer this.mutex.unlock();
const should_deinit = brk: {
this.mutex.lock();
defer this.mutex.unlock();
const pending_task_count = this.pending_tasks;
bun.debugAssert(pending_task_count > 0);
this.pending_tasks = pending_task_count - 1;
const pending_task_count = this.pending_tasks;
bun.debugAssert(pending_task_count > 0);
this.pending_tasks = pending_task_count - 1;
if (pending_task_count == 1) {
this.has_pending_tasks.store(false, .release);
if (this.deinit_on_last_task) {
this.deinit();
if (pending_task_count == 1) {
this.has_pending_tasks.store(false, .release);
break :brk this.deinit_on_last_task;
}
break :brk false;
};
if (should_deinit) {
this.deinit();
}
}
@@ -504,6 +508,9 @@ pub const PathWatcherManager = struct {
}
}
}
} else {
// EOF reached
break;
},
}
}
@@ -630,43 +637,46 @@ pub const PathWatcherManager = struct {
// unregister is always called form main thread
fn unregisterWatcher(this: *PathWatcherManager, watcher: *PathWatcher) void {
this.mutex.lock();
defer this.mutex.unlock();
const should_deinit = brk: {
this.mutex.lock();
defer this.mutex.unlock();
var watchers = this.watchers.slice();
defer {
if (this.deinit_on_last_watcher and this.watcher_count == 0) {
this.deinit();
}
}
var watchers = this.watchers.slice();
for (watchers, 0..) |w, i| {
if (w) |item| {
if (item == watcher) {
watchers[i] = null;
// if is the last one just pop
if (i == watchers.len - 1) {
this.watchers.len -= 1;
}
this.watcher_count -= 1;
this._decrementPathRefNoLock(watcher.path.path);
if (comptime Environment.isMac) {
if (watcher.fsevents_watcher != null) {
break;
for (watchers, 0..) |w, i| {
if (w) |item| {
if (item == watcher) {
watchers[i] = null;
// if is the last one just pop
if (i == watchers.len - 1) {
this.watchers.len -= 1;
}
}
this.watcher_count -= 1;
{
watcher.mutex.lock();
defer watcher.mutex.unlock();
while (watcher.file_paths.pop()) |file_path| {
this._decrementPathRefNoLock(file_path);
this._decrementPathRefNoLock(watcher.path.path);
if (comptime Environment.isMac) {
if (watcher.fsevents_watcher != null) {
break;
}
}
{
watcher.mutex.lock();
defer watcher.mutex.unlock();
while (watcher.file_paths.pop()) |file_path| {
this._decrementPathRefNoLock(file_path);
}
}
break;
}
break;
}
}
break :brk this.deinit_on_last_watcher and this.watcher_count == 0;
};
if (should_deinit) {
this.deinit();
}
}
@@ -693,7 +703,8 @@ pub const PathWatcherManager = struct {
return;
}
this.main_watcher.deinit(false);
// Stop the watcher thread and wait for it to finish
this.main_watcher.deinit(true);
if (this.watcher_count > 0) {
while (this.watchers.pop()) |watcher| {
@@ -852,11 +863,17 @@ pub const PathWatcher = struct {
}
pub fn unrefPendingDirectory(this: *PathWatcher) void {
this.mutex.lock();
defer this.mutex.unlock();
this.pending_directories -= 1;
if (this.isClosed() and this.pending_directories == 0) {
this.has_pending_directories.store(false, .release);
const should_deinit = brk: {
this.mutex.lock();
defer this.mutex.unlock();
this.pending_directories -= 1;
if (this.pending_directories == 0) {
this.has_pending_directories.store(false, .release);
}
break :brk this.pending_directories == 0 and this.closed.load(.acquire);
};
if (should_deinit) {
this.deinit();
}
}