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()