diff --git a/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt b/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt new file mode 100644 index 00000000..39ddebc9 --- /dev/null +++ b/app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt @@ -0,0 +1,54 @@ +package com.bitchat.android.util + +/** + * Reads GitHub's rate-limit rejections so the app can stop asking. + * + * Unauthenticated requests are capped at 60 an hour *per IP*, and when the app routes through Tor + * that IP belongs to an exit node shared with every other user on it, so the ceiling arrives much + * sooner than the per-user maths suggests. Retrying a rejection is pure waste, and repeating it on + * every screen open is what turns a brief limit into a permanent one. + */ +internal object GitHubRateLimit { + + /** Used when GitHub rejects a request without saying when to come back. */ + const val DEFAULT_BACKOFF_MILLIS = 10 * 60 * 1000L + + /** Never sit out longer than this, however far ahead the reset header claims to be. */ + 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. + */ + fun isRateLimited(code: Int, remaining: String?): Boolean = + code == 429 || (code == 403 && remaining?.trim() == "0") + + /** + * Epoch millis before which no further request should be sent, or null when the response was + * not a rate-limit rejection at all. + */ + fun blockedUntilMillis( + code: Int, + remaining: String?, + resetEpochSeconds: String?, + retryAfterSeconds: String?, + nowMillis: Long, + ): Long? { + if (!isRateLimited(code, remaining)) 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 } + + // Dropped when it is not in the future: a skewed device clock must not turn a genuine + // rejection into "retry immediately". + val fromReset = resetEpochSeconds?.trim()?.toLongOrNull() + ?.let { it * 1000 } + ?.takeIf { it > nowMillis } + + val target = fromRetryAfter ?: fromReset ?: (nowMillis + DEFAULT_BACKOFF_MILLIS) + return target.coerceIn(nowMillis, nowMillis + MAX_BACKOFF_MILLIS) + } +} 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 699637bd..92f8e88c 100644 --- a/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt +++ b/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt @@ -23,12 +23,30 @@ object GitHubReleaseClient { private const val CACHE_TTL_MILLIS = 10 * 60 * 1000L private const val MAX_FETCH_ATTEMPTS = 3 private const val ROUTE_READY_TIMEOUT_MILLIS = 60_000L + private const val HTTP_NOT_MODIFIED = 304 private val fetchMutex = Mutex() @Volatile private var cachedRelease: CachedRelease? = null + /** + * ETag of the cached release, replayed as `If-None-Match`. GitHub does not charge a 304 + * against the rate limit, so revalidating an expired cache this way costs nothing where an + * unconditional refetch costs one of only 60 hourly requests. + */ + @Volatile + private var cachedEtag: String? = null + + /** + * Epoch millis before which GitHub has already told us it will reject anything we send. + * + * Without this, an exhausted quota fed itself: nothing cached the failure, so every screen + * that asked for release info spent three more requests discovering the same limit. + */ + @Volatile + private var blockedUntilMillis = 0L + private val client get() = OkHttpProvider.httpClient().newBuilder() // GitHub requests may travel through Tor, where a 15-second total @@ -46,10 +64,31 @@ object GitHubReleaseClient { suspend fun fetchLatestRelease(forceRefresh: Boolean = false): Result = withContext(Dispatchers.IO) { fetchMutex.withLock { - if (!forceRefresh) { - cachedRelease - ?.takeIf { System.currentTimeMillis() - it.fetchedAtMillis < CACHE_TTL_MILLIS } - ?.let { return@withLock Result.success(it.release) } + val now = System.currentTimeMillis() + val cached = cachedRelease + + if (!forceRefresh && + cached != null && + now - cached.fetchedAtMillis < CACHE_TTL_MILLIS + ) { + return@withLock Result.success(cached.release) + } + + // Honoured even on an explicit refresh: sending a request GitHub has already said + // it will reject helps nobody and pushes the reset further out. A stale release is + // a better answer than an error the user cannot act on. + if (now < blockedUntilMillis) { + val waitMinutes = (blockedUntilMillis - now) / 60_000 + 1 + Log.w(TAG, "Rate limited; not contacting GitHub for another ${waitMinutes}min") + cached?.let { return@withLock Result.success(it.release) } + return@withLock Result.failure( + ReleaseFetchException( + message = "GitHub API rate limit reached. Try again in " + + "$waitMinutes minute${if (waitMinutes == 1L) "" else "s"}.", + httpCode = 429, + retryable = false + ) + ) } if (!awaitSelectedNetworkRoute()) { @@ -95,6 +134,8 @@ object GitHubReleaseClient { } private fun fetchLatestReleaseOnce(): Result { + val cached = cachedRelease + val etag = cachedEtag return try { Log.d(TAG, "Fetching latest release from GitHub API") val request = Request.Builder() @@ -102,29 +143,48 @@ object GitHubReleaseClient { .addHeader("User-Agent", USER_AGENT) .addHeader("Accept", "application/vnd.github+json") .addHeader("X-GitHub-Api-Version", "2022-11-28") + .apply { + // Revalidate rather than refetch. GitHub does not charge a 304 against the + // hourly quota, so an unchanged release costs nothing to confirm. + if (cached != null && etag != null) addHeader("If-None-Match", etag) + } .build() client.newCall(request).execute().use { response -> + if (response.code == HTTP_NOT_MODIFIED && cached != null) { + Log.d(TAG, "Release unchanged; cache revalidated at no quota cost") + cachedRelease = cached.copy(fetchedAtMillis = System.currentTimeMillis()) + return Result.success(cached.release) + } + if (!response.isSuccessful) { - val remaining = response.header("X-RateLimit-Remaining") - val resetAt = response.header("X-RateLimit-Reset") - val message = when { - response.code == 403 && remaining == "0" -> - "GitHub API rate limit exceeded. Try again after reset time $resetAt." - response.code == 429 -> - "GitHub API rate limit exceeded. Please try again later." - else -> - "GitHub release request failed: HTTP ${response.code} ${response.message}" + val blockedUntil = GitHubRateLimit.blockedUntilMillis( + code = response.code, + remaining = response.header("X-RateLimit-Remaining"), + resetEpochSeconds = response.header("X-RateLimit-Reset"), + retryAfterSeconds = response.header("Retry-After"), + nowMillis = System.currentTimeMillis(), + ) + if (blockedUntil != null) blockedUntilMillis = blockedUntil + + val message = if (blockedUntil != null) { + val waitMinutes = + (blockedUntil - System.currentTimeMillis()) / 60_000 + 1 + "GitHub API rate limit exceeded. Try again in " + + "$waitMinutes minute${if (waitMinutes == 1L) "" else "s"}." + } else { + "GitHub release request failed: HTTP ${response.code} ${response.message}" } Log.e(TAG, message) return Result.failure( ReleaseFetchException( message = message, httpCode = response.code, - retryable = response.code == 403 || - response.code == 408 || - response.code == 429 || - response.code >= 500 + // A rate limit is never worth an in-loop retry: the gate in + // fetchLatestRelease decides when it is worth asking again. A plain + // 403 is a permissions failure and will not fix itself either. + retryable = blockedUntil == null && + (response.code == 408 || response.code >= 500) ) ) } @@ -146,6 +206,10 @@ object GitHubReleaseClient { retryable = false ) ) + // Kept alongside the release so the pair can never drift: a stale ETag would + // revalidate to a 304 that confirms a release we no longer hold. + cachedEtag = response.header("ETag") + blockedUntilMillis = 0L Result.success(release) } } catch (e: IOException) { diff --git a/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt b/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt new file mode 100644 index 00000000..3bd7e982 --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt @@ -0,0 +1,126 @@ +package com.bitchat.android.util + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Unauthenticated GitHub allows 60 requests an hour per IP, and over Tor that IP is an exit node + * shared with everyone else using it. Reading the rejection correctly is what keeps the app from + * hammering a quota it has already exhausted. + */ +class GitHubRateLimitTest { + + private val now = 1_700_000_000_000L + + @Test + fun `a 403 that still has quota is a permissions error, not a rate limit`() { + assertFalse(GitHubRateLimit.isRateLimited(code = 403, remaining = "42")) + assertNull( + GitHubRateLimit.blockedUntilMillis( + code = 403, + remaining = "42", + resetEpochSeconds = null, + retryAfterSeconds = null, + nowMillis = now, + ) + ) + } + + @Test + fun `a 403 with no quota left blocks until the advertised reset`() { + val resetSeconds = now / 1000 + 900 + + assertTrue(GitHubRateLimit.isRateLimited(code = 403, remaining = "0")) + assertEquals( + resetSeconds * 1000, + GitHubRateLimit.blockedUntilMillis( + code = 403, + remaining = "0", + resetEpochSeconds = resetSeconds.toString(), + retryAfterSeconds = null, + nowMillis = now, + ) + ) + } + + @Test + fun `a 429 is a rate limit even without a remaining header`() { + assertTrue(GitHubRateLimit.isRateLimited(code = 429, remaining = null)) + } + + @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 + // sooner than the primary window the reset header describes. + assertEquals( + now + 30_000, + GitHubRateLimit.blockedUntilMillis( + code = 429, + remaining = "0", + resetEpochSeconds = (now / 1000 + 3_000).toString(), + retryAfterSeconds = "30", + nowMillis = now, + ) + ) + } + + @Test + fun `a rejection with no timing headers falls back to a fixed backoff`() { + assertEquals( + now + GitHubRateLimit.DEFAULT_BACKOFF_MILLIS, + GitHubRateLimit.blockedUntilMillis( + code = 429, + remaining = null, + resetEpochSeconds = null, + retryAfterSeconds = null, + nowMillis = now, + ) + ) + } + + @Test + fun `a reset time already in the past falls back rather than unblocking immediately`() { + // A skewed device clock must not turn a real rejection into "retry right now". + assertEquals( + now + GitHubRateLimit.DEFAULT_BACKOFF_MILLIS, + GitHubRateLimit.blockedUntilMillis( + code = 429, + remaining = null, + resetEpochSeconds = (now / 1000 - 500).toString(), + retryAfterSeconds = null, + nowMillis = now, + ) + ) + } + + @Test + fun `an absurd reset time is clamped so the app is never locked out for long`() { + assertEquals( + now + GitHubRateLimit.MAX_BACKOFF_MILLIS, + GitHubRateLimit.blockedUntilMillis( + code = 429, + remaining = null, + resetEpochSeconds = (now / 1000 + 86_400).toString(), + retryAfterSeconds = null, + nowMillis = now, + ) + ) + } + + @Test + fun `unparseable headers fall back instead of throwing`() { + assertEquals( + now + GitHubRateLimit.DEFAULT_BACKOFF_MILLIS, + GitHubRateLimit.blockedUntilMillis( + code = 429, + remaining = null, + resetEpochSeconds = "not-a-number", + retryAfterSeconds = "Wed, 21 Oct 2015 07:28:00 GMT", + nowMillis = now, + ) + ) + } +}