fix(apk): stop a reset header alone from marking a 403 rate limited

GitHub sends X-RateLimit-Reset on every REST response, an ordinary 403
included, and it always points at the current window. Feeding it through
retryAtMillis() therefore produced a non-null deadline for any 403, and
the classifier accepted that as proof of a limit. A permissions failure
with the quota untouched came back as RateLimited, so the caller persisted
a cooldown on that route and served stale metadata until a reset window
the failure had nothing to do with.

Classification now looks only at signals that actually mean this request
was the one refused: a spent quota, or an explicit Retry-After. Nothing
real is lost, because GitHub marks a primary limit with
X-RateLimit-Remaining: 0 and a secondary limit with Retry-After. The reset
header keeps its job of supplying the deadline once a limit is
established some other way.

ApkDownloadSourceTest already claimed this contract - its name is "403 is
only treated as a limit when response headers say so" - but its
permissions case passed no reset header at all, which is the one input
that hides the bug. Adds the case it was missing, which fails without this
change, and pins the secondary-limit path so tightening the reset header
cannot blind the client to a Retry-After.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Moe Hamade 2026-08-09 15:16:51 +03:00
parent 7b86bafbac
commit 52f14cc5b5
2 changed files with 51 additions and 1 deletions

View File

@ -150,8 +150,17 @@ internal object ApkDownloadHttpErrors {
rateLimitResetEpochSeconds = rateLimitResetEpochSeconds,
nowMillis = nowMillis
)
// X-RateLimit-Reset rides on every GitHub response, an ordinary 403 included, so it
// cannot tell an exhausted quota from a permissions failure. Only a spent quota or an
// explicit Retry-After says this request was the one that got limited. The reset header
// still supplies the deadline below, once being limited is established some other way.
val retryAfterMillis = retryAtMillis(
retryAfter = retryAfter,
rateLimitResetEpochSeconds = null,
nowMillis = nowMillis
)
val rateLimited = code == 429 ||
(code == 403 && (rateLimitRemaining?.trim() == "0" || retryAt != null))
(code == 403 && (rateLimitRemaining?.trim() == "0" || retryAfterMillis != null))
if (rateLimited) {
return ApkDownloadException(

View File

@ -101,6 +101,47 @@ class ApkDownloadSourceTest {
assertEquals(ApkDownloadFailureReason.RateLimited, quotaFailure.reason)
}
@Test
fun `a reset header alone does not make a 403 a rate limit`() {
// GitHub sends X-RateLimit-Reset on every response, so a permissions failure carries one
// while the quota is untouched. Reading it as a limit would park the route in a cooldown
// and serve stale metadata until a window the failure has nothing to do with.
val failure = ApkDownloadHttpErrors.fromResponse(
source = source,
code = 403,
responseMessage = "Forbidden",
retryAfter = null,
rateLimitRemaining = "4999",
rateLimitResetEpochSeconds = (now / 1000L + 1_800L).toString(),
nowMillis = now
)
assertEquals(ApkDownloadFailureReason.HttpFailure, failure.reason)
assertNull(failure.retryAtMillis)
assertEquals(
listOf(source.displayName, "403", "Forbidden"),
failure.messageArgs
)
}
@Test
fun `a secondary limit is still caught by its Retry-After`() {
// The quota is intact, so only Retry-After marks this one. It has to keep working, or
// tightening the reset-header case would blind the client to secondary limits.
val failure = ApkDownloadHttpErrors.fromResponse(
source = source,
code = 403,
responseMessage = "Forbidden",
retryAfter = "90",
rateLimitRemaining = "4999",
rateLimitResetEpochSeconds = (now / 1000L + 1_800L).toString(),
nowMillis = now
)
assertEquals(ApkDownloadFailureReason.RateLimited, failure.reason)
assertEquals(now + 90_000L, failure.retryAtMillis)
}
@Test
fun `invalid or overflowing retry headers never crash error mapping`() {
assertNull(