ADFA-5037: Stop install() from swallowing doInstall() failures - #1632
Merged
davidschachterADFA merged 5 commits intoAug 6, 2026
Conversation
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 Walkthrough
WalkthroughThe asset installation flow now propagates corrupt-archive and installation failures with their causes. Cleanup runs after ChangesInstallation failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AssetsInstallationHelper
participant preInstall
participant postInstall
participant StagingDirectory
Caller->>AssetsInstallationHelper: install()
AssetsInstallationHelper->>preInstall: prepare assets
preInstall-->>AssetsInstallationHelper: throw installation exception
AssetsInstallationHelper->>postInstall: attempt cleanup
AssetsInstallationHelper->>StagingDirectory: delete staging directory
AssetsInstallationHelper-->>Caller: return failure with original cause
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 128-145: Restructure doInstall so ASSETS_INSTALLER.preInstall and
the subsequent installation work share the same outer try/finally block, with
cleanup always invoking postInstall and stagingDir.deleteRecursively(). Preserve
the existing exception logging, progress reporting, and rethrow behavior while
ensuring failures from preInstall also trigger symmetric installer and
staging-directory cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 292102f5-5ef7-4022-9941-ba904467ef79
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt
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.
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.
- 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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse JUnit Jupiter and Truth for this test class.
Replace the
org.junit.*imports and JUnit 4Assert.*assertions with JUnit Jupiter lifecycle annotations and Truth assertions for the new coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt` at line 19, Update AssetsInstallationHelperTest to use JUnit Jupiter lifecycle annotations instead of the org.junit imports, and replace all JUnit 4 Assert assertions with Truth assertions. Keep the existing test behavior and coverage unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt`:
- Line 19: Update AssetsInstallationHelperTest to use JUnit Jupiter lifecycle
annotations instead of the org.junit imports, and replace all JUnit 4 Assert
assertions with Truth assertions. Keep the existing test behavior and coverage
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 091f2215-8561-4b45-956b-f9c69ff72f8c
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktapp/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt
Daniel-ADFA
approved these changes
Aug 5, 2026
- 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.
Daniel-ADFA
approved these changes
Aug 6, 2026
davidschachterADFA
deleted the
bugfix/ADFA-5037-install-swallows-failure
branch
August 6, 2026 20:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AssetsInstallationHelper.install()wrapsdoInstall()inrunCatching, butdoInstall()'spreInstallfailure handling caughtFileNotFoundException/ZipException/IOExceptionand returned aResult.Failurevalue instead of throwing.runCatchingonly detects thrown exceptions, so that returnedFailurewas silently discarded andinstall()fell through toResult.Successregardless.install()'s exception handling (including the existingMissingAssetsEntryExceptionwrapping for the "missing/corrupt assets" case).SplitAssetsInstaller.preInstall()threwFileNotFoundException(external QA-testing zip not present), yetInstallationViewModellogged "Assets installation result: Success" and the app proceeded past onboarding with a half-installed toolchain (no JDK, no Gradle distribution extracted). Filed as ADFA-5037.Test plan
AssetsInstallationHelperTest's`install reports Failure when doInstall's own preInstall catch block swallows an exception`— lets the realdoInstall()run (unlike the existing`install with missing asset skips glitchtip`test, which mocksdoInstall()itself and so never exercises this bug), stubbing only the underlying installer'spreInstallto throw.dcda33cae(pre-fix) and passes with this change (:app:testV8DebugUnitTest --tests "com.itsaky.androidide.assets.AssetsInstallationHelperTest").com.itsaky.androidide.assets.*package tests pass.