fix: respect context cancellation in RetryTransport by yoshi-taka · Pull Request #3383 · integrations/terraform-provider-github

yoshi-taka · 2026-05-01T06:28:12Z

Replace time.Sleep with context-aware sleep() in RetryTransport.RoundTrip
Skip the delay after the final retry attempt (retry < maxRetries)

RateLimitTransport already uses the existing sleep(ctx, dur) helper (transport.go:122), which respects context cancellation. RetryTransport was using bare time.Sleep, causing it to ignore ctx.Done() during retry delays.

This aligns RetryTransport with the existing pattern in the codebase.

Add TestRetryTransport_cancelled: verifies context cancellation interrupts retry delay
Add TestRetryTransport_no_sleep_after_last_retry: verifies no delay after final attempt

Resolves #ISSUE_NUMBER

Before the change?

RetryTransport used time.Sleep between retry attempts.
During retry delays, request context cancellation was not respected.
RetryTransport also slept after the final retryable attempt, even when no further retry would be made.

After the change?

RetryTransport now uses the existing context-aware sleep(ctx, dur) helper.
Retry delays can now be interrupted when the request context is cancelled.
RetryTransport no longer sleeps after the final retryable attempt.

Pull request checklist

Schema migrations have been created if needed (not needed)
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?

Yes
No

github-actions · 2026-05-01T06:28:27Z

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

deiga · 2026-08-09T17:55:00Z

Please rebase this branch ☺️

Copilot

Pull request overview

These provider review instructions are being used. This PR makes retry delays context-aware and avoids sleeping after the final attempt.

Changes:

Uses context-aware retry sleeping.
Adds cancellation and final-attempt timing tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`github/transport.go`	Updates retry-delay behavior.
`github/transport_test.go`	Tests cancellation and final retry timing.

Suppressed comments (1)

github/transport_test.go:537

This aliases and mutates the process-wide http.DefaultClient, leaving the retry transport installed after the test and making later tests depend on execution order. Use a dedicated client for this test.

	httpClient := http.DefaultClient
	httpClient.Transport = NewRetryTransport(http.DefaultTransport,
		WithMaxRetries(1),
		WithRetryDelay(10*time.Second),
	)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	httpClient := http.DefaultClient
+	httpClient.Transport = NewRetryTransport(http.DefaultTransport,
+		WithMaxRetries(1),
+		WithRetryDelay(10*time.Second),
+	)


deiga

LGTM

stevehipwell

@yoshi-taka please could you look at the Copilot review comments.

yoshi-taka · 2026-09-06T12:29:36Z

Addressed the Copilot review comments in 280d6c6:

Stop retrying immediately when the request context is cancelled.
Use test-local HTTP clients instead of mutating http.DefaultClient.
Fix the final-attempt test so it only verifies that no sleep occurs after the last attempt.

The RetryTransport and related transport tests pass. Could you please take another look?

Copilot

🟡 Changes recommended

Intermediate retry response bodies can leak connections and resources.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Balanced

stevehipwell

@yoshi-taka please address the Copilot review comments. If you want a reference for how this should work you can check out the new client implementation of the logic which uses the hashicorp/retriable-http package.

yoshi-taka · 2026-10-25T09:34:25Z

I've rebased the branch onto the latest main (1b9c5c13) and added a commit that addresses the Copilot review comment about intermediate retry response bodies.

What changed

Before retrying, RetryTransport now drains and closes the discarded response body so its connection and resources are not leaked. The drain is capped at 4KB, mirroring drainResponseBody in internal/ghclient, so small GitHub error bodies allow the connection to be reused while oversized bodies are still closed safely.
A response returned together with a transport error is also drained and discarded, since it can never reach the caller.
The final response (either non-retryable or after the retries are exhausted) is still returned untouched so the caller can read it.
This affects the legacy transport in github/transport.go, which is still the default (legacy_client = true). The new client already handles this via internal/ghclient/retry.go.

Tests

TestRetryTransport_closes_intermediate_response_bodies: intermediate responses are closed, the final one is not.
TestRetryTransport_closes_response_returned_with_error: a response returned alongside an error is closed.

Verification

make test (all unit tests) passes
go vet, gofumpt -extra, and goimports are clean
The rebase was conflict-free; the branch is now 3 commits on top of main

Copilot

🟡 Changes recommended

