Handle GitHub rate limits with specialized error and Prometheus metrics

go.mod

@@ -1,6 +1,6 @@
 module github.com/mindersec/minder
 
-go 1.25.8
+go 1.26
 
 require (
 	buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.10-20250912141014-52f32327d4b0.1

internal/providers/github/common.go

@@ -588,13 +588,13 @@ func (c *GitHub) ListHooks(ctx context.Context, owner, repo string) ([]*github.H
 // DeleteHook deletes a specified Hook.
 func (c *GitHub) DeleteHook(ctx context.Context, owner, repo string, id int64) error {
 	resp, err := c.client.Repositories.DeleteHook(ctx, owner, repo, id)
-	if resp != nil && resp.StatusCode == http.StatusNotFound {
+	if isRateLimitError(err) {
+		return c.waitForRateLimitReset(ctx, err)
+	}
+	if resp != nil && (resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusForbidden) {
 		// If the hook is not found, we can ignore the
 		// error, user might have deleted it manually.
-		return nil
-	}
-	if resp != nil && resp.StatusCode == http.StatusForbidden {
-		// We ignore deleting webhooks that we're not
+		// We also ignore deleting webhooks that we're not
 		// allowed to touch. This is usually the case
 		// with repository transfer.
 		return nil
@@ -605,12 +605,22 @@ func (c *GitHub) DeleteHook(ctx context.Context, owner, repo string, id int64) e
 // CreateHook creates a new Hook.
 func (c *GitHub) CreateHook(ctx context.Context, owner, repo string, hook *github.Hook) (*github.Hook, error) {
 	h, _, err := c.client.Repositories.CreateHook(ctx, owner, repo, hook)
+	if isRateLimitError(err) {
+		if waitErr := c.waitForRateLimitReset(ctx, err); waitErr != nil {
+			return nil, waitErr
+		}
+	}
 	return h, err
 }
 
 // EditHook edits an existing Hook.
 func (c *GitHub) EditHook(ctx context.Context, owner, repo string, id int64, hook *github.Hook) (*github.Hook, error) {
 	h, _, err := c.client.Repositories.EditHook(ctx, owner, repo, id, hook)
+	if isRateLimitError(err) {
+		if waitErr := c.waitForRateLimitReset(ctx, err); waitErr != nil {
+			return nil, waitErr
+		}
+	}
 	return h, err
 }
 
@@ -861,16 +871,12 @@ func (c *GitHub) setAsRateLimited() {
 // than MaxRateLimitWait or requests' context is cancelled.
 func (c *GitHub) waitForRateLimitReset(ctx context.Context, err error) error {
 	var rateLimitError *github.RateLimitError
-	isRateLimitErr := errors.As(err, &rateLimitError)
-
-	if isRateLimitErr {
+	if errors.As(err, &rateLimitError) {
 		return c.processPrimaryRateLimitErr(ctx, rateLimitError)
 	}
 
 	var abuseRateLimitError *github.AbuseRateLimitError
-	isAbuseRateLimitErr := errors.As(err, &abuseRateLimitError)
-
-	if isAbuseRateLimitErr {
+	if errors.As(err, &abuseRateLimitError) {
 		return c.processAbuseRateLimitErr(ctx, abuseRateLimitError)
 	}
 
@@ -893,7 +899,7 @@ func (c *GitHub) processPrimaryRateLimitErr(ctx context.Context, err *github.Rat
 
 		if waitTime > MaxRateLimitWait {
 			logger.Debug().Msgf("rate limit reset time: %v exceeds maximum wait time: %v", waitTime, MaxRateLimitWait)
-			return err
+			return engerrors.NewRateLimitError(err, int64(rate.Limit), int64(rate.Remaining), resetTime)
 		}
 
 		// Wait for the rate limit to reset
@@ -902,7 +908,7 @@ func (c *GitHub) processPrimaryRateLimitErr(ctx context.Context, err *github.Rat
 			return nil
 		case <-ctx.Done():
 			logger.Debug().Err(ctx.Err()).Msg("context done while waiting for rate limit to reset")
-			return err
+			return engerrors.NewRateLimitError(err, int64(rate.Limit), int64(rate.Remaining), resetTime)
 		}
 	}
 
@@ -923,7 +929,7 @@ func (c *GitHub) processAbuseRateLimitErr(ctx context.Context, err *github.Abuse
 
 	if waitTime > MaxRateLimitWait {
 		logger.Debug().Msgf("abuse rate limit wait time: %v exceeds maximum wait time: %v", waitTime, MaxRateLimitWait)
-		return err
+		return engerrors.NewRateLimitError(err, 0, 0, time.Now().Add(waitTime))
 	}
 
 	// Wait for the rate limit to reset
@@ -932,7 +938,7 @@ func (c *GitHub) processAbuseRateLimitErr(ctx context.Context, err *github.Abuse
 		return nil
 	case <-ctx.Done():
 		logger.Debug().Err(ctx.Err()).Msg("context done while waiting for rate limit to reset")
-		return err
+		return engerrors.NewRateLimitError(err, 0, 0, time.Now().Add(waitTime))
 	}
 }
 
@@ -966,13 +972,13 @@ func performWithRetry[T any](ctx context.Context, op backoffv4.OperationWithData
 }
 
 func isRateLimitError(err error) bool {
-	var rateLimitError *github.RateLimitError
-	isRateLimitErr := errors.As(err, &rateLimitError)
-
-	var abuseRateLimitError *github.AbuseRateLimitError
-	isAbuseRateLimitErr := errors.As(err, &abuseRateLimitError)
-
-	return isRateLimitErr || isAbuseRateLimitErr
+	if _, ok := errors.AsType[*github.RateLimitError](err); ok {
+		return true
+	}
+	if _, ok := errors.AsType[*github.AbuseRateLimitError](err); ok {
+		return true
+	}
+	return false
 }
 
 // IsMinderHook checks if a GitHub hook is a Minder hook
@@ -1041,11 +1047,14 @@ func (c *GitHub) StartCheckRun(
 
 	run, resp, err := c.client.Checks.CreateCheckRun(ctx, owner, repo, *opts)
 	if err != nil {
+		if isRateLimitError(err) {
+			return nil, c.waitForRateLimitReset(ctx, err)
+		}
 		// If error is 403 then it means we are missing permissions
 		if resp != nil && resp.StatusCode == 403 {
 			return nil, ErroNoCheckPermissions
 		}
-		return nil, fmt.Errorf("creating check run: %w", err)
+		return nil, fmt.Errorf("creating check: %w", err)
 	}
 	return run, nil
 }
@@ -1057,6 +1066,9 @@ func (c *GitHub) UpdateCheckRun(
 ) (*github.CheckRun, error) {
 	run, resp, err := c.client.Checks.UpdateCheckRun(ctx, owner, repo, checkRunID, *opts)
 	if err != nil {
+		if isRateLimitError(err) {
+			return nil, c.waitForRateLimitReset(ctx, err)
+		}
 		// If error is 403 then it means we are missing permissions
 		if resp != nil && resp.StatusCode == 403 {
 			return nil, ErroNoCheckPermissions

internal/providers/github/common_test.go

@@ -26,6 +26,7 @@ import (
 	mock_ratecache "github.com/mindersec/minder/internal/providers/ratecache/mock"
 	minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
 	config "github.com/mindersec/minder/pkg/config/server"
+	engerrors "github.com/mindersec/minder/pkg/engine/errors"
 	provifv1 "github.com/mindersec/minder/pkg/providers/v1"
 )
 
@@ -120,7 +121,7 @@ func TestWaitForRateLimitReset(t *testing.T) {
 				).Return()
 			},
 			wantErr:     true,
-			expectedErr: &github.RateLimitError{},
+			expectedErr: &engerrors.RateLimitError{Base: &github.RateLimitError{}},
 		},
 		{
 			name: "abuse rate limit error with retry after under max wait",
@@ -817,7 +818,7 @@ func TestCreateHook(t *testing.T) {
 				headers := make(http.Header)
 				headers.Set("X-RateLimit-Limit", "60")
 				headers.Set("X-RateLimit-Remaining", "0")
-				headers.Set("X-RateLimit-Reset", "1632182400")
+				headers.Set("X-RateLimit-Reset", fmt.Sprint(time.Now().Add(1*time.Hour).Unix()))
 				headers.Set("Content-Type", "application/json; charset=utf-8")
 
 				client := &http.Client{
@@ -853,7 +854,7 @@ func TestCreateHook(t *testing.T) {
 				).AnyTimes()
 			},
 			wantErr:     true,
-			expectedErr: &github.RateLimitError{},
+			expectedErr: &engerrors.RateLimitError{Base: &github.RateLimitError{}},
 		},
 	}
 

internal/providers/telemetry/telemetry.go

@@ -4,8 +4,11 @@
 package telemetry
 
 import (
+	"context"
 	"fmt"
+	"math"
 	"net/http"
+	"strconv"
 	"time"
 
 	"github.com/puzpuzpuz/xsync/v3"
@@ -20,8 +23,10 @@ import (
 var _ http.RoundTripper = (*instrumentedRoundTripper)(nil)
 
 type instrumentedRoundTripper struct {
-	baseRoundTripper  http.RoundTripper
-	durationHistogram metric.Int64Histogram
+	baseRoundTripper            http.RoundTripper
+	durationHistogram           metric.Int64Histogram
+	rateLimitRemainingHistogram metric.Int64Histogram
+	throttledCounter            metric.Int64Counter
 }
 
 func (irt *instrumentedRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
@@ -41,9 +46,46 @@ func (irt *instrumentedRoundTripper) RoundTrip(r *http.Request) (*http.Response,
 
 	irt.durationHistogram.Record(r.Context(), duration, metric.WithAttributes(labels...))
 
+	if resp != nil {
+		irt.recordRateLimitMetrics(r.Context(), resp, labels)
+	}
+
 	return resp, err
 }
 
+func (irt *instrumentedRoundTripper) recordRateLimitMetrics(
+	ctx context.Context,
+	resp *http.Response,
+	labels []attribute.KeyValue,
+) {
+	remainingHeader := resp.Header.Get("X-RateLimit-Remaining")
+	// Parse the header once; use math.MinInt64 as sentinel when absent or unparseable.
+	remaining := int64(math.MinInt64)
+	if remainingHeader != "" {
+		if parsed, err := strconv.ParseInt(remainingHeader, 10, 64); err == nil {
+			remaining = parsed
+		}
+	}
+
+	if remaining != math.MinInt64 {
+		irt.rateLimitRemainingHistogram.Record(ctx, remaining, metric.WithAttributes(labels...))
+	}
+
+	// Check for throttling (403 or 429 with rate limit headers)
+	if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusTooManyRequests {
+		// A 429 is always throttled; a 403 is throttled only when quota is exhausted.
+		isThrottled := resp.StatusCode == http.StatusTooManyRequests
+		// then add the case where we don't get a 429, but we have 0 quota remaining
+		if !isThrottled && remaining == 0 {
+			isThrottled = true
+		}
+
+		if isThrottled {
+			irt.throttledCounter.Add(ctx, 1, metric.WithAttributes(labels...))
+		}
+	}
+}
+
 var _ ProviderMetrics = (*providerMetrics)(nil)
 
 type providerMetrics struct {
@@ -62,7 +104,9 @@ var _ HttpClientMetrics = (*httpClientMetrics)(nil)
 type httpClientMetrics struct {
 	providersMeter metric.Meter
 
-	httpProviderHistograms *xsync.MapOf[db.ProviderType, metric.Int64Histogram]
+	httpProviderHistograms       *xsync.MapOf[db.ProviderType, metric.Int64Histogram]
+	rateLimitRemainingHistograms *xsync.MapOf[db.ProviderType, metric.Int64Histogram]
+	throttledCounters            *xsync.MapOf[db.ProviderType, metric.Int64Counter]
 }
 
 func newProviderMapOf[V any]() *xsync.MapOf[db.ProviderType, V] {
@@ -72,8 +116,10 @@ func newProviderMapOf[V any]() *xsync.MapOf[db.ProviderType, V] {
 // newHttpClientMetrics creates a new http provider metrics instance.
 func newHttpClientMetrics() *httpClientMetrics {
 	return &httpClientMetrics{
-		providersMeter:         otel.Meter("providers"),
-		httpProviderHistograms: newProviderMapOf[metric.Int64Histogram](),
+		providersMeter:               otel.Meter("providers"),
+		httpProviderHistograms:       newProviderMapOf[metric.Int64Histogram](),
+		rateLimitRemainingHistograms: newProviderMapOf[metric.Int64Histogram](),
+		throttledCounters:            newProviderMapOf[metric.Int64Counter](),
 	}
 }
 
@@ -85,29 +131,81 @@ func (m *httpClientMetrics) createProviderHistogram(providerType db.ProviderType
 	)
 }
 
+func (m *httpClientMetrics) createRateLimitRemainingHistogram(providerType db.ProviderType) (metric.Int64Histogram, error) {
+	histogramName := fmt.Sprintf("%s.http.ratelimit.remaining", providerType)
+	return m.providersMeter.Int64Histogram(histogramName,
+		metric.WithDescription("HTTP rate limit remaining for provider"),
+		metric.WithUnit("1"),
+	)
+}
+
+func (m *httpClientMetrics) createThrottledCounter(providerType db.ProviderType) (metric.Int64Counter, error) {
+	counterName := fmt.Sprintf("%s.http.throttled.count", providerType)
+	return m.providersMeter.Int64Counter(counterName,
+		metric.WithDescription("Count of throttled HTTP requests for provider"),
+		metric.WithUnit("1"),
+	)
+}
+
 func (m *httpClientMetrics) getHistogramForProvider(providerType db.ProviderType) metric.Int64Histogram {
 	histogram, _ := m.httpProviderHistograms.LoadOrCompute(providerType, func() metric.Int64Histogram {
 		newHistogram, err := m.createProviderHistogram(providerType)
 		if err != nil {
-			log.Printf("failed to create histogram for provider %s: %v", providerType, err)
+			log.Printf("failed to create duration histogram for provider %s: %v", providerType, err)
+			return nil
+		}
+		return newHistogram
+	})
+	return histogram
+}
+
+func (m *httpClientMetrics) getRateLimitHistogramForProvider(providerType db.ProviderType) metric.Int64Histogram {
+	histogram, _ := m.rateLimitRemainingHistograms.LoadOrCompute(providerType, func() metric.Int64Histogram {
+		newHistogram, err := m.createRateLimitRemainingHistogram(providerType)
+		if err != nil {
+			log.Printf("failed to create rate limit histogram for provider %s: %v", providerType, err)
 			return nil
 		}
 		return newHistogram
 	})
 	return histogram
 }
 
+func (m *httpClientMetrics) getThrottledCounterForProvider(providerType db.ProviderType) metric.Int64Counter {
+	counter, _ := m.throttledCounters.LoadOrCompute(providerType, func() metric.Int64Counter {
+		newCounter, err := m.createThrottledCounter(providerType)
+		if err != nil {
+			log.Printf("failed to create throttled counter for provider %s: %v", providerType, err)
+			return nil
+		}
+		return newCounter
+	})
+	return counter
+}
+
 func (m *httpClientMetrics) NewDurationRoundTripper(
 	wrapped http.RoundTripper,
 	providerType db.ProviderType,
 ) (http.RoundTripper, error) {
 	histogram := m.getHistogramForProvider(providerType)
 	if histogram == nil {
-		return nil, fmt.Errorf("failed to retrieve histogram for provider %s", providerType)
+		return nil, fmt.Errorf("failed to retrieve duration histogram for provider %s", providerType)
+	}
+
+	rateLimitRemainingHistogram := m.getRateLimitHistogramForProvider(providerType)
+	if rateLimitRemainingHistogram == nil {
+		return nil, fmt.Errorf("failed to retrieve rate limit histogram for provider %s", providerType)
+	}
+
+	throttledCounter := m.getThrottledCounterForProvider(providerType)
+	if throttledCounter == nil {
+		return nil, fmt.Errorf("failed to retrieve throttled counter for provider %s", providerType)
 	}
 
 	return &instrumentedRoundTripper{
-		baseRoundTripper:  wrapped,
-		durationHistogram: histogram,
+		baseRoundTripper:            wrapped,
+		durationHistogram:           histogram,
+		rateLimitRemainingHistogram: rateLimitRemainingHistogram,
+		throttledCounter:            throttledCounter,
 	}, nil
 }