From c6f96512c9d235d222c1c4fc20fc6eb9f1e70858 Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Fri, 23 Oct 2026 17:11:41 +0000 Subject: [PATCH 1/3] windows: test that sendError keeps Close's reply channel Close queues a reply channel on done and waits for the reader to send on it after its cleanup. sendError takes that channel when the watcher is closing and drops it, so the reader never sees the shutdown request and Close blocks forever. These tests pin the channel handoff and the end-to-end Close; they fail before the fix. --- backend_windows_test.go | 59 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/backend_windows_test.go b/backend_windows_test.go index 6364f43a..93e968c7 100644 --- a/backend_windows_test.go +++ b/backend_windows_test.go @@ -3,9 +3,11 @@ package fsnotify import ( + "errors" "fmt" "strings" "testing" + "time" ) func TestRemoveState(t *testing.T) { @@ -71,3 +73,60 @@ func TestWindowsRemWatch(t *testing.T) { t.Fatal("Should be fail with closed handle\n") } } + +// sendError on a closing watcher must leave Close's reply channel queued for +// the reader. If it is dropped, the reader never runs the shutdown cleanup and +// Close waits on the reply forever. +func TestSendErrorKeepsCloseReply(t *testing.T) { + w := &readDirChangesW{ + Errors: make(chan error), // nobody reads errors any more + done: make(chan chan<- error, 1), + } + reply := make(chan error) + w.done <- reply + + if w.sendError(errors.New("overflow")) { + t.Fatal("sendError reported the error as delivered") + } + select { + case got := <-w.done: + if got != reply { + t.Fatal("sendError replaced Close's reply channel") + } + default: + t.Fatal("sendError dropped Close's reply channel") + } +} + +// Close must return when the error consumer has gone away and the backend +// then reports an error: the reader keeps the reply and completes shutdown. +func TestCloseAfterUnreadError(t *testing.T) { + w, err := NewWatcher() + if err != nil { + t.Fatal(err) + } + b := w.b.(*readDirChangesW) + + // Close queues its reply and wakes the reader; an error raised before the + // reader takes it meets an Errors channel nobody reads. + b.mu.Lock() + b.closed = true + b.mu.Unlock() + reply := make(chan error) + b.done <- reply + if b.sendError(errors.New("overflow")) { + t.Fatal("sendError reported the error as delivered") + } + if err := b.wakeupReader(); err != nil { + t.Fatal(err) + } + + select { + case err := <-reply: + if err != nil { + t.Fatalf("shutdown: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("the reader never completed shutdown after an undelivered error") + } +} From 73a141ee4ffb12b86ddd7644dc97ee0a22b5086f Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Fri, 23 Oct 2026 17:11:41 +0000 Subject: [PATCH 2/3] windows: keep Close's reply channel when an error cannot be delivered sendError received the reply channel that Close queued on done and discarded it, unlike sendEvent, which puts it back. When nobody reads Errors any more and the backend then reports an error (an overflow, a failed CancelIo or CloseHandle), the reader handled the wakeup without the shutdown request, skipped the cleanup of its watches and the completion port, and Close waited on the reply forever. sendError now puts the channel back, as sendEvent does; TestSendErrorKeepsCloseReply and TestCloseAfterUnreadError fail without this change on windows-latest and pass with it. --- backend_windows.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend_windows.go b/backend_windows.go index e9004ab9..869158c9 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -76,12 +76,16 @@ func (w *readDirChangesW) sendEvent(name, renamedFrom string, mask uint64) bool } // Returns true if the error was sent, or false if watcher is closed. +// +// Close's reply channel is put back, as sendEvent does: the reader takes it +// on its next wakeup and runs the cleanup, and Close blocks until it does. func (w *readDirChangesW) sendError(err error) bool { if err == nil { return true } select { - case <-w.done: + case ch := <-w.done: + w.done <- ch return false case w.Errors <- err: return true From 17a6ca9fec2be51a501029b163fce259b653e334 Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Fri, 23 Oct 2026 17:42:10 +0000 Subject: [PATCH 3/3] windows: check and set closed under one lock in Close Close read closed under the lock, released it, and set it under a second acquisition, so two concurrent calls could both proceed and queue two reply channels. done holds one, so the second Close blocked sending, and with an undeliverable error between them the reader, now keeping the first reply, could not drain the second: both calls and the reader hung. The check and the set now happen under one lock, so a second Close returns at once. TestConcurrentClose runs eight concurrent Close calls under a timeout and checks that no reply is left on done; the interleaving is a race, so it guards rather than reproduces. --- backend_windows.go | 9 ++++++--- backend_windows_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/backend_windows.go b/backend_windows.go index 869158c9..8fc84207 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -93,11 +93,14 @@ func (w *readDirChangesW) sendError(err error) bool { } func (w *readDirChangesW) Close() error { - if w.isClosed() { + // Check and set under one lock: a second concurrent Close must not queue + // a second reply, which the one-slot done channel and the single reader + // cannot both serve. + w.mu.Lock() + if w.closed { + w.mu.Unlock() return nil } - - w.mu.Lock() w.closed = true w.mu.Unlock() diff --git a/backend_windows_test.go b/backend_windows_test.go index 93e968c7..362ab57a 100644 --- a/backend_windows_test.go +++ b/backend_windows_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "strings" + "sync" "testing" "time" ) @@ -130,3 +131,34 @@ func TestCloseAfterUnreadError(t *testing.T) { t.Fatal("the reader never completed shutdown after an undelivered error") } } + +// Concurrent Close calls must all return: only the first queues a reply. +func TestConcurrentClose(t *testing.T) { + w, err := NewWatcher() + if err != nil { + t.Fatal(err) + } + if err := w.Add(t.TempDir()); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _ = w.Close() + }() + } + closed := make(chan struct{}) + go func() { wg.Wait(); close(closed) }() + + select { + case <-closed: + case <-time.After(10 * time.Second): + t.Fatal("concurrent Close calls did not all return") + } + if b := w.b.(*readDirChangesW); len(b.done) != 0 { + t.Fatalf("%d reply channels left on done", len(b.done)) + } +}