Harden download routine against redownloads and flaky networks

- enqueue() no longer resets an active/verifying download back to QUEUED, so the periodic update check and repeated taps can't trigger needless re-downloads; when files are already downloaded & verified it installs directly instead of re-running the pipeline. - DownloadWorker only purges partial files on a genuine user cancellation; system-initiated stops (lost connectivity, quota) keep partials and retry, fixing downloads appearing to restart on a flaky network. - Add NetworkType.CONNECTED constraint + exponential backoff and return Result.retry() for transient/network failures (capped) instead of always succeeding. - Validate HTTP 206 before resuming a .tmp (overwrite on 200) and drop corrupt files on verification failure so retries restart clean.
2026-05-30 00:19:33 +05:30
parent d13d50e442
commit ada5a1f899
3 changed files with 156 additions and 22 deletions
--- a/app/src/main/java/com/aurora/store/data/helper/DownloadHelper.kt
+++ b/app/src/main/java/com/aurora/store/data/helper/DownloadHelper.kt
@@ -2,14 +2,19 @@ package com.aurora.store.data.helper

 import android.content.Context
 import android.util.Log
+import androidx.work.BackoffPolicy
+import androidx.work.Constraints
 import androidx.work.Data
 import androidx.work.ExistingWorkPolicy
+import androidx.work.NetworkType
 import androidx.work.OneTimeWorkRequestBuilder
 import androidx.work.OutOfQuotaPolicy
 import androidx.work.WorkManager
+import androidx.work.WorkRequest
 import com.aurora.extensions.TAG
 import com.aurora.gplayapi.data.models.App
 import com.aurora.store.AuroraApp
+import com.aurora.store.data.installer.AppInstaller
 import com.aurora.store.data.model.DownloadStatus
 import com.aurora.store.data.room.download.Download
 import com.aurora.store.data.room.download.DownloadDao
@@ -18,6 +23,7 @@ import com.aurora.store.data.room.update.Update
 import com.aurora.store.data.work.DownloadWorker
 import com.aurora.store.util.PathUtil
 import dagger.hilt.android.qualifiers.ApplicationContext
+import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 import kotlinx.coroutines.flow.SharingStarted
 import kotlinx.coroutines.flow.first
