Skip to content

fix: Correct retry logic - #3703

Open
stevehipwell wants to merge 1 commit into
integrations:mainfrom
stevehipwell:fix-retry-transport
Open

stevehipwell wants to merge 1 commit into
integrations:mainfrom
stevehipwell:fix-retry-transport

Conversation

@stevehipwell

Copy link
Copy Markdown
Collaborator

Resolves #ISSUE_NUMBER


Before the change?

  • New client retry logic was incorrect and mutated the request

After the change?

  • New client retry correctly clones the request
  • New client retry correctly resets the body

Pull request checklist

  • Schema migrations have been created if needed (example)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@stevehipwell stevehipwell added this to the v6.14.0 milestone Oct 26, 2026
@stevehipwell
stevehipwell requested a balanced review from Copilot October 26, 2026 21:20
@stevehipwell stevehipwell self-assigned this Oct 26, 2026
@stevehipwell stevehipwell added the Type: Bug Something isn't working as documented label Oct 26, 2026
@github-actions

Copy link
Copy Markdown

👋 Hi, and thank you for this contribution!

This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can.

You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions.


🤖 This is an automated message.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Retry preparation can interfere with the previous attempt’s request, and cancellation cleanup needs regression coverage.

2 open issues
What changed in this PR

These provider review instructions are being used.

This PR corrects request handling in the provider’s internal GitHub HTTP client retry transport.

Changes:

  • Copies requests and restores request bodies before retrying.
  • Cleans up retained responses when retries end with an error.
  • Adds body-rewind tests and checks request payloads and attempt counts.
File Description
internal/​ghclient/​retry.go Updates request copying, body replay, and response cleanup.
internal/​ghclient/​retry_test.go Verifies retry counts and request payload preservation.
internal/​ghclient/​body.go Adds a helper to restore request bodies using GetBody.
internal/​ghclient/​body_test.go Tests body restoration and invalid request handling.

Review effort: Balanced

Comment thread internal/ghclient/retry.go Resolved
Comment thread internal/ghclient/retry.go Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Retry attempts lose generated body-replay metadata, and the early-cancellation test introduces a scheduling race.

2 open issues
2 resolved since last review

Review effort: Balanced

Comment thread internal/ghclient/retry.go Resolved
Comment thread internal/ghclient/retry_test.go Resolved
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Caller-request preservation needs regression assertions to satisfy the provider's behavior-change coverage requirements.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread internal/ghclient/rewind.go Resolved
@stevehipwell stevehipwell added the needs-github-review Request a review from GitHub label Oct 27, 2026
@team-review-router
team-review-router Bot requested a review from a team October 27, 2026 17:43
@stevehipwell
stevehipwell requested a review from deiga October 27, 2026 17:43

@robert-crandall robert-crandall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up! Splitting rewindableTransport out from retryTransport makes the transport stack much easier to follow. I left a couple of inline notes on transport.go and one small test comment in retry_test.go.

@@ -87,6 +79,12 @@ func newTransport(tokenSource oauth2.TokenSource, opts ClientOptions) (http.Roun

tr = newRateLimitTransport(tr)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because newRateLimitTransport sits outside retryTransport, secondary rate-limit retries (403/429 with Retry-After) loop inside go-github-ratelimit without going back through retryTransport's if i 0 rewind. Over HTTP/2 (ForceAttemptHTTP2 = true), the second attempt sends an empty req.Body (request declared a Content-Length of ... but only wrote 0 bytes).

Here is a quick test sketch that reproduces it:

srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	body, _ := io.ReadAll(r.Body) // attempt 1: `{"query":"mutation"}`, attempt 2: ""
	if attempt.Add(1) == 1 {
		w.Header().Set("Retry-After", "1")
		w.WriteHeader(http.StatusForbidden)
		w.Write([]byte(`{"message": "You have exceeded a secondary rate limit"}`))
		return
	}
	if len(body) == 0 {
		w.WriteHeader(http.StatusBadRequest)
		return
	}
	w.WriteHeader(http.StatusOK)
}))
srv.EnableHTTP2 = true
srv.StartTLS()

tr := newRateLimitTransport(srv.Client().Transport)
tr, _ = newRewindableTransport(tr, 0)

req, _ := http.NewRequest(http.MethodPost, srv.URL, bytes.NewBufferString(`{"query":"mutation"}`))
resp, _ := tr.RoundTrip(req) // resp.StatusCode == 400 because attempt 2 had an empty body

Can you rewind req.Body before every attempt that enters newRateLimitTransport's inner transport when req.GetBody != nil?

tr = ctr
}

tr = logging.NewLoggingHTTPTransport(tr)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving logging.NewLoggingHTTPTransport to the outermost layer means httputil.DumpRequestOut(req, true) runs before rewindableTransport makes its shallow copy (reqCopy := *req) or throttleTransport acquires a concurrency slot. When TF_LOG is enabled, DumpRequestOut drains and replaces the caller's req.Body in place, and it also hides individual retry/rate-limit attempts from debug logs.

Here is a quick test sketch showing the caller mutation:

t.Setenv("TF_LOG", "DEBUG")

tr, _ := newRewindableTransport(srv.Client().Transport, 0)
tr = logging.NewLoggingHTTPTransport(tr)

origBody := io.NopCloser(strings.NewReader("hello"))
req, _ := http.NewRequest(http.MethodPost, srv.URL, origBody)
_, _ = tr.RoundTrip(req)
// req.Body != origBody (mutated in place by httputil.DumpRequestOut before rewindableTransport's reqCopy)

Can you keep logging.NewLoggingHTTPTransport inside retryTransport (around the base/cache/OAuth transport, as before) so rewindableTransport protects the caller's req first and debug logs still show each wire attempt?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-github-review Request a review from GitHub Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants