From 7c05f294301c00ad7f4feef536d50b0b01c00e4c Mon Sep 17 00:00:00 2001
From: syedsaifalialvi <saifalialvi@gmail.com>
Date: Sat, 6 Dec 2025 18:52:29 +0530
Subject: [PATCH] Goroutine leak for Too Many Requests error response

When a 429 (Too Many Requests) response is received,
the function returns early with RailsRateLimitedError{}
Without response.Body.Close(), the HTTP response body remains open.
The underlying HTTP transport keeps a goroutine alive to handle the connection.
Since the response is never returned to the caller, it can never be properly closed.

As part of this fix, rate limit check is moved inside parseError
to prevent goroutine leaks specifically in the rate-limiting scenario.
---
 internal/gitlab/client/gitlabnet.go      | 19 +++++---
 internal/gitlab/client/gitlabnet_test.go | 62 ++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/internal/gitlab/client/gitlabnet.go b/internal/gitlab/client/gitlabnet.go
index 5d7b0a4348..7a625cf874 100644
--- a/internal/gitlab/client/gitlabnet.go
+++ b/internal/gitlab/client/gitlabnet.go
@@ -121,6 +121,14 @@ func parseError(resp *http.Response) error {
 		return nil
 	}
 	defer resp.Body.Close()
+
+	// Handle rate limiting errors
+	if resp.StatusCode == http.StatusTooManyRequests {
+		// This error is unwrapped in statushandler.go to return an
+		// appropriate gRPC status code.
+		return RailsRateLimitedError{}
+	}
+
 	parsedResponse := &ErrorResponse{}
 
 	if err := json.NewDecoder(resp.Body).Decode(parsedResponse); err != nil {
@@ -191,14 +199,9 @@ func (c *GitlabNetClient) DoRequest(ctx context.Context, method, path string, da
 		return nil, &APIError{"Internal API unreachable"}
 	}
 
-	if response != nil {
-		logger = logger.WithField("status", response.StatusCode)
-		if response.StatusCode == http.StatusTooManyRequests {
-			// This error is unwrapped in statushandler.go to return an
-			// appropriate gRPC status code.
-			return nil, RailsRateLimitedError{}
-		}
-	}
+	// At this point, response is guaranteed to be non-nil as per httpClient.Do doc
+	logger = logger.WithField("status", response.StatusCode)
+
 	if err := parseError(response); err != nil {
 		logger.WithError(err).Error("Internal API error")
 		return nil, err
diff --git a/internal/gitlab/client/gitlabnet_test.go b/internal/gitlab/client/gitlabnet_test.go
index b6d8d8779b..83662a3ad2 100644
--- a/internal/gitlab/client/gitlabnet_test.go
+++ b/internal/gitlab/client/gitlabnet_test.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"runtime"
 	"testing"
 	"time"
 
@@ -178,3 +179,64 @@ func TestServerErrors(t *testing.T) {
 		})
 	}
 }
+
+func TestRateLimitGoroutineLeak(t *testing.T) {
+	// This test verifies the prevention of rate limit goroutine leaks.
+	// Without that, each 429 response would leak a goroutine because the response body
+	// is never closed when we return early with RailsRateLimitedError.
+
+	// Create a server that returns 429 Too Many Requests with a response body
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusTooManyRequests)
+		// Write a response body to ensure there's data to be read
+		_, err := w.Write([]byte(`{"message": "rate limited"}`))
+		require.NoError(t, err)
+	}))
+	defer server.Close()
+
+	logger := testhelper.NewLogger(t)
+
+	gitlabnet, err := NewGitlabNetClient(
+		logger,
+		"user",
+		"password",
+		"secret",
+		&HTTPClient{Client: server.Client(), Host: server.URL},
+	)
+	require.NoError(t, err)
+
+	// Get initial goroutine count
+	runtime.GC()
+	runtime.GC() // Run GC twice to ensure cleanup
+	initialGoroutines := runtime.NumGoroutine()
+
+	// Make multiple requests that will hit the rate limit
+	const numRequests = 10
+	ctx := testhelper.Context(t)
+
+	for i := 0; i < numRequests; i++ {
+		response, err := gitlabnet.DoRequest(ctx, http.MethodGet, "/test", nil)
+
+		// Should return RailsRateLimitedError and nil response
+		require.Error(t, err)
+		require.IsType(t, RailsRateLimitedError{}, err)
+		require.Nil(t, response)
+	}
+
+	// Force garbage collection to clean up any properly closed resources
+	runtime.GC()
+	runtime.GC()
+
+	// Give some time for goroutines to be cleaned up
+	time.Sleep(100 * time.Millisecond)
+
+	finalGoroutines := runtime.NumGoroutine()
+	goroutineIncrease := finalGoroutines - initialGoroutines
+
+	// The increase should be much less than the number of requests made
+	// Without proper body closing, we'd see an increase close to numRequests
+	require.Less(t, goroutineIncrease, numRequests/2,
+		"Goroutine count increased too much, suggesting a leak. Expected increase < %d, got %d",
+		numRequests/2, goroutineIncrease)
+}
-- 
GitLab