@@ -32,7 +38,8 @@ import kotlinx.coroutines.launch
 */
 class DownloadHelper @Inject constructor(
    @ApplicationContext private val context: Context,
-    private val downloadDao: DownloadDao
+    private val downloadDao: DownloadDao,
+    private val appInstaller: AppInstaller
 ) {

    companion object {
@@ -92,7 +99,7 @@ class DownloadHelper @Inject constructor(
     * @param app [App] to download
     */
    suspend fun enqueueApp(app: App) {
-        downloadDao.insert(Download.fromApp(app))
+        enqueue(Download.fromApp(app))
    }

    /**
@@ -100,7 +107,7 @@ class DownloadHelper @Inject constructor(
     * @param update [Update] to download
     */
    suspend fun enqueueUpdate(update: Update) {
-        downloadDao.insert(Download.fromUpdate(update))
+        enqueue(Download.fromUpdate(update))
    }

    /**
@@ -108,7 +115,40 @@ class DownloadHelper @Inject constructor(
     * @param externalApk [ExternalApk] to download
     */
    suspend fun enqueueStandalone(externalApk: ExternalApk) {
-        downloadDao.insert(Download.fromExternalApk(externalApk))
+        enqueue(Download.fromExternalApk(externalApk))
+    }
+
+    /**
+     * Inserts a new download row, but only when a (re)download is actually needed. For an
+     * existing record of the same version this:
+     * - **installs without re-downloading** if the files are already downloaded & verified
+     *   (e.g. the user missed the system install prompt, or the periodic update check runs
+     *   again before a pending install completed); or
+     * - **skips** entirely if the download is still active (queued/purchasing/downloading/
+     *   verifying), so the periodic [UpdateWorker] and repeated user taps can't reset it back
+     *   to [DownloadStatus.QUEUED] and re-download it.
+     *
+     * A genuinely newer version, or a previously failed/cancelled download whose files are
+     * gone, falls through and is (re)enqueued.
+     */
+    private suspend fun enqueue(download: Download) {
+        val existing = getDownload(download.packageName)
+        if (existing != null && existing.versionCode == download.versionCode) {
+            if (existing.canInstall(context)) {
+                Log.i(TAG, "${download.packageName} already downloaded, installing directly")
+                runCatching { appInstaller.getPreferredInstaller().install(existing) }
+                    .onFailure { Log.e(TAG, "Failed to install ${download.packageName}", it) }
+                return
+            }
+            if (existing.isActive) {
+                Log.i(
+                    TAG,
+                    "Skipping enqueue for ${download.packageName}; already ${existing.status}"
+                )
+                return
+            }
+        }
+        downloadDao.insert(download)
    }

    /**
@@ -197,11 +237,24 @@ class DownloadHelper @Inject constructor(
            .putString(PACKAGE_NAME, download.packageName)
            .build()

+        // Require connectivity so the worker doesn't spin up (or keep running) without a
+        // network, and back off exponentially so transient failures resume cleanly once the
+        // connection returns instead of hammering the server.
+        val constraints = Constraints.Builder()
+            .setRequiredNetworkType(NetworkType.CONNECTED)
+            .build()
+
        val work = OneTimeWorkRequestBuilder<DownloadWorker>()
            .addTag(DOWNLOAD_WORKER)
            .addTag("$PACKAGE_NAME:${download.packageName}")
            .addTag("$VERSION_CODE:${download.versionCode}")
            .addTag(if (download.isInstalled) DOWNLOAD_UPDATE else DOWNLOAD_APP)
+            .setConstraints(constraints)
+            .setBackoffCriteria(
+                BackoffPolicy.EXPONENTIAL,
+                WorkRequest.MIN_BACKOFF_MILLIS,
+                TimeUnit.MILLISECONDS
+            )
            .setExpedited(OutOfQuotaPolicy.DROP_WORK_REQUEST)
            .setInputData(inputData)
            .build()
--- a/app/src/main/java/com/aurora/store/data/room/download/Download.kt
+++ b/app/src/main/java/com/aurora/store/data/room/download/Download.kt
@@ -43,6 +43,13 @@ data class Download(
    val isRunning get() = status in DownloadStatus.running
    private val isSuccessful get() = status == DownloadStatus.COMPLETED

+    /**
+     * `true` while the download is queued, purchasing, downloading or verifying, i.e.
+     * the pipeline is actively working on it. Unlike [isRunning] this also covers
+     * [DownloadStatus.VERIFYING], which sits between downloading and completion.
+     */
+    val isActive get() = isRunning || status == DownloadStatus.VERIFYING
+
    companion object {
        fun fromApp(app: App): Download = Download(
            app.packageName,
--- a/app/src/main/java/com/aurora/store/data/work/DownloadWorker.kt
+++ b/app/src/main/java/com/aurora/store/data/work/DownloadWorker.kt
@@ -48,6 +48,7 @@ import dagger.assisted.Assisted
 import dagger.assisted.AssistedInject
 import java.io.File
 import java.io.FileOutputStream
+import java.net.HttpURLConnection.HTTP_PARTIAL
 import java.net.SocketException
 import java.net.SocketTimeoutException
 import java.net.UnknownHostException
@@ -77,6 +78,10 @@ class DownloadWorker @AssistedInject constructor(

    companion object {
        private const val NOTIFICATION_ID: Int = 200
+
+        // Upper bound on automatic WorkManager retries for transient (network) failures
+        // before the download is marked as failed and left for the user to retry.
+        private const val MAX_DOWNLOAD_RETRIES = 5
    }

    private lateinit var download: Download
@@ -184,17 +189,21 @@ class DownloadWorker @AssistedInject constructor(
                download.downloadedFiles++
            }
        } catch (exception: Exception) {
-            if (exception is DownloadCancelledException) {
-                Log.i(TAG, "Download cancelled for ${download.packageName}")
-                // Try to delete all downloaded files
+            // Only purge partial files on a genuine user/app cancellation. A stop caused
+            // by lost connectivity, quota or device-state must keep the partials so the
+            // retry resumes instead of re-downloading from scratch (this was the source of
+            // downloads appearing to "restart" on a flaky network).
+            if (exception is DownloadCancelledException && isCancelledByUser()) {
+                Log.i(TAG, "Download cancelled by user for ${download.packageName}")
                runCatching { files.forEach { deleteFile(it) } }
            }

            return onFailure(exception)
        }

-        // Report failure if download was stopped or failed
-        if (isStopped) return onFailure(DownloadFailedException())
+        // A stop that isn't a user cancellation (e.g. connectivity constraint) should be
+        // retried with the partials intact rather than treated as a hard failure.
+        if (isStopped) return onFailure(DownloadCancelledException())

        // Verify downloaded files
        try {
@@ -202,6 +211,9 @@ class DownloadWorker @AssistedInject constructor(
            files.forEach { file -> require(verifyFile(file)) }
        } catch (exception: Exception) {
            Log.e(TAG, "Failed to verify ${download.packageName}", exception)
+            // Drop the corrupt files so the next attempt re-downloads them clean instead
+            // of resuming from a poisoned offset.
+            runCatching { files.forEach { deleteFile(it) } }
            return onFailure(VerificationFailedException())
        }

@@ -223,12 +235,58 @@ class DownloadWorker @AssistedInject constructor(
        }
    }

+    /**
+     * Whether the current stop/cancellation was initiated by the user (or the app on the
+     * user's behalf) rather than by the system (connectivity/quota/device-state). Uses the
+     * S+ [stopReason] when available and otherwise falls back to the persisted status, which
+     * [DownloadHelper.cancelDownload] sets to [DownloadStatus.CANCELLED] before cancelling
+     * the work — making this reliable below Android 12 too.
+     */
+    private suspend fun isCancelledByUser(): Boolean {
+        val cancelReasons = listOf(STOP_REASON_USER, STOP_REASON_CANCELLED_BY_APP)
+        if (isSAndAbove && stopReason in cancelReasons) return true
+        return runCatching {
+            downloadDao.getDownload(download.packageName).status == DownloadStatus.CANCELLED
+        }.getOrDefault(false)
+    }
+
+    /**
+     * Transient errors worth retrying once connectivity returns. Walks the cause chain so a
+     * wrapped network error is still recognised.
+     */
+    private fun isRetryable(throwable: Throwable?): Boolean = when (throwable) {
+        null -> false
+        is NoNetworkException,
+        is SocketException,
+        is SocketTimeoutException,
+        is UnknownHostException -> true
+
+        else -> isRetryable(throwable.cause)
+    }
+
    private suspend fun onFailure(exception: Exception): Result {
        return withContext(NonCancellable) {
            Log.i(TAG, "Job failed: ${download.packageName}", exception)

-            val cancelReasons = listOf(STOP_REASON_USER, STOP_REASON_CANCELLED_BY_APP)
-            if (isSAndAbove && stopReason in cancelReasons) {
+            val cancelledByUser = isCancelledByUser()
+
+            // Retry transient failures (lost connectivity, system-initiated stops) with
+            // backoff, keeping any partial download for resume. The network constraint on
+            // the work request means the retry only runs once connectivity is back.
+            val isSystemStop = exception is DownloadCancelledException && !cancelledByUser
+            if (!cancelledByUser &&
+                (isRetryable(exception) || isSystemStop) &&
+                runAttemptCount < MAX_DOWNLOAD_RETRIES
+            ) {
+                Log.w(
+                    TAG,
+                    "Transient failure for ${download.packageName}, " +
+                        "retrying (attempt $runAttemptCount)"
+                )
+                return@withContext Result.retry()
+            }
+
+            if (cancelledByUser) {
                notifyStatus(DownloadStatus.CANCELLED)
            } else {
                when (exception) {
@@ -301,23 +359,39 @@ class DownloadWorker @AssistedInject constructor(
            }

            try {
-                val tmpFileSuffix = ".tmp"
-                val tmpFile = File(file.absolutePath + tmpFileSuffix)
-
                // Download as a temporary file to avoid installing corrupted files
-                val isNewFile = tmpFile.createNewFile()
+                val tmpFile = File(file.absolutePath + ".tmp")
+                val existingBytes = if (tmpFile.exists()) tmpFile.length() else 0L

                val okHttpClient = httpClient as HttpClient
                val headers = mutableMapOf<String, String>()
-
-                if (!isNewFile) {
-                    Log.i(TAG, "$tmpFile has an unfinished download, resuming!")
-                    downloadedBytes += tmpFile.length()
-                    headers["Range"] = "bytes=${tmpFile.length()}-"
+                if (existingBytes > 0) {
+                    Log.i(TAG, "$tmpFile has an unfinished download, requesting resume!")
+                    headers["Range"] = "bytes=$existingBytes-"
                }

-                okHttpClient.call(gFile.url, headers).body.byteStream().use { input ->
-                    FileOutputStream(tmpFile, !isNewFile).use {
+                val response = okHttpClient.call(gFile.url, headers)
+                if (!response.isSuccessful) {
+                    response.close()
+                    throw DownloadFailedException()
+                }
+
+                // Only resume when the server actually honored the Range request (206). If
+                // it replied 200 with the full body we must overwrite from the start,
+                // otherwise the full payload would be appended onto the existing partial and
+                // silently corrupt the file.
+                val resuming = existingBytes > 0 && response.code == HTTP_PARTIAL
+                if (resuming) {
+                    downloadedBytes += existingBytes
+                } else if (existingBytes > 0) {
+                    Log.w(
+                        TAG,
+                        "Server ignored Range for $tmpFile (code=${response.code}), restarting"
+                    )
+                }
+
+                response.body.byteStream().use { input ->
+                    FileOutputStream(tmpFile, resuming).use {
                        input.copyTo(it, gFile.size).collect { info -> onProgress(info) }
                    }
                }