The initial cancellation path leaves request bodies open, potentially leaking file or stream resources.

2 open issues

2 resolved since last review

Close the response body before waiting for another attempt. Each retryable response that is…
Cancellation only wakes sleep; the loop then invokes the wrapped transport again with the…

_{Review effort: Balanced}

Use context-aware retry delays and check cancellation before each attempt. Do not sleep after the final attempt, and close unsent request bodies when cancellation prevents the initial attempt. Drain and close discarded responses before retrying, with a 4KB bound matching the new client implementation. Preserve the final response body for the caller, and discard responses returned alongside transport errors. Add deterministic virtual-time tests for cancellation and retry delays, plus coverage for request body ownership, exhausted responses, bounded draining, and read errors.

Copilot

🟡 Changes recommended

Synchronous response draining can block subsequent retries indefinitely when a response body stalls and no request deadline is set.

2 open issues

Bound retryable response body draining to prevent indefinite blocking · New
This aliases and mutates the process-wide http.DefaultClient, so the 10-second retry transport…

1 resolved since last review

Close request body before returning on cancelled request

_{Review effort: Balanced}

+	if resp == nil || resp.Body == nil {
+		return
+	}
+	_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4*1024))


yoshi-taka · 2026-10-28T14:40:14Z

The 4KB limit bounds bytes read, not the time spent draining, so the stalled-body concern is valid.

This helper follows the existing drainResponseBody implementation in internal/ghclient/body.go, which has the same behavior. Would you prefer addressing this consistently in both clients as a separate change, or changing the legacy transport in this PR to close discarded bodies without draining them?

stevehipwell · 2026-10-28T18:43:18Z

@yoshi-taka there is new PR (#3703) to improve the retry logic in the new client. The drain logic is fine for now, but will no longer be required when we bump to Go 1.27.

github-actions Bot added the Type: Bug Something isn't working as documented label May 1, 2026

deiga requested a balanced review from Copilot August 12, 2026 18:28

Copilot started reviewing on behalf of deiga August 12, 2026 18:28

View session

Copilot AI reviewed Aug 12, 2026

View reviewed changes

Comment thread github/transport.go Resolved

Comment thread github/transport_test.go Outdated

Comment on lines +495 to +499

httpClient := http.DefaultClient

httpClient.Transport = NewRetryTransport(http.DefaultTransport,

WithMaxRetries(1),

WithRetryDelay(10*time.Second),

)

deiga requested a review from stevehipwell August 13, 2026 12:26

deiga previously approved these changes Aug 13, 2026

View reviewed changes

stevehipwell requested changes Aug 13, 2026

View reviewed changes

yoshi-taka dismissed deiga’s stale review via 280d6c6 September 6, 2026 12:28

deiga requested a balanced review from Copilot September 6, 2026 15:47

Copilot started reviewing on behalf of deiga September 6, 2026 15:48

View session

Copilot AI reviewed Sep 6, 2026

View reviewed changes

Comment thread github/transport.go Resolved

stevehipwell requested changes Sep 7, 2026

View reviewed changes

deiga added the Awaiting response label Oct 13, 2026

yoshi-taka force-pushed the fix/retry-transport-ctx-cancellation branch from f3d177f to bb9dd12 Compare October 25, 2026 09:27

stevehipwell requested a balanced review from Copilot October 26, 2026 09:51

Copilot started reviewing on behalf of stevehipwell October 26, 2026 09:52

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

Comment thread github/transport.go Resolved

yoshi-taka mentioned this pull request Oct 26, 2026

Review sandbox: RetryTransport context cancellation and body ownership yoshi-taka/terraform-provider-github#1

Open

yoshi-taka force-pushed the fix/retry-transport-ctx-cancellation branch from bb9dd12 to 6dd7be4 Compare October 26, 2026 15:49

stevehipwell requested a balanced review from Copilot October 26, 2026 17:09

Copilot started reviewing on behalf of stevehipwell October 26, 2026 17:09

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

Comment thread github/transport.go

if resp == nil || resp.Body == nil {

return

}

_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4*1024))

Conversation

yoshi-taka commented May 1, 2026

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions Bot commented May 1, 2026

Uh oh!

deiga commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

yoshi-taka commented Sep 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

yoshi-taka commented Oct 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

yoshi-taka commented Oct 28, 2026

Uh oh!

stevehipwell commented Oct 28, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants