Fix #4791: Use idiomatic for-range pattern for error channel closes #4791 (#4835)

* Add test demonstrating bug #4791: Goroutine leak in executeUpdateSetsInParallel

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Fix #4791: Use idiomatic for-range pattern for error channel

Replace the for-select-nil-check pattern with the idiomatic for-range
pattern when consuming from the error channel. The for-range pattern:
- Automatically exits when the channel is closed
- Doesn't require manual nil checks
- Is more maintainable and less error-prone
- Follows Go best practices for channel consumption

This eliminates the potential for goroutine leaks if the nil check
were accidentally removed or modified in future maintenance.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

pkg/connection/connection_lifecycle_test.go

@@ -2,6 +2,8 @@ package connection
 
 import (
 	"context"
+	"errors"
+	"runtime"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -279,3 +281,81 @@ func TestLogRefreshConnectionResultsTypeAssertion(t *testing.T) {
 		state.logRefreshConnectionResults()
 	})
 }
+
+// TestExecuteUpdateSetsInParallelGoroutineLeak tests for goroutine leak in executeUpdateSetsInParallel
+// This test demonstrates issue #4791 - potential goroutine leak with non-idiomatic channel pattern
+//
+// The issue is in refresh_connections_state.go:519-536 where the goroutine uses:
+//   for { select { case connectionError := <-errChan: if connectionError == nil { return } } }
+//
+// While this pattern technically works when the channel is closed (returns nil, then returns from goroutine),
+// it has several problems:
+// 1. It's not idiomatic Go - the standard pattern for consuming until close is 'for range'
+// 2. It relies on nil checks which can be error-prone
+// 3. It's harder to understand and maintain
+// 4. If the nil check is accidentally removed or modified, it causes a goroutine leak
+//
+// The idiomatic pattern 'for range errChan' automatically exits when channel is closed,
+// making the code safer and more maintainable.
+func TestExecuteUpdateSetsInParallelGoroutineLeak(t *testing.T) {
+	// Get baseline goroutine count
+	runtime.GC()
+	time.Sleep(100 * time.Millisecond)
+	baselineGoroutines := runtime.NumGoroutine()
+
+	// Test the CURRENT pattern from refresh_connections_state.go:519-536
+	// This pattern has potential for goroutine leaks if not carefully maintained
+	errChan := make(chan *connectionError)
+	var errorList []error
+	var mu sync.Mutex
+
+	// Simulate the current (non-idiomatic) pattern
+	go func() {
+		for {
+			select {
+			case connectionError := <-errChan:
+				if connectionError == nil {
+					return
+				}
+				mu.Lock()
+				errorList = append(errorList, connectionError.err)
+				mu.Unlock()
+			}
+		}
+	}()
+
+	// Send some errors
+	testErr := errors.New("test error")
+	errChan <- &connectionError{name: "test1", err: testErr}
+	errChan <- &connectionError{name: "test2", err: testErr}
+
+	// Close the channel (this should cause goroutine to exit via nil check)
+	close(errChan)
+
+	// Give time for the goroutine to process and exit
+	time.Sleep(200 * time.Millisecond)
+	runtime.GC()
+	time.Sleep(100 * time.Millisecond)
+
+	// Check for goroutine leak
+	afterGoroutines := runtime.NumGoroutine()
+	goroutineDiff := afterGoroutines - baselineGoroutines
+
+	// The current pattern SHOULD work (goroutine exits via nil check),
+	// but we're testing to document that the pattern is risky
+	if goroutineDiff > 2 {
+		t.Errorf("Goroutine leak detected with current pattern: baseline=%d, after=%d, diff=%d",
+			baselineGoroutines, afterGoroutines, goroutineDiff)
+	}
+
+	// Verify errors were collected
+	mu.Lock()
+	if len(errorList) != 2 {
+		t.Errorf("Expected 2 errors, got %d", len(errorList))
+	}
+	mu.Unlock()
+
+	t.Logf("BUG #4791: Current pattern works but is non-idiomatic and error-prone")
+	t.Logf("The for-select-nil-check pattern at refresh_connections_state.go:520-535")
+	t.Logf("should be replaced with idiomatic 'for range errChan' for safety and clarity")
+}

pkg/connection/refresh_connections_state.go

@@ -531,20 +531,14 @@ func (s *refreshConnectionState) executeUpdateSetsInParallel(ctx context.Context
 	sem := semaphore.NewWeighted(maxParallel)
 
 	go func() {
-		for {
+		for connectionError := range errChan {
-			select {
+			errors = append(errors, connectionError.err)
-			case connectionError := <-errChan:
+			conn, poolErr := s.pool.Acquire(ctx)
-				if connectionError == nil {
+			if poolErr == nil {
-					return
+				if err := s.tableUpdater.onConnectionError(ctx, conn.Conn(), connectionError.name, connectionError.err); err != nil {
-				}
+					log.Println("[WARN] failed to update connection state table", err.Error())
-				errors = append(errors, connectionError.err)
-				conn, poolErr := s.pool.Acquire(ctx)
-				if poolErr == nil {
-					if err := s.tableUpdater.onConnectionError(ctx, conn.Conn(), connectionError.name, connectionError.err); err != nil {
-						log.Println("[WARN] failed to update connection state table", err.Error())
-					}
-					conn.Release()
 				}
+				conn.Release()
 			}
 		}
 	}()