From 7b86bafbac35906aa8c6d257699d7f2fdbdef2d3 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Sun, 9 Aug 2026 14:57:32 +0300 Subject: [PATCH] fix(apk): give the persisted store sole ownership of rate-limit cooldowns The cooldown was tracked in two places. ApkRateLimitStore persisted it per scope and per route, and the ViewModel kept a second copy in downloadRetryAtMillis plus a retryAtMillis on Resumable and Error, frozen into WorkManager output data on the way through. The copy was the weaker of the two: it lived in memory, so a cold start lost it, and the deadline it froze belonged to whichever route earned it, which is exactly the drift the per-route store exists to prevent. It also could not expire on its own. downloadRetryBlocked was computed with System.currentTimeMillis() during composition, so nothing recomposed when the deadline passed; scheduleRetryUnlock papered over that with a viewModelScope delay that died with the process. Meanwhile the disabled row and icon gave the user no countdown to read, so a tap simply did nothing. Drops the copy. The store is consulted where the request is actually made and the UI stays enabled, which costs a worker that fails in well under a tenth of a second without touching the network. RateLimitedWithWait goes with it. Its "try again in %2$s min" was computed at failure time and baked into static text that never ticked down, so it was wrong within a minute; RateLimited says "try again later" and stays true. Removing it leaves nothing pre-formatted, so Resumable and Error now carry an ApkFailureMessage of string id plus arguments and the row resolves it during composition. Failure text follows the device locale rather than the locale the worker happened to run under. Anchors the GitHub cooldown at the moment it is judged. now was sampled before awaitRoute(), which can hold a request for the full 60-second route timeout, so a relative Retry-After interpreted against it could land in the past and let the very next check reach GitHub - the loop this branch set out to close. Reads the clock again once the route is ready and once the response arrives, and uses each where it applies. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/com/bitchat/android/ui/AboutSheet.kt | 29 ++-- .../android/ui/ApkDownloadViewModel.kt | 131 ++++++++---------- .../bitchat/android/util/ApkDownloadSource.kt | 13 +- .../bitchat/android/util/ApkDownloadWorker.kt | 5 +- .../com/bitchat/android/util/ApkDownloader.kt | 8 +- .../bitchat/android/util/ApkRateLimitStore.kt | 11 +- .../android/util/GitHubReleaseClient.kt | 28 ++-- .../android/util/UniversalApkManager.kt | 8 +- .../android/util/WorkManagerApkDownloader.kt | 6 +- app/src/main/res/values/strings.xml | 6 +- .../android/ui/ApkDownloadViewModelTest.kt | 87 ++++++++++-- .../android/ui/PrepareRowTapActionTest.kt | 43 ++---- .../android/util/ApkDownloadSourceTest.kt | 9 +- .../android/util/GitHubReleaseClientTest.kt | 93 ++++++++++++- 14 files changed, 289 insertions(+), 188 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/ui/AboutSheet.kt b/app/src/main/java/com/bitchat/android/ui/AboutSheet.kt index 5db34876..54ab0318 100644 --- a/app/src/main/java/com/bitchat/android/ui/AboutSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/AboutSheet.kt @@ -630,8 +630,6 @@ fun AboutSheet( } val availableUpdate = (releaseStatus as? ApkReleaseStatus.Known) ?.takeIf { it.isNewerThanSharedApk } - val downloadRetryBlocked = apkUiState.downloadRetryAtMillis - ?.let { it > System.currentTimeMillis() } == true // Handle one-shot effects (navigation, toasts, share intents) LaunchedEffect(Unit) { @@ -672,8 +670,7 @@ fun AboutSheet( .clickable( enabled = prepareRowTapAction( apkStatus, - releaseStatus, - apkUiState.downloadRetryAtMillis + releaseStatus ) != null ) { apkViewModel.onEvent(ApkUiEvent.PrepareRowClicked) @@ -734,9 +731,7 @@ fun AboutSheet( modifier = Modifier .padding(start = 6.dp) .size(18.dp) - .clickable( - enabled = !downloadRetryBlocked - ) { + .clickable { apkViewModel.onEvent( ApkUiEvent.DownloadUniversalClicked ) @@ -748,7 +743,10 @@ fun AboutSheet( Text( text = when (val status = apkStatus) { is ApkPreparationStatus.Loading -> stringResource(R.string.checking) - is ApkPreparationStatus.NotDownloaded -> stringResource(R.string.prepare_apk_status_not_downloaded) + is ApkPreparationStatus.NotDownloaded -> + stringResource( + R.string.prepare_apk_status_not_downloaded + ) is ApkPreparationStatus.Ready -> { val source = when { status.source == UniversalApkManager.ApkSource.DOWNLOADED -> @@ -777,10 +775,15 @@ fun AboutSheet( is ApkPreparationStatus.Resumable -> stringResource( R.string.prepare_apk_status_resumable, - status.message, + context.resolveApkFailureMessage( + status.failure + ), status.progressPercent ) - is ApkPreparationStatus.Error -> status.message + is ApkPreparationStatus.Error -> + context.resolveApkFailureMessage( + status.failure + ) }, style = MaterialTheme.typography.bodySmall, color = when (apkStatus) { @@ -830,7 +833,6 @@ fun AboutSheet( ApkUiEvent.DownloadUniversalClicked ) }, - enabled = !downloadRetryBlocked, tint = colorScheme.primary ) } else if ( @@ -863,11 +865,6 @@ fun AboutSheet( ApkUiEvent.PrepareRowClicked ) }, - enabled = prepareRowTapAction( - apkStatus, - releaseStatus, - apkUiState.downloadRetryAtMillis - ) != null, tint = colorScheme.primary ) else -> {} 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 a76b8ecb..b076477f 100644 --- a/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt @@ -1,7 +1,9 @@ package com.bitchat.android.ui import android.app.Application +import android.content.Context import android.util.Log +import androidx.annotation.StringRes import androidx.core.content.FileProvider import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope @@ -16,7 +18,6 @@ import com.bitchat.android.util.WorkManagerApkDownloader import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.channels.Channel -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -43,10 +44,31 @@ sealed class ApkPreparationStatus { ) : ApkPreparationStatus() data class Resumable( val progressPercent: Int, - val message: String, - val retryAtMillis: Long? = null + val failure: ApkFailureMessage ) : ApkPreparationStatus() - data class Error(val message: String, val retryAtMillis: Long? = null) : ApkPreparationStatus() + data class Error(val failure: ApkFailureMessage) : ApkPreparationStatus() +} + +/** A localizable failure kept as data until the UI or a one-shot effect renders it. */ +data class ApkFailureMessage( + @StringRes val messageRes: Int, + val messageArgs: List = emptyList() +) + +/** + * Resolves a failure defensively. The reason and its arguments cross the WorkManager boundary + * independently, so an argument list that does not match the format string is possible; a row + * showing generic text beats one that throws while formatting. + */ +internal fun Context.resolveApkFailureMessage(failure: ApkFailureMessage): String { + return runCatching { + getString( + failure.messageRes, + *failure.messageArgs.toTypedArray() + ) + }.getOrElse { + getString(R.string.prepare_apk_error_generic) + } } sealed class ApkReleaseStatus { @@ -63,7 +85,6 @@ sealed class ApkReleaseStatus { data class ApkUiState( val apkStatus: ApkPreparationStatus = ApkPreparationStatus.Loading, val releaseStatus: ApkReleaseStatus = ApkReleaseStatus.Unknown, - val downloadRetryAtMillis: Long? = null, val downloadProgress: Int = 0, val showPrepareDialog: Boolean = false, val showDeleteDialog: Boolean = false, @@ -105,19 +126,12 @@ internal enum class PrepareRowTapAction { */ internal fun prepareRowTapAction( status: ApkPreparationStatus, - releaseStatus: ApkReleaseStatus = ApkReleaseStatus.Unknown, - downloadRetryAtMillis: Long? = null, - nowMillis: Long = System.currentTimeMillis() + releaseStatus: ApkReleaseStatus = ApkReleaseStatus.Unknown ): PrepareRowTapAction? = when { - downloadRetryAtMillis != null && downloadRetryAtMillis > nowMillis -> null status is ApkPreparationStatus.NotDownloaded -> PrepareRowTapAction.OpenPrepareDialog // Consent was already given for these; resuming straight away avoids a redundant prompt. - status is ApkPreparationStatus.Resumable && - (status.retryAtMillis == null || status.retryAtMillis <= nowMillis) -> - PrepareRowTapAction.StartDownload - status is ApkPreparationStatus.Error && - (status.retryAtMillis == null || status.retryAtMillis <= nowMillis) -> - PrepareRowTapAction.StartDownload + status is ApkPreparationStatus.Resumable -> PrepareRowTapAction.StartDownload + status is ApkPreparationStatus.Error -> PrepareRowTapAction.StartDownload status is ApkPreparationStatus.Ready && (status.source == UniversalApkManager.ApkSource.INSTALLED || (releaseStatus as? ApkReleaseStatus.Known)?.isNewerThanSharedApk == true) -> @@ -162,7 +176,6 @@ class ApkDownloadViewModel internal constructor( val effect = _effect.receiveAsFlow() private var metadataRefreshJob: Job? = null - private var retryUnlockJob: Job? = null init { observeDownloader() @@ -190,8 +203,7 @@ class ApkDownloadViewModel internal constructor( when ( prepareRowTapAction( _state.value.apkStatus, - _state.value.releaseStatus, - _state.value.downloadRetryAtMillis + _state.value.releaseStatus ) ) { PrepareRowTapAction.OpenPrepareDialog -> @@ -207,9 +219,6 @@ class ApkDownloadViewModel internal constructor( } private fun onDownloadUniversalClicked() { - if (_state.value.downloadRetryAtMillis?.let { it > System.currentTimeMillis() } == true) { - return - } val status = _state.value.apkStatus val hasUpdate = (_state.value.releaseStatus as? ApkReleaseStatus.Known) ?.isNewerThanSharedApk == true @@ -283,14 +292,6 @@ class ApkDownloadViewModel internal constructor( private fun startDownload() { val current = _state.value.apkStatus - val stateRetryAt = _state.value.downloadRetryAtMillis - val retryAt = when (current) { - is ApkPreparationStatus.Resumable -> current.retryAtMillis - is ApkPreparationStatus.Error -> current.retryAtMillis - else -> null - } - if ((stateRetryAt ?: retryAt)?.let { it > System.currentTimeMillis() } == true) return - val fallback = when (current) { is ApkPreparationStatus.Ready -> current is ApkPreparationStatus.Downloading -> current.shareableFallback @@ -302,7 +303,6 @@ class ApkDownloadViewModel internal constructor( apkStatus = ApkPreparationStatus.Downloading( shareableFallback = fallback ), - downloadRetryAtMillis = null, downloadProgress = partial ?: 0 ) } @@ -325,7 +325,10 @@ class ApkDownloadViewModel internal constructor( if (current.apkStatus is ApkPreparationStatus.Downloading) { current } else { - current.copy(apkStatus = resolvedStatus, downloadProgress = 0) + current.copy( + apkStatus = resolvedStatus, + downloadProgress = 0 + ) } } // Local availability is resolved and published before this independent network task @@ -406,12 +409,12 @@ class ApkDownloadViewModel internal constructor( it.copy( apkStatus = ready, releaseStatus = releaseStatusFor(ready, it.releaseStatus), - downloadRetryAtMillis = null, downloadProgress = 100 ) } } is ApkDownloader.DownloadState.Failed -> { + val failure = downloadState.toFailureMessage() val fallback = (_state.value.apkStatus as? ApkPreparationStatus.Downloading) ?.shareableFallback ?: apkManager.getCachedApkInfo()?.toReady() @@ -420,36 +423,31 @@ class ApkDownloadViewModel internal constructor( it.copy( apkStatus = fallback, releaseStatus = releaseStatusFor(fallback, it.releaseStatus), - downloadRetryAtMillis = downloadState.retryAtMillis, downloadProgress = 0 ) } - _effect.send(ApkUiEffect.ShowToast(failureMessage(downloadState))) + _effect.send( + ApkUiEffect.ShowToast( + getApplication().resolveApkFailureMessage( + failure + ) + ) + ) } else { - val message = failureMessage(downloadState) _state.update { if (downloadState.resumablePercent != null) { it.copy( apkStatus = ApkPreparationStatus.Resumable( progressPercent = downloadState.resumablePercent, - message = message, - retryAtMillis = downloadState.retryAtMillis + failure = failure ), - downloadRetryAtMillis = downloadState.retryAtMillis, downloadProgress = downloadState.resumablePercent ) } else { - it.copy( - apkStatus = ApkPreparationStatus.Error( - message, - downloadState.retryAtMillis - ), - downloadRetryAtMillis = downloadState.retryAtMillis - ) + it.copy(apkStatus = ApkPreparationStatus.Error(failure)) } } } - scheduleRetryUnlock(downloadState.retryAtMillis) } } } @@ -466,35 +464,10 @@ class ApkDownloadViewModel internal constructor( return getApplication().getString(resId) } - /** - * The single place a download failure turns into words. The downloader names the failure and - * this resolves it, so the message follows the device locale rather than the worker's. - */ - private fun failureMessage(state: ApkDownloader.DownloadState.Failed): String = - runCatching { - getApplication().getString( - state.reason.messageRes, - *state.messageArgs.toTypedArray() - ) - }.getOrElse { - getString(R.string.prepare_apk_error_generic) - } - - private fun scheduleRetryUnlock(retryAtMillis: Long?) { - if (retryAtMillis == null) return - retryUnlockJob?.cancel() - retryUnlockJob = viewModelScope.launch { - delay((retryAtMillis - System.currentTimeMillis()).coerceAtLeast(0L)) - _state.update { - val unlocked = when (val status = it.apkStatus) { - is ApkPreparationStatus.Resumable -> status.copy(retryAtMillis = null) - is ApkPreparationStatus.Error -> status.copy(retryAtMillis = null) - else -> status - } - it.copy(apkStatus = unlocked, downloadRetryAtMillis = null) - } - } - } + private fun ApkDownloader.DownloadState.Failed.toFailureMessage() = ApkFailureMessage( + messageRes = reason.messageRes, + messageArgs = messageArgs + ) private fun shareableReady(status: ApkPreparationStatus): ApkPreparationStatus.Ready? = when (status) { @@ -527,7 +500,9 @@ class ApkDownloadViewModel internal constructor( if (partial != null) { ApkPreparationStatus.Resumable( progressPercent = partial, - message = getString(R.string.prepare_apk_download_interrupted) + failure = ApkFailureMessage( + messageRes = R.string.prepare_apk_download_interrupted + ) ) } else { ApkPreparationStatus.NotDownloaded @@ -536,7 +511,9 @@ class ApkDownloadViewModel internal constructor( } catch (e: Exception) { // The exception text is English and often internal; log it, show a translated line. Log.e(TAG, "Error reading APK status", e) - ApkPreparationStatus.Error(getString(R.string.share_apk_error)) + ApkPreparationStatus.Error( + ApkFailureMessage(messageRes = R.string.share_apk_error) + ) } } } diff --git a/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt b/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt index cf0f5e90..73f168ba 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt @@ -8,7 +8,6 @@ import java.io.IOException import java.time.Instant import java.time.ZonedDateTime import java.time.format.DateTimeFormatter -import kotlin.math.ceil /** * A trusted location that serves the latest signed universal BitChat APK. @@ -71,7 +70,6 @@ internal object DefaultApkDownloadSources { enum class ApkDownloadFailureReason(@StringRes val messageRes: Int) { Generic(R.string.prepare_apk_error_generic), Cancelled(R.string.prepare_apk_download_cancelled), - RateLimitedWithWait(R.string.prepare_apk_error_rate_limited_wait), RateLimited(R.string.prepare_apk_error_rate_limited), NoUniversalApk(R.string.prepare_apk_error_no_universal), HttpFailure(R.string.prepare_apk_error_http), @@ -156,17 +154,10 @@ internal object ApkDownloadHttpErrors { (code == 403 && (rateLimitRemaining?.trim() == "0" || retryAt != null)) if (rateLimited) { - val minutes = retryAt?.let { deadline -> - ceil((deadline - nowMillis).coerceAtLeast(1L) / 60_000.0).toLong() - } return ApkDownloadException( message = "${source.id} rate limited: HTTP $code, retryAt=$retryAt", - reason = if (minutes != null) { - ApkDownloadFailureReason.RateLimitedWithWait - } else { - ApkDownloadFailureReason.RateLimited - }, - messageArgs = listOfNotNull(source.displayName, minutes?.toString()), + reason = ApkDownloadFailureReason.RateLimited, + messageArgs = listOf(source.displayName), retryable = false, sourceId = source.id, httpCode = code, diff --git a/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt b/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt index ade699b5..40edde59 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt @@ -39,7 +39,6 @@ class ApkDownloadWorker( const val KEY_ERROR_REASON = "error_reason" const val KEY_ERROR_ARGS = "error_args" const val KEY_RESUMABLE_PERCENT = "resumable_percent" - const val KEY_RETRY_AT = "retry_at" private const val CHANNEL_ID = "apk_download" private const val NOTIFICATION_ID = 4201 @@ -121,7 +120,9 @@ class ApkDownloadWorker( failure?.messageArgs.orEmpty().toTypedArray() ) .putInt(KEY_RESUMABLE_PERCENT, partial ?: -1) - .apply { failure?.retryAtMillis?.let { putLong(KEY_RETRY_AT, it) } } + // No retry deadline is recorded: a rate-limit cooldown belongs to the route it was + // earned on, and ApkRateLimitStore already keeps it that way. A copy frozen here + // would outlive both the route and the cooldown. .build() Result.failure(outputData) } diff --git a/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt b/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt index 75cd6b14..01be6280 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt @@ -35,15 +35,13 @@ interface ApkDownloader { ) : DownloadState() data class Success(val version: String, val sizeMB: Int) : DownloadState() /** - * [reason] and [messageArgs] are resolved by the ViewModel, which has a Context. - * Carrying the reason rather than formatted text keeps the failure localizable all the - * way across the WorkManager boundary. + * [reason] and [messageArgs] stay structured until the presentation boundary. Carrying + * them instead of formatted text keeps the failure localizable across WorkManager. */ data class Failed( val reason: ApkDownloadFailureReason, val messageArgs: List, - val resumablePercent: Int?, - val retryAtMillis: Long? = null + val resumablePercent: Int? ) : DownloadState() } diff --git a/app/src/main/java/com/bitchat/android/util/ApkRateLimitStore.kt b/app/src/main/java/com/bitchat/android/util/ApkRateLimitStore.kt index 0a0f6a0f..963099b3 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkRateLimitStore.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkRateLimitStore.kt @@ -3,7 +3,6 @@ package com.bitchat.android.util import android.content.Context import androidx.core.content.edit import com.bitchat.android.net.OkHttpProvider -import kotlin.math.ceil /** Persistent, route-specific cooldowns for APK-related network requests. */ internal class ApkRateLimitStore(context: Context) { @@ -52,16 +51,12 @@ internal class ApkRateLimitStore(context: Context) { fun blockedException( source: ApkDownloadSource, - retryAtMillis: Long, - nowMillis: Long = System.currentTimeMillis() + retryAtMillis: Long ): ApkDownloadException { - val minutes = ceil( - (retryAtMillis - nowMillis).coerceAtLeast(1L) / 60_000.0 - ).toLong() return ApkDownloadException( message = "${source.id} is in a persisted rate-limit cooldown until $retryAtMillis", - reason = ApkDownloadFailureReason.RateLimitedWithWait, - messageArgs = listOf(source.displayName, minutes.toString()), + reason = ApkDownloadFailureReason.RateLimited, + messageArgs = listOf(source.displayName), retryable = false, sourceId = source.id, retryAtMillis = retryAtMillis 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 dbb27c5d..d00122d8 100644 --- a/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt +++ b/app/src/main/java/com/bitchat/android/util/GitHubReleaseClient.kt @@ -94,9 +94,16 @@ internal class GitHubReleaseClient( if (!awaitRoute()) return@withLock cached.orRouteFailure() val routeSnapshot = routedClient() - rateLimits.retryAtMillis(RATE_LIMIT_SCOPE, routeSnapshot.route, now)?.let { deadline -> + // Route readiness can take longer than a cooldown. Judge an existing deadline at the + // point where the request can actually start, not with the pre-wait cache timestamp. + val routeReadyNow = nowMillis() + rateLimits.retryAtMillis( + RATE_LIMIT_SCOPE, + routeSnapshot.route, + routeReadyNow + )?.let { deadline -> return@withLock cached.orFailure( - rateLimits.blockedException(SOURCE, deadline, now) + rateLimits.blockedException(SOURCE, deadline) ) } @@ -115,13 +122,17 @@ internal class GitHubReleaseClient( try { client.newCall(request).awaitResponse().use { response -> + // The route wait and the call itself can each take a minute, so `now` is too + // old to interpret a relative Retry-After: anchoring the cooldown there can + // date it into the past and let the very next check reach GitHub. + val responseNow = nowMillis() if (apiUrl.startsWith("https://") && !response.request.url.isHttps) { return@withLock cached.orFailure( IOException("GitHub redirected release metadata to an insecure URL") ) } if (response.code == 304 && cached != null) { - val refreshed = cached.copy(fetchedAtMillis = now) + val refreshed = cached.copy(fetchedAtMillis = responseNow) writeCache(refreshed) rateLimits.clear(RATE_LIMIT_SCOPE, routeSnapshot.route) return@withLock Result.success( @@ -136,19 +147,18 @@ internal class GitHubReleaseClient( retryAfter = response.header("Retry-After"), rateLimitRemaining = response.header("X-RateLimit-Remaining"), rateLimitResetEpochSeconds = response.header("X-RateLimit-Reset"), - nowMillis = now + nowMillis = responseNow ) val persistedFailure = if ( - failure.reason == ApkDownloadFailureReason.RateLimited || - failure.reason == ApkDownloadFailureReason.RateLimitedWithWait + failure.reason == ApkDownloadFailureReason.RateLimited ) { val deadline = rateLimits.recordRateLimit( RATE_LIMIT_SCOPE, routeSnapshot.route, failure.retryAtMillis, - now + responseNow ) - rateLimits.blockedException(SOURCE, deadline, now) + rateLimits.blockedException(SOURCE, deadline) } else { failure } @@ -163,7 +173,7 @@ internal class GitHubReleaseClient( val entry = CachedRelease( release = release, etag = response.header("ETag"), - fetchedAtMillis = now + fetchedAtMillis = responseNow ) writeCache(entry) rateLimits.clear(RATE_LIMIT_SCOPE, routeSnapshot.route) 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 df44ab83..8480fc27 100644 --- a/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt +++ b/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt @@ -372,7 +372,7 @@ class UniversalApkManager( val rateLimitScope = "apk_asset_${source.id}" val now = System.currentTimeMillis() rateLimits.retryAtMillis(rateLimitScope, routedClient.route, now)?.let { deadline -> - throw rateLimits.blockedException(source, deadline, now) + throw rateLimits.blockedException(source, deadline) } val request = Request.Builder() @@ -482,9 +482,7 @@ class UniversalApkManager( rateLimitResetEpochSeconds = response.header("X-RateLimit-Reset") ) - if (failure.reason == ApkDownloadFailureReason.RateLimited || - failure.reason == ApkDownloadFailureReason.RateLimitedWithWait - ) { + if (failure.reason == ApkDownloadFailureReason.RateLimited) { val now = System.currentTimeMillis() val deadline = rateLimits.recordRateLimit( rateLimitScope, @@ -492,7 +490,7 @@ class UniversalApkManager( failure.retryAtMillis, now ) - throw rateLimits.blockedException(source, deadline, now) + throw rateLimits.blockedException(source, deadline) } throw failure } diff --git a/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt b/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt index 50bb2b1c..6d482c34 100644 --- a/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt +++ b/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt @@ -92,11 +92,7 @@ class WorkManagerApkDownloader(context: Context) : ApkDownloader { ApkDownloader.DownloadState.Failed( reason = reason, messageArgs = args, - resumablePercent = if (resumable >= 0) resumable else null, - retryAtMillis = workInfo.outputData.getLong( - ApkDownloadWorker.KEY_RETRY_AT, - 0L - ).takeIf { it > 0L } + resumablePercent = if (resumable >= 0) resumable else null ) } WorkInfo.State.CANCELLED -> { diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 18925640..a46d596f 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -274,10 +274,10 @@ Network error. Check your connection. Not enough storage space. - %1$s is temporarily rate limited. Try again in %2$s min. %1$s is temporarily rate limited. Try again later. %1$s does not currently have a universal APK. %1$s download failed: HTTP %2$s %3$s diff --git a/app/src/test/kotlin/com/bitchat/android/ui/ApkDownloadViewModelTest.kt b/app/src/test/kotlin/com/bitchat/android/ui/ApkDownloadViewModelTest.kt index 6b4998c7..6e146e6e 100644 --- a/app/src/test/kotlin/com/bitchat/android/ui/ApkDownloadViewModelTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/ui/ApkDownloadViewModelTest.kt @@ -2,7 +2,9 @@ package com.bitchat.android.ui import android.app.Application import androidx.test.core.app.ApplicationProvider +import com.bitchat.android.R import com.bitchat.android.util.ApkDownloader +import com.bitchat.android.util.ApkDownloadFailureReason import com.bitchat.android.util.GitHubReleaseClient import com.bitchat.android.util.LatestReleaseProvider import com.bitchat.android.util.ShareableApkVariant @@ -10,9 +12,9 @@ import com.bitchat.android.util.UniversalApkManager import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.delay import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest @@ -71,11 +73,12 @@ class ApkDownloadViewModelTest { fun `notification cancellation returning idle restores shareable fallback`() = runTest { val manager = managerWithLocalApk() val downloader = FakeDownloader() - val metadata = object : LatestReleaseProvider { - override suspend fun latestRelease(): Result = - Result.failure(IllegalStateException("synthetic offline response")) - } - val viewModel = ApkDownloadViewModel(application, manager, downloader, metadata) + val viewModel = ApkDownloadViewModel( + application, + manager, + downloader, + offlineMetadata() + ) viewModel.onEvent(ApkUiEvent.CheckStatus) val originalReady = awaitReady(viewModel) @@ -87,8 +90,62 @@ class ApkDownloadViewModelTest { assertEquals(1, downloader.startCount) downloader.emit(ApkDownloader.DownloadState.Idle) - val restored = awaitReady(viewModel) - assertEquals(originalReady, restored) + assertEquals(originalReady, awaitReady(viewModel)) + } + + @Test + fun `rate limits remain stable failures without countdowns`() = runTest { + val manager = mock() + whenever(manager.getCachedApkInfo()).thenReturn(null) + val downloader = FakeDownloader() + val viewModel = ApkDownloadViewModel( + application, + manager, + downloader, + offlineMetadata() + ) + + downloader.emit( + ApkDownloader.DownloadState.Failed( + reason = ApkDownloadFailureReason.RateLimited, + messageArgs = listOf("GitHub Releases"), + resumablePercent = null + ) + ) + + val failure = awaitError(viewModel).failure + assertEquals(R.string.prepare_apk_error_rate_limited, failure.messageRes) + assertEquals(listOf("GitHub Releases"), failure.messageArgs) + } + + @Test + fun `non-rate failures keep their own message`() = runTest { + val manager = mock() + whenever(manager.getCachedApkInfo()).thenReturn(null) + val downloader = FakeDownloader() + val viewModel = ApkDownloadViewModel( + application, + manager, + downloader, + offlineMetadata() + ) + + downloader.emit( + ApkDownloader.DownloadState.Failed( + reason = ApkDownloadFailureReason.AllSourcesFailed, + messageArgs = emptyList(), + resumablePercent = null + ) + ) + + val failure = awaitError(viewModel).failure + assertEquals(R.string.prepare_apk_error_all_sources, failure.messageRes) + assertEquals(emptyList(), failure.messageArgs) + } + + private fun offlineMetadata() = object : LatestReleaseProvider { + override suspend fun latestRelease(): Result = + Result.failure(IllegalStateException("synthetic offline response")) } private suspend fun managerWithLocalApk(): UniversalApkManager { @@ -112,7 +169,19 @@ class ApkDownloadViewModelTest { viewModel: ApkDownloadViewModel ): ApkPreparationStatus.Ready = withTimeout(5_000L) { while (true) { - (viewModel.state.value.apkStatus as? ApkPreparationStatus.Ready)?.let { return@withTimeout it } + (viewModel.state.value.apkStatus as? ApkPreparationStatus.Ready) + ?.let { return@withTimeout it } + delay(1L) + } + error("unreachable") + } + + private suspend fun awaitError( + viewModel: ApkDownloadViewModel + ): ApkPreparationStatus.Error = withTimeout(5_000L) { + while (true) { + (viewModel.state.value.apkStatus as? ApkPreparationStatus.Error) + ?.let { return@withTimeout it } delay(1L) } error("unreachable") diff --git a/app/src/test/kotlin/com/bitchat/android/ui/PrepareRowTapActionTest.kt b/app/src/test/kotlin/com/bitchat/android/ui/PrepareRowTapActionTest.kt index d1e3a431..7d8051de 100644 --- a/app/src/test/kotlin/com/bitchat/android/ui/PrepareRowTapActionTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/ui/PrepareRowTapActionTest.kt @@ -1,5 +1,6 @@ package com.bitchat.android.ui +import com.bitchat.android.R import com.bitchat.android.util.ApkDownloader import com.bitchat.android.util.ShareableApkVariant import com.bitchat.android.util.UniversalApkManager @@ -24,6 +25,10 @@ class PrepareRowTapActionTest { variant = variant ) + private fun error() = ApkPreparationStatus.Error( + ApkFailureMessage(messageRes = R.string.prepare_apk_error_generic) + ) + @Test fun `an arm64-only build offers the universal download`() { // The only entry point besides the trailing icon, which has no label to explain itself. @@ -79,11 +84,16 @@ class PrepareRowTapActionTest { // The user already consented to the download; re-prompting would be noise. assertEquals( PrepareRowTapAction.StartDownload, - prepareRowTapAction(ApkPreparationStatus.Resumable(43, "Download interrupted")) + prepareRowTapAction( + ApkPreparationStatus.Resumable( + 43, + ApkFailureMessage(R.string.prepare_apk_download_interrupted) + ) + ) ) assertEquals( PrepareRowTapAction.StartDownload, - prepareRowTapAction(ApkPreparationStatus.Error("Network error")) + prepareRowTapAction(error()) ) } @@ -96,33 +106,4 @@ class PrepareRowTapActionTest { assertNull(prepareRowTapAction(ApkPreparationStatus.Loading)) } - @Test - fun `a persisted rate limit disables manual retry until its deadline`() { - val retryAt = 50_000L - assertNull( - prepareRowTapAction( - ApkPreparationStatus.Error("Rate limited", retryAtMillis = retryAt), - nowMillis = retryAt - 1 - ) - ) - assertEquals( - PrepareRowTapAction.StartDownload, - prepareRowTapAction( - ApkPreparationStatus.Error("Rate limited", retryAtMillis = retryAt), - nowMillis = retryAt - ) - ) - } - - @Test - fun `rate limit also disables optional update while an apk remains shareable`() { - val retryAt = 50_000L - assertNull( - prepareRowTapAction( - ready(ShareableApkVariant.UNIVERSAL), - downloadRetryAtMillis = retryAt, - nowMillis = retryAt - 1 - ) - ) - } } diff --git a/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt b/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt index 0b0eaf03..4d6702d6 100644 --- a/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt @@ -53,7 +53,7 @@ class ApkDownloadSourceTest { } @Test - fun `rate limit response gives the user the advertised retry time`() { + fun `rate limit response retains the server deadline without exposing a countdown`() { val failure = ApkDownloadHttpErrors.fromResponse( source = source, code = 429, @@ -66,9 +66,8 @@ class ApkDownloadSourceTest { assertFalse(failure.retryable) assertEquals(now + 120_000L, failure.retryAtMillis) - // The wait is carried as an argument, not baked into an English sentence. - assertEquals(ApkDownloadFailureReason.RateLimitedWithWait, failure.reason) - assertEquals(listOf(source.displayName, "2"), failure.messageArgs) + assertEquals(ApkDownloadFailureReason.RateLimited, failure.reason) + assertEquals(listOf(source.displayName), failure.messageArgs) } @Test @@ -99,7 +98,7 @@ class ApkDownloadSourceTest { permissionsFailure.messageArgs ) assertEquals(now + 300_000L, quotaFailure.retryAtMillis) - assertEquals(ApkDownloadFailureReason.RateLimitedWithWait, quotaFailure.reason) + assertEquals(ApkDownloadFailureReason.RateLimited, quotaFailure.reason) } @Test diff --git a/app/src/test/kotlin/com/bitchat/android/util/GitHubReleaseClientTest.kt b/app/src/test/kotlin/com/bitchat/android/util/GitHubReleaseClientTest.kt index 399ae3a2..6377752d 100644 --- a/app/src/test/kotlin/com/bitchat/android/util/GitHubReleaseClientTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/util/GitHubReleaseClientTest.kt @@ -23,6 +23,12 @@ class GitHubReleaseClientTest { private var nowMillis = 1_700_000_000_000L private var route = OkHttpProvider.Route.DIRECT + /** How far the clock advances while awaitRoute() waits for Tor to finish bootstrapping. */ + private var routeWaitMillis = 0L + + /** How far the clock advances after the server responds but before the client observes it. */ + private var responseWaitMillis = 0L + @Before fun setUp() { context = ApplicationProvider.getApplicationContext() @@ -100,17 +106,100 @@ class GitHubReleaseClientTest { assertEquals(2, server.requestCount) } + /** + * A Tor cold start can hold the request for the full 60-second route timeout, which is longer + * than the relative delay GitHub asks for. The cooldown has to outlast the wait that preceded + * it, so it is anchored at the response rather than at the start of the attempt. + */ + @Test + fun `a slow route wait does not shorten a relative retry-after cooldown`() = runTest { + routeWaitMillis = 90_000L + server.enqueue( + MockResponse.Builder() + .code(429) + .addHeader("Retry-After", "60") + .build() + ) + val client = client() + + assertTrue(client.latestRelease().isFailure) + routeWaitMillis = 0L + assertTrue(client.latestRelease().isFailure) + + assertEquals(1, server.requestCount) + } + + @Test + fun `a slow route wait does not shorten the header-less fallback cooldown`() = runTest { + routeWaitMillis = 90_000L + server.enqueue(MockResponse.Builder().code(429).build()) + val client = client() + + assertTrue(client.latestRelease().isFailure) + routeWaitMillis = 0L + assertTrue(client.latestRelease().isFailure) + + assertEquals(1, server.requestCount) + } + + @Test + fun `a slow response does not shorten a relative retry-after cooldown`() = runTest { + responseWaitMillis = 90_000L + server.enqueue( + MockResponse.Builder() + .code(429) + .addHeader("Retry-After", "60") + .build() + ) + val client = client() + + assertTrue(client.latestRelease().isFailure) + responseWaitMillis = 0L + assertTrue(client.latestRelease().isFailure) + + assertEquals(1, server.requestCount) + } + + @Test + fun `a cooldown that expires while the route becomes ready does not suppress the request`() = + runTest { + server.enqueue( + MockResponse.Builder() + .code(429) + .addHeader("Retry-After", "60") + .build() + ) + val client = client() + + assertTrue(client.latestRelease().isFailure) + + routeWaitMillis = 90_000L + server.enqueue(successResponse(etag = "release-after-cooldown")) + assertTrue(client.latestRelease().isSuccess) + + assertEquals(2, server.requestCount) + } + private fun client() = GitHubReleaseClient( context = context, apiUrl = server.url("/releases/latest").toString(), nowMillis = { nowMillis }, routedClient = { OkHttpProvider.RoutedClient( - client = OkHttpClient.Builder().build(), + client = OkHttpClient.Builder() + .addInterceptor { chain -> + chain.proceed(chain.request()).also { + nowMillis += responseWaitMillis + } + } + .build(), route = route ) }, - awaitRoute = { true } + awaitRoute = { + nowMillis += routeWaitMillis + true + } ) private fun successResponse(etag: String): MockResponse = MockResponse.Builder()