feat(github): support HTTP-date format in Retry-After header #110
@@ -89,6 +89,10 @@ func asAPIError(err error) (*APIError, bool) {
|
||||
// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
|
||||
// be called before any goroutines issue requests; they have no synchronization.
|
||||
type Client struct {
|
||||
// TODO: baseURL is populated by NewClient but not yet consumed by doRequest/doGet.
|
||||
|
|
||||
// Higher-level exported methods (GetPullRequest, etc.) will use it to
|
||||
sonnet-review-bot
commented
[NIT] Struct field alignment: **[NIT]** Struct field alignment: `httpClient *http.Client` is not aligned with the other fields (`baseURL` and `token`). `gofmt` will handle tab alignment within struct definitions, but the blank line between `token` and `httpClient` creates a visual group separation that might be intentional (separating identity fields from transport config). Either way this is fine.
|
||||
// construct request URLs; remove this field if those methods end up
|
||||
// accepting full URLs instead.
|
||||
baseURL string
|
||||
token string
|
||||
http *http.Client
|
||||
gpt-review-bot
commented
[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.
|
||||
@@ -126,6 +130,11 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
|
||||
|
||||
// SetRetryBackoff sets the delays between retry attempts.
|
||||
|
security-review-bot marked this conversation as resolved
[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
[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.
|
||||
// This is intended for testing to speed up retry tests.
|
||||
//
|
||||
// Note: if an empty non-nil slice is provided, Retry-After delays parsed from
|
||||
// server responses will be computed and capped but not applied (because
|
||||
// attempt < len(backoff) is always false). This is acceptable for the
|
||||
|
[MINOR] doRequest accepts an arbitrary reqURL and unconditionally applies the Authorization header (line 167). If future internal callers inadvertently pass non-GitHub or user-influenced URLs (e.g., pre-signed S3 links), this could lead to SSRF and/or token leakage to third-party hosts. Consider constraining requests to c.baseURL or verifying the host before attaching Authorization. **[MINOR]** doRequest accepts an arbitrary reqURL and unconditionally applies the Authorization header (line 167). If future internal callers inadvertently pass non-GitHub or user-influenced URLs (e.g., pre-signed S3 links), this could lead to SSRF and/or token leakage to third-party hosts. Consider constraining requests to c.baseURL or verifying the host before attaching Authorization.
|
||||
// test-only use case but callers should be aware of this edge case.
|
||||
func (c *Client) SetRetryBackoff(backoff []time.Duration) {
|
||||
c.retryBackoff = backoff
|
||||
}
|
||||
|
||||
@@ -48,7 +48,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
w.Header().Set("Retry-After", "1")
|
||||
w.Header().Set("Retry-After", "0")
|
||||
w.WriteHeader(http.StatusTooManyRequests)
|
||||
w.Write([]byte("rate limited"))
|
||||
return
|
||||
@@ -242,18 +242,36 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestDoRequest_ContextCanceled(t *testing.T) {
|
||||
// This test exercises the timer-cancel path in the retry select:
|
||||
// select { case <-timer.C; case <-ctx.Done() }
|
||||
// The server returns 429 with a long Retry-After, and we cancel the
|
||||
// context shortly after the first response so that cancellation races
|
||||
// against the timer rather than preventing the initial HTTP round-trip.
|
||||
requestReceived := make(chan struct{}, 1)
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
select {
|
||||
case requestReceived <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
w.Header().Set("Retry-After", "10")
|
||||
w.WriteHeader(http.StatusTooManyRequests)
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("tok", srv.URL)
|
||||
c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
|
||||
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
// Cancel immediately so the retry timer is interrupted.
|
||||
cancel()
|
||||
defer cancel()
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `TestDoRequest_ContextCanceled` test uses `time.Sleep(50 * time.Millisecond)` to create a timing window, which is a potential source of flakiness on very slow CI machines. A more robust approach would use a channel to signal that the select is about to be entered. However, since the retry backoff is 10 seconds and the sleep is 50ms, the window is generous enough to be reliable in practice.
|
||||
|
||||
// Cancel the context after the first request completes, while the
|
||||
// client is blocked in the retry timer select.
|
||||
go func() {
|
||||
<-requestReceived
|
||||
// Small delay to ensure we're inside the timer select.
|
||||
time.Sleep(50 * time.Millisecond)
|
||||
cancel()
|
||||
}()
|
||||
|
||||
_, err := c.doGet(ctx, srv.URL+"/test")
|
||||
if err == nil {
|
||||
|
||||
[MINOR] The baseURL field is currently not used by doRequest/doGet. This may be confusing to callers; consider integrating baseURL into request construction or removing it until higher-level methods use it.