mirror of
https://github.com/permissionlesstech/bitchat-android.git
synced 2026-08-29 07:16:08 +00:00
fix: carry download failures by stable name, not resource id
Codex flagged this on the string-extraction change and it is right. WorkManager keeps failed records in its own database across app updates, and AAPT2 reassigns R.string ids on every build. A failure written by one build and read by the next would resolve its persisted int against a different resource table: wrong string, or NotFoundException, or IllegalFormatException when the placeholder arity no longer matches. The existing zero-check only caught an absent key, not a stale valid one. ApkDownloadFailureReason now names each failure and owns its string, and the boundary carries the enum name. This is the same treatment DownloadPhase.fromKey already gives the phase across the same boundary, including tolerating a name this build no longer has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Moe Hamade
parent
bfb0c82ef9
commit
700e9aa0e5
@ -340,7 +340,7 @@ class ApkDownloadViewModel(application: Application) : AndroidViewModel(applicat
|
||||
*/
|
||||
private fun failureMessage(state: ApkDownloader.DownloadState.Failed): String =
|
||||
getApplication<Application>().getString(
|
||||
state.messageRes,
|
||||
state.reason.messageRes,
|
||||
*state.messageArgs.toTypedArray()
|
||||
)
|
||||
|
||||
|
||||
@ -58,17 +58,56 @@ internal object DefaultApkDownloadSources {
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Why a download failed, and which string says so.
|
||||
*
|
||||
* Crosses a WorkManager `Data` boundary by [name], never by resource id: WorkManager keeps failed
|
||||
* records in its own database across app updates, and AAPT2 reassigns `R.string` ids on every
|
||||
* build, so a persisted id would resolve against the wrong resource table after an update. Same
|
||||
* reasoning as [ApkDownloader.DownloadPhase.fromKey].
|
||||
*/
|
||||
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),
|
||||
InsufficientStorage(R.string.prepare_apk_error_storage_needed),
|
||||
NoSources(R.string.prepare_apk_error_no_sources),
|
||||
TorConnecting(R.string.prepare_apk_error_tor_connecting),
|
||||
NoUsableUrl(R.string.prepare_apk_error_no_url),
|
||||
Unreachable(R.string.prepare_apk_error_unreachable),
|
||||
InsecureRedirect(R.string.prepare_apk_error_insecure_redirect),
|
||||
ResumeRejected(R.string.prepare_apk_error_resume_rejected),
|
||||
Incomplete(R.string.prepare_apk_error_incomplete),
|
||||
InvalidResume(R.string.prepare_apk_error_invalid_resume),
|
||||
UntrustedKey(R.string.prepare_apk_error_untrusted_key),
|
||||
NotUniversal(R.string.prepare_apk_error_not_universal),
|
||||
ApkUnreadable(R.string.prepare_apk_error_apk_unreadable),
|
||||
NotBitchat(R.string.prepare_apk_error_not_bitchat),
|
||||
NoVersion(R.string.prepare_apk_error_no_version),
|
||||
SourceFailed(R.string.prepare_apk_error_source_failed),
|
||||
AllSourcesFailed(R.string.prepare_apk_error_all_sources);
|
||||
|
||||
companion object {
|
||||
/** Work enqueued by an older build may name a reason this build no longer has. */
|
||||
fun fromKey(key: String?): ApkDownloadFailureReason =
|
||||
entries.firstOrNull { it.name == key } ?: Generic
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A host-neutral download failure that tells the worker whether backoff can help.
|
||||
*
|
||||
* [messageRes] and [messageArgs] name what the user should be told without saying it in any
|
||||
* [reason] and [messageArgs] name what the user should be told without saying it in any
|
||||
* particular language. This layer has no Context by design — that is what keeps its tests plain
|
||||
* JUnit — so the ViewModel resolves them. The inherited [message] stays English for logs and
|
||||
* stack traces, and is never shown.
|
||||
*/
|
||||
class ApkDownloadException(
|
||||
message: String,
|
||||
@StringRes val messageRes: Int,
|
||||
val reason: ApkDownloadFailureReason,
|
||||
val messageArgs: List<String> = emptyList(),
|
||||
val retryable: Boolean,
|
||||
val sourceId: String? = null,
|
||||
@ -120,10 +159,10 @@ internal object ApkDownloadHttpErrors {
|
||||
}
|
||||
return ApkDownloadException(
|
||||
message = "${source.id} rate limited: HTTP $code, retryAt=$retryAt",
|
||||
messageRes = if (minutes != null) {
|
||||
R.string.prepare_apk_error_rate_limited_wait
|
||||
reason = if (minutes != null) {
|
||||
ApkDownloadFailureReason.RateLimitedWithWait
|
||||
} else {
|
||||
R.string.prepare_apk_error_rate_limited
|
||||
ApkDownloadFailureReason.RateLimited
|
||||
},
|
||||
messageArgs = listOfNotNull(source.displayName, minutes?.toString()),
|
||||
retryable = false,
|
||||
@ -136,10 +175,10 @@ internal object ApkDownloadHttpErrors {
|
||||
val retryable = code == 408 || code == 425 || code >= 500
|
||||
return ApkDownloadException(
|
||||
message = "${source.id} failed: HTTP $code $responseMessage",
|
||||
messageRes = if (code == 404) {
|
||||
R.string.prepare_apk_error_no_universal
|
||||
reason = if (code == 404) {
|
||||
ApkDownloadFailureReason.NoUniversalApk
|
||||
} else {
|
||||
R.string.prepare_apk_error_http
|
||||
ApkDownloadFailureReason.HttpFailure
|
||||
},
|
||||
messageArgs = if (code == 404) {
|
||||
listOf(source.displayName)
|
||||
|
||||
@ -36,7 +36,7 @@ class ApkDownloadWorker(
|
||||
const val KEY_PHASE = "phase"
|
||||
const val KEY_VERSION = "version"
|
||||
const val KEY_SIZE_MB = "size_mb"
|
||||
const val KEY_ERROR_RES = "error_res"
|
||||
const val KEY_ERROR_REASON = "error_reason"
|
||||
const val KEY_ERROR_ARGS = "error_args"
|
||||
const val KEY_RESUMABLE_PERCENT = "resumable_percent"
|
||||
|
||||
@ -109,9 +109,11 @@ class ApkDownloadWorker(
|
||||
// generic one rather than leaking an untranslated exception string to the user.
|
||||
val failure = error as? ApkDownloadException
|
||||
val outputData = Data.Builder()
|
||||
.putInt(
|
||||
KEY_ERROR_RES,
|
||||
failure?.messageRes ?: R.string.prepare_apk_error_generic
|
||||
// The reason's name, never its resource id: this record can outlive the build
|
||||
// that wrote it, and resource ids are reassigned on every build.
|
||||
.putString(
|
||||
KEY_ERROR_REASON,
|
||||
(failure?.reason ?: ApkDownloadFailureReason.Generic).name
|
||||
)
|
||||
.putStringArray(
|
||||
KEY_ERROR_ARGS,
|
||||
|
||||
@ -1,6 +1,5 @@
|
||||
package com.bitchat.android.util
|
||||
|
||||
import androidx.annotation.StringRes
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
|
||||
/**
|
||||
@ -36,12 +35,12 @@ interface ApkDownloader {
|
||||
) : DownloadState()
|
||||
data class Success(val version: String, val sizeMB: Int) : DownloadState()
|
||||
/**
|
||||
* [messageRes] and [messageArgs] are resolved by the ViewModel, which has a Context.
|
||||
* Carrying the ids rather than formatted text keeps the failure localizable all the way
|
||||
* across the WorkManager boundary.
|
||||
* [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.
|
||||
*/
|
||||
data class Failed(
|
||||
@StringRes val messageRes: Int,
|
||||
val reason: ApkDownloadFailureReason,
|
||||
val messageArgs: List<String>,
|
||||
val resumablePercent: Int?
|
||||
) : DownloadState()
|
||||
|
||||
@ -4,8 +4,6 @@ import android.content.Context
|
||||
import android.content.pm.PackageManager
|
||||
import android.os.Build
|
||||
import android.util.Log
|
||||
import androidx.annotation.StringRes
|
||||
import com.bitchat.android.R
|
||||
import com.bitchat.android.BuildConfig
|
||||
import com.bitchat.android.net.ArtiTorManager
|
||||
import com.bitchat.android.net.OkHttpProvider
|
||||
@ -168,7 +166,7 @@ class UniversalApkManager(
|
||||
Log.e(TAG, error)
|
||||
throw ApkDownloadException(
|
||||
message = error,
|
||||
messageRes = R.string.prepare_apk_error_storage_needed,
|
||||
reason = ApkDownloadFailureReason.InsufficientStorage,
|
||||
messageArgs = listOf(requiredMB.toString(), availableMB.toString()),
|
||||
retryable = false
|
||||
)
|
||||
@ -187,7 +185,7 @@ class UniversalApkManager(
|
||||
return@withContext Result.failure(
|
||||
ApkDownloadException(
|
||||
message = "No APK download sources are configured.",
|
||||
messageRes = R.string.prepare_apk_error_no_sources,
|
||||
reason = ApkDownloadFailureReason.NoSources,
|
||||
retryable = false
|
||||
)
|
||||
)
|
||||
@ -199,7 +197,7 @@ class UniversalApkManager(
|
||||
return@withContext Result.failure(
|
||||
ApkDownloadException(
|
||||
message = "Tor is still connecting.",
|
||||
messageRes = R.string.prepare_apk_error_tor_connecting,
|
||||
reason = ApkDownloadFailureReason.TorConnecting,
|
||||
retryable = true
|
||||
)
|
||||
)
|
||||
@ -341,7 +339,7 @@ class UniversalApkManager(
|
||||
throw lastFailure
|
||||
?: ApkDownloadException(
|
||||
message = "${source.id} has no usable APK URL.",
|
||||
messageRes = R.string.prepare_apk_error_no_url,
|
||||
reason = ApkDownloadFailureReason.NoUsableUrl,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = false
|
||||
)
|
||||
@ -409,7 +407,7 @@ class UniversalApkManager(
|
||||
ApkDownloadException(
|
||||
message = "${source.id} could not be reached" +
|
||||
(e.message?.let { ": $it" } ?: "."),
|
||||
messageRes = R.string.prepare_apk_error_unreachable,
|
||||
reason = ApkDownloadFailureReason.Unreachable,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = true,
|
||||
sourceId = source.id,
|
||||
@ -424,7 +422,7 @@ class UniversalApkManager(
|
||||
if (!response.request.url.isHttps) {
|
||||
throw ApkDownloadException(
|
||||
message = "${source.id} redirected to an insecure URL.",
|
||||
messageRes = R.string.prepare_apk_error_insecure_redirect,
|
||||
reason = ApkDownloadFailureReason.InsecureRedirect,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = false,
|
||||
sourceId = source.id
|
||||
@ -441,7 +439,7 @@ class UniversalApkManager(
|
||||
clearPartialDownload()
|
||||
throw ApkDownloadException(
|
||||
message = "${source.id} rejected the saved download position.",
|
||||
messageRes = R.string.prepare_apk_error_resume_rejected,
|
||||
reason = ApkDownloadFailureReason.ResumeRejected,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = true,
|
||||
sourceId = source.id,
|
||||
@ -536,7 +534,7 @@ class UniversalApkManager(
|
||||
if (tempFile.length() > expectedSize) clearPartialDownload()
|
||||
throw ApkDownloadException(
|
||||
message = "${source.id} download ended before all bytes arrived.",
|
||||
messageRes = R.string.prepare_apk_error_incomplete,
|
||||
reason = ApkDownloadFailureReason.Incomplete,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = true,
|
||||
sourceId = source.id
|
||||
@ -562,7 +560,7 @@ class UniversalApkManager(
|
||||
progressFile.delete()
|
||||
return ApkDownloadException(
|
||||
message = "${source.id} returned an invalid resume response.",
|
||||
messageRes = R.string.prepare_apk_error_invalid_resume,
|
||||
reason = ApkDownloadFailureReason.InvalidResume,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = true,
|
||||
sourceId = source.id
|
||||
@ -574,7 +572,7 @@ class UniversalApkManager(
|
||||
clearPartialDownload()
|
||||
throw ApkDownloadException(
|
||||
message = "APK from ${source.id} is not signed by a trusted BitChat release key.",
|
||||
messageRes = R.string.prepare_apk_error_untrusted_key,
|
||||
reason = ApkDownloadFailureReason.UntrustedKey,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = false,
|
||||
sourceId = source.id
|
||||
@ -584,7 +582,7 @@ class UniversalApkManager(
|
||||
clearPartialDownload()
|
||||
throw ApkDownloadException(
|
||||
message = "${source.id} returned an architecture-specific APK.",
|
||||
messageRes = R.string.prepare_apk_error_not_universal,
|
||||
reason = ApkDownloadFailureReason.NotUniversal,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = false,
|
||||
sourceId = source.id
|
||||
@ -594,20 +592,23 @@ class UniversalApkManager(
|
||||
|
||||
private fun downloadedVersionName(apkFile: File): String {
|
||||
val packageInfo = context.packageManager.getPackageArchiveInfo(apkFile.absolutePath, 0)
|
||||
?: invalidDownloadedApk(R.string.prepare_apk_error_apk_unreadable, "unreadable APK")
|
||||
?: invalidDownloadedApk(ApkDownloadFailureReason.ApkUnreadable, "unreadable APK")
|
||||
if (packageInfo.packageName != context.packageName) {
|
||||
invalidDownloadedApk(R.string.prepare_apk_error_not_bitchat, "wrong package")
|
||||
invalidDownloadedApk(ApkDownloadFailureReason.NotBitchat, "wrong package")
|
||||
}
|
||||
return packageInfo.versionName
|
||||
?.takeIf { it.isNotBlank() }
|
||||
?: invalidDownloadedApk(R.string.prepare_apk_error_no_version, "no version name")
|
||||
?: invalidDownloadedApk(ApkDownloadFailureReason.NoVersion, "no version name")
|
||||
}
|
||||
|
||||
private fun invalidDownloadedApk(@StringRes messageRes: Int, logReason: String): Nothing {
|
||||
private fun invalidDownloadedApk(
|
||||
reason: ApkDownloadFailureReason,
|
||||
logReason: String
|
||||
): Nothing {
|
||||
clearPartialDownload()
|
||||
throw ApkDownloadException(
|
||||
message = "Downloaded APK rejected: $logReason",
|
||||
messageRes = messageRes,
|
||||
reason = reason,
|
||||
retryable = false
|
||||
)
|
||||
}
|
||||
@ -616,7 +617,7 @@ class UniversalApkManager(
|
||||
if (this is ApkDownloadException) return this
|
||||
return ApkDownloadException(
|
||||
message = "${source.id} download failed" + (message?.let { ": $it" } ?: "."),
|
||||
messageRes = R.string.prepare_apk_error_source_failed,
|
||||
reason = ApkDownloadFailureReason.SourceFailed,
|
||||
messageArgs = listOf(source.displayName),
|
||||
retryable = this is IOException,
|
||||
sourceId = source.id,
|
||||
@ -631,7 +632,7 @@ class UniversalApkManager(
|
||||
if (failures.isEmpty()) {
|
||||
return ApkDownloadException(
|
||||
message = "APK download failed with no recorded source failure.",
|
||||
messageRes = R.string.prepare_apk_error_generic,
|
||||
reason = ApkDownloadFailureReason.Generic,
|
||||
retryable = false
|
||||
)
|
||||
}
|
||||
@ -640,7 +641,7 @@ class UniversalApkManager(
|
||||
return ApkDownloadException(
|
||||
message = "All configured APK sources failed: " +
|
||||
failures.joinToString(" • ") { it.message ?: "Unknown error" },
|
||||
messageRes = R.string.prepare_apk_error_all_sources,
|
||||
reason = ApkDownloadFailureReason.AllSourcesFailed,
|
||||
retryable = failures.any { it.retryable },
|
||||
cause = failures.last()
|
||||
)
|
||||
|
||||
@ -3,7 +3,6 @@ package com.bitchat.android.util
|
||||
import android.content.Context
|
||||
import androidx.work.Constraints
|
||||
import androidx.work.BackoffPolicy
|
||||
import com.bitchat.android.R
|
||||
import androidx.work.ExistingWorkPolicy
|
||||
import androidx.work.NetworkType
|
||||
import androidx.work.OneTimeWorkRequestBuilder
|
||||
@ -79,19 +78,17 @@ class WorkManagerApkDownloader(context: Context) : ApkDownloader {
|
||||
ApkDownloader.DownloadState.Success(version, sizeMB)
|
||||
}
|
||||
WorkInfo.State.FAILED -> {
|
||||
// Work enqueued by an older build carries no resource id; fall back rather than
|
||||
// resolve 0 and crash.
|
||||
val messageRes = workInfo.outputData
|
||||
.getInt(ApkDownloadWorker.KEY_ERROR_RES, 0)
|
||||
.takeIf { it != 0 }
|
||||
?: R.string.prepare_apk_error_generic
|
||||
// Tolerates a missing or retired reason from an older build's record.
|
||||
val reason = ApkDownloadFailureReason.fromKey(
|
||||
workInfo.outputData.getString(ApkDownloadWorker.KEY_ERROR_REASON)
|
||||
)
|
||||
val args = workInfo.outputData
|
||||
.getStringArray(ApkDownloadWorker.KEY_ERROR_ARGS)
|
||||
?.toList()
|
||||
.orEmpty()
|
||||
val resumable = workInfo.outputData.getInt(ApkDownloadWorker.KEY_RESUMABLE_PERCENT, -1)
|
||||
ApkDownloader.DownloadState.Failed(
|
||||
messageRes = messageRes,
|
||||
reason = reason,
|
||||
messageArgs = args,
|
||||
resumablePercent = if (resumable >= 0) resumable else null
|
||||
)
|
||||
@ -100,7 +97,7 @@ class WorkManagerApkDownloader(context: Context) : ApkDownloader {
|
||||
val partial = apkManager.getPartialDownloadProgress()
|
||||
if (partial != null) {
|
||||
ApkDownloader.DownloadState.Failed(
|
||||
messageRes = R.string.prepare_apk_download_cancelled,
|
||||
reason = ApkDownloadFailureReason.Cancelled,
|
||||
messageArgs = emptyList(),
|
||||
resumablePercent = partial
|
||||
)
|
||||
|
||||
@ -1,6 +1,5 @@
|
||||
package com.bitchat.android.util
|
||||
|
||||
import com.bitchat.android.R
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNull
|
||||
@ -63,7 +62,7 @@ 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(R.string.prepare_apk_error_rate_limited_wait, failure.messageRes)
|
||||
assertEquals(ApkDownloadFailureReason.RateLimitedWithWait, failure.reason)
|
||||
assertEquals(listOf(source.displayName, "2"), failure.messageArgs)
|
||||
}
|
||||
|
||||
@ -89,13 +88,13 @@ class ApkDownloadSourceTest {
|
||||
)
|
||||
|
||||
assertNull(permissionsFailure.retryAtMillis)
|
||||
assertEquals(R.string.prepare_apk_error_http, permissionsFailure.messageRes)
|
||||
assertEquals(ApkDownloadFailureReason.HttpFailure, permissionsFailure.reason)
|
||||
assertEquals(
|
||||
listOf(source.displayName, "403", "Forbidden"),
|
||||
permissionsFailure.messageArgs
|
||||
)
|
||||
assertEquals(now + 300_000L, quotaFailure.retryAtMillis)
|
||||
assertEquals(R.string.prepare_apk_error_rate_limited_wait, quotaFailure.messageRes)
|
||||
assertEquals(ApkDownloadFailureReason.RateLimitedWithWait, quotaFailure.reason)
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -141,13 +140,35 @@ class ApkDownloadSourceTest {
|
||||
runAttemptCount = 0,
|
||||
ApkDownloadException(
|
||||
message = "invalid APK",
|
||||
messageRes = R.string.prepare_apk_error_generic,
|
||||
reason = ApkDownloadFailureReason.Generic,
|
||||
retryable = false
|
||||
)
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a failure reason survives the round trip through its key`() {
|
||||
// WorkManager keeps failed records across app updates, so the key written by one build is
|
||||
// read by the next. Resource ids are reassigned per build and would resolve to the wrong
|
||||
// string; the name does not move.
|
||||
ApkDownloadFailureReason.entries.forEach { reason ->
|
||||
assertEquals(reason, ApkDownloadFailureReason.fromKey(reason.name))
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an absent or retired reason falls back instead of resolving nothing`() {
|
||||
assertEquals(
|
||||
ApkDownloadFailureReason.Generic,
|
||||
ApkDownloadFailureReason.fromKey(null)
|
||||
)
|
||||
assertEquals(
|
||||
ApkDownloadFailureReason.Generic,
|
||||
ApkDownloadFailureReason.fromKey("ReasonFromAFutureBuild")
|
||||
)
|
||||
}
|
||||
|
||||
private fun httpError(code: Int): ApkDownloadException {
|
||||
return ApkDownloadHttpErrors.fromResponse(
|
||||
source = source,
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user