ADFA-2787 Handle aborted onboarding flow correctly without raising an error

[hal-eisen-adfa]

Stop incorrectly throwing an error for JobCancellation
Clean up partial asset installation (idempotent)
Stop and restart web server to eliminate EADDRINUSE error in the same Sentry issue

[coderabbitai]

📝 Walkthrough

Release Notes - ADFA-2787: Handle aborted onboarding flow correctly without raising an error

Features & Improvements

• Proper CancellationException Handling: Coroutine cancellations are now properly propagated instead of being treated as regular failures across the asset installation flow and view model setup, allowing graceful cancellation of onboarding operations
• Idempotent Asset Installation: Asset installation now cleans up partial/stale files before extraction by deleting and recreating destination directories (Gradle distribution, Android SDK, Maven repos, and plugin artifacts), preventing conflicts from incomplete previous attempts
• Improved WebServer Lifecycle Management: Added explicit stop() method to WebServer for clean shutdown and proper resource cleanup, with explicit socket reuse configuration to prevent EADDRINUSE (port already in use) errors on restart
• Enhanced Socket Error Handling: WebServer now distinguishes between accept failures (when server is closed) and client-driven socket exceptions, providing more granular error recovery

⚠️ Breaking Changes & Risks

• API Breaking Change: BundledAssetsInstaller.doInstall() signature updated from (context: Context, stagingDir: Path) to (context: Context, stagingDir: Path, cpuArch: CpuArch, entryName: String) - all callers must be updated
• Destructive Directory Cleanup: Asset installation now uses deleteRecursively() to remove entire destination directories before extraction. Risk: If extraction fails partway through, the original content is lost and cannot be recovered; ensure adequate backups exist for critical asset locations
• New State Management Requirement: MainActivity now holds a WebServer reference field requiring careful lifecycle management; failure to properly stop the server in onDestroy() could leak resources or prevent port reuse
• Experimental API Usage: Code now depends on Kotlin's ExperimentalPathApi for path operations (deleteRecursively), which may change in future Kotlin releases

Testing Recommendations

• Verify CancellationException propagation correctly interrupts and cleans up ongoing installation without corrupting app state
• Test asset installation with network interruptions to ensure partial extraction cleanup works correctly
• Validate WebServer can be stopped and restarted multiple times without port binding errors
• Confirm no resource leaks occur when MainActivity is destroyed during active WebServer operations

Walkthrough

Adds cancellation propagation and cleanup-before-extract behavior to asset installation flows, changes BundledAssetsInstaller.doInstall signature to accept CPU arch and entry name, and hardens the local web server with a stop() method and safer socket/connection handling; MainActivity retains a reference to stop the server.

Changes

Cohort / File(s)	Summary
Cancellation handling app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt, app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt	Detect and rethrow CancellationException in failure paths so coroutine cancellation is propagated instead of treated as regular errors; added necessary imports and state adjustments.
Bundled assets installer API & extraction cleanup app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt	Changed doInstall signature to (context, stagingDir, cpuArch, entryName). For specific archive entries, delete existing destination directories recursively, recreate destinations, perform path-traversal checks for plugins, and extract into cleaned directories. Uses @OptIn(ExperimentalPathApi).
Split assets installer cleanup app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt	Adds pre-extraction deletion and recreation of destination/plugin directories, path normalization checks, and @OptIn(ExperimentalPathApi) usage for deleteRecursively.
MainActivity web server lifecycle app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt	Introduces a webServer field, assigns the running server to it when started, clears it in finally, and stops the server from onDestroy to ensure proper shutdown.
Local WebServer robustness app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt	Adds a public stop() method to close the server socket safely, reworks socket bind/reuse and accept loop to handle SocketException on close, improves per-connection error handling and ensures client sockets are closed in all paths.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• ADFA-2446 | Improve terminal bootstrap install resilience and user messaging #812: Overlaps on AssetsInstallationHelper exception handling and retry/propagation behavior.
• ADFA-2498 Plumbing for plugin-artifacts.zip #792: Related changes to BundledAssetsInstaller/SplitAssetsInstaller around plugin extraction and path-traversal protections.
• ADFA-2206: Fix NPE in asset installation when zip entry is missing #699: Touches the same installation and viewmodel failure-handling paths.

Suggested reviewers

• itsaky-adfa
• jomen-adfa
• Daniel-ADFA

