Skip to content

Commit edbbca3

Browse files
danmoseleyCopilot
andcommitted
Fix edge cases: sub-second rate limit durations and UTF-8 BOM
- Primary rate limit: compute time.Until(resetTime) once and check the rounded result is >0 before showing 'Retry after X'. This avoids a TOCTOU race between the After(time.Now()) guard and the subsequent time.Until call, and prevents showing 'Retry after 0s.' when the reset time is imminent. - Secondary rate limit: round RetryAfter first, then check >0. Previously, a RetryAfter of e.g. 200ms would pass the >0 guard but format as 'Retry after 0s.' after rounding. - Add tests for both sub-second edge cases. - Remove UTF-8 BOM accidentally introduced in error_test.go by .NET WriteAllText with the default UTF8 encoding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 52d8383 commit edbbca3

2 files changed

Lines changed: 49 additions & 8 deletions

File tree

pkg/errors/error.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,21 +165,26 @@ func NewGitHubAPIErrorResponse(ctx context.Context, message string, resp *github
165165
var rateLimitErr *github.RateLimitError
166166
if stderrors.As(err, &rateLimitErr) {
167167
resetTime := rateLimitErr.Rate.Reset.Time
168-
if !resetTime.IsZero() && resetTime.After(time.Now()) {
168+
if !resetTime.IsZero() {
169169
retryIn := time.Until(resetTime).Round(time.Second)
170-
return utils.NewToolResultError(fmt.Sprintf(
171-
"%s: GitHub API rate limit exceeded. Retry after %v.", message, retryIn))
170+
if retryIn > 0 {
171+
return utils.NewToolResultError(fmt.Sprintf(
172+
"%s: GitHub API rate limit exceeded. Retry after %v.", message, retryIn))
173+
}
172174
}
173175
return utils.NewToolResultError(fmt.Sprintf(
174176
"%s: GitHub API rate limit exceeded. Wait before retrying.", message))
175177
}
176178

177179
var abuseErr *github.AbuseRateLimitError
178180
if stderrors.As(err, &abuseErr) {
179-
if abuseErr.RetryAfter != nil && *abuseErr.RetryAfter > 0 {
180-
return utils.NewToolResultError(fmt.Sprintf(
181-
"%s: GitHub secondary rate limit exceeded. Retry after %v.",
182-
message, abuseErr.RetryAfter.Round(time.Second)))
181+
if abuseErr.RetryAfter != nil {
182+
retryAfter := abuseErr.RetryAfter.Round(time.Second)
183+
if retryAfter > 0 {
184+
return utils.NewToolResultError(fmt.Sprintf(
185+
"%s: GitHub secondary rate limit exceeded. Retry after %v.",
186+
message, retryAfter))
187+
}
183188
}
184189
return utils.NewToolResultError(fmt.Sprintf(
185190
"%s: GitHub secondary rate limit exceeded. Wait before retrying.", message))

pkg/errors/error_test.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package errors
1+
package errors
22

33
import (
44
"context"
@@ -563,6 +563,24 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) {
563563
assertContextHasError(t, ctx, abuseErr)
564564
})
565565

566+
t.Run("AbuseRateLimitError with sub-second RetryAfter falls back to wait message", func(t *testing.T) {
567+
ctx := ContextWithGitHubErrors(context.Background())
568+
569+
// 200ms rounds to 0s, so should fall back to the generic wait message
570+
retryAfter := 200 * time.Millisecond
571+
abuseErr := &github.AbuseRateLimitError{
572+
Response: &http.Response{StatusCode: 403},
573+
Message: "You have exceeded a secondary rate limit.",
574+
RetryAfter: &retryAfter,
575+
}
576+
resp := &github.Response{Response: abuseErr.Response}
577+
578+
result := NewGitHubAPIErrorResponse(ctx, "create issue", resp, abuseErr)
579+
580+
text := requireErrorText(t, result)
581+
assert.Contains(t, text, "GitHub secondary rate limit exceeded. Wait before retrying.")
582+
})
583+
566584
t.Run("RateLimitError with reset time in the past falls back to wait message", func(t *testing.T) {
567585
ctx := ContextWithGitHubErrors(context.Background())
568586

@@ -580,6 +598,24 @@ func TestNewGitHubAPIErrorResponse_RateLimits(t *testing.T) {
580598
assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.")
581599
})
582600

601+
t.Run("RateLimitError with sub-second reset time falls back to wait message", func(t *testing.T) {
602+
ctx := ContextWithGitHubErrors(context.Background())
603+
604+
// 250ms in the future: still positive, but rounds to 0s, so should fall back
605+
resetTime := time.Now().Add(250 * time.Millisecond)
606+
rateLimitErr := &github.RateLimitError{
607+
Rate: github.Rate{Reset: github.Timestamp{Time: resetTime}},
608+
Response: &http.Response{StatusCode: 403},
609+
Message: "API rate limit exceeded",
610+
}
611+
resp := &github.Response{Response: rateLimitErr.Response}
612+
613+
result := NewGitHubAPIErrorResponse(ctx, "search code", resp, rateLimitErr)
614+
615+
text := requireErrorText(t, result)
616+
assert.Contains(t, text, "GitHub API rate limit exceeded. Wait before retrying.")
617+
})
618+
583619
t.Run("RateLimitError with zero reset time falls back to wait message", func(t *testing.T) {
584620
ctx := ContextWithGitHubErrors(context.Background())
585621

0 commit comments

Comments
 (0)