Skip to content

Fix MakeIngesterClient leaking grpc.ClientConn and goroutines on Run() failure - #7839

Open
DeviousCardi wants to merge 3 commits into
cortexproject:masterfrom
DeviousCardi:fix/7759-ingester-client-leak
Open

DeviousCardi wants to merge 3 commits into
cortexproject:masterfrom
DeviousCardi:fix/7759-ingester-client-leak

Conversation

@DeviousCardi

Copy link
Copy Markdown

Summary

Fixes #7759.

When useStreamConnection is true, MakeIngesterClient creates a grpc.ClientConn, then calls Run() to start INGESTER_CLIENT_STREAM_WORKER_COUNT (100) push-stream workers. If Run() returns an error, the function returned it without closing conn or cancelling the stream context — leaking the connection and any goroutines from workers that had already started successfully. In production this showed up as orphaned ClientConns reconnecting forever to unreachable ingester addresses after rollouts (observed via addrConn.resetTransportAndUnlock goroutine counts and steady SYN traffic to dead pod IPs).

Two related issues in the same Run() were fixed alongside the primary leak:

  • Orphaned workers on partial failure: as soon as one worker's 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.
  • Data race: workerErr was written from all 100 worker goroutines with no synchronization. Replaced with a buffered chan error (sized to the worker count) so the first error is collected safely.

Changes

  • MakeIngesterClient: close conn and cancel streamCtx before returning an error from Run().
  • Run(): collect worker errors via a channel instead of an unsynchronized shared variable; cancel streamCtx as soon as any worker fails, so siblings stop promptly.
  • Added a CHANGELOG.md entry.

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 via runtime.Stack scanning that no resetTransportAndUnlock/newClientStreamWithParams goroutines survive after MakeIngesterClient returns an error.
  • Ran the new tests 20x under -race with no flakiness.

🤖 Generated with Claude Code

@DeviousCardi
DeviousCardi requested a review from a team as a code owner September 15, 2026 08:46
Comment thread CHANGELOG.md Outdated
@DeviousCardi
DeviousCardi force-pushed the fix/7759-ingester-client-leak branch from 2cc8757 to 70be36e Compare September 15, 2026 17:02

@friedrichg friedrichg 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.

Thanks!

DeviousCardi and others added 3 commits October 8, 2026 08:40
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>
Per review feedback from @friedrichg.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: DeviousCardi <aaravsjadav@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: DeviousCardi <aaravsjadav@gmail.com>
@DeviousCardi
DeviousCardi force-pushed the fix/7759-ingester-client-leak branch from ab46387 to d7679fc Compare October 8, 2026 03:13

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MakeIngesterClient leaks grpc.ClientConn and goroutines on failure

2 participants