diff --git a/internal/controller/shared.go b/internal/controller/shared.go index a48765a..77f2a05 100644 --- a/internal/controller/shared.go +++ b/internal/controller/shared.go @@ -35,8 +35,10 @@ func handleRequeueError(ctx context.Context, err error) (controllerruntime.Resul resetTime = &ownRateLimitErr.ResetTime } if resetTime != nil { - logPkg.FromContext(ctx).Info("GitHub API rate limit reached, requeuing after reset time", "resetTime", *resetTime) - return controllerruntime.Result{RequeueAfter: time.Until(*resetTime)}, nil + // requeue after at least 30 seconds + delay := max(time.Until(*resetTime), 30*time.Second) + logPkg.FromContext(ctx).Info("GitHub API rate limit reached, requeuing after reset time", "resetTime", *resetTime, "requeueAfter", delay) + return controllerruntime.Result{RequeueAfter: delay}, nil } // requeue because spreading requires it diff --git a/internal/controller/shared_test.go b/internal/controller/shared_test.go index ba4d51d..5b5d9cb 100644 --- a/internal/controller/shared_test.go +++ b/internal/controller/shared_test.go @@ -215,7 +215,7 @@ var _ = Describe("Shared Controller Functions", func() { }) Context("rate limit error with past reset time", func() { - It("should handle reset time in the past gracefully", func() { + It("should apply minimum backoff when reset time is in the past", func() { resetTime := time.Now().Add(-1 * time.Minute) err := &ghclient.RateLimitedError{ ResetTime: resetTime, @@ -224,8 +224,8 @@ var _ = Describe("Shared Controller Functions", func() { result, returnErr := handleRequeueError(ctx, err) Expect(returnErr).ToNot(HaveOccurred()) - // RequeueAfter will be negative, which controller-runtime interprets as immediate - Expect(result.RequeueAfter).To(BeNumerically("<", 0)) + // Minimum backoff of 30s applied to avoid tight-loop + Expect(result.RequeueAfter).To(Equal(30 * time.Second)) }) }) }) diff --git a/internal/ratelimit/org_registry.go b/internal/ratelimit/org_registry.go index c590f6d..62cb90c 100644 --- a/internal/ratelimit/org_registry.go +++ b/internal/ratelimit/org_registry.go @@ -221,8 +221,14 @@ func (r *OrgRateLimitRegistry) ShouldStall(orgLogin string, categories ...Catego continue } if catState.Remaining < threshold { + // If the reset time (plus grace period) has already passed, the rate limit window + // has renewed and the cached Remaining value is stale. Do NOT stall — let the + // request through so the rateLimitTrackerTransport can record fresh headers. + delay := time.Until(catState.ResetTime.Add(r.config.ResetGracePeriod)) + if delay <= 0 { + continue + } stalled = true - delay := max(time.Until(catState.ResetTime.Add(r.config.ResetGracePeriod)), 0) if delay > maxDelay { maxDelay = delay } diff --git a/internal/ratelimit/org_registry_test.go b/internal/ratelimit/org_registry_test.go index 0f8afe4..ad2cb5e 100644 --- a/internal/ratelimit/org_registry_test.go +++ b/internal/ratelimit/org_registry_test.go @@ -131,12 +131,12 @@ var _ = Describe("OrgRateLimitRegistry", func() { }) Context("when reset time is already in the past", func() { - It("returns stalled=true but with zero delay", func() { + It("returns stalled=false because the rate limit window has renewed", func() { past := time.Now().Add(-5 * time.Minute) registry.Update("my-org", ratelimit.CategoryCore, 5, 5000, past, 1) stalled, delay := registry.ShouldStall("my-org", ratelimit.CategoryCore) - Expect(stalled).To(BeTrue()) + Expect(stalled).To(BeFalse()) Expect(delay).To(Equal(time.Duration(0))) }) })