Skip to content

fix(fetcher): wait for IDLE goroutines#1342

Merged
andrinoff merged 2 commits into
floatpane:masterfrom
mvanhorn:osc/731-idle-goroutine-leak
May 26, 2026
Merged

fix(fetcher): wait for IDLE goroutines#1342
andrinoff merged 2 commits into
floatpane:masterfrom
mvanhorn:osc/731-idle-goroutine-leak

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

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.

Testing

  • go test ./fetcher/... -run TestIdleWatcher -count=1 - pass
  • go test -race ./fetcher - pass (added tests plus existing tests)
  • go build ./... - clean
  • gofmt -l fetcher/idle.go daemon/daemon.go fetcher/idle_test.go - clean

Fixes #731

AI-assisted.

@mvanhorn
mvanhorn requested a review from a team as a code owner May 22, 2026 07:16
@floatpanebot floatpanebot added area/fetcher IMAP fetch / IDLE / search area/daemon Daemon / RPC bug Something isn't working labels May 22, 2026

@floatpanebot floatpanebot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@floatpanebot floatpanebot added ci CI / build pipeline size/M Diff: 51–200 lines labels May 22, 2026
@mvanhorn mvanhorn changed the title fix(fetcher): track IDLE goroutines so shutdown can wait for them fix(fetcher): wait for IDLE goroutines May 22, 2026
@floatpanebot
floatpanebot dismissed their stale review May 22, 2026 08:21

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
andrinoff force-pushed the osc/731-idle-goroutine-leak branch from 4f9122b to b2c95f3 Compare May 23, 2026 09:59

@mx-gp mx-gp left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@andrinoff andrinoff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@andrinoff
andrinoff merged commit 83b3fd9 into floatpane:master May 26, 2026
27 checks passed
@floatpane floatpane deleted a comment from mvanhorn May 26, 2026
@floatpane floatpane deleted a comment from andrinoff May 26, 2026
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Thanks @andrinoff for merging the IdleWatcher fix that waits for the IDLE watcher goroutines.

@mvanhorn

mvanhorn commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Really appreciate the merge @andrinoff. Tracking the IDLE watcher goroutines on a WaitGroup so StopAllAndWait catches the lingering ones is a solid fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Daemon / RPC area/fetcher IMAP fetch / IDLE / search bug Something isn't working ci CI / build pipeline size/M Diff: 51–200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Potential goroutine leak in IDLE watcher cleanup

4 participants