fix(apps): serialize sequential installs and unstick stale installedVersion#541
Merged
Conversation
…edVersion when versionCode is 0
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR introduces SystemInstallSerializer and DefaultSystemInstallSerializer, registers them in DI, and uses awaitFreeAndMarkPending/markCompleted to serialize system APK installs across the download orchestrator, auto-update worker, package event receiver, and presentation view models; localized release notes updated. ChangesInstall Serialization Coordination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/DefaultDownloadOrchestrator.kt (1)
343-348:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGate stays locked for 60 seconds after any install exception — missing
markCompletedin bothrunInstallandrunStandaloneInstallcatch blocks.Both
AutoUpdateWorkerandAppsViewModelcorrectly callmarkCompletedin their error handlers, butDefaultDownloadOrchestratordoes not. Ifinstaller.install(...)throws (or the coroutine is cancelled aftermarkPendingis called), thependingslot stays occupied and every subsequent queued install must wait the full 60 s timeout before it can fire.
markCompletedusescompareAndSet, so adding it beforemarkPendingwas ever reached is safe (no-op CAS).🐛 Proposed fix — `runInstall` (lines 343-348)
} catch (e: CancellationException) { + systemInstallSerializer.markCompleted(spec.packageName) throw e } catch (t: Throwable) { Logger.e(t) { "Orchestrator: install failed for ${spec.packageName}" } + systemInstallSerializer.markCompleted(spec.packageName) markFailed(spec.packageName, t.message) }🐛 Proposed fix — `runStandaloneInstall` (lines 572-578)
} catch (e: CancellationException) { + systemInstallSerializer.markCompleted(packageName) throw e } catch (t: Throwable) { Logger.e(t) { "Orchestrator: standalone install failed for $packageName" } + systemInstallSerializer.markCompleted(packageName) markFailed(packageName, t.message) null }Also applies to: 572-578
🤖 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/DefaultDownloadOrchestrator.kt` around lines 343 - 348, The catch blocks in runInstall and runStandaloneInstall currently log and call markFailed when installer.install(...) throws (or a coroutine is cancelled), but they do not call markCompleted, leaving the pending slot locked for 60s; update both catch (t: Throwable) handlers to call markCompleted(spec.packageName) (in addition to markFailed(spec.packageName, t.message)) so the pending slot is cleared immediately; keep the existing CancellationException rethrow behavior intact and ensure markCompleted is called safely (it's a no-op if markPending was never set).
🤖 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
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.kt`:
- Around line 12-28: The current two-step wait-then-claim (awaitFreeOrTimeout +
markPending) is racy; replace it with a single atomic suspend that waits and
claims the slot for a package: add suspend fun
awaitFreeAndMarkPending(packageName: String, timeoutMs: Long) that uses
withTimeoutOrNull(timeoutMs) { while (!pending.compareAndSet(null, packageName))
{ pending.first { it == null } } } and on timeout logs and force-sets
pending.value = packageName; keep awaitFreeOrTimeout as a no-op for binary
compatibility (or call into the new API) and update callers to use
awaitFreeAndMarkPending instead of calling awaitFreeOrTimeout then markPending
to eliminate the TOCTOU race.
- Around line 14-23: The timeout check misinterprets success as timeout because
the withTimeoutOrNull block returns the sentinel null from pending.first, so
change the block in DefaultSystemInstallSerializer that assigns freed (the
withTimeoutOrNull { pending.first { it == null } }) to return Unit on success
(e.g., call pending.first { it == null } and then return Unit) so that
withTimeoutOrNull returns Unit on success and null only on a real timeout;
update the subsequent conditional to treat freed == null as a genuine timeout
and only then log the warning and set pending.value = null.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 2081-2083: Calls to systemInstallSerializer.awaitFreeOrTimeout()
and systemInstallSerializer.markPending(...) are executed unconditionally which
causes desktop/JVM installs to block for the full timeout because
markCompleted() is never called there; wrap the two calls in a platform guard so
they only run on Android: check the current platform (compare to
Platform.ANDROID) and only invoke systemInstallSerializer.awaitFreeOrTimeout()
and systemInstallSerializer.markPending(validatedApkInfo?.packageName ?: "")
when on Android, leaving installer.install(filePath, ext) unchanged for
non-Android targets; reference the existing symbols systemInstallSerializer,
awaitFreeOrTimeout, markPending, markCompleted, installer.install, and
Platform.ANDROID when making the change.
- Around line 2081-2083: The gate can be left locked when validatedApkInfo is
null and suffers a race between awaitFreeOrTimeout() and markPending(); fix by
only calling systemInstallSerializer.markPending(packageName) when
validatedApkInfo != null (i.e., pass the real package name instead of ""),
and/or make markCompleted() clear the gate unconditionally (remove the strict
compareAndSet(packageName, null) behavior). Also serialize the check-and-set
pair by adding a Mutex around the sequence
systemInstallSerializer.awaitFreeOrTimeout() followed immediately by
systemInstallSerializer.markPending(...) (or implement an atomic compare-and-set
inside systemInstallSerializer) so two coroutines cannot both pass the free
check and overwrite pending.value. Ensure callers use the
validatedApkInfo.packageName when present and avoid marking pending for null
APKs.
---
Outside diff comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt`:
- Around line 343-348: The catch blocks in runInstall and runStandaloneInstall
currently log and call markFailed when installer.install(...) throws (or a
coroutine is cancelled), but they do not call markCompleted, leaving the pending
slot locked for 60s; update both catch (t: Throwable) handlers to call
markCompleted(spec.packageName) (in addition to markFailed(spec.packageName,
t.message)) so the pending slot is cleared immediately; keep the existing
CancellationException rethrow behavior intact and ensure markCompleted is called
safely (it's a no-op if markPending was never set).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8592b693-8ea9-4e6a-a860-39d420f86b71
📒 Files selected for processing (23)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/SystemInstallSerializer.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/17.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/17.jsonfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
…roid-only details install gating
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
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt`:
- Around line 347-348: The catch (CancellationException) blocks in
DefaultDownloadOrchestrator (where
systemInstallSerializer.markCompleted(spec.packageName) is called)
unconditionally release the install gate; modify those handlers to check whether
the install was delegated to the platform installer before calling
markCompleted(spec.packageName) — e.g., consult the delegation state variable
used when delegating the install (the same flag checked by the broadcast
receiver/timeout) and only call systemInstallSerializer.markCompleted(...) if
delegation did not occur, otherwise skip immediate release and let the
receiver/timeout complete the flow; apply the same change to the other catch
block at the alternate location referenced in the review.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc3f9dab-52b5-4568-96ea-1ba4f01dbafe
📒 Files selected for processing (6)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/SystemInstallSerializer.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/SystemInstallSerializer.kt
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt
- feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultSystemInstallSerializer.kt
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
1 participant
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.
Fixes two related bugs surfaced when a user updates several apps in succession from the Apps screen.
Bug 1 — Stacking ACTION_VIEW intents on OEM PackageInstaller activities
AndroidInstaller.installreturnsDELEGATED_TO_SYSTEMimmediately — the system install dialog shows but the coroutine continues.AppsViewModel.updateSingleApp,DefaultDownloadOrchestrator.runStandaloneInstall/runInstall,AutoUpdateWorker.updateApp, andDetailsViewModel.installReleaseall then proceed to the next install before the previous system dialog has settled. Android'sPackageInstalleractivity issingleTask; secondACTION_VIEWintents go throughonNewIntent. AOSP refreshes the dialog data, but Xiaomi MIUI / OPPO ColorOS / vivo Funtouch / Honor MagicOS / several Samsung One UI variants do not — the dialog keeps showing the first APK. User confirms what's displayed, the first APK installs again, the second app's install never happens.Fix
New
SystemInstallSerializer(single-flightMutableStateFlow<String?>), registered as a Koin singleton incoreModule:awaitFreeOrTimeout(timeoutMs = 60_000)— suspends until pending package clears or timeout (force-releases on timeout to recover from a dropped broadcast / user dismissal).markPending(packageName)— call right beforeinstaller.install.markCompleted(packageName)— released byPackageEventReceiver.onPackageInstalled/onPackageRemovedfor the matching package, plus by every error path on the call site.Wired into all four
installer.installcall sites and into both broadcast handlers. Sequential installs now wait for eachPACKAGE_REPLACED(or 60s timeout) before firing the nextACTION_VIEW, eliminating the stacking on OEM ROMs.Bug 2 —
installedVersiontag stays stale whenlatestVersionCode == 0PackageEventReceiver.onPackageInstalleddecided "did the install reach the target version" viaexpectedVersionCode > 0L && systemInfo.versionCode >= expectedVersionCode. When the orchestrator's earlier APK-info extraction failed (split APKs, large APKs, encrypted manifest, AOSPPackageManager.getPackageArchiveInfoedge cases),latestVersionCodewas0L, so the heuristic always returned false → the else-branch ran →repo.updateApp(app.copy(installedVersionName = …, installedVersionCode = …))wrote the system fields but never touchedinstalledVersion(the release tag). Apps row kept rendering the pre-install tag forever after.Fix
wasActuallyUpdatedto fall back toversionNamechange detection whenexpectedVersionCode <= 0L.installedVersion = app.latestVersion ?: systemInfo.versionNameso the row's primary version label moves forward whenever the user actually accepted some install.Test plan
installedVersionupdates after install.:composeApp:compileDebugKotlinAndroid✅,:core:data:compileKotlinJvm✅.Notes
DefaultSystemInstallSerializerdeferred — repo has zero*Test.ktfiles. When test infra lands, cover: free path, suspend-on-pending, timeout force-release, compareAndSet onmarkCompleted.AutoUpdateWorkerstill uses unscoped filenames at line 140/159/162 instead ofAssetFileName.scoped. Mitigated by the existing signing-fingerprint + packageName checks beforeinstaller.install. Filed for a follow-up sweep that routesAutoUpdateWorkerthroughDownloadOrchestrator.What's-new
Summary by CodeRabbit