fix: gate secondary rate limits and name the Tor wait correctly

Addresses review on #812.

Secondary rate limits were classed as permissions failures. GitHub
serves them as 403 with Retry-After while X-RateLimit-Remaining is
still nonzero, because the primary hourly quota is not what was hit --
so isRateLimited() returned false, blockedUntilMillis was never set, no
stale release was served, and every About sheet open kept contacting
GitHub through exactly the cooldown it had been asked to observe. The
predicate now also admits a 403 carrying a usable Retry-After; one that
cannot be parsed is still a permissions failure.

The phase reported during the Tor wait was the wrong one.
fetchLatestRelease() waits on the selected route itself, so the UI read
"Checking latest release..." for the whole bootstrap and only switched
to AwaitingNetworkRoute afterwards, when the second route check returns
immediately -- putting the wrong label on the one wait the phase exists
to explain. Reordering the two calls would have made cache hits wait on
Tor, since the cache returns before the route check, so the fetch now
reports from the inside via onAwaitingNetworkRoute and the caller keeps
its own check for the cache-hit path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Moe Hamade 2026-07-29 14:17:56 +03:00
parent 389fbd28fe
commit f33ce0bb98
4 changed files with 72 additions and 10 deletions

View File

@ -17,11 +17,20 @@ internal object GitHubRateLimit {
const val MAX_BACKOFF_MILLIS = 60 * 60 * 1000L
/**
* A 403 alone is not enough: GitHub also uses it for ordinary permission failures. Only a 403
* that reports zero remaining quota, or an explicit 429, is a rate limit.
* A 403 alone is not enough: GitHub also uses it for ordinary permission failures.
*
* Three things count as a rate limit. An explicit 429. A 403 reporting zero remaining quota,
* which is the primary hourly limit. And a 403 carrying Retry-After while quota remains, which
* is how secondary limits arrive abuse detection rather than the hourly budget, so treating
* it as a permissions failure leaves the gate unset and keeps the app calling during exactly
* the cooldown GitHub asked for.
*/
fun isRateLimited(code: Int, remaining: String?): Boolean =
code == 429 || (code == 403 && remaining?.trim() == "0")
fun isRateLimited(code: Int, remaining: String?, retryAfterSeconds: String? = null): Boolean =
code == 429 ||
(code == 403 && (remaining?.trim() == "0" || retryAfterDelayMillis(retryAfterSeconds) != null))
private fun retryAfterDelayMillis(retryAfterSeconds: String?): Long? =
retryAfterSeconds?.trim()?.toLongOrNull()?.takeIf { it > 0 }?.let { it * 1000 }
/**
* Epoch millis before which no further request should be sent, or null when the response was
@ -34,13 +43,11 @@ internal object GitHubRateLimit {
retryAfterSeconds: String?,
nowMillis: Long,
): Long? {
if (!isRateLimited(code, remaining)) return null
if (!isRateLimited(code, remaining, retryAfterSeconds)) return null
// Retry-After is a delta and is what GitHub sends for secondary limits, which can lift
// sooner than the primary window X-RateLimit-Reset describes.
val fromRetryAfter = retryAfterSeconds?.trim()?.toLongOrNull()
?.takeIf { it > 0 }
?.let { nowMillis + it * 1000 }
val fromRetryAfter = retryAfterDelayMillis(retryAfterSeconds)?.let { nowMillis + it }
// Dropped when it is not in the future: a skewed device clock must not turn a genuine
// rejection into "retry immediately".

View File

@ -61,7 +61,15 @@ object GitHubReleaseClient {
* Successful metadata is cached briefly so the status screen and download
* worker use the same release snapshot instead of making duplicate calls.
*/
suspend fun fetchLatestRelease(forceRefresh: Boolean = false): Result<Release> =
/**
* @param onAwaitingNetworkRoute invoked if this call is about to block on the selected route
* (a Tor bootstrap can take the better part of a minute). A cache hit returns before that
* point and never invokes it, so callers can report the wait only when there is one.
*/
suspend fun fetchLatestRelease(
forceRefresh: Boolean = false,
onAwaitingNetworkRoute: (() -> Unit)? = null,
): Result<Release> =
withContext(Dispatchers.IO) {
fetchMutex.withLock {
val now = System.currentTimeMillis()
@ -91,6 +99,7 @@ object GitHubReleaseClient {
)
}
onAwaitingNetworkRoute?.invoke()
if (!awaitSelectedNetworkRoute()) {
return@withLock Result.failure(
ReleaseFetchException(

View File

@ -222,10 +222,18 @@ class UniversalApkManager(private val context: Context) {
// status check. If this worker is running after process death, the
// client performs a retried network fetch instead.
phaseCallback?.invoke(ApkDownloader.DownloadPhase.ResolvingRelease)
val release = GitHubReleaseClient.fetchLatestRelease().getOrElse { error ->
// The fetch waits on the route itself when it has to go to the network, so it
// reports that from the inside. Labelling the whole call "resolving release"
// would put the app's own name on the long Tor wait this phase exists to explain.
val release = GitHubReleaseClient.fetchLatestRelease(
onAwaitingNetworkRoute = {
phaseCallback?.invoke(ApkDownloader.DownloadPhase.AwaitingNetworkRoute)
}
).getOrElse { error ->
return@withContext Result.failure(error)
}
// A cache hit skips the fetch's own wait, so for that path the route wait is here.
phaseCallback?.invoke(ApkDownloader.DownloadPhase.AwaitingNetworkRoute)
if (!GitHubReleaseClient.awaitSelectedNetworkRoute()) {
return@withContext Result.failure(

View File

@ -51,6 +51,44 @@ class GitHubRateLimitTest {
assertTrue(GitHubRateLimit.isRateLimited(code = 429, remaining = null))
}
@Test
fun `a secondary limit is a 403 with Retry-After while quota remains`() {
// GitHub serves secondary limits as 403 + Retry-After without exhausting the
// primary quota, so remaining is still nonzero.
assertTrue(
GitHubRateLimit.isRateLimited(
code = 403,
remaining = "42",
retryAfterSeconds = "60",
)
)
}
@Test
fun `a secondary limit blocks for the Retry-After it advertises`() {
assertEquals(
now + 60_000,
GitHubRateLimit.blockedUntilMillis(
code = 403,
remaining = "42",
resetEpochSeconds = null,
retryAfterSeconds = "60",
nowMillis = now,
)
)
}
@Test
fun `a 403 with an unusable Retry-After stays a permissions error`() {
assertFalse(
GitHubRateLimit.isRateLimited(
code = 403,
remaining = "42",
retryAfterSeconds = "not-a-number",
)
)
}
@Test
fun `Retry-After takes precedence over the reset header`() {
// Retry-After is a delta and is what GitHub sends for secondary limits, which can expire