From 95fd8e0d9edfdbede21d6afef956d53aa25b736d Mon Sep 17 00:00:00 2001 From: Fahim Masud Choudhury Date: Mon, 20 Apr 2026 16:59:02 +0600 Subject: [PATCH] fix(install): clean up canceled installs Cancel tracked downloads when install work is interrupted so stale install records do not survive worker cancellation. Ignore late download callbacks once the canceled app install has already been removed. --- .../install/core/AppInstallationFacade.kt | 9 +++ .../install/workmanager/InstallAppWorker.kt | 19 ++++- .../e/apps/data/install/AppManagerImplTest.kt | 75 ++++++++++++++++++- .../download/DownloadManagerUtilsTest.kt | 28 +++++++ .../AppInstallationFacadeTest.kt | 22 ++++++ .../installProcessor/InstallAppWorkerTest.kt | 28 +++++++ 6 files changed, 178 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/foundation/e/apps/data/install/core/AppInstallationFacade.kt b/app/src/main/java/foundation/e/apps/data/install/core/AppInstallationFacade.kt index e4b9e287b..22b5de1d6 100644 --- a/app/src/main/java/foundation/e/apps/data/install/core/AppInstallationFacade.kt +++ b/app/src/main/java/foundation/e/apps/data/install/core/AppInstallationFacade.kt @@ -104,4 +104,13 @@ class AppInstallationFacade @Inject constructor( } } } + + suspend fun cancelInstall(downloadId: String) { + if (downloadId.isBlank()) { + error("App download ID can't be blank.") + } + + val appInstall = appManager.getDownloadById(AppInstall(id = downloadId)) ?: return + appManager.cancelDownload(appInstall) + } } diff --git a/app/src/main/java/foundation/e/apps/data/install/workmanager/InstallAppWorker.kt b/app/src/main/java/foundation/e/apps/data/install/workmanager/InstallAppWorker.kt index b095fb55f..8ffe5f4fd 100644 --- a/app/src/main/java/foundation/e/apps/data/install/workmanager/InstallAppWorker.kt +++ b/app/src/main/java/foundation/e/apps/data/install/workmanager/InstallAppWorker.kt @@ -33,6 +33,10 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import foundation.e.apps.R import foundation.e.apps.data.install.core.AppInstallationFacade +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext +import timber.log.Timber import java.util.concurrent.atomic.AtomicInteger @HiltWorker @@ -65,8 +69,19 @@ class InstallAppWorker @AssistedInject constructor( } val isPackageUpdate = params.inputData.getBoolean(IS_UPDATE_WORK, false) - val response = appInstallationFacade.processInstall(fusedDownloadId, isPackageUpdate) { title -> - setForeground(createForegroundInfo("${context.getString(R.string.installing)} $title")) + val response = try { + appInstallationFacade.processInstall(fusedDownloadId, isPackageUpdate) { title -> + setForeground(createForegroundInfo("${context.getString(R.string.installing)} $title")) + } + } catch (exception: CancellationException) { + runCatching { + withContext(NonCancellable) { + appInstallationFacade.cancelInstall(fusedDownloadId) + } + }.onFailure { throwable -> + Timber.w(throwable, "Failed to clean up cancelled install %s", fusedDownloadId) + } + throw exception } return if (response.isSuccess) { diff --git a/app/src/test/java/foundation/e/apps/data/install/AppManagerImplTest.kt b/app/src/test/java/foundation/e/apps/data/install/AppManagerImplTest.kt index 6c2275492..9ebe81c8e 100644 --- a/app/src/test/java/foundation/e/apps/data/install/AppManagerImplTest.kt +++ b/app/src/test/java/foundation/e/apps/data/install/AppManagerImplTest.kt @@ -35,6 +35,7 @@ import foundation.e.apps.data.install.workmanager.InstallWorkManager import foundation.e.apps.data.installation.model.AppInstall import foundation.e.apps.data.installation.model.SharedLib import foundation.e.apps.data.installation.repository.AppInstallRepository +import foundation.e.apps.data.parentalcontrol.ContentRatingDao import foundation.e.apps.domain.model.install.Status import foundation.e.apps.domain.preferences.AppPreferencesRepository import foundation.e.apps.installProcessor.FakeAppInstallDAO @@ -73,6 +74,7 @@ class AppManagerImplTest { private val appLoungePackageManager = mockk(relaxed = true) private val appPreferencesRepository = mockk(relaxed = true) private val fDroidRepository = mockk(relaxed = true) + private val contentRatingDao = mockk(relaxed = true) private val downloadChannel = mockk(relaxed = true) private val updateChannel = mockk(relaxed = true) private val context: Context = ApplicationProvider.getApplicationContext() @@ -99,6 +101,7 @@ class AppManagerImplTest { context, fDroidRepository, ) + appManager.contentRatingDao = contentRatingDao appManager.appPreferencesRepository = appPreferencesRepository } @@ -267,6 +270,75 @@ class AppManagerImplTest { assertTrue(isSuccessful) } + @Test + fun cancelDownload_removesTrackedDownloadsAndDeletesPendingAppInstall() = runTest { + val appInstall = createAppInstall().apply { + status = Status.DOWNLOADING + downloadIdMap = mutableMapOf(11L to false, 12L to true) + } + appInstallDAO.addDownload(appInstall) + createApk(appInstall.packageName, "cached.apk") + DownloadProgressLD.setDownloadId(11L) + + appManager.cancelDownload(appInstall) + + verify { downloadManager.remove(11L) } + verify { downloadManager.remove(12L) } + assertThat(appInstallDAO.getDownloadById(appInstall.id)).isNull() + assertThat(File(tempDir, appInstall.packageName).exists()).isFalse() + assertThat(DownloadProgressLD.downloadId).isEmpty() + } + + @Test + fun getFusedDownload_returnsEmptyAfterCancelRemovesTrackedIds() = runTest { + val appInstall = createAppInstall().apply { + status = Status.INSTALLING + downloadIdMap = mutableMapOf(21L to true) + } + appInstallDAO.addDownload(appInstall) + + appManager.cancelDownload(appInstall) + + assertThat(appManager.getFusedDownload(21L).id).isEmpty() + } + + @Test + fun cancelDownload_keepsOtherActiveAppsUntouched() = runTest { + val canceledApp = createAppInstall(packageName = "com.example.cancel").apply { + status = Status.DOWNLOADING + downloadIdMap = mutableMapOf(31L to false) + } + val otherApp = createAppInstall(id = "122", packageName = "com.example.other").apply { + status = Status.INSTALLING + downloadIdMap = mutableMapOf(41L to true) + } + appInstallDAO.addDownload(canceledApp) + appInstallDAO.addDownload(otherApp) + + appManager.cancelDownload(canceledApp) + + assertThat(appInstallDAO.getDownloadById(canceledApp.id)).isNull() + assertThat(appInstallDAO.getDownloadById(otherApp.id)?.packageName) + .isEqualTo(otherApp.packageName) + verify(exactly = 1) { downloadManager.remove(31L) } + verify(exactly = 0) { downloadManager.remove(41L) } + } + + @Test + fun cancelDownload_isIdempotentWhenAppAlreadyRemoved() = runTest { + val appInstall = createAppInstall(packageName = "com.example.repeat").apply { + status = Status.DOWNLOADING + downloadIdMap = mutableMapOf(51L to false) + } + appInstallDAO.addDownload(appInstall) + + appManager.cancelDownload(appInstall) + appManager.cancelDownload(appInstall) + + assertThat(appInstallDAO.getDownloadById(appInstall.id)).isNull() + verify(exactly = 2) { downloadManager.remove(51L) } + } + private fun initTest(hasAnyExistingWork: Boolean = false): AppInstall { mockkObject(InstallWorkManager) every { InstallWorkManager.checkWorkIsAlreadyAvailable(any(), any()) } returns hasAnyExistingWork @@ -274,10 +346,11 @@ class AppManagerImplTest { } private fun createAppInstall( + id: String = "121", packageName: String? = null, downloadUrlList: MutableList? = null ) = AppInstall( - id = "121", + id = id, status = Status.AWAITING, downloadURLList = downloadUrlList ?: mutableListOf("apk1", "apk2"), packageName = packageName ?: "com.unit.test" diff --git a/app/src/test/java/foundation/e/apps/data/install/download/DownloadManagerUtilsTest.kt b/app/src/test/java/foundation/e/apps/data/install/download/DownloadManagerUtilsTest.kt index 0e5f347a9..2c9c5d710 100644 --- a/app/src/test/java/foundation/e/apps/data/install/download/DownloadManagerUtilsTest.kt +++ b/app/src/test/java/foundation/e/apps/data/install/download/DownloadManagerUtilsTest.kt @@ -31,6 +31,7 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.verify +import kotlinx.coroutines.DelicateCoroutinesApi import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runCurrent @@ -97,4 +98,31 @@ class DownloadManagerUtilsTest { verify(exactly = 1) { appManagerWrapper.moveOBBFilesToOBBDirectory(appInstall) } coVerify(atLeast = 3) { appManagerWrapper.updateAppInstall(appInstall) } } + + @OptIn(DelicateCoroutinesApi::class) + @Test + fun updateDownloadStatus_ignoresLateCallbackWhenCanceledAppIsAlreadyRemoved() = runTest { + val context = mockk(relaxed = true) + val appManagerWrapper = mockk(relaxed = true) + val downloadManager = mockk(relaxed = true) + val storageNotificationManager = mockk(relaxed = true) + val utils = DownloadManagerUtils( + context, + appManagerWrapper, + downloadManager, + storageNotificationManager, + backgroundScope + ) + + coEvery { appManagerWrapper.getFusedDownload(99L, any()) } returns AppInstall() + + utils.updateDownloadStatus(99L) + advanceTimeBy(1500) + runCurrent() + + coVerify { appManagerWrapper.getFusedDownload(99L, any()) } + coVerify(exactly = 0) { appManagerWrapper.updateAppInstall(any()) } + coVerify(exactly = 0) { appManagerWrapper.reportInstallationIssue(any()) } + coVerify(exactly = 0) { appManagerWrapper.cancelDownload(any(), any()) } + } } diff --git a/app/src/test/java/foundation/e/apps/installProcessor/AppInstallationFacadeTest.kt b/app/src/test/java/foundation/e/apps/installProcessor/AppInstallationFacadeTest.kt index 74f6b894e..af538bf61 100644 --- a/app/src/test/java/foundation/e/apps/installProcessor/AppInstallationFacadeTest.kt +++ b/app/src/test/java/foundation/e/apps/installProcessor/AppInstallationFacadeTest.kt @@ -131,4 +131,26 @@ class AppInstallationFacadeTest { assertEquals(ResultStatus.OK, result.getOrNull()) coVerify { installationProcessor.processInstall("123", false, any()) } } + + @Test + fun cancelInstall_cancelsExistingAppInstall() = runTest { + val appInstall = AppInstall(id = "123", packageName = "com.example.app") + coEvery { appManager.getDownloadById(match { it.id == "123" }) } returns appInstall + + appInstallationFacade.cancelInstall("123") + + coVerify { appManager.getDownloadById(match { it.id == "123" }) } + coVerify { appManager.cancelDownload(appInstall) } + coVerify(exactly = 0) { appManager.reportInstallationIssue(any()) } + } + + @Test + fun cancelInstall_ignoresMissingAppInstall() = runTest { + coEvery { appManager.getDownloadById(match { it.id == "missing" }) } returns null + + appInstallationFacade.cancelInstall("missing") + + coVerify { appManager.getDownloadById(match { it.id == "missing" }) } + coVerify(exactly = 0) { appManager.cancelDownload(any()) } + } } diff --git a/app/src/test/java/foundation/e/apps/installProcessor/InstallAppWorkerTest.kt b/app/src/test/java/foundation/e/apps/installProcessor/InstallAppWorkerTest.kt index 953424eb5..41833f0b0 100644 --- a/app/src/test/java/foundation/e/apps/installProcessor/InstallAppWorkerTest.kt +++ b/app/src/test/java/foundation/e/apps/installProcessor/InstallAppWorkerTest.kt @@ -30,12 +30,15 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.delay import kotlinx.coroutines.test.runTest import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config +import kotlin.test.assertFailsWith @RunWith(RobolectricTestRunner::class) @Config(sdk = [Build.VERSION_CODES.R]) @@ -94,6 +97,31 @@ class InstallAppWorkerTest { coVerify { appInstallationFacade.processInstall("123", false, any()) } } + @Test + fun doWork_cleansUpTargetedInstallWhenCancelled() = runTest { + coEvery { + appInstallationFacade.processInstall("123", false, any()) + } throws CancellationException("cancelled") + var cleanupCompleted = false + coEvery { appInstallationFacade.cancelInstall("123") } coAnswers { + delay(1) + cleanupCompleted = true + } + val worker = createWorker( + Data.Builder() + .putString(InstallAppWorker.INPUT_DATA_FUSED_DOWNLOAD, "123") + .putBoolean(InstallAppWorker.IS_UPDATE_WORK, false) + .build() + ) + + assertFailsWith { + worker.doWork() + } + + coVerify { appInstallationFacade.cancelInstall("123") } + assertThat(cleanupCompleted).isTrue() + } + private fun createWorker(inputData: Data): InstallAppWorker { val params = mockk(relaxed = true) every { params.inputData } returns inputData -- GitLab