fix(install): unstick install gate + clean download log noise#547
Conversation
WalkthroughThis PR addresses stuck system installations where packages remain at 100% when the broadcast releasing the installation gate never arrives. It reduces the timeout from 60 to 15 seconds, adds detailed logging to track gate acquisition blocking, and improves cancellation handling in AndroidDownloader to distinguish transient cancellations from actual errors. ChangesSystem Install Timeout and Cancellation Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.kt (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
initiallyHeldByinstead ofpending.valuein the timeout warning.By the time line 32 executes,
pending.valuemay already benull(if the prior holder calledmarkCompletedin the narrow window after the timeout fired but before this log line) or may show a different package (if another force-claim raced in). Either way, the message "timed out waiting for X to clear" would show a misleading or empty holder name.initiallyHeldBy, captured before the timeout block, is the package that was actually blocking this caller when it started waiting.🛠️ Proposed fix
if (acquired == null) { Logger.w { - "SystemInstallSerializer: timed out waiting for ${pending.value} to clear; force-claiming for $packageName" + "SystemInstallSerializer: timed out waiting for $initiallyHeldBy to clear; force-claiming for $packageName" } pending.value = packageName🤖 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 `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.kt` around lines 30 - 34, The timeout log uses pending.value which can be changed by races; replace that with the earlier-captured initiallyHeldBy variable in the Logger.w message inside the branch where acquired == null so the log reads that it timed out waiting for initiallyHeldBy to clear before force-claiming for packageName; update the message construction in the DefaultSystemInstallSerializer's timeout path (where acquired, pending.value, initiallyHeldBy and packageName are in scope) to reference initiallyHeldBy instead of pending.value.
🤖 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.
Outside diff comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.kt`:
- Around line 30-34: The timeout log uses pending.value which can be changed by
races; replace that with the earlier-captured initiallyHeldBy variable in the
Logger.w message inside the branch where acquired == null so the log reads that
it timed out waiting for initiallyHeldBy to clear before force-claiming for
packageName; update the message construction in the
DefaultSystemInstallSerializer's timeout path (where acquired, pending.value,
initiallyHeldBy and packageName are in scope) to reference initiallyHeldBy
instead of pending.value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bd0156a-1723-4197-827c-fc2d1ab2c6c3
📒 Files selected for processing (3)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/SystemInstallSerializer.kt
1 participant
Surfaced from a field report of "downloads complete at 100% but never start installing" when mirror download is on and 2-3 apps are queued.
Root cause (Candidate A — confirmed by code read)
DefaultDownloadOrchestrator.runInstall(the AlwaysInstall path used by Shizuku/Dhizuku) handles the twoInstallOutcomevalues asymmetrically:COMPLETED(silent install succeeded) →markCompletedreleases the gate immediately. ✅DELEGATED_TO_SYSTEM(silent install fell back to the AndroidInstaller default-dialog path —SilentInstallerDispatcher.kt:110-112does this when the Shizuku binder is null, returns non-zero, or throws) → gate is intentionally not released, by design, because the system installer dialog is still up.The intentional non-release is correct for the dialog stacking case it was designed for. But when the user dismisses the dialog (or the dialog never opens because the fallback failed too), no
PACKAGE_REPLACEDbroadcast arrives, nomarkCompletedfires, and the gate stays held until the timeout. The other 2-3 apps in the queue are downloaded fully (UI shows 100%), then sit atawaitFreeAndMarkPendingwaiting on a gate that nothing will release.Default timeout was 60s. With 3 apps queued behind a stuck gate, that's up to 3×60s of "stuck at 100%" before the queue recovers. Matches the field-report symptom exactly.
Fix
Three small changes; no behaviour change for the happy path:
SystemInstallSerializer.DEFAULT_TIMEOUT_MS60_000L → 15_000L. Long enough for a normal Shizuku/Dhizuku silent install round-trip, short enough that a stuck gate recovers before the user gives up. Force-claim semantics on timeout are unchanged.DefaultSystemInstallSerializer.awaitFreeAndMarkPendingnow logs when it has to wait.Logger.i { "$packageName waiting for $heldBy to clear (timeout 15000ms)" }on entry-while-blocked, and a matching "acquired gate after waiting for $heldBy" on success. Future stuck reproductions show the queue clearly in Logcat.AndroidDownloaderno longer logsCancellationExceptionas an error. TheMultiSourceDownloaderrace cancels the loser racer on every download when a mirror is configured — that's the normal happy path. The previous catch-allcatch (e: Exception)was logging every losing racer asLogger.e { "Download failed" }, flooding Logcat with red lines that look like real failures and obscured the actual stuck-install signal in the field report. Cancellation now gets a dedicated catch that cleans up the temp file and rethrows without logging; non-cancellationExceptionstill logs as before.Test plan
:core:data:compileDebugKotlinAndroid✅:core:data:compileKotlinJvm✅Download failedfor losing racers.awaiting / acquiredpair so the next bug report is actionable.Notes
markCompletedafter the fallback intent fires (since the fallback intent IS the system dialog and the gate is held by definition there too — same logic as the inherentDELEGATED_TO_SYSTEMpath).Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements