Retry 429s after waiting a specified time #4689
Conversation
IMB11
added
app
packages/app-lib/src/util/fetch.rs
Outdated
| let backup_error = resp.error_for_status_ref().unwrap_err(); | ||
| if resp.status() == 429 | ||
| && let Some(reset_header) = | ||
| resp.headers().get("X-Ratelimit-Reset") |
There was a problem hiding this comment.
X-Ratelimit-Reset is a non-standard header, and the concept of rate-limiting headers beyond Retry-After has only recently seen some standardization efforts by the IETF. In particular, the latest draft proposal on the matter from last month, which is not yet a standard, states:
[...] different headers, with the same semantics, are used by different implementers:
- X-RateLimit-Limit and X-Rate-Limit-Limit
- X-RateLimit-Remaining and X-Rate-Limit-Remaining
- X-RateLimit-Reset and X-Rate-Limit-Reset
With that in mind, I have two questions:
- Which CDNs or file hosts you have seen sending this header? I tried hammering requests from several threads to one of the Modrinth CDN URLs listed in the
.mrpackfile attached to hitting a 429 rate limit cancels installing a .mrpack #3805, but I couldn't get any 429 responses. I also wasn't able to find any documentation from the CDN provider regarding this header. - Do the affected CDNs or file hosts also send the standard
Retry-Afterheader? If so, I'd prefer to use that one instead.
There was a problem hiding this comment.
This was based off of the Modrinth API ratelimit documentation which uses X-Ratelimit-Reset as number of seconds until the ratelimit resets. I will have to look into the portability of this, and seeing if Retry-After is a better solution
There was a problem hiding this comment.
I see! That ratelimit documentation applies to API routes served by Labrinth, but the CDN endpoints from which project version files are downloaded don't go through Labrinth, so that documentation doesn't apply in this case.
Edit: nevertheless, it sounds plausible that during installation of a big .mrpack the app may be sending lots of separate requests to rate-limited endpoints, which may include some in our API or elsewhere. It'd be nice to track those down and confirm that's the case before merging this.
There was a problem hiding this comment.
I've tried to make the detection more portable, especially since X-Ratelimit-Reset isn't portable, while Retry-After is a standard
There was a problem hiding this comment.
I see, nice! I no longer work for Modrinth, so I'm unable to move your PR forward myself.
As for the latest changes you pushed, they look good overall. I'd remove the stray dbg! macro that slipped into the commit, but aside from that, everything seems solid.
Out of curiosity, did you look further into which hosts are sending which headers under what conditions? As mentioned, the CDN endpoints shouldn't be sending X-Ratelimit-Reset at all.
3 participants
Resolves #3805