Poem

🐰 I hopped through dirs both clean and neat,

Cleared old bundles off my feet,
If cancellation knocks the door,
I hop away — not handle more,
Safe sockets closed, the server sleeps so sweet.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'ADFA-2787 Handle aborted onboarding flow correctly without raising an error' directly aligns with the main objectives: preventing errors on JobCancellation/coroutine cancellation and managing the onboarding flow state.
Description check	✅ Passed	The description lists three related changes (JobCancellation handling, asset cleanup, web server restart) that directly correspond to the actual modifications in the changeset across multiple files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ADFA-2787-JobCancellationException-Job-was-cancelled

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt (1)

75-75: Consider marking webServer as @Volatile.

This field is written on an IO dispatcher thread (lines 333, 338) and read on the main thread in onDestroy (line 344). Without @Volatile (or another memory-barrier mechanism), the main thread could theoretically read a stale null and skip calling stop(), leaving the server socket open.

Suggested fix

-	private var webServer: WebServer? = null
+	`@Volatile`
+	private var webServer: WebServer? = null

app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)

118-125: Prefer serverSocket.isClosed over parsing the exception message.

Checking e.message?.contains("Closed") is fragile — the message text is implementation-defined and could differ across Android versions or JVM vendors. Checking serverSocket.isClosed is deterministic and conveys intent more clearly.

The same applies to the client-handling catch at line 129.

Suggested fix

                     } catch (e: java.net.SocketException) {
-                        if (e.message?.contains("Closed", ignoreCase = true) == true) {
+                        if (serverSocket.isClosed) {
                             if (debugEnabled) log.debug("WebServer socket closed, shutting down.")
                             break
                         }
                         log.error("Accept failed: {}", e.message)
                         continue
                     }

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt (1)

70-119: ⚠️ Potential issue | 🟠 Major

State not reset on cancellation — UI may get stuck in Installing state.

When CancellationException is rethrown on line 108, the _state remains Installing() (set on line 71). If the user re-enters this screen or the ViewModel survives the cancellation, startIdeSetup will bail out on line 60-62 because _state.value is Installing. Consider adding a finally block to reset the state on cancellation.

Suggested approach

 			try {
 				_state.update { Installing() }
 				// ... existing code ...
 			} catch (e: Exception) {
 				if (e is CancellationException) {
+					_state.update { InstallationPending }
 					throw e
 				}
 				// ... existing error handling ...
+			} finally {
+				// Alternatively, reset here if state is still Installing
 			}

app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (1)

71-79: ⚠️ Potential issue | 🟡 Minor

Ensure parent directory exists before writing GRADLE_API_NAME_JAR file.

The destFile.outputStream() call on line 75 will fail if GRADLE_GEN_JARS doesn't exist. While the directory is initialized at app startup, adding an explicit mkdirs() provides defensive robustness and matches the pattern used for similar operations (e.g., LLAMA_AAR at line 140, PLUGIN_ARTIFACTS_ZIP at line 178).

Suggested fix

 GRADLE_API_NAME_JAR_ZIP -> {
 	val assetPath = ToolsManager.getCommonAsset(GRADLE_API_NAME_JAR_BR)
 	BrotliInputStream(assets.open(assetPath)).use { input ->
 		val destFile = Environment.GRADLE_GEN_JARS.resolve(GRADLE_API_NAME_JAR)
+		destFile.parentFile?.mkdirs()
 		destFile.outputStream().use { output ->
 			input.copyTo(output)
 		}
 	}
 }

Note: The DOCUMENTATION_DB installation (line 119–120) has the same vulnerability.

🧹 Nitpick comments (2)

app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)

134-166: Plugin zip extraction duplicates logic from extractZipToDir — consider reusing.

The manual ZipInputStream iteration with path traversal checks (lines 144-164) mirrors AssetsInstallationHelper.extractZipToDir but skips directory entries. Since extractZipToDir already handles both files and directories with the same security checks, you could potentially call it here instead.

app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (1)

170-207: Same plugin extraction duplication as in SplitAssetsInstaller.

Both BundledAssetsInstaller and SplitAssetsInstaller have nearly identical plugin zip extraction logic (manual ZipInputStream iteration + path traversal check). Consider extracting this into a shared helper method, potentially on AssetsInstallationHelper or BaseAssetsInstaller.