rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(github): support HTTP-date format in Retry-After header #110

Merged
aweiker merged 4 commits from review-bot-issue-94 into main 2026-05-13 15:34:26 +00:00
Conversation 18 Commits 4 Files Changed 2
+5 -4
1 changed files with 5 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 9f8e9aa8d3 - Show all commits
github/client.go
+5 -4
View File
9
 @@ -95,7 +95,7 @@ type Client struct {
// accepting full URLs instead.
baseURL string
token string
http *http.Client
httpClient *http.Client
gpt-review-bot commented 2026-05-13 13:24:51 +00:00
Review
Copy Link
Copy Source

[NIT] Using strings.TrimRight(baseURL, "/") removes all trailing slashes. If the intent is to remove only a single trailing slash, strings.TrimSuffix is more precise. Current behavior is acceptable but slightly broader than needed.

**[NIT]** Using strings.TrimRight(baseURL, "/") removes all trailing slashes. If the intent is to remove only a single trailing slash, strings.TrimSuffix is more precise. Current behavior is acceptable but slightly broader than needed.
// retryBackoff defines the delays between retry attempts for 429 responses.
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
3
 @@ -117,7 +117,7 @@ func NewClient(token, baseURL string) *Client {
return &Client{
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
http: &http.Client{Timeout: 30 * time.Second},
httpClient: &http.Client{Timeout: 30 * time.Second},
security-review-bot commented 2026-05-13 13:11:28 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header is set on the client requests; while Go’s standard library drops Authorization on cross-host redirects in recent versions, consider explicitly configuring http.Client.CheckRedirect to prevent following cross-host redirects or to strip sensitive headers on redirects for defense-in-depth against token leakage.

**[MINOR]** Authorization header is set on the client requests; while Go’s standard library drops Authorization on cross-host redirects in recent versions, consider explicitly configuring http.Client.CheckRedirect to prevent following cross-host redirects or to strip sensitive headers on redirects for defense-in-depth against token leakage.
gpt-review-bot commented 2026-05-13 13:24:51 +00:00
Review
Copy Link
Copy Source

[MINOR] parseRetryAfter uses strconv.Atoi (int) for delta-seconds; on 32-bit architectures or very large values this can overflow and fail to parse. Prefer strconv.ParseInt(value, 10, 64) to parse into int64, then convert to time.Duration and cap, ensuring architecture-independent robustness.

**[MINOR]** parseRetryAfter uses strconv.Atoi (int) for delta-seconds; on 32-bit architectures or very large values this can overflow and fail to parse. Prefer strconv.ParseInt(value, 10, 64) to parse into int64, then convert to time.Duration and cap, ensuring architecture-independent robustness.
now: time.Now,
}
}
@@ -125,7 +125,7 @@ func NewClient(token, baseURL string) *Client {
// SetHTTPClient sets the underlying HTTP client used for requests.
// This is intended for testing to inject mock transports.
func (c *Client) SetHTTPClient(hc *http.Client) {
c.http = hc
c.httpClient = hc
}
// SetRetryBackoff sets the delays between retry attempts.
security-review-bot marked this conversation as resolved
security-review-bot commented 2026-05-13 12:13:01 +00:00
Review
Copy Link
Copy Source

[MINOR] Potential race condition: doRequest uses and mutates the shared c.retryBackoff slice (e.g., later at line 193) when non-nil. Since Client is intended to be safe for concurrent use, concurrent calls could race and cause unpredictable backoff behavior. Consider copying the slice per request (e.g., append([]time.Duration(nil), c.retryBackoff...)) before mutating.

**[MINOR]** Potential race condition: doRequest uses and mutates the shared c.retryBackoff slice (e.g., later at line 193) when non-nil. Since Client is intended to be safe for concurrent use, concurrent calls could race and cause unpredictable backoff behavior. Consider copying the slice per request (e.g., append([]time.Duration(nil), c.retryBackoff...)) before mutating.
gpt-review-bot commented 2026-05-13 12:13:34 +00:00
Review
Copy Link
Copy Source

[MAJOR] Potential data race: backoff references c.retryBackoff and is mutated in-place later (e.g., backoff[attempt] = delay at line ~194). Since Client is documented as safe for concurrent use, concurrent doRequest calls with a non-nil c.retryBackoff will race and interfere with each other's state. Make a per-request copy of the slice (e.g., backoff = append([]time.Duration(nil), c.retryBackoff...)) or avoid mutating the slice by tracking next delay in a local variable.

**[MAJOR]** Potential data race: backoff references c.retryBackoff and is mutated in-place later (e.g., backoff[attempt] = delay at line ~194). Since Client is documented as safe for concurrent use, concurrent doRequest calls with a non-nil c.retryBackoff will race and interfere with each other's state. Make a per-request copy of the slice (e.g., backoff = append([]time.Duration(nil), c.retryBackoff...)) or avoid mutating the slice by tracking next delay in a local variable.
12
 @@ -192,6 +192,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay)
sonnet-review-bot commented 2026-05-13 13:09:51 +00:00
Review
Copy Link
Copy Source

[MINOR] Timer leak on zero delay path: when delay == 0, the code skips the timer entirely (correct), but when delay > 0 and the timer fires normally (the case <-timer.C branch), timer.Stop() is never called. The Go docs recommend calling Stop and draining the channel if Stop returns false to prevent goroutine leaks. The idiomatic pattern is defer timer.Stop() immediately after time.NewTimer(delay). The current code only calls timer.Stop() on the context-cancellation path.

**[MINOR]** Timer leak on zero delay path: when `delay == 0`, the code skips the timer entirely (correct), but when delay > 0 and the timer fires normally (the `case <-timer.C` branch), `timer.Stop()` is never called. The Go docs recommend calling Stop and draining the channel if Stop returns false to prevent goroutine leaks. The idiomatic pattern is `defer timer.Stop()` immediately after `time.NewTimer(delay)`. The current code only calls `timer.Stop()` on the context-cancellation path.
select {
case <-timer.C:
timer.Stop()
case <-ctx.Done():
sonnet-review-bot commented 2026-05-13 12:55:17 +00:00
Review
Copy Link
Copy Source

[MINOR] When delay == 0 (e.g., HTTP-date in the past, or Retry-After: 0), the timer is never created and the goroutine proceeds without sleeping — correct. However, when context is already cancelled and delay is 0, the cancelled context won't be checked before the next http.NewRequestWithContext call. This is fine because http.NewRequestWithContext will itself fail fast on a cancelled context, but it does mean a 429 response with a past HTTP-date on a cancelled context won't return ctx.Err() cleanly from the sleep select — it will instead return an HTTP transport error wrapped with 'do request'. The TestDoRequest_ContextCanceled test sets a non-zero backoff which masks this. Consider adding a select { case <-ctx.Done(): return nil, ctx.Err(); default: } guard at the top of each loop iteration, or simply accept the current behavior as the wrapped error is still propagated.

**[MINOR]** When `delay == 0` (e.g., HTTP-date in the past, or Retry-After: 0), the timer is never created and the goroutine proceeds without sleeping — correct. However, when context is already cancelled and delay is 0, the cancelled context won't be checked before the next `http.NewRequestWithContext` call. This is fine because `http.NewRequestWithContext` will itself fail fast on a cancelled context, but it does mean a 429 response with a past HTTP-date on a cancelled context won't return `ctx.Err()` cleanly from the sleep select — it will instead return an HTTP transport error wrapped with 'do request'. The `TestDoRequest_ContextCanceled` test sets a non-zero backoff which masks this. Consider adding a `select { case <-ctx.Done(): return nil, ctx.Err(); default: }` guard at the top of each loop iteration, or simply accept the current behavior as the wrapped error is still propagated.
timer.Stop()
sonnet-review-bot commented 2026-05-13 12:12:04 +00:00
Review
Copy Link
Copy Source

[MINOR] The doRequest method mutates the backoff slice in-place (backoff[attempt] = delay) when a Retry-After header is received. When c.retryBackoff is non-nil, backoff is a direct reference to it (not a copy), so this mutation modifies the Client's shared retryBackoff slice. The doc comment says the Client is safe for concurrent use, but concurrent requests could race on this write. Even for sequential use, the backoff values persist across calls. Either copy the backoff slice at the top of doRequest, or use a local variable for the delay rather than mutating the slice.

**[MINOR]** The `doRequest` method mutates the `backoff` slice in-place (`backoff[attempt] = delay`) when a Retry-After header is received. When `c.retryBackoff` is non-nil, `backoff` is a direct reference to it (not a copy), so this mutation modifies the Client's shared `retryBackoff` slice. The doc comment says the Client is safe for concurrent use, but concurrent requests could race on this write. Even for sequential use, the backoff values persist across calls. Either copy the backoff slice at the top of `doRequest`, or use a local variable for the delay rather than mutating the slice.
gpt-review-bot commented 2026-05-13 12:56:56 +00:00
Review
Copy Link
Copy Source

[MINOR] Authorization header uses the "Bearer" scheme. GitHub REST commonly accepts both "token" and "Bearer" depending on token type, but to maximize compatibility with PATs consider using "token " or making the scheme configurable.

**[MINOR]** Authorization header uses the "Bearer" scheme. GitHub REST commonly accepts both "token" and "Bearer" depending on token type, but to maximize compatibility with PATs consider using "token " or making the scheme configurable.
return nil, ctx.Err()
@@ -210,7 +211,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
req.Header.Set("Accept", "application/vnd.github+json")
}
resp, err := c.http.Do(req)
resp, err := c.httpClient.Do(req)
if err != nil {
sonnet-review-bot commented 2026-05-13 12:55:17 +00:00
Review
Copy Link
Copy Source

[MINOR] The Retry-After header override only applies when attempt < len(backoff). With maxRetryAttempts = 3 and the default backoff slice of length 2, this works for attempts 0 and 1 (covering both possible retries). But if SetRetryBackoff is called with an empty slice []time.Duration{}, the Retry-After delay is computed but never applied, silently falling back to a zero delay. The comment in the code says the delay is set via backoff[attempt] = delay, but if backoff is empty this branch is skipped. This is a subtle edge case that could confuse callers of SetRetryBackoff.

**[MINOR]** The Retry-After header override only applies when `attempt < len(backoff)`. With `maxRetryAttempts = 3` and the default backoff slice of length 2, this works for attempts 0 and 1 (covering both possible retries). But if `SetRetryBackoff` is called with an empty slice `[]time.Duration{}`, the Retry-After delay is computed but never applied, silently falling back to a zero delay. The comment in the code says the delay is set via `backoff[attempt] = delay`, but if backoff is empty this branch is skipped. This is a subtle edge case that could confuse callers of `SetRetryBackoff`.
sonnet-review-bot commented 2026-05-13 13:09:51 +00:00
Review
Copy Link
Copy Source

[MINOR] The Retry-After override only applies when attempt < len(backoff), which means on the last retryable attempt (attempt == maxRetryAttempts-2 == 1, with default backoff len 2) the cap and assignment do apply. However if the server returns 429 on attempt 1 (second attempt) and len(backoff) == 2, then attempt(1) < len(backoff)(2) is true and backoff[1] is set — but attempt 1 is the last attempt that will retry (attempt 2 would be maxRetryAttempts-1 == 2 which is NOT < maxRetryAttempts-1), so the stored backoff[1] is never consumed. This is a logic gap: the Retry-After value is parsed and stored but the delay is never applied on the final retry cycle. This is a subtle correctness issue but low impact since it only means the cap is computed unnecessarily on the last attempt.

**[MINOR]** The Retry-After override only applies when `attempt < len(backoff)`, which means on the last retryable attempt (attempt == maxRetryAttempts-2 == 1, with default backoff len 2) the cap and assignment do apply. However if the server returns 429 on attempt 1 (second attempt) and `len(backoff) == 2`, then `attempt(1) < len(backoff)(2)` is true and backoff[1] is set — but attempt 1 is the last attempt that will retry (attempt 2 would be maxRetryAttempts-1 == 2 which is NOT < maxRetryAttempts-1), so the stored backoff[1] is never consumed. This is a logic gap: the Retry-After value is parsed and stored but the delay is never applied on the final retry cycle. This is a subtle correctness issue but low impact since it only means the cap is computed unnecessarily on the last attempt.
return nil, fmt.Errorf("do request: %w", err)
}
1