goldmane: fix goroutine leaks in flow cache and emitter client#12160
Open
caseydavenport wants to merge 2 commits intoprojectcalico:masterfrom
Open
goldmane: fix goroutine leaks in flow cache and emitter client#12160caseydavenport wants to merge 2 commits intoprojectcalico:masterfrom
caseydavenport wants to merge 2 commits intoprojectcalico:masterfrom
Conversation
ExpiringFlowCache.Run() looped forever with no cancellation, leaking a goroutine each time FlowClient.Connect() was called or when the server shut down. Add a context parameter so callers can stop the cleanup loop. The emitter HTTP client launched two goroutines (cert file watcher and reload consumer) against context.Background(), making them impossible to stop. Move goroutine startup out of the constructor into a new watchCerts(ctx) method called from Emitter.Run(), so they respect the emitter's lifecycle context.
caseydavenport
added
docs-not-required
caseydavenport
added
docs-not-required
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two goroutine leak sources in goldmane: the ExpiringFlowCache.Run() loop that had no cancellation mechanism, and the emitter client's certificate watcher goroutines that were started against context.Background() with no shutdown path. Both are now tied to a context.Context for proper lifecycle management.
Changes:
ExpiringFlowCache.Run()now accepts acontext.Contextand uses a ticker with select for cancellation support- Certificate watcher goroutines moved from
newEmitterClient()constructor to a newwatchCerts(ctx)method called fromEmitter.Run() - All callers updated to pass context through
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| goldmane/pkg/internal/flowcache/timed_cache.go | Add ctx parameter to Run(), replace time.After with ticker + select on ctx.Done() |
| goldmane/pkg/client/client.go | Pass ctx to cache.Run() |
| goldmane/pkg/server/flow_collector_service.go | Pass ctx to deduplicator.Run() |
| goldmane/pkg/daemon/daemon.go | Pass ctx to collector.Run() |
| goldmane/pkg/emitter/http_client.go | Extract cert watching into watchCerts(ctx) method |
| goldmane/pkg/emitter/emitter.go | Call watchCerts(ctx) from Emitter.Run() |
Select on ctx.Done() alongside time.After in the file watcher loop so it exits promptly on cancellation instead of blocking up to 30s.
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
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.
Two sources of goroutine leaks in goldmane:
ExpiringFlowCache.Run() looped forever with no cancellation. Each call to
FlowClient.Connect()launched a new cache cleanup goroutine that could never be stopped — so reconnections accumulated goroutines, andFlowClient.Close()left one behind. The server-side deduplicator cache had the same issue. Fix: add acontext.Contextparameter toRun()and select onctx.Done()alongside a ticker.emitterClient cert watchers launched two goroutines (file watcher + reload consumer) against
context.Background()in the constructor, with no way to shut them down. Fix: move goroutine startup out ofnewEmitterClient()into a newwatchCerts(ctx)method, called fromEmitter.Run()where we have the real lifecycle context. When ctx is cancelled the file watcher exits, closesupdChan, and the reload goroutine drains and returns.