fix(windows): close handles immediately on shutdown notification to p… #24989
Conversation
…revent resource leak When a shutdown notification is received from the IOCP (nbytes == 0), the Windows watcher was not closing handles immediately, leading to resource leaks. This fix: - Adds handle tracking with handles_closed flag - Implements idempotent closeHandles() function - Closes handles immediately when shutdown notification is received - Makes stop() function use the idempotent cleanup - Prevents double-close errors by marking handles as INVALID_HANDLE_VALUE This ensures proper resource cleanup and prevents handle leaks on Windows.
WalkthroughAdds a Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/WindowsWatcher.zig(3 hunks)
🔇 Additional comments (1)
src/watcher/WindowsWatcher.zig (1)
225-225: stop() now correctly reuses the centralized, idempotent cleanupSwitching
stop()to delegate tocloseHandles()is a nice simplification and ensures all shutdown paths share the same idempotent logic and INVALID_HANDLE_VALUE marking.
| /// Track if handles have been closed to prevent double-close | ||
| handles_closed: bool = false, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reset handles_closed in init to avoid leaks on reuse
The flag is great for preventing double-closes, but if a WindowsWatcher instance is ever re‑initialized (calling init again after stop/shutdown), handles_closed will still be true, so closeHandles() becomes a no‑op and new handles will leak.
Consider explicitly clearing it at the start of init:
pub fn init(this: *WindowsWatcher, root: []const u8) !void {
+ this.handles_closed = false;
+
var pathbuf: bun.WPathBuffer = undefined;
const wpath = bun.strings.toNTPath(&pathbuf, root);This is a cheap change and makes the lifecycle more robust even if reuse is rare.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/watcher/WindowsWatcher.zig around lines 10-11, the boolean flag
handles_closed is never reset when an instance is re-initialized, so after
stop/shutdown handles_closed remains true and subsequent init calls will skip
closing old handles causing leaks; modify the init function to explicitly set
self.handles_closed = false at the start of initialization (before allocating or
opening any handles) so the lifecycle can be reused safely, and ensure any
concurrent access is still correctly synchronized if applicable.
| if (nbytes == 0) { | ||
| // shutdown notification | ||
| // TODO close handles? | ||
| // shutdown notification - close handles immediately to prevent resource leaks | ||
| log("shutdown notification in WindowsWatcher.next", .{}); | ||
| this.closeHandles(); | ||
| return .{ .err = .{ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good: close on shutdown; optionally short‑circuit next after ESHUTDOWN
Closing handles on the nbytes == 0 “shutdown” completion is exactly what was missing and matches the intent.
One small hardening you might consider: if next is accidentally called again after this path, it will call GetQueuedCompletionStatus on an iocp that closeHandles() has already set to w.INVALID_HANDLE_VALUE. Windows will fail that call, but you could avoid making the syscall at all by bailing out early when the watcher is already shut down:
pub fn next(this: *WindowsWatcher, timeout: Timeout) bun.sys.Maybe(?EventIterator) {
+ if (this.handles_closed) {
+ return .{ .err = .{
+ .errno = @intFromEnum(bun.sys.SystemErrno.ESHUTDOWN),
+ .syscall = .watch,
+ } };
+ }
+
switch (this.watcher.prepare()) {Not required for correctness given current callers, but it makes the API more self‑defensive.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/watcher/WindowsWatcher.zig around lines 177 to 181, add a defensive
early-return in next so it does not call GetQueuedCompletionStatus after
closeHandles has invalidated the iocp; detect the shutdown state (e.g. check if
this.iocp == w.INVALID_HANDLE_VALUE or a dedicated closed flag) at the top of
next and immediately return the same shutdown error/result used for the nbytes
== 0 path so subsequent calls short-circuit instead of making a syscall on an
invalid handle.
| /// Close handles if they haven't been closed already. | ||
| /// This function is idempotent and safe to call multiple times. | ||
| /// | ||
| /// When a shutdown notification is received from the IOCP, we need to close | ||
| /// handles immediately to prevent resource leaks. This function ensures handles | ||
| /// are properly closed and marked as invalid to prevent double-close errors. | ||
| fn closeHandles(this: *WindowsWatcher) void { | ||
| if (this.handles_closed) { | ||
| return; | ||
| } | ||
| this.handles_closed = true; | ||
|
|
||
| // Close directory handle if it's still valid | ||
| if (this.watcher.dirHandle != w.INVALID_HANDLE_VALUE) { | ||
| _ = bun.windows.CloseHandle(this.watcher.dirHandle); | ||
| this.watcher.dirHandle = w.INVALID_HANDLE_VALUE; | ||
| log("closed directory handle", .{}); | ||
| } | ||
|
|
||
| // Close IOCP handle if it's still valid | ||
| if (this.iocp != w.INVALID_HANDLE_VALUE) { | ||
| _ = bun.windows.CloseHandle(this.iocp); | ||
| this.iocp = w.INVALID_HANDLE_VALUE; | ||
| log("closed IOCP handle", .{}); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify concurrency assumptions and make closeHandles safe before full init
Centralizing cleanup here looks good and the INVALID_HANDLE_VALUE marking is a nice touch, but two edge cases are worth tightening up:
-
Concurrency / double‑close race
handles_closedis a plainbool. Ifstop()andnext()can ever run on different threads without external synchronization, both could observehandles_closed == falseand race to callCloseHandleon the same OS handles. That still results in a double‑close at the Win32 level.If the invariant is “all access to a given
WindowsWatcher(includingstopandnext) is single‑threaded or externally serialized”, it’d be good to document that. Otherwise, consider guardingcloseHandles()with themutexor using an atomic CAS on the flag. -
Uninitialized handle values if
closeHandlesis called before successfulinitthis.iocpandthis.watcher.dirHandleare declared as= undefinedand only assigned real values insideinit. Ifstop()(and hencecloseHandles()) is ever called on a partially‑initialized or failed instance, these comparisons:
if (this.watcher.dirHandle != w.INVALID_HANDLE_VALUE) { ... }
if (this.iocp != w.INVALID_HANDLE_VALUE) { ... }read potentially uninitialized memory and might call CloseHandle on garbage.
To make closeHandles() safe in more states, you can initialize both handles to w.INVALID_HANDLE_VALUE as their default:
- iocp: w.HANDLE = undefined,
- watcher: DirWatcher = undefined,
+ iocp: w.HANDLE = w.INVALID_HANDLE_VALUE,
+ watcher: DirWatcher = .{ .dirHandle = w.INVALID_HANDLE_VALUE },and keep the rest of init as‑is. That way any early stop()/closeHandles() call is guaranteed to be a no‑op at the OS level.
I'll now gather the necessary information to verify the review comment by examining the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the WindowsWatcher struct definition to see field declarations
rg -n "struct WindowsWatcher|const WindowsWatcher" src/watcher/WindowsWatcher.zig -A 20 | head -80
Length of output: 527
🏁 Script executed:
#!/bin/bash
# Find the init function to see initialization of iocp and dirHandle
rg -n "fn init\(" src/watcher/WindowsWatcher.zig -A 30 | head -100Length of output: 1205
🏁 Script executed:
#!/bin/bash
# Find stop and next functions to understand call flow
rg -n "fn stop\(|fn next\(" src/watcher/WindowsWatcher.zig -A 10Length of output: 1382
🏁 Script executed:
#!/bin/bash
# Search for mutex usage and threading patterns
rg -n "mutex|Mutex|lock|Lock|atomic" src/watcher/WindowsWatcher.zig -B 2 -A 2Length of output: 228
🏁 Script executed:
#!/bin/bash
# Get the full context around closeHandles declaration and how handles are declared
rg -n "iocp:|dirHandle:|handles_closed" src/watcher/WindowsWatcher.zig -B 3 -A 3 | head -100Length of output: 1121
I need to gather more details about the initialization and synchronization patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the full init() function to see where iocp and dirHandle are assigned
rg -n "fn init\(" src/watcher/WindowsWatcher.zig -A 60 | grep -E "this\.(iocp|watcher\.dirHandle)|dirHandle =|iocp =" | head -20
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Find DirWatcher struct definition to see if dirHandle has a default
rg -n "struct DirWatcher|const DirWatcher" src/watcher/WindowsWatcher.zig -A 15 | head -40Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Check the complete init function for error handling and assignment order
sed -n '85,143p' src/watcher/WindowsWatcher.zigLength of output: 1888
🏁 Script executed:
#!/bin/bash
# Look for where dirHandle is assigned or initialized in init
rg -n "dirHandle\s*=" src/watcher/WindowsWatcher.zig -B 2 -A 2Length of output: 413
🏁 Script executed:
#!/bin/bash
# Check if mutex is actually used in stop() or next()
sed -n '145,195p' src/watcher/WindowsWatcher.zigLength of output: 1954
🏁 Script executed:
#!/bin/bash
# Check if there's any mutex lock/unlock calls around stop or closeHandles
rg -n "lock\(|unlock\(|with_lock|protect" src/watcher/WindowsWatcher.zig -B 3 -A 3Length of output: 0
Address concurrency race on handle closure and guard against uninitialized handle reads
Both concerns in the review are confirmed:
-
Concurrency race is real: The
mutexfield exists but is never used—nolock(),unlock(), orwith_lock()calls found anywhere in the file. Bothstop()(line 224) andnext()(line 177) can callcloseHandles()without synchronization. If they run on different threads, both could observehandles_closed == falseand race to callCloseHandle. -
Uninitialized handles are reachable: The
init()function can fail atNtCreateFile()(line 117) and return early, leavingthis.iocpandthis.watcherasundefined. Ifstop()is then called on the partially-initialized instance,closeHandles()will read undefined memory when comparingthis.watcher.dirHandleagainstw.INVALID_HANDLE_VALUE.
The suggested fix is sound: initialize both iocp and watcher.dirHandle to w.INVALID_HANDLE_VALUE at declaration, making closeHandles() safe at any point in the object's lifetime.
- iocp: w.HANDLE = undefined,
- watcher: DirWatcher = undefined,
+ iocp: w.HANDLE = w.INVALID_HANDLE_VALUE,
+ watcher: DirWatcher = .{ .dirHandle = w.INVALID_HANDLE_VALUE },For the concurrency issue, either document the single-threaded invariant or protect closeHandles() with the existing mutex.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
prevent resource leak
When a shutdown notification is received from the IOCP (nbytes == 0), the Windows watcher was not closing handles immediately, leading to resource leaks. This fix:
This ensures proper resource cleanup and prevents handle leaks on Windows.