ADFA-5037: Stop install() from swallowing doInstall() failures

app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt

@@ -7,9 +7,9 @@ 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.NonCancellable
 import kotlinx.coroutines.async
 import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.delay
@@ -81,7 +81,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,33 +124,27 @@ object AssetsInstallationHelper {
 		val stagingDir = Files.createTempDirectory(UUID.randomUUID().toString())
 		logger.debug("Staging directory ({}): {}", cpuArch, stagingDir)
 
-		// Ensure relevant shared libraries are loaded
-		Brotli4jLoader.ensureAvailability()
-
-		// pre-install hook
-		val isPreInstallSuccessful =
+		try {
+			// 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. 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)
-				true
 			} catch (e: FileNotFoundException) {
-				logger.error("ZIP file not found: {}", e.message)
-				flashError("File not found - ${e.message}")
-				false
+				logAndRethrow("ZIP file not found", e)
 			} catch (e: ZipException) {
-				logger.error("Invalid ZIP format: {}", e.message)
-				onProgress(Progress("Corrupt zip file ${e.message}"))
-				false
+				logAndRethrow("Invalid ZIP format", e)
 			} catch (e: IOException) {
-				logger.error("I/O error during preInstall: {}", e.message)
-				onProgress(Progress("Failed to load ${e.message}"))
-				false
+				logAndRethrow("I/O error during preInstall", e)
 			}
 
-		if (!isPreInstallSuccessful) {
-			return@coroutineScope Result.Failure(IOException("preInstall failed"))
-		}
-
-		try {
 			val entrySizes: Map<String, Long> =
 				expectedEntries.associateWith { entry ->
 					ASSETS_INSTALLER.expectedSize(entry)
@@ -222,15 +219,35 @@ object AssetsInstallationHelper {
 			// then cancel progress updater
 			progressUpdater.cancel()
 		} finally {
-			// Always run postInstall so zip/FS resources are closed (e.g. SplitAssetsInstaller.zipFile)
-			runCatching { ASSETS_INSTALLER.postInstall(context, stagingDir) }
-				.onFailure { e -> logger.warn("postInstall failed", e) }
-			if (Files.exists(stagingDir)) {
-				stagingDir.deleteRecursively()
-			}
+			// 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 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 { withContext(NonCancellable) { ASSETS_INSTALLER.postInstall(context, stagingDir) } }
+				.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,

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 -> {

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
@@ -20,6 +21,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 +113,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,9 +135,11 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() {
 										)
 									}
 
-								if (result !is TerminalInstaller.InstallResult.Success) {
-									logger.error("Failed to install terminal: {}", result)
-								}
+								// 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)
 							}

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")
+		}
+	}
+}

app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt

@@ -1,12 +1,22 @@
 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 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.unmockkAll
 import kotlinx.coroutines.runBlocking
+import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertThrows
@@ -27,37 +37,98 @@ 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<Context>(), any<AssetsInstallerProgressConsumer>())
+		} 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<Context>(), any<AssetsInstallerProgressConsumer>())
-			} returns null
-
 			coEvery {
 				helper["doInstall"](any<Context>(), any<AssetsInstallerProgressConsumer>())
 			} 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
+	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.
+			//
+			// 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)
+			mockkObject(SplitAssetsInstaller)
+
+			// 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<IDEBuildConfigProvider>(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<Path>()
+			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),
 			)
 		}