From de3f8172f1df1281beacb30be59c647f7639043f Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 5 Aug 2026 13:55:59 -0700 Subject: [PATCH 1/5] ADFA-5037: Stop install() from swallowing doInstall() failures install() wrapped doInstall() in runCatching, but doInstall()'s preInstall failure path caught FileNotFoundException/ZipException/ IOException and returned a Result.Failure value instead of throwing. runCatching only detects thrown exceptions, so that returned Failure was discarded and install() reported Success regardless. Rethrow after logging/flashing in each catch block instead, so the failure actually reaches install()'s exception handling. --- .../assets/AssetsInstallationHelper.kt | 38 +++++------ .../assets/AssetsInstallationHelperTest.kt | 68 +++++++++++++++++++ 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 07d7ddd71d..f3b053c36d 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -125,26 +125,24 @@ object AssetsInstallationHelper { Brotli4jLoader.ensureAvailability() // pre-install hook - val isPreInstallSuccessful = - try { - ASSETS_INSTALLER.preInstall(context, stagingDir) - true - } catch (e: FileNotFoundException) { - logger.error("ZIP file not found: {}", e.message) - flashError("File not found - ${e.message}") - false - } catch (e: ZipException) { - logger.error("Invalid ZIP format: {}", e.message) - onProgress(Progress("Corrupt zip file ${e.message}")) - false - } catch (e: IOException) { - logger.error("I/O error during preInstall: {}", e.message) - onProgress(Progress("Failed to load ${e.message}")) - false - } - - if (!isPreInstallSuccessful) { - return@coroutineScope Result.Failure(IOException("preInstall failed")) + // Log/report the failure here for diagnostics, then rethrow so install()'s + // runCatching actually observes it -- returning a Result.Failure value here + // instead would be silently discarded, since doInstall() otherwise has no + // meaningful return value on its success path. + try { + ASSETS_INSTALLER.preInstall(context, stagingDir) + } catch (e: FileNotFoundException) { + logger.error("ZIP file not found: {}", e.message) + flashError("File not found - ${e.message}") + throw e + } catch (e: ZipException) { + logger.error("Invalid ZIP format: {}", e.message) + onProgress(Progress("Corrupt zip file ${e.message}")) + throw e + } catch (e: IOException) { + logger.error("I/O error during preInstall: {}", e.message) + onProgress(Progress("Failed to load ${e.message}")) + throw e } try { diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index 2a4a8e8c28..d94f232645 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -1,11 +1,20 @@ package com.itsaky.androidide.assets import android.content.Context +import com.aayushatharva.brotli4j.Brotli4jLoader +import com.itsaky.androidide.app.configuration.CpuArch +import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.assets.AssetsInstallationHelper.Result.Failure +import com.itsaky.androidide.utils.flashError +import io.mockk.Runs import io.mockk.coEvery import io.mockk.every +import io.mockk.just import io.mockk.mockk import io.mockk.mockkObject +import io.mockk.mockkStatic +import io.mockk.unmockkObject +import io.mockk.unmockkStatic import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -61,6 +70,65 @@ class AssetsInstallationHelperTest { ) } + @Test + fun `install reports Failure when doInstall's own preInstall catch block swallows an exception`() = + runBlocking { + // Unlike the test above (which mocks doInstall itself to throw, bypassing its + // internal try/catch entirely), this lets the real doInstall() run and only + // stubs the underlying installer's preInstall, so it actually exercises the + // catch-then-rethrow path inside doInstall -- the path ADFA-5037 found silently + // swallowing failures by returning a Result.Failure value instead of throwing, + // which runCatching in install() can't observe. + val helper = AssetsInstallationHelper + + every { + helper["checkStorageAccessibility"](any(), any()) + } returns null + + mockkObject(IDEBuildConfigProvider.Companion) + mockkStatic(Brotli4jLoader::class) + mockkStatic("com.itsaky.androidide.utils.FlashbarUtilsKt") + mockkObject(SplitAssetsInstaller) + try { + // doInstall() looks up the build's CpuArch before reaching preInstall; the + // real IDEBuildConfigProviderImpl needs a live BaseApplication instance to + // do that, which isn't available in this unit test, so stub it directly. + val buildConfigProvider = mockk(relaxed = true) + every { buildConfigProvider.cpuArch } returns CpuArch.AARCH64 + every { IDEBuildConfigProvider.getInstance() } returns buildConfigProvider + + // doInstall() also loads the Brotli native library before reaching + // preInstall; it isn't available in this unit test either. + every { Brotli4jLoader.ensureAvailability() } just Runs + + // The FileNotFoundException catch block flashes an error via a live + // Activity, which also isn't available in this unit test. + every { flashError(any()) } just Runs + + coEvery { + SplitAssetsInstaller.preInstall(any(), any()) + } throws FileNotFoundException("assets-arm64-v8a.zip") + + val result = helper.install(ctx) + + assertTrue("Expected Result.Failure", result is Failure) + val failure = result as Failure + assertTrue( + "Expected MissingAssetsEntryException as cause", + failure.cause is MissingAssetsEntryException, + ) + assertTrue( + "Expected FileNotFoundException as root cause", + (failure.cause?.cause) is FileNotFoundException, + ) + } finally { + unmockkObject(SplitAssetsInstaller) + unmockkStatic("com.itsaky.androidide.utils.FlashbarUtilsKt") + unmockkStatic(Brotli4jLoader::class) + unmockkObject(IDEBuildConfigProvider.Companion) + } + } + @Test fun `extractZipToDir creates parent directories for nested entries with no directory entries`() { val destDir = Files.createTempDirectory("extract-zip-to-dir-test") From 7c92e951bb2616e3e5492559d3076b14a6733861 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 5 Aug 2026 14:22:23 -0700 Subject: [PATCH 2/5] ADFA-5037: Run postInstall/staging cleanup on preInstall failure too preInstall's try/catch was a separate block from the main install work's try/finally, so a rethrown preInstall failure skipped postInstall() (closing installer resources like SplitAssetsInstaller's zipFile) and never deleted the staging directory. Merge both into one outer try/finally so cleanup always runs. --- .../assets/AssetsInstallationHelper.kt | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index f3b053c36d..55b562f70f 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -124,28 +124,28 @@ object AssetsInstallationHelper { // Ensure relevant shared libraries are loaded Brotli4jLoader.ensureAvailability() - // pre-install hook - // Log/report the failure here for diagnostics, then rethrow so install()'s - // runCatching actually observes it -- returning a Result.Failure value here - // instead would be silently discarded, since doInstall() otherwise has no - // meaningful return value on its success path. try { - ASSETS_INSTALLER.preInstall(context, stagingDir) - } catch (e: FileNotFoundException) { - logger.error("ZIP file not found: {}", e.message) - flashError("File not found - ${e.message}") - throw e - } catch (e: ZipException) { - logger.error("Invalid ZIP format: {}", e.message) - onProgress(Progress("Corrupt zip file ${e.message}")) - throw e - } catch (e: IOException) { - logger.error("I/O error during preInstall: {}", e.message) - onProgress(Progress("Failed to load ${e.message}")) - throw e - } + // pre-install hook + // Log/report the failure here for diagnostics, then rethrow so install()'s + // runCatching actually observes it -- returning a Result.Failure value here + // instead would be silently discarded, since doInstall() otherwise has no + // meaningful return value on its success path. + try { + ASSETS_INSTALLER.preInstall(context, stagingDir) + } catch (e: FileNotFoundException) { + logger.error("ZIP file not found: {}", e.message) + flashError("File not found - ${e.message}") + throw e + } catch (e: ZipException) { + logger.error("Invalid ZIP format: {}", e.message) + onProgress(Progress("Corrupt zip file ${e.message}")) + throw e + } catch (e: IOException) { + logger.error("I/O error during preInstall: {}", e.message) + onProgress(Progress("Failed to load ${e.message}")) + throw e + } - try { val entrySizes: Map = expectedEntries.associateWith { entry -> ASSETS_INSTALLER.expectedSize(entry) From 16c3a34d6a7f32dbb6dd3bf3417668837d54adc4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 5 Aug 2026 14:36:44 -0700 Subject: [PATCH 3/5] ADFA-5037: Assert postInstall/staging cleanup runs on preInstall failure The regression test only checked the returned Failure's cause chain, not that postInstall() and staging-dir deletion actually still ran -- the specific behavior the previous commit added. Capture the staging dir passed to preInstall, stub postInstall, and verify both. --- .../assets/AssetsInstallationHelperTest.kt | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index d94f232645..69ecbb6dbd 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -8,11 +8,13 @@ import com.itsaky.androidide.assets.AssetsInstallationHelper.Result.Failure import com.itsaky.androidide.utils.flashError import io.mockk.Runs import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.mockkObject import io.mockk.mockkStatic +import io.mockk.slot import io.mockk.unmockkObject import io.mockk.unmockkStatic import kotlinx.coroutines.runBlocking @@ -105,10 +107,18 @@ class AssetsInstallationHelperTest { // Activity, which also isn't available in this unit test. every { flashError(any()) } just Runs + val stagingDirSlot = slot() coEvery { - SplitAssetsInstaller.preInstall(any(), any()) + SplitAssetsInstaller.preInstall(any(), capture(stagingDirSlot)) } throws FileNotFoundException("assets-arm64-v8a.zip") + // Stubbed (rather than left to call the real implementation) because the + // real postInstall() chmods paths under Environment.BUILD_TOOLS_DIR, which + // requires Environment.init() -- unrelated to what this test verifies. + coEvery { + SplitAssetsInstaller.postInstall(any(), any()) + } just Runs + val result = helper.install(ctx) assertTrue("Expected Result.Failure", result is Failure) @@ -121,6 +131,16 @@ class AssetsInstallationHelperTest { "Expected FileNotFoundException as root cause", (failure.cause?.cause) is FileNotFoundException, ) + + // A preInstall failure must not skip the symmetric cleanup that a + // successful install would get: postInstall() (closes installer + // resources) and deleting the staging directory. + coVerify(exactly = 1) { SplitAssetsInstaller.postInstall(any(), any()) } + assertTrue("Expected stagingDir to have been captured", stagingDirSlot.isCaptured) + assertFalse( + "Expected staging directory to be deleted even though preInstall failed", + Files.exists(stagingDirSlot.captured), + ) } finally { unmockkObject(SplitAssetsInstaller) unmockkStatic("com.itsaky.androidide.utils.FlashbarUtilsKt") From 7e6cb30c2129c4c3b941cb6fa8cf40ef2c6d05e7 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 5 Aug 2026 16:11:48 -0700 Subject: [PATCH 4/5] ADFA-5037: Address code review findings - Move Brotli4jLoader.ensureAvailability() inside doInstall's try block so a load failure also gets the symmetric postInstall/staging-dir cleanup, not just preInstall failures. - Extract the three preInstall catch blocks' log-then-rethrow into a logAndRethrow(prefix, e): Nothing helper, and drop the flashError/ onProgress calls in them -- install()'s failure handling already notifies the user once (onProgress + ShowError event) with a better message; leaving those in doInstall meant the user saw two different flashbars for one failure now that the exception actually propagates. - Wrap stagingDir.deleteRecursively() in the same runCatching pattern already used for postInstall, instead of Files.exists() (always true -- nothing between createTempDirectory and here ever removes it). A cleanup failure can no longer replace the real exception already propagating out of the try block. - Rethrow CancellationException from postInstall's cleanup runCatching, consistent with install()'s own explicit handling. - Broaden the "missing or corrupt assets" friendly-message check to also cover ZipException (corrupt archive), not just FileNotFoundException -- both are now reachable via the same preInstall catch blocks and share the same "reinstall" remedy. - SplitAssetsInstaller: fix the sibling instance of the exact bug this ticket is about. The BOOTSTRAP_ENTRY_NAME branch logged and returned on retry failure / non-Success TerminalInstaller results instead of throwing, so that entry's async job reported FINISHED regardless. Mirrors BundledAssetsInstaller's equivalent branch, which already throws in both cases. Test changes: - Hoist the repeated checkStorageAccessibility stub into @Before and the repeated Result.Failure cause-chain assertions into a shared assertMissingAssetFailure() helper. - Add @After { unmockkAll() }, removing the class-wide mock leak (the @Before mockkObject was never torn down) and the new test's manual per-mock unmock bookkeeping. - Drop the flashError stub (no longer called) and a redundant isCaptured assertion (the following .captured access already fails loudly if nothing was captured). - Documented (comment only) that the new test depends on AssetsInstaller.CURRENT_INSTALLER resolving to SplitAssetsInstaller, which holds for :app:testV8DebugUnitTest but not every build variant. --- .../assets/AssetsInstallationHelper.kt | 57 ++++--- .../androidide/assets/SplitAssetsInstaller.kt | 28 +++- .../assets/AssetsInstallationHelperTest.kt | 151 ++++++++---------- 3 files changed, 126 insertions(+), 110 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 55b562f70f..b3297c1c13 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -7,7 +7,6 @@ import com.aayushatharva.brotli4j.Brotli4jLoader import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT -import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.useEntriesEach import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async @@ -81,7 +80,10 @@ object AssetsInstallationHelper { val e = result.exceptionOrNull() ?: RuntimeException(context.getString(R.string.error_installation_failed)) if (e is CancellationException) throw e - val isMissingAsset = generateSequence(e) { it.cause }.any { it is FileNotFoundException } + // ZipException means the asset archive itself is corrupt, not just missing -- + // same "reinstall/redownload" remedy as a missing file, so it shares the + // friendly message and GlitchTip suppression below. + val isMissingAsset = generateSequence(e) { it.cause }.any { it is FileNotFoundException || it is ZipException } val cause = if (isMissingAsset) MissingAssetsEntryException(e) else e val msg = if (isMissingAsset) { @@ -121,29 +123,25 @@ object AssetsInstallationHelper { val stagingDir = Files.createTempDirectory(UUID.randomUUID().toString()) logger.debug("Staging directory ({}): {}", cpuArch, stagingDir) - // Ensure relevant shared libraries are loaded - Brotli4jLoader.ensureAvailability() - try { - // pre-install hook - // Log/report the failure here for diagnostics, then rethrow so install()'s + // Ensure relevant shared libraries are loaded + Brotli4jLoader.ensureAvailability() + + // pre-install hook. Log here for diagnostics, then rethrow so install()'s // runCatching actually observes it -- returning a Result.Failure value here // instead would be silently discarded, since doInstall() otherwise has no - // meaningful return value on its success path. + // meaningful return value on its success path. The user-facing message is + // left entirely to install()'s failure handling (onProgress/ShowError), so + // there is exactly one notification per failure, not one here plus another + // once the exception unwinds. try { ASSETS_INSTALLER.preInstall(context, stagingDir) } catch (e: FileNotFoundException) { - logger.error("ZIP file not found: {}", e.message) - flashError("File not found - ${e.message}") - throw e + logAndRethrow("ZIP file not found", e) } catch (e: ZipException) { - logger.error("Invalid ZIP format: {}", e.message) - onProgress(Progress("Corrupt zip file ${e.message}")) - throw e + logAndRethrow("Invalid ZIP format", e) } catch (e: IOException) { - logger.error("I/O error during preInstall: {}", e.message) - onProgress(Progress("Failed to load ${e.message}")) - throw e + logAndRethrow("I/O error during preInstall", e) } val entrySizes: Map = @@ -220,15 +218,30 @@ object AssetsInstallationHelper { // then cancel progress updater progressUpdater.cancel() } finally { - // Always run postInstall so zip/FS resources are closed (e.g. SplitAssetsInstaller.zipFile) + // Always run postInstall so zip/FS resources are closed (e.g. SplitAssetsInstaller.zipFile), + // and always clean up the staging dir -- on any exit path, including a preInstall + // failure. Both are runCatching so a cleanup failure can't replace whatever + // exception is already propagating out of the try block above (e.g. the very + // preInstall failure logAndRethrow just rethrew). runCatching { ASSETS_INSTALLER.postInstall(context, stagingDir) } - .onFailure { e -> logger.warn("postInstall failed", e) } - if (Files.exists(stagingDir)) { - stagingDir.deleteRecursively() - } + .onFailure { e -> + if (e is CancellationException) throw e + logger.warn("postInstall failed", e) + } + runCatching { stagingDir.deleteRecursively() } + .onFailure { e -> logger.warn("Failed to delete staging directory {}", stagingDir, e) } } } + /** Logs [e] with [prefix], then rethrows it -- never swallow-and-return here. */ + private fun logAndRethrow( + prefix: String, + e: Exception, + ): Nothing { + logger.error("{}: {}", prefix, e.message) + throw e + } + @WorkerThread internal fun extractZipToDir( srcFile: Path, diff --git a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt index ba532d2f8a..4fd6058bab 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt @@ -20,6 +20,7 @@ import org.adfa.constants.TEMPLATE_CORE_ARCHIVE import org.slf4j.LoggerFactory import java.io.File import java.io.FileNotFoundException +import java.io.IOException import java.nio.file.Files import java.nio.file.Path import java.util.zip.ZipFile @@ -111,8 +112,10 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() { retryOnceOnNoSuchFile( onFirstFailure = { Files.createDirectories(stagingDir) }, onSecondFailure = { e2 -> - logger.error("Failed to open temporary bootstrap zip after retry", e2) - return@withContext + throw IOException( + context.getString(R.string.terminal_installation_failed_low_storage), + e2, + ) }, ) { withTempZipChannel( @@ -131,8 +134,25 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() { ) } - if (result !is TerminalInstaller.InstallResult.Success) { - logger.error("Failed to install terminal: {}", result) + // Mirrors BundledAssetsInstaller's equivalent branch: every non-Success + // result must throw, or this entry's async job reports STATUS_FINISHED + // and install() sees no failure even though the terminal never installed. + when (result) { + is TerminalInstaller.InstallResult.Success -> {} + + is TerminalInstaller.InstallResult.Error.Interactive -> { + throw IOException("${result.title}: ${result.message}") + } + + is TerminalInstaller.InstallResult.Error.IsSecondaryUser -> { + throw IOException( + context.getString(R.string.terminal_installation_failed_secondary_user), + ) + } + + is TerminalInstaller.InstallResult.NotInstalled -> { + throw IllegalStateException("Terminal installation failed: NotInstalled state") + } } logger.debug("Completed extracting 'bootstrap.zip' to dir: {}", stagingDir) diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index 69ecbb6dbd..dda5ce2954 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -5,7 +5,6 @@ import com.aayushatharva.brotli4j.Brotli4jLoader import com.itsaky.androidide.app.configuration.CpuArch import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.assets.AssetsInstallationHelper.Result.Failure -import com.itsaky.androidide.utils.flashError import io.mockk.Runs import io.mockk.coEvery import io.mockk.coVerify @@ -15,9 +14,9 @@ import io.mockk.mockk import io.mockk.mockkObject import io.mockk.mockkStatic import io.mockk.slot -import io.mockk.unmockkObject -import io.mockk.unmockkStatic +import io.mockk.unmockkAll import kotlinx.coroutines.runBlocking +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertThrows @@ -38,38 +37,45 @@ import java.util.zip.ZipOutputStream class AssetsInstallationHelperTest { private val ctx: Context = mockk(relaxed = true) + private val helper = AssetsInstallationHelper @Before fun setup() { - mockkObject(AssetsInstallationHelper) + mockkObject(helper) + every { + helper["checkStorageAccessibility"](any(), any()) + } returns null + } + + @After + fun tearDown() { + unmockkAll() + } + + private fun assertMissingAssetFailure(result: AssetsInstallationHelper.Result): Failure { + assertTrue("Expected Result.Failure", result is Failure) + val failure = result as Failure + assertTrue( + "Expected MissingAssetsEntryException as cause", + failure.cause is MissingAssetsEntryException, + ) + assertTrue( + "Expected FileNotFoundException as root cause", + (failure.cause?.cause) is FileNotFoundException, + ) + return failure } @Test fun `install with missing asset skips glitchtip`() = runBlocking { - val helper = AssetsInstallationHelper - - every { - helper["checkStorageAccessibility"](any(), any()) - } returns null - coEvery { helper["doInstall"](any(), any()) } throws FileNotFoundException("data/common/gradle.zip.br") - val result = helper.install(ctx) + val failure = assertMissingAssetFailure(helper.install(ctx)) - assertTrue("Expected Result.Failure", result is Failure) - val failure = result as Failure assertFalse("Should skip GlitchTip report", failure.shouldReportToGlitchTip) - assertTrue( - "Expected MissingAssetsEntryException as cause", - failure.cause is MissingAssetsEntryException, - ) - assertTrue( - "Expected FileNotFoundException as root cause", - (failure.cause?.cause) is FileNotFoundException, - ) } @Test @@ -81,72 +87,49 @@ class AssetsInstallationHelperTest { // catch-then-rethrow path inside doInstall -- the path ADFA-5037 found silently // swallowing failures by returning a Result.Failure value instead of throwing, // which runCatching in install() can't observe. - val helper = AssetsInstallationHelper - - every { - helper["checkStorageAccessibility"](any(), any()) - } returns null - + // + // This relies on AssetsInstaller.CURRENT_INSTALLER resolving to SplitAssetsInstaller, + // which only holds for debug builds (see AssetsInstaller.kt's USE_BUNDLED_ASSETS). + // Run via :app:testV8DebugUnitTest, which satisfies that -- a bare aggregate `test` + // task fanning out to other build variants would silently bypass this stub instead + // of exercising the intended code path. mockkObject(IDEBuildConfigProvider.Companion) mockkStatic(Brotli4jLoader::class) - mockkStatic("com.itsaky.androidide.utils.FlashbarUtilsKt") mockkObject(SplitAssetsInstaller) - try { - // doInstall() looks up the build's CpuArch before reaching preInstall; the - // real IDEBuildConfigProviderImpl needs a live BaseApplication instance to - // do that, which isn't available in this unit test, so stub it directly. - val buildConfigProvider = mockk(relaxed = true) - every { buildConfigProvider.cpuArch } returns CpuArch.AARCH64 - every { IDEBuildConfigProvider.getInstance() } returns buildConfigProvider - - // doInstall() also loads the Brotli native library before reaching - // preInstall; it isn't available in this unit test either. - every { Brotli4jLoader.ensureAvailability() } just Runs - - // The FileNotFoundException catch block flashes an error via a live - // Activity, which also isn't available in this unit test. - every { flashError(any()) } just Runs - - val stagingDirSlot = slot() - coEvery { - SplitAssetsInstaller.preInstall(any(), capture(stagingDirSlot)) - } throws FileNotFoundException("assets-arm64-v8a.zip") - - // Stubbed (rather than left to call the real implementation) because the - // real postInstall() chmods paths under Environment.BUILD_TOOLS_DIR, which - // requires Environment.init() -- unrelated to what this test verifies. - coEvery { - SplitAssetsInstaller.postInstall(any(), any()) - } just Runs - - val result = helper.install(ctx) - - assertTrue("Expected Result.Failure", result is Failure) - val failure = result as Failure - assertTrue( - "Expected MissingAssetsEntryException as cause", - failure.cause is MissingAssetsEntryException, - ) - assertTrue( - "Expected FileNotFoundException as root cause", - (failure.cause?.cause) is FileNotFoundException, - ) - - // A preInstall failure must not skip the symmetric cleanup that a - // successful install would get: postInstall() (closes installer - // resources) and deleting the staging directory. - coVerify(exactly = 1) { SplitAssetsInstaller.postInstall(any(), any()) } - assertTrue("Expected stagingDir to have been captured", stagingDirSlot.isCaptured) - assertFalse( - "Expected staging directory to be deleted even though preInstall failed", - Files.exists(stagingDirSlot.captured), - ) - } finally { - unmockkObject(SplitAssetsInstaller) - unmockkStatic("com.itsaky.androidide.utils.FlashbarUtilsKt") - unmockkStatic(Brotli4jLoader::class) - unmockkObject(IDEBuildConfigProvider.Companion) - } + + // doInstall() looks up the build's CpuArch before reaching preInstall; the + // real IDEBuildConfigProviderImpl needs a live BaseApplication instance to + // do that, which isn't available in this unit test, so stub it directly. + val buildConfigProvider = mockk(relaxed = true) + every { buildConfigProvider.cpuArch } returns CpuArch.AARCH64 + every { IDEBuildConfigProvider.getInstance() } returns buildConfigProvider + + // doInstall() also loads the Brotli native library before reaching + // preInstall; it isn't available in this unit test either. + every { Brotli4jLoader.ensureAvailability() } just Runs + + val stagingDirSlot = slot() + coEvery { + SplitAssetsInstaller.preInstall(any(), capture(stagingDirSlot)) + } throws FileNotFoundException("assets-arm64-v8a.zip") + + // Stubbed (rather than left to call the real implementation) because the + // real postInstall() chmods paths under Environment.BUILD_TOOLS_DIR, which + // requires Environment.init() -- unrelated to what this test verifies. + coEvery { + SplitAssetsInstaller.postInstall(any(), any()) + } just Runs + + assertMissingAssetFailure(helper.install(ctx)) + + // A preInstall failure must not skip the symmetric cleanup that a + // successful install would get: postInstall() (closes installer + // resources) and deleting the staging directory. + coVerify(exactly = 1) { SplitAssetsInstaller.postInstall(any(), any()) } + assertFalse( + "Expected staging directory to be deleted even though preInstall failed", + Files.exists(stagingDirSlot.captured), + ) } @Test From 776f44624ef47d01605b239f2d0a65b5d1567add Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 5 Aug 2026 17:20:46 -0700 Subject: [PATCH 5/5] ADFA-5037: Address /code-review high findings - Run postInstall() under withContext(NonCancellable) in doInstall's finally block. When one of the parallel installerJobs fails, the coroutineScope is already Cancelling by the time this finally runs, so postInstall()'s own withContext(Dispatchers.IO) would otherwise throw CancellationException at that suspension point before its body -- closing SplitAssetsInstaller.zipFile, running the chmod loop -- ever executes. Verified empirically with a standalone kotlinx-coroutines-core 1.10.2 repro: cleanup only actually ran with the NonCancellable wrap; the real underlying failure still propagates correctly either way. - Extract the identical "throw if the terminal install result wasn't Success" when-block, now duplicated between SplitAssetsInstaller and BundledAssetsInstaller, into a shared TerminalInstaller.InstallResult.throwIfNotSuccess(context) extension (new file, to avoid pulling the large non-tab-formatted TerminalInstaller.kt under the Spotless ratchet for an unrelated reformat). Two copies of this exact logic were exactly how ADFA-5037 happened in the first place. --- .../assets/AssetsInstallationHelper.kt | 10 ++++-- .../assets/BundledAssetsInstaller.kt | 23 ++++---------- .../androidide/assets/SplitAssetsInstaller.kt | 26 ++++------------ .../utils/TerminalInstallResultExtensions.kt | 31 +++++++++++++++++++ 4 files changed, 51 insertions(+), 39 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/utils/TerminalInstallResultExtensions.kt diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index b3297c1c13..9359ece5aa 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -9,6 +9,7 @@ import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT import com.itsaky.androidide.utils.useEntriesEach import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.delay @@ -220,10 +221,15 @@ object AssetsInstallationHelper { } finally { // Always run postInstall so zip/FS resources are closed (e.g. SplitAssetsInstaller.zipFile), // and always clean up the staging dir -- on any exit path, including a preInstall - // failure. Both are runCatching so a cleanup failure can't replace whatever + // failure or one of the parallel installerJobs failing. postInstall() runs under + // NonCancellable: when a job above throws, this coroutineScope is already + // Cancelling by the time this finally block runs, and postInstall()'s own + // withContext(Dispatchers.IO) would otherwise throw CancellationException at that + // suspension point before its body -- the real cleanup -- ever executes. Both + // cleanup calls are runCatching so a cleanup failure can't replace whatever // exception is already propagating out of the try block above (e.g. the very // preInstall failure logAndRethrow just rethrew). - runCatching { ASSETS_INSTALLER.postInstall(context, stagingDir) } + runCatching { withContext(NonCancellable) { ASSETS_INSTALLER.postInstall(context, stagingDir) } } .onFailure { e -> if (e is CancellationException) throw e logger.warn("postInstall failed", e) diff --git a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt index 33fcfc988f..a01d15a636 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt @@ -9,6 +9,7 @@ import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.TerminalInstaller import com.itsaky.androidide.utils.retryOnceOnNoSuchFile +import com.itsaky.androidide.utils.throwIfNotSuccess import com.itsaky.androidide.utils.withTempZipChannel import com.itsaky.androidide.utils.writeBrotliAssetToPath import kotlinx.coroutines.Dispatchers @@ -132,23 +133,11 @@ data object BundledAssetsInstaller : BaseAssetsInstaller() { ) } - when (result) { - is TerminalInstaller.InstallResult.Success -> {} - - is TerminalInstaller.InstallResult.Error.Interactive -> { - throw IOException("${result.title}: ${result.message}") - } - - is TerminalInstaller.InstallResult.Error.IsSecondaryUser -> { - throw IOException( - context.getString(R.string.terminal_installation_failed_secondary_user), - ) - } - - is TerminalInstaller.InstallResult.NotInstalled -> { - throw IllegalStateException("Terminal installation failed: NotInstalled state") - } - } + // Every non-Success result must throw, or this entry's async job reports + // STATUS_FINISHED and install() sees no failure even though the terminal + // never installed -- shared with SplitAssetsInstaller's equivalent branch + // so the two can't drift out of sync again. + result.throwIfNotSuccess(context) } DOCUMENTATION_DB -> { diff --git a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt index 4fd6058bab..f178ba7201 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt @@ -7,6 +7,7 @@ import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.TerminalInstaller import com.itsaky.androidide.utils.retryOnceOnNoSuchFile +import com.itsaky.androidide.utils.throwIfNotSuccess import com.itsaky.androidide.utils.withTempZipChannel import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext @@ -134,26 +135,11 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() { ) } - // Mirrors BundledAssetsInstaller's equivalent branch: every non-Success - // result must throw, or this entry's async job reports STATUS_FINISHED - // and install() sees no failure even though the terminal never installed. - when (result) { - is TerminalInstaller.InstallResult.Success -> {} - - is TerminalInstaller.InstallResult.Error.Interactive -> { - throw IOException("${result.title}: ${result.message}") - } - - is TerminalInstaller.InstallResult.Error.IsSecondaryUser -> { - throw IOException( - context.getString(R.string.terminal_installation_failed_secondary_user), - ) - } - - is TerminalInstaller.InstallResult.NotInstalled -> { - throw IllegalStateException("Terminal installation failed: NotInstalled state") - } - } + // Every non-Success result must throw, or this entry's async job reports + // STATUS_FINISHED and install() sees no failure even though the terminal + // never installed -- shared with BundledAssetsInstaller's equivalent + // branch so the two can't drift out of sync again. + result.throwIfNotSuccess(context) logger.debug("Completed extracting 'bootstrap.zip' to dir: {}", stagingDir) } diff --git a/app/src/main/java/com/itsaky/androidide/utils/TerminalInstallResultExtensions.kt b/app/src/main/java/com/itsaky/androidide/utils/TerminalInstallResultExtensions.kt new file mode 100644 index 0000000000..873d454863 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/TerminalInstallResultExtensions.kt @@ -0,0 +1,31 @@ +package com.itsaky.androidide.utils + +import android.content.Context +import com.itsaky.androidide.resources.R +import java.io.IOException + +/** + * Throws if this result is anything other than [TerminalInstaller.InstallResult.Success]. + * Shared by SplitAssetsInstaller and BundledAssetsInstaller so a non-Success result can't + * be logged-and-ignored by one of them without the other -- that mismatch is exactly how + * ADFA-5037's "install() reports Success when it actually failed" bug happened once already. + */ +fun TerminalInstaller.InstallResult.throwIfNotSuccess(context: Context) { + when (this) { + is TerminalInstaller.InstallResult.Success -> {} + + is TerminalInstaller.InstallResult.Error.Interactive -> { + throw IOException("$title: $message") + } + + is TerminalInstaller.InstallResult.Error.IsSecondaryUser -> { + throw IOException( + context.getString(R.string.terminal_installation_failed_secondary_user), + ) + } + + is TerminalInstaller.InstallResult.NotInstalled -> { + throw IllegalStateException("Terminal installation failed: NotInstalled state") + } + } +}