mirror of
https://github.com/permissionlesstech/bitchat-android.git
synced 2026-08-15 06:56:30 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
Moe Hamade
parent
000a8acdb9
commit
7b86bafbac
@ -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 -> {}
|
||||
|
||||
@ -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<String> = 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<Application>().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<Application>().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<Application>().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)
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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)
|
||||
}
|
||||
|
||||
@ -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<String>,
|
||||
val resumablePercent: Int?,
|
||||
val retryAtMillis: Long? = null
|
||||
val resumablePercent: Int?
|
||||
) : DownloadState()
|
||||
}
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
@ -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 -> {
|
||||
|
||||
@ -274,10 +274,10 @@
|
||||
<string name="prepare_apk_error_network">Network error. Check your connection.</string>
|
||||
<string name="prepare_apk_error_storage">Not enough storage space.</string>
|
||||
<!--
|
||||
Download failures. These are named in the util layer, which has no Context, and resolved by
|
||||
the ViewModel. A short "min" unit keeps the wait quantity-neutral so no plural form is needed.
|
||||
Download failures. These are named in the util layer, which has no Context, and resolved at
|
||||
the presentation boundary: the row resolves its own status text, the ViewModel resolves the
|
||||
toast shown when an APK is still shareable.
|
||||
-->
|
||||
<string name="prepare_apk_error_rate_limited_wait">%1$s is temporarily rate limited. Try again in %2$s min.</string>
|
||||
<string name="prepare_apk_error_rate_limited">%1$s is temporarily rate limited. Try again later.</string>
|
||||
<string name="prepare_apk_error_no_universal">%1$s does not currently have a universal APK.</string>
|
||||
<string name="prepare_apk_error_http">%1$s download failed: HTTP %2$s %3$s</string>
|
||||
|
||||
@ -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<GitHubReleaseClient.ReleaseSnapshot> =
|
||||
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<UniversalApkManager>()
|
||||
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<UniversalApkManager>()
|
||||
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<String>(), failure.messageArgs)
|
||||
}
|
||||
|
||||
private fun offlineMetadata() = object : LatestReleaseProvider {
|
||||
override suspend fun latestRelease(): Result<GitHubReleaseClient.ReleaseSnapshot> =
|
||||
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")
|
||||
|
||||
@ -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
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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()
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user