From 657fee0de6c4e77e9c83b2cf77999bf1c148d438 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Tue, 28 Jul 2026 20:36:43 +0300 Subject: [PATCH] fix: stop the GitHub release check exhausting its own rate limit Opening the About sheet runs a release check, and an exhausted quota fed itself: only successes were cached, and a 403 reporting zero remaining was classed retryable, so every sheet open spent three more requests rediscovering the same limit. Unauthenticated GitHub allows 60 requests an hour per IP, and over Tor that IP is an exit node shared with every other user on it, so the ceiling arrives far sooner than per-user maths suggests. Three changes: - Conditional requests. The client now stores the release ETag and replays it as If-None-Match. GitHub does not charge a 304 against the rate limit, so revalidating an expired cache is free where an unconditional refetch costs one of the 60. This is why neither polling nor long polling is the right answer here. - A rate-limit gate. X-RateLimit-Reset and Retry-After were read only to interpolate into an error string; they now set a deadline before which no request is sent at all. While blocked, a stale cached release is served in preference to an error the user cannot act on. Clamped to an hour so a bad header cannot lock the feature out, and a reset time in the past falls back to a fixed backoff rather than unblocking a skewed clock immediately. - Rate limits are no longer retried in-loop. The gate decides when it is worth asking again. A plain 403 is a permissions failure and is no longer retried either. The gate's decision logic is pure and unit tested. The wiring around it is not: that needs a MockWebServer, which is not currently a dependency. Known gap: the cache and ETag are in memory only, so a process restart still costs one request. Persisting them needs a Context threaded into what is currently a context-free object; left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) --- .../bitchat/android/util/GitHubRateLimit.kt | 54 ++++++++ .../android/util/GitHubReleaseClient.kt | 98 +++++++++++--- .../android/util/GitHubRateLimitTest.kt | 126 ++++++++++++++++++ 3 files changed, 261 insertions(+), 17 deletions(-) create mode 100644 app/src/main/java/com/bitchat/android/util/GitHubRateLimit.kt create mode 100644 app/src/test/kotlin/com/bitchat/android/util/GitHubRateLimitTest.kt 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, + ) + ) + } +}