From 808929f9533221611bad1a46218c2e42875fb59d Mon Sep 17 00:00:00 2001 From: AdamMagued Date: Wed, 21 Oct 2026 22:17:59 +0300 Subject: [PATCH 1/3] github: restore request body on primary rate limit retry When retrying a request after primary rate limit reset, clone does not recreate or rewind the consumed body stream. Restore newReq.Body using req.GetBody() when present, and skip retry if the body cannot be replayed. Fixes #4656 --- github/github.go | 9 +++ github/github_test.go | 179 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+) diff --git a/github/github.go b/github/github.go index a5d90e225da..1b4b369fd71 100644 --- a/github/github.go +++ b/github/github.go @@ -1526,6 +1526,15 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro } // retry the request now the rate limit should have been reset newReq := req.Clone(context.WithValue(ctx, SleepUntilPrimaryRateLimitResetWhenRateLimited, nil)) + if req.GetBody != nil { + body, err := req.GetBody() + if err != nil { + return response, err + } + newReq.Body = body + } else if req.Body != nil && req.Body != http.NoBody { + return response, err + } return c.bareDo(caller, newReq) } diff --git a/github/github_test.go b/github/github_test.go index c927a46f0cd..34ee60cc686 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -21,6 +21,7 @@ import ( "reflect" "strconv" "strings" + "sync" "sync/atomic" "testing" "testing/synctest" @@ -3073,6 +3074,184 @@ func TestDo_rateLimit_sleepUntilResponseResetLimitRetryOnce(t *testing.T) { } } +// Verify request body is replayed when retrying after rate limit reset. +func TestDo_rateLimit_sleepUntilResponseResetLimit_replaysBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + reset := time.Now().UTC().Add(time.Second) + + type testPayload struct { + Name *string `json:"name"` + } + + var requestCount atomic.Int32 + var bodies []string + var mu sync.Mutex + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + requestCount.Add(1) + b, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("failed to read body: %v", err) + } + mu.Lock() + bodies = append(bodies, string(b)) + mu.Unlock() + + if requestCount.Load() == 1 { + w.Header().Set(HeaderRateLimit, "60") + w.Header().Set(HeaderRateRemaining, "0") + w.Header().Set(HeaderRateUsed, "60") + w.Header().Set(HeaderRateReset, fmt.Sprint(reset.Unix())) + w.Header().Set(HeaderRateResource, "core") + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusForbidden) + fmt.Fprintln(w, `{ + "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)", + "documentation_url": "https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits" +}`) + return + } + + w.Header().Set(HeaderRateLimit, "5000") + w.Header().Set(HeaderRateRemaining, "5000") + w.Header().Set(HeaderRateUsed, "0") + w.Header().Set(HeaderRateReset, fmt.Sprint(reset.Add(time.Hour).Unix())) + w.Header().Set(HeaderRateResource, "core") + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusOK) + fmt.Fprintln(w, `{}`) + }) + + input := &testPayload{Name: Ptr("test-name")} + req, err := client.NewRequest(context.WithValue(t.Context(), SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "POST", ".", input) + if err != nil { + t.Fatalf("NewRequest returned error: %v", err) + } + + resp, err := client.Do(req, nil) + if err != nil { + t.Fatalf("Do returned unexpected error: %v", err) + } + if got, want := resp.StatusCode, http.StatusOK; got != want { + t.Errorf("Response status code = %v, want %v", got, want) + } + if got, want := int(requestCount.Load()), 2; got != want { + t.Fatalf("Expected 2 requests, got %v", got) + } + + mu.Lock() + defer mu.Unlock() + wantBody := `{"name":"test-name"}` + "\n" + if len(bodies) != 2 { + t.Fatalf("Expected 2 captured bodies, got %d", len(bodies)) + } + if bodies[0] != wantBody { + t.Errorf("First request body = %q, want %q", bodies[0], wantBody) + } + if bodies[1] != wantBody { + t.Errorf("Second request body (retried) = %q, want %q", bodies[1], wantBody) + } +} + +// Verify request with non-replayable body returns rate limit error without retrying. +func TestDo_rateLimit_sleepUntilResponseResetLimit_nonReplayableBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + reset := time.Now().UTC().Add(time.Second) + + var requestCount atomic.Int32 + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + requestCount.Add(1) + w.Header().Set(HeaderRateLimit, "60") + w.Header().Set(HeaderRateRemaining, "0") + w.Header().Set(HeaderRateUsed, "60") + w.Header().Set(HeaderRateReset, fmt.Sprint(reset.Unix())) + w.Header().Set(HeaderRateResource, "core") + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusForbidden) + fmt.Fprintln(w, `{ + "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)", + "documentation_url": "https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits" +}`) + }) + + type testPayload struct { + Name *string `json:"name"` + } + input := &testPayload{Name: Ptr("test-name")} + req, err := client.NewRequest(context.WithValue(t.Context(), SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "POST", ".", input) + if err != nil { + t.Fatalf("NewRequest returned error: %v", err) + } + // Explicitly clear GetBody to simulate a body stream that cannot be replayed. + req.GetBody = nil + + resp, err := client.Do(req, nil) + if err == nil { + t.Fatal("Expected error to be returned, got nil") + } + var rateLimitError *RateLimitError + if !errors.As(err, &rateLimitError) { + t.Fatalf("Expected *RateLimitError, got %#v", err) + } + if got, want := int(requestCount.Load()), 1; got != want { + t.Errorf("Expected 1 request, got %v", got) + } + if resp == nil { + t.Error("Expected non-nil Response") + } else if got, want := resp.StatusCode, http.StatusForbidden; got != want { + t.Errorf("Response status code = %v, want %v", resp.StatusCode, http.StatusForbidden) + } +} + +// Verify GetBody error aborts retry and is returned directly. +func TestDo_rateLimit_sleepUntilResponseResetLimit_getBodyError(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + reset := time.Now().UTC().Add(time.Second) + + var requestCount atomic.Int32 + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + requestCount.Add(1) + w.Header().Set(HeaderRateLimit, "60") + w.Header().Set(HeaderRateRemaining, "0") + w.Header().Set(HeaderRateUsed, "60") + w.Header().Set(HeaderRateReset, fmt.Sprint(reset.Unix())) + w.Header().Set(HeaderRateResource, "core") + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusForbidden) + fmt.Fprintln(w, `{ + "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)", + "documentation_url": "https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits" +}`) + }) + + type testPayload struct { + Name *string `json:"name"` + } + input := &testPayload{Name: Ptr("test-name")} + req, err := client.NewRequest(context.WithValue(t.Context(), SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "POST", ".", input) + if err != nil { + t.Fatalf("NewRequest returned error: %v", err) + } + expectedErr := errors.New("cannot recreate body") + req.GetBody = func() (io.ReadCloser, error) { + return nil, expectedErr + } + + _, err = client.Do(req, nil) + if !errors.Is(err, expectedErr) { + t.Fatalf("Expected getBody error, got: %v", err) + } + if got, want := int(requestCount.Load()), 1; got != want { + t.Errorf("Expected 1 request, got %v", got) + } +} + // Ensure a network call is not made when it's known that API rate limit is still exceeded. func TestDo_rateLimit_sleepUntilClientResetLimit(t *testing.T) { t.Parallel() From d5dbe3d970b55d2a24a6d09bb7852e141ba1c3ef Mon Sep 17 00:00:00 2001 From: AdamMagued Date: Thu, 22 Oct 2026 16:58:23 +0300 Subject: [PATCH 2/3] test(github): isolate rate limit body replay with fresh connections and fix modernize lint --- github/github_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/github/github_test.go b/github/github_test.go index 34ee60cc686..9aafacda755 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -3078,6 +3078,10 @@ func TestDo_rateLimit_sleepUntilResponseResetLimitRetryOnce(t *testing.T) { func TestDo_rateLimit_sleepUntilResponseResetLimit_replaysBody(t *testing.T) { t.Parallel() client, mux, _ := setup(t) + // Disable keep-alives so the retried request opens a fresh connection rather + // than reusing the transport connection pool, verifying that the retried POST + // sends the replayed payload reconstituted by bareDo. + client.client.Transport.(*http.Transport).DisableKeepAlives = true reset := time.Now().UTC().Add(time.Second) @@ -3159,6 +3163,8 @@ func TestDo_rateLimit_sleepUntilResponseResetLimit_replaysBody(t *testing.T) { func TestDo_rateLimit_sleepUntilResponseResetLimit_nonReplayableBody(t *testing.T) { t.Parallel() client, mux, _ := setup(t) + // Disable keep-alives so requests do not reuse connections. + client.client.Transport.(*http.Transport).DisableKeepAlives = true reset := time.Now().UTC().Add(time.Second) @@ -3193,8 +3199,7 @@ func TestDo_rateLimit_sleepUntilResponseResetLimit_nonReplayableBody(t *testing. if err == nil { t.Fatal("Expected error to be returned, got nil") } - var rateLimitError *RateLimitError - if !errors.As(err, &rateLimitError) { + if _, ok := errors.AsType[*RateLimitError](err); !ok { t.Fatalf("Expected *RateLimitError, got %#v", err) } if got, want := int(requestCount.Load()), 1; got != want { @@ -3211,6 +3216,8 @@ func TestDo_rateLimit_sleepUntilResponseResetLimit_nonReplayableBody(t *testing. func TestDo_rateLimit_sleepUntilResponseResetLimit_getBodyError(t *testing.T) { t.Parallel() client, mux, _ := setup(t) + // Disable keep-alives so requests do not reuse connections. + client.client.Transport.(*http.Transport).DisableKeepAlives = true reset := time.Now().UTC().Add(time.Second) From 034e6b698b07d30af2f9830cbabf5e0b4a6ed883 Mon Sep 17 00:00:00 2001 From: AdamMagued Date: Sat, 24 Oct 2026 00:53:00 +0300 Subject: [PATCH 3/3] fix: validate body replayability before rate limit sleep --- github/github.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/github/github.go b/github/github.go index 1b4b369fd71..f37199b1c41 100644 --- a/github/github.go +++ b/github/github.go @@ -1518,12 +1518,16 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro err = aerr } - var rateLimitError *RateLimitError - if errors.As(err, &rateLimitError) && + if rlErr, ok := errors.AsType[*RateLimitError](err); ok && ctx.Value(SleepUntilPrimaryRateLimitResetWhenRateLimited) != nil { - if err := sleepUntilResetWithBuffer(ctx, rateLimitError.Rate.Reset.Time); err != nil { + if req.Body != nil && req.Body != http.NoBody && req.GetBody == nil { return response, err } + + if err := sleepUntilResetWithBuffer(ctx, rlErr.Rate.Reset.Time); err != nil { + return response, err + } + // retry the request now the rate limit should have been reset newReq := req.Clone(context.WithValue(ctx, SleepUntilPrimaryRateLimitResetWhenRateLimited, nil)) if req.GetBody != nil { @@ -1532,8 +1536,6 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro return response, err } newReq.Body = body - } else if req.Body != nil && req.Body != http.NoBody { - return response, err } return c.bareDo(caller, newReq) }