Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/controller/shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/shared_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))
})
})
})
Expand Down
8 changes: 7 additions & 1 deletion internal/ratelimit/org_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions internal/ratelimit/org_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
})
})
Expand Down