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..9359ece5aa 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/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 = 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, 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 ba532d2f8a..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 @@ -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) } 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") + } + } +} 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..dda5ce2954 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/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(), 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 + 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(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), ) }