Ingester client: close conn and cancel stream ctx when Run() fails - #7800
Open
pujitha24 wants to merge 1 commit into
Open
Ingester client: close conn and cancel stream ctx when Run() fails#7800pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Motivation: MakeIngesterClient dials the ingester connection before starting the 100 stream-push workers used by -distributor.use-stream-push=true. If starting those workers fails, the function returned the error without closing the connection or cancelling the stream context. The grpc.ClientConn was left alive and unreferenced, with its addrConn retrying forever, and any job-processing goroutines started by workers that did succeed were left running with nothing to stop them. This happens whenever PushStream fails fast (no WaitForReady) against an address that is still in the ring but no longer reachable, e.g. during an ingester rollout, and it is also reachable from pure read traffic since the querier builds an ingester client pool through the same factory. Run() also wrote its workerErr result from up to 100 goroutines with no synchronization, a data race flagged in the same report. Approach: On MakeIngesterClient's Run() error path, cancel the stream context and close the connection before returning, matching the fix suggested in the report. Cancelling the context unblocks the job-processing goroutines of any workers that had already started successfully. Change Run()'s workerErr from a plain error to a go.uber.org/atomic.Error (already imported in this file), using Store/Load instead of an unsynchronized assignment, preserving the existing last-error-wins behavior with defined semantics under the race detector. Validation: Added TestMakeIngesterClient_CleansUpOnRunFailure, which points MakeIngesterClient (useStreamConnection=true) at a TCP port nothing listens on, so every stream-push worker fails fast with "connection refused" - the same failure mode described in the report. The test takes a runtime.NumGoroutine() baseline and polls for the goroutine count to return to it after the call returns. Ran: go test -race -tags "netgo slicelabels" ./pkg/ingester/client/... -count=1 -v all tests pass. Verified the new test fails without the fix (reverted client.go via git stash) and passes with it restored, repeated several times with no flakiness. Also ran golangci-lint run ./pkg/ingester/client/... (0 issues) and goimports -local github.com/cortexproject/cortex -l on both changed files (no output). Not run: integration tests and a live cluster reproduction - this failure mode isn't reachable through the integration harness without simulating an ingester rollout, which the unit test reproduces more directly. Checked that upstream/master's own CI is currently green. Report: cortexproject#7759 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
CharlieTLe
requested review from
a team and
friedrichg
and removed request for
a team
August 21, 2026 18:11
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
1 participant
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 this PR does:
Fixes a
grpc.ClientConnand goroutine leak inMakeIngesterClient(pkg/ingester/client/client.go). When-distributor.use-stream-push=true,MakeIngesterClientdials the ingester and then starts 100 stream-push workers viaRun(). IfRun()returns an error (e.g. the ingester's address is still in the ring but the pod is gone, soPushStreamfails fast withoutWaitForReady), the function returned the error without closing the just-createdconnor cancelling the stream context. TheClientConnwas left unreferenced but alive, with itsaddrConnreconnecting forever, and any job-processing goroutines started by workers that did succeed were left running with nothing left to stop them.The fix cancels the stream context and closes the connection on that error path, exactly as suggested in the linked issue.
It also fixes a data race flagged in the same issue:
Run()'sworkerErrwas a plainerrorwritten from up to 100 goroutines with no synchronization. It's now ago.uber.org/atomic.Error(already imported in this file for other fields), preserving the existing "last error wins" behavior with defined semantics under-race.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flagsValidation:
Added
TestMakeIngesterClient_CleansUpOnRunFailureinpkg/ingester/client/client_test.go. It pointsMakeIngesterClient(withuseStreamConnection=true) at a TCP port that nothing listens on, so every stream-push worker fails fast onPushStreamwith "connection refused" (confirmed empirically:NewStreamon an unreachable address returns in ~1-2ms without ever completing a TCP handshake — this is exactly the failure mode described in the issue). The test takes aruntime.NumGoroutine()baseline, callsMakeIngesterClient, and polls for the goroutine count to return to baseline.Ran:
go build ./...— passes.go test -race -tags "netgo slicelabels" ./pkg/ingester/client/... -count=1 -v— all tests pass, including the new one.git stashonclient.go): the test fails without the fix (goroutines never return to baseline) and passes with it restored, run repeatedly with no flakiness observed.golangci-lint run ./pkg/ingester/client/...— 0 issues.goimports -local github.com/cortexproject/cortex -lon both changed files — no output (already formatted).Not run: integration tests (this bug isn't reachable through the integration harness without simulating an ingester rollout/unreachable pod, which the unit test above reproduces more directly and deterministically) and a live cluster reproduction. The base branch's own CI is currently green (checked
gh run list --branch master), so this isn't masking a pre-existing failure.AI assistance: this change was drafted with Claude Code.
Fixes #7759