From 700e9aa0e50487071209520ff26cc61b9241b465 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Fri, 31 Jul 2026 03:05:22 +0300 Subject: [PATCH] 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) --- .../android/ui/ApkDownloadViewModel.kt | 2 +- .../bitchat/android/util/ApkDownloadSource.kt | 55 ++++++++++++++++--- .../bitchat/android/util/ApkDownloadWorker.kt | 10 ++-- .../com/bitchat/android/util/ApkDownloader.kt | 9 ++- .../android/util/UniversalApkManager.kt | 43 ++++++++------- .../android/util/WorkManagerApkDownloader.kt | 15 ++--- .../android/util/ApkDownloadSourceTest.kt | 31 +++++++++-- 7 files changed, 112 insertions(+), 53 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt b/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt index 29b066c1..ed8de52a 100644 --- a/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ApkDownloadViewModel.kt @@ -340,7 +340,7 @@ class ApkDownloadViewModel(application: Application) : AndroidViewModel(applicat */ private fun failureMessage(state: ApkDownloader.DownloadState.Failed): String = getApplication().getString( - state.messageRes, + state.reason.messageRes, *state.messageArgs.toTypedArray() ) 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 f8955e60..98e4ca1f 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloadSource.kt @@ -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 = 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) 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 710ec1c1..68c8c487 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloadWorker.kt @@ -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, 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 615e2357..b3f342c1 100644 --- a/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt +++ b/app/src/main/java/com/bitchat/android/util/ApkDownloader.kt @@ -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, val resumablePercent: Int? ) : DownloadState() 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 a50bfd8f..a20f8419 100644 --- a/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt +++ b/app/src/main/java/com/bitchat/android/util/UniversalApkManager.kt @@ -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() ) 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 0b3e597b..214b8118 100644 --- a/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt +++ b/app/src/main/java/com/bitchat/android/util/WorkManagerApkDownloader.kt @@ -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 ) 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 8caa9c61..db448d59 100644 --- a/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/util/ApkDownloadSourceTest.kt @@ -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,