Repository navigation
Review sandbox: RetryTransport context cancellation and body ownership - #1
Open
yoshi-taka wants to merge 4 commits into
Open
yoshi-taka wants to merge 4 commits into
yoshi-taka wants to merge 4 commits into
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
Drain and close responses discarded before a retry so their connections and resources are not leaked, and discard a response returned alongside a transport error which can never reach the caller. The drain is capped at 4KB to match the new client implementation, so small error bodies allow connection reuse while oversized bodies are still closed safely. Adds tests covering body closure for intermediate responses and for responses returned together with an error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fork-only review sandbox for integrations#3383. The base is pinned to upstream main (1b9c5c1), so this PR contains the entire proposed upstream diff, not just the latest fix.
This change makes retry delays cancellable, stops attempts after cancellation, skips the final retry delay, and drains/closes discarded response bodies. It also closes unsent request bodies on initial cancellation without draining them; bodies already handed to the underlying transport remain its responsibility, including asynchronous closure.
Regression tests cover cancellation before sending (nil, NoBody, stream and GetBody), ownership after sending, intermediate response closure and preserving the final response. No process-wide HTTP client is modified.
Validation: full unit suite with -race, repeated cancellation tests, go build ./..., go vet ./..., golangci-lint including forcetypeassert on new code, and formatter checks.
AI-assisted implementation; changes have been inspected and tested locally. This sandbox will not be merged. The upstream PR will be updated only after review findings have been addressed.