Retry HTTP 429 responses as throttling errors#16
Open
thundergolfer wants to merge 3 commits intomainfrom
Open
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
S3-compatible backends like Cloudflare R2 return HTTP 429 for bandwidth throttling instead of S3's usual 503 SlowDown. The CRT only maps 503 to AWS_ERROR_S3_SLOW_DOWN, so 429 was falling through to a terminal ResponseError and not being retried. Add a 429 arm to try_parse_generic_error that returns S3RequestError::Throttled, enabling the existing exponential backoff retry strategy (up to 10 attempts) for these responses. Co-Authored-By: Jonathon Belotti <jonathon@modal.com>
The Throttled variant now carries ClientErrorMetadata so that HTTP 429 responses report the real status code (429), error code (e.g. ServiceUnavailable), and message from the response body. The existing CRT-error path (AWS_ERROR_S3_SLOW_DOWN) continues to report 503/SlowDown as before. Co-Authored-By: Jonathon Belotti <jonathon@modal.com>
Co-Authored-By: Jonathon Belotti <jonathon@modal.com>
d5e0397 to
da13c05
Compare
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.
S3-compatible backends like Cloudflare R2 return HTTP 429 for bandwidth throttling instead of S3's native 503 SlowDown response. The CRT retry logic is keyed on
AWS_ERROR_S3_SLOW_DOWN(which maps to 503), sotry_parse_throttlednever matches for 429. The 429 falls through to_ => Noneintry_parse_generic_error, becomes a terminalResponseError, and the FUSE mount dies with EIO.This adds a
429arm to thetry_parse_generic_errormatch that returnsS3RequestError::Throttled(...), enabling the existing exponential backoff retry strategy (up to 10 attempts, 500ms base with full jitter) for these responses. The 429 arm parses the XML response body to extract the actual error code and message, so error metadata accurately reflects the real response (HTTP 429 /ServiceUnavailable) rather than hardcoding 503 /SlowDown.To support this,
S3RequestError::Throttledwas changed from a unit variant toThrottled(ClientErrorMetadata). The existing CRT-error path (AWS_ERROR_S3_SLOW_DOWN, i.e. S3's 503 SlowDown) continues to populate the metadata with the same synthetic 503/SlowDown values as before.Observed in production when a Modal
CloudBucketMountbacked by R2 failed immediately on the firstGetObjectwith:"You have exceeded your available bandwidth limit."(HTTP 429,<Code>ServiceUnavailable</Code>).Does this change impact existing behavior?
Yes — HTTP 429 responses that were previously treated as terminal errors are now retried. No other response codes are affected. The
Throttledvariant is now a tuple variant carryingClientErrorMetadata, which is a breaking change to the enum shape but does not affect runtime behavior for the existing 503 path (same metadata values are produced).Does this change need a changelog entry? Does it require a version change?
Not required — this is a Modal fork-only change for R2 compatibility, not an upstream contribution.
Human review checklist
Nonefor error code/message if parsing fails). Confirm this is acceptable vs. providing a generic fallback message.Throttledvariant is nowThrottled(ClientErrorMetadata)— any downstream code pattern-matching onThrottledwill need updating. Onlys3_crt_client.rsmatches on it within this repo.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).
Link to Devin session: https://modal.devinenterprise.com/sessions/1742473af31243ae8ce6830f659271eb
Requested by: @thundergolfer