fix(fetcher): wait for IDLE goroutines#1342
Open
mvanhorn wants to merge 1 commit into
Open
Conversation
floatpanebot
added
area/fetcher
floatpanebot
previously requested changes
May 22, 2026
Member
floatpanebot
left a comment
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.
mvanhorn
changed the title
floatpanebot
dismissed
their stale review
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
andrinoff
force-pushed
the
osc/731-idle-goroutine-leak
branch
from
4f9122b to
b2c95f3
Compare
Member
Benchmark report — no significant changeMetrics worse: 0 · better: 0 (threshold: ±3%). benchstat outputauto-generated by benchmarks.yml |
mx-gp
approved these changes
May 23, 2026
mx-gp
left a comment
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.
Contributor
Author
|
Thanks @mx-gp. |
Member
|
@mvanhorn please, fix the conflicts with the new linter fix, and then merge the branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.