diff --git a/internal/gitlab/client/gitlabnet.go b/internal/gitlab/client/gitlabnet.go index 5d7b0a4348a4a6129ab1154808743477aed5c8cc..7a625cf8740e2e8b156ea506613469e6b3dd87b5 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 b6d8d8779b9cf0777f9bf830d1baca892c84b49a..83662a3ad2ee56c76087d1048ce3558ae624a6c8 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) +}