Description
HTTP 429 responses are not retried by default in the retry transport, even though TooManyRequestsErrorCode is included in temporaryErrorCodes. The OCI Distribution Spec defines 429 as a standard error response: https://github.com/distribution/distribution/blob/da404778edd3faa665e48ca3bb791b6144f3355e/docs/content/spec/api.md#base
Current Behavior
-
The defaultRetryStatusCodes (408, 500, 502, 503, 504, 499, 522) does not include 429:
|
var defaultRetryStatusCodes = []int{ |
|
http.StatusRequestTimeout, |
|
http.StatusInternalServerError, |
|
http.StatusBadGateway, |
|
http.StatusServiceUnavailable, |
|
http.StatusGatewayTimeout, |
|
499, // nginx-specific, client closed request |
|
522, // Cloudflare-specific, connection timeout |
|
} |
-
TooManyRequestsErrorCode is included in temporaryErrorCodes:
|
var temporaryErrorCodes = map[ErrorCode]struct{}{ |
|
BlobUploadInvalidErrorCode: {}, |
|
TooManyRequestsErrorCode: {}, |
|
UnknownErrorCode: {}, |
|
UnavailableErrorCode: {}, |
|
} |
-
The retry logic in retryTransport.RoundTrip only converts responses to transport.Error if the status code is in t.codes:
|
for _, code := range t.codes { |
|
if out.StatusCode == code { |
|
return retryError(out) |
|
} |
|
} |
Problem
Since 429 is not in defaultRetryStatusCodes, the response is never converted to a transport.Error via retryError(). This means:
- The
Temporary() method is never called
- The check for
TooManyRequestsErrorCode in temporaryErrorCodes is never reached from retryTransport.
- 429 responses are not retried
Question
Is this behavior intentional? It seems reasonable to retry rate-limited requests with appropriate backoff.
If this is intentional (perhaps because rate limiting requires different handling than other temporary errors), should users be expected to explicitly configure retry behavior for rate limiting using WithRetryStatusCodes()?
Please let me know if I'm missing something in my understanding of the retry logic.
Description
HTTP 429 responses are not retried by default in the retry transport, even though
TooManyRequestsErrorCodeis included intemporaryErrorCodes. The OCI Distribution Spec defines 429 as a standard error response: https://github.com/distribution/distribution/blob/da404778edd3faa665e48ca3bb791b6144f3355e/docs/content/spec/api.md#baseCurrent Behavior
The
defaultRetryStatusCodes(408, 500, 502, 503, 504, 499, 522) does not include 429:go-containerregistry/pkg/v1/remote/options.go
Lines 93 to 101 in 59a4b85
TooManyRequestsErrorCodeis included intemporaryErrorCodes:go-containerregistry/pkg/v1/remote/transport/error.go
Lines 140 to 145 in 59a4b85
The retry logic in
retryTransport.RoundTriponly converts responses totransport.Errorif the status code is int.codes:go-containerregistry/pkg/v1/remote/transport/retry.go
Lines 101 to 105 in 59a4b85
Problem
Since 429 is not in
defaultRetryStatusCodes, the response is never converted to atransport.ErrorviaretryError(). This means:Temporary()method is never calledTooManyRequestsErrorCodeintemporaryErrorCodesis never reached from retryTransport.Question
Is this behavior intentional? It seems reasonable to retry rate-limited requests with appropriate backoff.
If this is intentional (perhaps because rate limiting requires different handling than other temporary errors), should users be expected to explicitly configure retry behavior for rate limiting using
WithRetryStatusCodes()?Please let me know if I'm missing something in my understanding of the retry logic.