acknowledge and use aws-retry-after when received#6527
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b43486d3f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let delay = error | ||
| .retry_after() | ||
| .unwrap_or_else(|| retry_params.compute_delay(num_attempts)); |
There was a problem hiding this comment.
When a service or S3-compatible endpoint returns a large x-amz-retry-after, this path sleeps the raw header value and bypasses RetryParams::max_delay entirely; for the standard/aggressive policies a 600000 ms header would stall the request for 10 minutes despite the configured 20 second cap. AWS's server-directed retry behavior clamps the header around the computed backoff, so compute the backoff and clamp the server value instead of replacing it wholesale.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn retry_after(&self) -> Option<Duration> { | ||
| self.0.retry_after() |
There was a problem hiding this comment.
When aws_retry is used with Retry<StorageError> from the S3 put paths, this call resolves to the new default None because the AwsRetryable for Retry<E> impl below does not override retry_after and the upload helpers convert the original SdkError into StorageError before retrying. As a result, throttled PutObject/UploadPart responses that carry x-amz-retry-after still use exponential backoff instead of the server delay, so the new behavior is silently skipped for write uploads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/bf291ba174a5e2c301e85f65ef9893e612c1f639/quickwit-common/src/retry.rs#L176
Fresh evidence in the current diff is that hinted responses still skip compute_delay entirely and sleep hint.min(max_delay). When an S3/S3-compatible service returns x-amz-retry-after on an early retry, a large value such as 600000 ms now becomes a full 20s sleep instead of being bounded around the computed first-retry backoff, while a very small value can bypass the client backoff altogether. Compute the normal backoff first and clamp the server hint against that value before sleeping.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a276d1e37e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| let delay = retry_params.compute_delay(num_attempts); | ||
| let delay = match error.retry_after() { | ||
| Some(hint) => hint.min(retry_params.max_delay), |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new Some(hint) branch only applies hint.min(retry_params.max_delay) and never computes the normal backoff. For x-amz-retry-after, the AWS retry behavior clamps the server delay to the computed backoff window (AWS docs); as written, a tiny hint such as 1 ms on a throttling response causes an immediate retry instead of respecting the exponential backoff floor, while a first-retry hint just below max_delay can sleep far longer than the intended computed-backoff-plus-slack window.
Useful? React with 👍 / 👎.
3 participants
No description provided.