Use `X-RateLimit-Reset` header to determine retry delay

[fregante]

Use `X-RateLimit-Reset` header to determine retry delay

[fregante]

Also GitHub's docs are here:

https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit

[sholladay]

If I understand this correctly, it's basically a small optimization where the server can inform the client ahead of time that the next request would receive a 429 response. Thus the client can avoid making one unnecessary request instead of simply reacting to the 429 when it happens. And such optimizations can become meaningful at scale, especially if a service counts a 429 against your rate limit quota.

That's kind of interesting, although I'm not really sure what we would do with this information. It implies that we would have to keep track of state across multiple ky() calls... yuck. If a request is made after the rate limit is exceeded, would we throw an error or delay it until RateLimit-Reset? Would ky.extend() instances share the state with their parent? This also seems prone to a backlog of requests building up and then all being sent at once, which seems counter-productive. A properly designed rate limiter shouldn't even have a counter that resets suddenly, it should take a sampling from a rolling window relative to the current request.

The whole proposal seems poorly designed to me. RateLimit-Limit, really? It's not even defined as an actual limit:

If the client exceeds that limit, it MAY not be served.

Given the above, plus the fact that it's not widely used beyond a few big names, my position is that until this is standardized, it should be implemented in a service client. See gh-got, for example, which does expose those headers, though I don't think it actually uses them internally.

[fregante]

ky already has this logic as part of the retry option and it even automatically reads the Retry-After header, so I believe all those questions are already answered by existing code.

This feature request is only about reading a couple more headers, I think.

[sholladay]

It's true that we handle Retry-After, which you would primarily see in a 429 status response.

However, Retry-After is easy to handle because we can decide what to do and trigger an action immediately when it is received (the action being a fetch delayed by a setTimeout() call is merely incidental).

We could certainly treat RateLimit-Remaining: 0 the same way, but I think that's wrong (otherwise these headers would provide almost no value).

As far as I can tell, the whole point of the proposal is really to make clients react to RateLimit-Remaining: 0 for status 2xx responses and mark the next request to be delayed preemptively before a 429 error even occurs. At the moment, Ky doesn't have a mechanism to do that.

I guess we could implement partial support and just handle error responses. Don't all of these APIs return Retry-After for 4xx errors, though?

[fregante]

the whole point of the proposal is really to make clients react to RateLimit-Remaining: 0 for status 2xx responses

I'll clarify my request by updating the title, that's not what I was suggesting.

Don't all of these APIs return Retry-After for 4xx errors, though?

Not in GitHub's case:

https://github.com/orgs/community/discussions/24760

[sholladay]

Interesting! To me, that seems like pure laziness on their part. I mean, if RateLimit-Reset can be calculated, then so can Retry-After. Just use the spec-compliant header for goodness sake.

At any rate 🙃, I can get behind treating these headers equivalently. Even though I don't think that's what the IETF proposal implies.

We should probably do headers.get('Retry-After') || headers.get('RateLimit-Reset') || headers.get('X-RateLimit-Reset') to be thorough.

[0Abdullah]

I'm currently using the Twitch API which implements RateLimit-Reset with a unix epoch timestamp instead of Retry-After.

I think a more general approach would be to allow optional custom parsing with access to the headers that returns the milliseconds to retry after.

something like this:

await ky.get('https://foo.com', {
	retry: {
		parseRetryHeader: (headers) => {
			const ratelimit_reset_epoch = headers.get('Ratelimit-Reset')
			return new Date(Number(ratelimit_reset_epoch) * 1000) - new Date()
		}
	}
})

What do you think? 🤔

[sholladay]

We already support timestamps for Retry-After, so it would be supported for RateLimit-Reset also, without the need for a function option.

[sholladay]

The standards say that neither we, nor GitHub et al., should be using the X- prefix for headers.

See: RFC 6648

From MDN:

Custom proprietary headers have historically been used with an X- prefix, but this convention was deprecated in June 2012 because of the inconveniences it caused when nonstandard fields became standard in RFC 6648; others are listed in the IANA HTTP Field Name Registry, whose original content was defined in RFC 4229.

I opened a PR that does support it, though. To be pragmatic.