fix: respect context cancellation in RetryTransport - #3383
Conversation
- 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
|
👋 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 |
|
Please rebase this branch |
There was a problem hiding this comment.
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.
|
|
||
| time.Sleep(t.retryDelay) | ||
| if retry < t.maxRetries { | ||
| sleep(req.Context(), t.retryDelay) |
| httpClient := http.DefaultClient | ||
| httpClient.Transport = NewRetryTransport(http.DefaultTransport, | ||
| WithMaxRetries(1), | ||
| WithRetryDelay(10*time.Second), | ||
| ) |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
@yoshi-taka please could you look at the Copilot review comments.
time.Sleepwith context-awaresleep()inRetryTransport.RoundTripretry < maxRetries)RateLimitTransportalready uses the existingsleep(ctx, dur)helper (transport.go:122), which respects context cancellation.RetryTransportwas using baretime.Sleep, causing it to ignorectx.Done()during retry delays.This aligns
RetryTransportwith the existing pattern in the codebase.TestRetryTransport_cancelled: verifies context cancellation interrupts retry delayTestRetryTransport_no_sleep_after_last_retry: verifies no delay after final attemptResolves #ISSUE_NUMBER
Before the change?
RetryTransportusedtime.Sleepbetween retry attempts.RetryTransportalso slept after the final retryable attempt, even when no further retry would be made.After the change?
RetryTransportnow uses the existing context-awaresleep(ctx, dur)helper.RetryTransportno longer sleeps after the final retryable attempt.Pull request checklist
Does this introduce a breaking change?