From ada5a1f899c9a1e699d4c41f6e67fb5b0a3f1b43 Mon Sep 17 00:00:00 2001 From: Rahul Patel Date: Sat, 30 May 2026 00:19:33 +0530 Subject: [PATCH] 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. --- .../store/data/helper/DownloadHelper.kt | 61 +++++++++- .../store/data/room/download/Download.kt | 7 ++ .../aurora/store/data/work/DownloadWorker.kt | 110 +++++++++++++++--- 3 files changed, 156 insertions(+), 22 deletions(-) diff --git a/app/src/main/java/com/aurora/store/data/helper/DownloadHelper.kt b/app/src/main/java/com/aurora/store/data/helper/DownloadHelper.kt index 1fe336c11..be720efa1 100644 --- 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() .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() diff --git 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 index 5a2ef3f40..634cb5b6d 100644 --- 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, diff --git a/app/src/main/java/com/aurora/store/data/work/DownloadWorker.kt b/app/src/main/java/com/aurora/store/data/work/DownloadWorker.kt index cc8319ccf..4bc4135a6 100644 --- 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() - - 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) } } }