diff --git a/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt b/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt index 39ddebc9..c03f25c5 100644 --- a/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt +++ b/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt @@ -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". diff --git a/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt b/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt index 92f8e88c..d33c1531 100644 --- a/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt +++ b/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt @@ -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 = + /** + * @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 = withContext(Dispatchers.IO) { fetchMutex.withLock { val now = System.currentTimeMillis() @@ -91,6 +99,7 @@ object GitHubReleaseClient { ) } + onAwaitingNetworkRoute?.invoke() if (!awaitSelectedNetworkRoute()) { return@withLock Result.failure( ReleaseFetchException( diff --git a/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt b/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt index bb958d0d..47789582 100644 --- a/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt +++ b/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt @@ -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( diff --git a/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt b/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt index 3bd7e982..9f9a7b3e 100644 --- a/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt @@ -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