Handle GitHub rate limits with specialized error and Prometheus metrics by krrish175-byte · Pull Request #6306 · mindersec/minder

krrish175-byte · 2026-04-08T17:07:53Z

Summary

This PR implements robust handling for GitHub API throttling and exposes relevant Prometheus metrics to monitor rate limit usage, resolving Issue #4616.

Key changes:

Specialized RateLimitError: Introduced a new error type in pkg/engine/errors that wraps GitHub rate limit information (limit, remaining, reset time), giving upper layers structured context for throttling events.
Telemetry Instrumentation: Extended the HTTP client instrumentation in internal/providers/telemetry to automatically parse GitHub-specific X-RateLimit-* headers from every response.
Observability Metrics:
- github.http.ratelimit.remaining (Histogram): Records the available quota to monitor usage patterns.
- github.http.throttled.count (Counter): Tracks the frequency of 403/429 responses specifically caused by rate limiting.
Provider Consistency: Updated core GitHub provider methods (CreateHook, DeleteHook, UpdateCheckRun, etc.) to consistently use the new error type and the existing rate-limit wait logic.
Structural Alignment: Migrated the engine errors package to pkg/engine/errors to match the latest upstream repository organization.

Fixes #4616

Testing

Automated Verification: Created a new test suite in internal/providers/telemetry/telemetry_test.go using OpenTelemetry's ManualReader to verify accurate header parsing and metric recording across normal and throttled scenarios.
Integration Stability: Updated and verified existing tests in internal/providers/github/common_test.go to ensure correct propagation of the new specialized error type.
Full Build: Verified project stability with a clean make build and a full test run.

… metrics

…ename

coveralls · 2026-04-08T17:19:21Z

coverage: 59.539% (+0.2%) from 59.327% — krrish175-byte:fix/issue-4616 into mindersec:main

evankanderson

A few small comments, but this is looking reasonable, thank you!

evankanderson · 2026-04-08T21:08:07Z

+		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.
+			// We also ignore deleting webhooks that we're not
+			// allowed to touch. This is usually the case
+			// with repository transfer.
+			return nil
+		}


I'm not sure these are in the "err is set" case. (They weren't in such a block before.) Are you sure that these set err?

Still wondering about these being in the err != nil case, since they weren't previously in such a block.

evankanderson · 2026-04-08T21:17:55Z

+		// or if it's an abuse rate limit.
+		isThrottled := false
+		if remainingHeader != "" {
+			if remaining, err := strconv.ParseInt(remainingHeader, 10, 64); err == nil && remaining == 0 {


I'd prefer to parse the remainingHeader once rather than re-processing it here. Maybe use a sentinel value like math.MinInt64 or the like to detect that the value couldn't be found?

evankanderson · 2026-04-08T21:19:09Z

+	if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusTooManyRequests {
+		// Verify it's actually a rate limit error by checking the remaining quota (if 0)
+		// or if it's an abuse rate limit.
+		isThrottled := false


You can simplify this with the case on 78-80:

Suggested change

isThrottled := false

isThrottled := resp.StatusCode == http.StatusTooManyRequests

// then add the case where we don't get a 429, but we have 0 quota remaining

- common.go: use errors.AsType[T] (Go 1.26) in isRateLimitError - go.mod: bump Go version to 1.26 - telemetry.go: parse remainingHeader once using math.MinInt64 sentinel, simplify isThrottled initialization to resp.StatusCode == 429 - telemetry_test.go: add t.Parallel() to test function and subtest loop

krrish175-byte · 2026-04-09T06:03:17Z

@evankanderson PTAL

- datasources/rest/handler.go: drain and close response body before retrying on 429 to fix gosec G104 unhandled error warnings - security.yml: add helm dependency update step before Helm scan so the common chart dependency is present for Trivy to render the chart

evankanderson

Looks like you have a merge conflict in internal/datasources/rest/handler.go (just discard your changes), and one question in common.go where you moved a block inside an if err != nil block where it wasn't before.

evankanderson · 2026-04-10T07:33:55Z

 				Msg("rate limited, retrying")
 			retryCount++
+			// Drain and close the body before retrying to allow connection reuse.
+			_, _ = io.Copy(io.Discard, resp.Body)


I think these were handled in #6312 -- maybe sync / discard these changes?

(Sorry, between a few CI flakes and about 5-6 PRs merging each day, things have gotten less clean than I'd like.)

evankanderson · 2026-04-10T07:34:41Z

+		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.
+			// We also ignore deleting webhooks that we're not
+			// allowed to touch. This is usually the case
+			// with repository transfer.
+			return nil
+		}


Still wondering about these being in the err != nil case, since they weren't previously in such a block.

krrish175-byte · 2026-04-11T11:18:15Z

I have addressed both items:

internal/datasources/rest/handler.go: Discarded the changes related to body draining as they are indeed handled in #6312.
internal/providers/github/common.go: Refactored DeleteHook to move the status code checks (404/403) outside of the explicit if err != nil block, restoring the original logic structure.

Will be fixing the tests now

evankanderson

One lint error, and you probably need to revert the change to security.yml.

evankanderson · 2026-04-12T13:55:49Z

+      - name: Fetch Helm chart dependencies
+        if: always()
+        run: helm dependency update ./deployment/helm
+


This is probably wrong -- it will end up updating the helm dependencies for the security scan but NOT for the actual built components, so we'd end up shipping vulnerabilities that our scans don't find.

Probably the correct fix here (in a separate PR!) is to add helm to the .github/dependabot.yml configuration if you want to do so.

evankanderson · 2026-04-12T14:01:47Z

+		}
 		// If error is 403 then it means we are missing permissions
 		if resp != nil && resp.StatusCode == 403 {
+			return nil, fmt.Errorf("missing permissions: check")


You have a lint error here (you return, then more code follows it -- in this case, another return).

… lint error

krrish175-byte · 2026-04-13T17:02:06Z

@evankanderson PTAL

krrish175-byte added 2 commits April 8, 2026 22:29

feat: handle GitHub rate limits with specialized error and Prometheus…

5dc536c

… metrics

chore: merge upstream main and resolve conflicts with engine/errors r…

07b8afa

…ename

krrish175-byte requested a review from a team as a code owner April 8, 2026 17:07

evankanderson reviewed Apr 8, 2026

View reviewed changes

evankanderson reviewed Apr 10, 2026

View reviewed changes

krrish175-byte added 2 commits April 11, 2026 16:42

refactor: address maintainer feedback on hook deletion and rest handler

58a2fe8

Merge branch 'main' into fix/issue-4616

d26e71b

evankanderson reviewed Apr 12, 2026

View reviewed changes

Address maintainer feedback: revert security workflow changes and fix…

d640293

… lint error

evankanderson approved these changes Apr 13, 2026

View reviewed changes

evankanderson merged commit d547963 into mindersec:main Apr 13, 2026
26 checks passed

krrish175-byte deleted the fix/issue-4616 branch April 14, 2026 04:39

	isThrottled := false
	isThrottled := resp.StatusCode == http.StatusTooManyRequests
	// then add the case where we don't get a 429, but we have 0 quota remaining

Conversation

krrish175-byte commented Apr 8, 2026

Summary

Testing

Uh oh!

coveralls commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krrish175-byte commented Apr 9, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krrish175-byte commented Apr 11, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krrish175-byte commented Apr 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Apr 8, 2026 •

edited

Loading

evankanderson Apr 8, 2026 •

edited

Loading