fix(fetcher): wait for IDLE goroutines#1342
Conversation
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @mvanhorn! Please fix the following issues with your PR:
- Title: Is too long (65 characters). The PR title must be strictly under 40 characters.
Formatting issues have been resolved. Thank you!
Watch() and Stop() removed accountIdle entries from the map before the goroutine had a chance to exit, leaving lingering goroutines unobservable to StopAllAndWait. A hung IMAP connection could turn that into a slow leak. Track every spawned goroutine on the IdleWatcher via sync.WaitGroup so the wait-for-completion path catches lingering goroutines whose map entries were already replaced. Add StopAllAndWaitTimeout for shutdown paths that need a bounded wait and switch the daemon shutdown to it so one stuck connection does not block the daemon. Fixes floatpane#731
4f9122b to
b2c95f3
Compare
mx-gp
left a comment
There was a problem hiding this comment.
I’ve reviewed the changes for potential breaking impacts. This is a solid, non-breaking fix for the goroutine leak. The addition of StopAllAndWaitTimeout and the switch to sync.WaitGroup for tracking IDLE goroutines correctly improves shutdown reliability without affecting existing public API signatures or configurations.
…ine-leak Signed-off-by: drew <me@andrinoff.com>
|
Thanks @andrinoff for merging the IdleWatcher fix that waits for the IDLE watcher goroutines. |
|
Really appreciate the merge @andrinoff. Tracking the IDLE watcher goroutines on a WaitGroup so |
What?
Track every IDLE watcher goroutine on the
IdleWatcherviasync.WaitGroupsoStopAllAndWaitcatches lingering goroutines whose map entry was already replaced. AddStopAllAndWaitTimeoutfor shutdown paths that need a bounded wait, and switch the daemon shutdown from the fire-and-forgetStopAll()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-60and:72-75doesclose(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 thedonechannel, andStopAllAndWait()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:129previously calledidleWatcher.StopAll()and immediately continued tocloseAllClients(). If any underlying connection hung, the lingering IDLE goroutines outlived the daemon's shutdown path with no log entry. The new bounded-wait shutdown logsidle watcher: stop timed outwhen 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.Testing
go test ./fetcher/... -run TestIdleWatcher -count=1- passgo test -race ./fetcher- pass (added tests plus existing tests)go build ./...- cleangofmt -l fetcher/idle.go daemon/daemon.go fetcher/idle_test.go- cleanFixes #731
AI-assisted.