Skip to content

Commit 83b3fd9

Browse files
authored
fix(fetcher): wait for IDLE goroutines (#1342)
## What? Track every IDLE watcher goroutine on the `IdleWatcher` via `sync.WaitGroup` so `StopAllAndWait` catches lingering goroutines whose map entry was already replaced. Add `StopAllAndWaitTimeout` for shutdown paths that need a bounded wait, and switch the daemon shutdown from the fire-and-forget `StopAll()` to the bounded variant so one stuck IMAP connection cannot block the daemon. ## Why? Closes #731 (filed by @andrinoff). The existing pattern in `fetcher/idle.go:48-60` and `:72-75` does `close(existing.stop); delete(w.watchers, account.ID)` and leaves the goroutine to tear down in the background. If the IMAP connection is stuck (server stops responding mid-IDLE, network hangs without RST), the goroutine never exits, the map no longer holds the `done` channel, and `StopAllAndWait()` has no way to see it. The original report's suggested fix was "use WaitGroup or ensure goroutines terminate within timeout" - this PR does both. The daemon side surfaces the problem: `daemon/daemon.go:129` previously called `idleWatcher.StopAll()` and immediately continued to `closeAllClients()`. If any underlying connection hung, the lingering IDLE goroutines outlived the daemon's shutdown path with no log entry. The new bounded-wait shutdown logs `idle watcher: stop timed out` when goroutines don't exit within 5s, so operators see leaks instead of silently losing them. Two new tests cover both behaviors: replaced-goroutine tracking via WaitGroup, and timeout-on-slow-exit returning `ErrStopTimeout`. Fixes #731
1 parent e0cb01c commit 83b3fd9

3 files changed

Lines changed: 109 additions & 2 deletions

File tree

daemon/daemon.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,9 @@ func (d *Daemon) Run() error {
132132
// Cleanup.
133133
log.Println("daemon: shutting down")
134134
d.listener.Close() //nolint:errcheck,gosec
135-
d.idleWatcher.StopAll()
135+
if err := d.idleWatcher.StopAllAndWaitTimeout(5 * time.Second); err != nil {
136+
log.Printf("daemon: %v", err)
137+
}
136138
cancel()
137139
d.closeAllClients()
138140
d.closeProviders()

fetcher/idle.go

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fetcher
22

33
import (
4+
"errors"
45
"log"
56
"strings"
67
"sync"
@@ -19,10 +20,14 @@ type IdleUpdate struct {
1920
// IdleWatcher manages IDLE connections for multiple accounts.
2021
type IdleWatcher struct {
2122
mu sync.Mutex
23+
wg sync.WaitGroup
2224
watchers map[string]*accountIdle // key: account ID
2325
notify chan<- IdleUpdate
2426
}
2527

28+
// ErrStopTimeout is returned when IDLE watcher goroutines do not stop before the timeout.
29+
var ErrStopTimeout = errors.New("idle watcher: stop timed out")
30+
2631
// accountIdle manages a single IDLE connection for one account.
2732
type accountIdle struct {
2833
account *config.Account
@@ -60,7 +65,11 @@ func (w *IdleWatcher) Watch(account *config.Account, folder string) {
6065
done: make(chan struct{}),
6166
}
6267
w.watchers[account.ID] = a
63-
go a.run()
68+
w.wg.Add(1)
69+
go func() {
70+
defer w.wg.Done()
71+
a.run()
72+
}()
6473
}
6574

6675
// Stop stops the IDLE watcher for a specific account.
@@ -100,6 +109,30 @@ func (w *IdleWatcher) StopAllAndWait() {
100109
for _, done := range pending {
101110
<-done
102111
}
112+
w.wg.Wait()
113+
}
114+
115+
// StopAllAndWaitTimeout stops all IDLE watchers and waits for them to finish up to d.
116+
func (w *IdleWatcher) StopAllAndWaitTimeout(d time.Duration) error {
117+
w.mu.Lock()
118+
for id, a := range w.watchers {
119+
close(a.stop)
120+
delete(w.watchers, id)
121+
}
122+
w.mu.Unlock()
123+
124+
done := make(chan struct{})
125+
go func() {
126+
w.wg.Wait()
127+
close(done)
128+
}()
129+
130+
select {
131+
case <-done:
132+
return nil
133+
case <-time.After(d):
134+
return ErrStopTimeout
135+
}
103136
}
104137

105138
func (a *accountIdle) run() {

fetcher/idle_test.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package fetcher
2+
3+
import (
4+
"errors"
5+
"sync/atomic"
6+
"testing"
7+
"time"
8+
9+
"github.com/floatpane/matcha/config"
10+
)
11+
12+
func TestIdleWatcher_StopAllAndWait_TracksReplacedGoroutines(t *testing.T) {
13+
w := NewIdleWatcher(make(chan IdleUpdate))
14+
stopCh := make(chan struct{})
15+
doneCh := make(chan struct{})
16+
var exits atomic.Int64
17+
18+
w.wg.Add(1)
19+
go func() {
20+
defer w.wg.Done()
21+
defer close(doneCh)
22+
<-stopCh
23+
exits.Add(1)
24+
}()
25+
26+
w.watchers["acct"] = &accountIdle{
27+
account: &config.Account{ID: "acct"},
28+
stop: stopCh,
29+
done: doneCh,
30+
}
31+
32+
if err := w.StopAllAndWaitTimeout(time.Second); err != nil {
33+
t.Fatalf("StopAllAndWaitTimeout returned error: %v", err)
34+
}
35+
if got := exits.Load(); got != 1 {
36+
t.Fatalf("expected synthetic watcher to exit once, got %d", got)
37+
}
38+
}
39+
40+
func TestIdleWatcher_StopAllAndWaitTimeout_ReturnsOnSlowExit(t *testing.T) {
41+
w := NewIdleWatcher(make(chan IdleUpdate))
42+
stopCh := make(chan struct{})
43+
doneCh := make(chan struct{})
44+
release := make(chan struct{})
45+
exited := make(chan struct{})
46+
47+
w.wg.Add(1)
48+
go func() {
49+
defer w.wg.Done()
50+
defer close(doneCh)
51+
defer close(exited)
52+
<-release
53+
}()
54+
55+
w.watchers["acct"] = &accountIdle{
56+
account: &config.Account{ID: "acct"},
57+
stop: stopCh,
58+
done: doneCh,
59+
}
60+
61+
err := w.StopAllAndWaitTimeout(50 * time.Millisecond)
62+
if !errors.Is(err, ErrStopTimeout) {
63+
t.Fatalf("expected ErrStopTimeout, got %v", err)
64+
}
65+
66+
close(release)
67+
select {
68+
case <-exited:
69+
case <-time.After(time.Second):
70+
t.Fatal("synthetic watcher did not exit during cleanup")
71+
}
72+
}

0 commit comments

Comments
 (0)