From 4200871ee9ef7d07ca6e7250523014abc52d0dcc Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:41:05 +0300 Subject: [PATCH] fix: key the rate-limit gate to the selected route, and drop the force flag Addresses review on #812. isProxyEnabled() reports readiness, not route selection: it is false while Tor is bootstrapping or restarting, even though requests will still go through Tor. Using it as the route identity cleared a Tor-earned gate mid-bootstrap and applied a direct-earned one to the first Tor request -- the opposite of what scoping the gate was for. The identity is now the selected mode from statusFlow. The force flag turned out to be both unnecessary and harmful. Leaving Downloading synchronously before the check already clears the entry guard, so force only reached the completion guard -- which must stay armed. A cancellation check can take a minute on the route timeout, and WorkManager can surface Resumable meanwhile, so the user may start a new download before it returns; force let the stale result overwrite work that was running and strip the progress and stop controls. Removing it restores that protection and needs no generation counter. Co-Authored-By: Claude Opus 5 (1M context) --- .../android/ui/ApkDownloadViewModel.kt | 21 +++++----- .../android/util/GitHubReleaseClient.kt | 41 ++++++++++++------- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt b/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt index ec31ab93..16dbdc8f 100644 --- a/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt @@ -206,9 +206,10 @@ class ApkDownloadViewModel(application: Application) : AndroidViewModel(applicat it.copy(apkStatus = ApkPreparationStatus.Loading, downloadProgress = 0) } - // force, because the guards in checkStatus() protect a job that is still - // running, which this one is not. - checkStatus(force = true) + // No force needed, and it would be harmful: leaving Downloading above already + // clears the entry guard, while the completion guard must stay armed so a + // download the user restarts during this check is not overwritten by its result. + checkStatus() } private fun startDownload() { @@ -222,23 +223,21 @@ class ApkDownloadViewModel(application: Application) : AndroidViewModel(applicat downloader.startDownload() } - /** - * @param force resolve even while the state says Downloading. Only cancellation - * should pass true: the guard below exists so a running job is never second-guessed - * from cache contents, but a cancelled one has no other route out of that state. - */ - private fun checkStatus(force: Boolean = false) { + private fun checkStatus() { viewModelScope.launch { // WorkManager is the source of truth for active work. A queued or // newly started job legitimately has no partial file yet, so never // infer that it is orphaned from cache contents. - if (!force && _state.value.apkStatus is ApkPreparationStatus.Downloading) { + if (_state.value.apkStatus is ApkPreparationStatus.Downloading) { return@launch } val resolvedStatus = resolveApkStatus() _state.update { current -> - if (!force && current.apkStatus is ApkPreparationStatus.Downloading) { + // Re-checked rather than trusted from entry: this resolve reaches the + // network and can take a minute, in which time the user may have started + // a download. Its result must not overwrite work that is now running. + if (current.apkStatus is ApkPreparationStatus.Downloading) { current } else { current.copy(apkStatus = resolvedStatus, downloadProgress = 0) 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 e0d1ed20..90110ccb 100644 --- a/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt +++ b/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt @@ -3,6 +3,7 @@ package com.bitchat.android.util import android.util.Log import com.bitchat.android.net.ArtiTorManager import com.bitchat.android.net.OkHttpProvider +import com.bitchat.android.net.TorMode import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.sync.Mutex @@ -48,23 +49,36 @@ object GitHubReleaseClient { private var blockedUntilMillis = 0L /** - * Whether the route was proxied when [blockedUntilMillis] was recorded, or null when - * no gate is set. GitHub counts unauthenticated requests per IP, so a Tor exit and a - * direct connection have separate quotas -- a cooldown earned on one must not be - * served to the other. + * Whether Tor was the selected route when [blockedUntilMillis] was recorded, or null + * when no gate is set. GitHub counts unauthenticated requests per IP, so a Tor exit + * and a direct connection have separate quotas -- a cooldown earned on one must not + * be served to the other. */ - private var blockedRouteUsedProxy: Boolean? = null + @Volatile + private var blockedRouteUsedTor: Boolean? = null + + /** + * The route requests will take, which is what the quota belongs to. + * + * Deliberately the selected mode rather than `isProxyEnabled()`: that reports + * readiness, and is false while Tor is still bootstrapping or restarting even though + * requests will still go through Tor once it is up. Using it as the route identity + * would clear a Tor-earned gate mid-bootstrap and apply a direct-earned one to the + * first Tor request. + */ + private fun selectedRouteUsesTor(): Boolean? = + runCatching { ArtiTorManager.getInstance().statusFlow.value.mode != TorMode.OFF } + .getOrNull() /** Drops a gate earned on a route the app is no longer using. */ private fun clearGateIfRouteChanged() { - val recordedRoute = blockedRouteUsedProxy ?: return - val currentRoute = runCatching { ArtiTorManager.getInstance().isProxyEnabled() } - .getOrNull() ?: return + val recordedRoute = blockedRouteUsedTor ?: return + val currentRoute = selectedRouteUsesTor() ?: return if (recordedRoute != currentRoute) { Log.i(TAG, "Route changed since the rate limit was recorded; clearing the gate") blockedUntilMillis = 0L - blockedRouteUsedProxy = null + blockedRouteUsedTor = null } } @@ -104,11 +118,11 @@ object GitHubReleaseClient { return@withLock Result.success(cached.release) } + clearGateIfRouteChanged() + // 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. - clearGateIfRouteChanged() - if (now < blockedUntilMillis) { val waitMinutes = (blockedUntilMillis - now) / 60_000 + 1 Log.w(TAG, "Rate limited; not contacting GitHub for another ${waitMinutes}min") @@ -218,8 +232,7 @@ object GitHubReleaseClient { if (blockedUntil != null) { blockedUntilMillis = blockedUntil // Record which route earned it: the quota belongs to that IP. - blockedRouteUsedProxy = - runCatching { ArtiTorManager.getInstance().isProxyEnabled() }.getOrNull() + blockedRouteUsedTor = selectedRouteUsesTor() } val message = if (blockedUntil != null) { @@ -265,7 +278,7 @@ object GitHubReleaseClient { // revalidate to a 304 that confirms a release we no longer hold. cachedEtag = response.header("ETag") blockedUntilMillis = 0L - blockedRouteUsedProxy = null + blockedRouteUsedTor = null Result.success(release) } } catch (e: IOException) {