Fix MakeIngesterClient leaking grpc.ClientConn and goroutines on Run() failure - #7839
Open
DeviousCardi wants to merge 2 commits into
Open
DeviousCardi wants to merge 2 commits into
DeviousCardi wants to merge 2 commits into
Conversation
When useStreamConnection is true, MakeIngesterClient() started stream-push workers via Run() but discarded the already-dialed grpc.ClientConn and the streamCtx/streamCancel pair on error, leaking the connection (and its reconnect loop) plus any job-processing goroutines started by workers that had already succeeded. Close the connection and cancel the stream context before returning the error. Run() also had two secondary defects: workerErr was written by every worker goroutine with no synchronization (a data race), and a failing worker never signalled its siblings to stop, letting up to 99 successful workers and their streams outlive the discarded client. Collect worker errors through a buffered channel instead, and cancel streamCtx as soon as any worker fails so siblings stop opening new streams and already-started job-processing goroutines exit via ctx.Done(). Fixes cortexproject#7759 Signed-off-by: DeviousCardi <aaravsjadav@gmail.com>
friedrichg
reviewed
Sep 15, 2026
| # Changelog | ||
|
|
||
| ## master / unreleased | ||
| * [BUGFIX] Ingester Client: Fix `MakeIngesterClient` leaking the `grpc.ClientConn` and stream-push worker goroutines when starting stream workers fails (`-distributor.use-stream-push=true`). Also fix a data race on the collected worker error, and stop already-started workers instead of leaving them running when a sibling worker fails. #7759 |
Member
There was a problem hiding this comment.
Suggested change
| * [BUGFIX] Ingester Client: Fix `MakeIngesterClient` leaking the `grpc.ClientConn` and stream-push worker goroutines when starting stream workers fails (`-distributor.use-stream-push=true`). Also fix a data race on the collected worker error, and stop already-started workers instead of leaving them running when a sibling worker fails. #7759 | |
| * [BUGFIX] Ingester Client: Fix `MakeIngesterClient` leaking the `grpc.ClientConn` and stream-push worker goroutines when starting stream workers fails (`-distributor.use-stream-push=true`). Also fix a data race on the collected worker error, and stop already-started workers instead of leaving them running when a sibling worker fails. #7839 |
Per review feedback from @friedrichg. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: DeviousCardi <aaravsjadav@gmail.com>
DeviousCardi
force-pushed
the
fix/7759-ingester-client-leak
branch
from
September 15, 2026 17:02
2cc8757 to
70be36e
Compare
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
2 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.
Summary
Fixes #7759.
When
useStreamConnectionis true,MakeIngesterClientcreates agrpc.ClientConn, then callsRun()to startINGESTER_CLIENT_STREAM_WORKER_COUNT(100) push-stream workers. IfRun()returns an error, the function returned it without closingconnor cancelling the stream context — leaking the connection and any goroutines from workers that had already started successfully. In production this showed up as orphanedClientConns reconnecting forever to unreachable ingester addresses after rollouts (observed viaaddrConn.resetTransportAndUnlockgoroutine counts and steady SYN traffic to dead pod IPs).Two related issues in the same
Run()were fixed alongside the primary leak:PushStream()fails,streamCancel()is now called immediately so sibling workers that already succeeded stop rather than continuing to run against a client that's about to be discarded.workerErrwas written from all 100 worker goroutines with no synchronization. Replaced with a bufferedchan error(sized to the worker count) so the first error is collected safely.Changes
MakeIngesterClient: closeconnand cancelstreamCtxbefore returning an error fromRun().Run(): collect worker errors via a channel instead of an unsynchronized shared variable; cancelstreamCtxas soon as any worker fails, so siblings stop promptly.Test plan
go build ./pkg/ingester/client/...go test ./pkg/ingester/client/... -race— including two new tests: a unit test with a mock scheduler exercising partial-failure cancellation, and an end-to-end test that reproduces the issue's own scenario (unreachable address) and asserts viaruntime.Stackscanning that noresetTransportAndUnlock/newClientStreamWithParamsgoroutines survive afterMakeIngesterClientreturns an error.-racewith no flakiness.🤖 Generated with Claude Code