From fa51e095224c307485eda573503ad988f2cc7f5e Mon Sep 17 00:00:00 2001 From: Henrique Velloso Date: Thu, 12 Feb 2026 20:31:59 -0300 Subject: [PATCH] fix: #319 install queue gets stuck with multiple concurrent downloads - Protect InstallCancelled from being overwritten by stale native events - Preserve watchdog timestamps across state transitions to prevent timeout reset - Clear install slot in orphan handler and sync to unblock the queue - Add watchdog coverage for ReadyToInstall (detect silent native failures) - Use Activity context for install dialog to prevent Android task suppression - Re-launch pending install dialog from Kotlin watchdog when app is in foreground - Exclude InstallCancelled from batch progress totals - Auto-clear completed operations after 3s with proper Timer cancellation - Dynamic "updated"/"installed" label in batch completion banner --- .../plugins/AndroidPackageManagerPlugin.kt | 73 +++++++++++++++++-- lib/screens/updates_screen.dart | 21 +++++- .../android_package_manager.dart | 51 +++++++++++-- .../package_manager/install_operation.dart | 20 ++++- .../package_manager/package_manager.dart | 63 +++++++++++----- 5 files changed, 190 insertions(+), 38 deletions(-) diff --git a/android/app/src/main/kotlin/dev/zapstore/app/plugins/AndroidPackageManagerPlugin.kt b/android/app/src/main/kotlin/dev/zapstore/app/plugins/AndroidPackageManagerPlugin.kt index 3132240..8816f87 100644 --- a/android/app/src/main/kotlin/dev/zapstore/app/plugins/AndroidPackageManagerPlugin.kt +++ b/android/app/src/main/kotlin/dev/zapstore/app/plugins/AndroidPackageManagerPlugin.kt @@ -17,6 +17,8 @@ import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.ProcessLifecycleOwner import io.flutter.embedding.engine.plugins.FlutterPlugin +import io.flutter.embedding.engine.plugins.activity.ActivityAware +import io.flutter.embedding.engine.plugins.activity.ActivityPluginBinding import io.flutter.plugin.common.EventChannel import io.flutter.plugin.common.MethodCall import io.flutter.plugin.common.MethodChannel @@ -90,7 +92,7 @@ object ErrorCode { * no polling, no probing, no hanging awaits. */ class AndroidPackageManagerPlugin : - FlutterPlugin, MethodCallHandler, EventChannel.StreamHandler, DefaultLifecycleObserver { + FlutterPlugin, MethodCallHandler, EventChannel.StreamHandler, DefaultLifecycleObserver, ActivityAware { private lateinit var methodChannel: MethodChannel private lateinit var eventChannel: EventChannel @@ -138,6 +140,15 @@ class AndroidPackageManagerPlugin : private var appContext: Context? = null + /** + * Weak reference to the current Activity. Used by launchConfirmDialog to start + * the PackageInstaller confirmation dialog in the same task as the app, which + * ensures the dialog appears on top of the current UI. Using Application context + * with FLAG_ACTIVITY_NEW_TASK creates a separate task that Android may place + * behind the current app, making the dialog invisible to the user. + */ + private var activityContext: java.lang.ref.WeakReference? = null + /** * Called by InstallResultReceiver when a broadcast arrives. Emits the status event to Dart * via EventChannel. @@ -218,7 +229,6 @@ class AndroidPackageManagerPlugin : } private fun launchConfirmDialog(packageName: String, intent: Intent) { - val ctx = appContext ?: return val inst = instance // Post with delay to ensure the system is ready to show the dialog. @@ -226,11 +236,31 @@ class AndroidPackageManagerPlugin : // immediately after session.commit() while the app is still processing. val launcher: () -> Unit = { try { - intent.addFlags( - Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP - ) - ctx.startActivity(intent) - Log.d(TAG, "Launched confirmation dialog for $packageName") + // CRITICAL: Prefer Activity context over Application context. + // Using Application context with FLAG_ACTIVITY_NEW_TASK creates + // the install dialog in a SEPARATE task. When the user is actively + // interacting with the app, Android may place this new task behind + // the current one, making the dialog invisible. + // Using Activity context starts the dialog in the SAME task, + // guaranteeing it appears on top of the current UI. + val activity = activityContext?.get() + if (activity != null && !activity.isFinishing && !activity.isDestroyed) { + // Remove NEW_TASK flag when launching from Activity context — + // we want the dialog in the same task stack. + intent.flags = intent.flags and Intent.FLAG_ACTIVITY_NEW_TASK.inv() + activity.startActivity(intent) + Log.d(TAG, "Launched confirmation dialog for $packageName (Activity context)") + } else { + // Fallback to Application context (e.g., during config change) + val ctx = appContext + if (ctx != null) { + intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + ctx.startActivity(intent) + Log.d(TAG, "Launched confirmation dialog for $packageName (Application context fallback)") + } else { + Log.w(TAG, "Cannot launch dialog for $packageName: no context available") + } + } } catch (e: Exception) { Log.w(TAG, "Failed to launch confirmation dialog for $packageName", e) } @@ -291,6 +321,26 @@ class AndroidPackageManagerPlugin : instance = null } + // ═══════════════════════════════════════════════════════════════════════════════ + // ACTIVITY AWARE (Provides Activity context for dialog launching) + // ═══════════════════════════════════════════════════════════════════════════════ + + override fun onAttachedToActivity(binding: ActivityPluginBinding) { + activityContext = java.lang.ref.WeakReference(binding.activity) + } + + override fun onDetachedFromActivityForConfigChanges() { + activityContext = null + } + + override fun onReattachedToActivityForConfigChanges(binding: ActivityPluginBinding) { + activityContext = java.lang.ref.WeakReference(binding.activity) + } + + override fun onDetachedFromActivity() { + activityContext = null + } + // ═══════════════════════════════════════════════════════════════════════════════ // LIFECYCLE OBSERVER (Auto foreground detection) // ═══════════════════════════════════════════════════════════════════════════════ @@ -504,6 +554,15 @@ class AndroidPackageManagerPlugin : if (now2 < deadline && (hasSession || hasPendingUi || verifyingAlive)) { // Nudge Dart to show the most accurate state (esp. after reconnect). if (hasPendingUi) { + // Re-launch the dialog in case it was suppressed by user interaction + // or Android's activity stack. The initial launchConfirmDialog may + // have failed silently if the user was actively using the app. + if (isAppInForeground) { + pendingUserActionIntents[appId]?.let { intent -> + Log.d(TAG, "Watchdog: Re-launching install dialog for $appId") + launchConfirmDialog(appId, intent) + } + } emitInstallStatus( appId, InstallStatus.PENDING_USER_ACTION, diff --git a/lib/screens/updates_screen.dart b/lib/screens/updates_screen.dart index 51616ac..90bbc6b 100644 --- a/lib/screens/updates_screen.dart +++ b/lib/screens/updates_screen.dart @@ -381,23 +381,36 @@ class _UpdatesListBody extends HookConsumerWidget { final showAllDone = useState(false); final wasInProgress = useRef(false); - // Detect transition from in-progress to all-complete + // Detect transition from in-progress to all-complete. + // FEAT-001 spec: "After 3 seconds with no in-progress operations, + // completed operations auto-clear." + final autoClearTimer = useRef(null); useEffect(() { if (progress != null && progress.hasInProgress) { + autoClearTimer.value?.cancel(); wasInProgress.value = true; showAllDone.value = false; // Reset if new operations start } else if (wasInProgress.value && progress != null && progress.isAllComplete) { - // Just finished - show "All done" until dismissed + // Just finished - show "All done" banner briefly, then auto-clear showAllDone.value = true; wasInProgress.value = false; + autoClearTimer.value = Timer(const Duration(seconds: 3), () { + if (showAllDone.value) { + showAllDone.value = false; + ref + .read(packageManagerProvider.notifier) + .clearCompletedOperations(); + } + }); } else if (progress == null) { // Operations cleared externally + autoClearTimer.value?.cancel(); wasInProgress.value = false; showAllDone.value = false; } - return null; + return () => autoClearTimer.value?.cancel(); }, [progress?.hasInProgress, progress?.isAllComplete]); // Determine what to show (mutually exclusive states): @@ -426,7 +439,7 @@ class _UpdatesListBody extends HookConsumerWidget { child: _StatusBannerContent( icon: Icons.check_circle_rounded, iconColor: Colors.green.shade400, - text: 'All done (${progress?.completed ?? 0} updated)', + text: 'All done (${progress?.completed ?? 0} ${progress?.completedLabel ?? 'completed'})', failedCount: progress?.failed ?? 0, showDismiss: true, onTap: () { diff --git a/lib/services/package_manager/android_package_manager.dart b/lib/services/package_manager/android_package_manager.dart index 67a76fd..b2c8749 100644 --- a/lib/services/package_manager/android_package_manager.dart +++ b/lib/services/package_manager/android_package_manager.dart @@ -164,6 +164,9 @@ final class AndroidPackageManager extends PackageManager { debugPrint( '[PackageManager] No tracked operation for appId=$appId, aborting orphaned native session', ); + // Release the install slot in case this app was the active install + // (e.g., sync cleared the operation before the native event arrived). + clearInstallSlot(appId); unawaited(abortInstall(appId)); } return; @@ -183,6 +186,15 @@ final class AndroidPackageManager extends PackageManager { return; } + // INVARIANT: Once the user cancels (or the system dismisses the dialog), + // the operation is InstallCancelled. The native side may retry the install + // session (sending verifying/started/pendingUserAction), but we must NOT + // let those events overwrite InstallCancelled. The only way out of + // InstallCancelled is the user tapping "Install (retry)" (FEAT-001 spec). + if (existingOp is InstallCancelled) { + return; + } + final target = existingOp.target; final filePath = existingOp.filePath; @@ -217,7 +229,12 @@ final class AndroidPackageManager extends PackageManager { case InstallStatus.pendingUserAction: // User action required. Ensure we don't get stuck in Verifying if the // STARTED event was missed; show Installing state. - if (filePath != null && existingOp is! Installing) { + // Skip if already in Installing or SystemProcessing to preserve the + // original startedAt timestamp — otherwise the watchdog timeout resets + // on every pendingUserAction/systemProcessing bounce and never fires. + if (filePath != null && + existingOp is! Installing && + existingOp is! SystemProcessing) { final pkg = state.installed[appId]; final isSilent = pkg?.canInstallSilently ?? false; setOperation( @@ -241,10 +258,20 @@ final class AndroidPackageManager extends PackageManager { case InstallStatus.systemProcessing: // Install session is committed and taking longer than expected. // Cannot be cancelled - system will eventually complete or fail. - if (filePath != null) { + // Only create a new SystemProcessing if not already in that state, + // to preserve the original startedAt timestamp for the watchdog. + // CRITICAL: When transitioning from Installing, preserve the original + // startedAt so the Dart watchdog doesn't reset its countdown. + if (filePath != null && existingOp is! SystemProcessing) { + final preservedStart = + existingOp is Installing ? existingOp.startedAt : null; setOperation( appId, - SystemProcessing(target: target, filePath: filePath), + SystemProcessing( + target: target, + filePath: filePath, + startedAt: preservedStart, + ), ); } break; @@ -256,13 +283,16 @@ final class AndroidPackageManager extends PackageManager { case InstallStatus.success: _deleteFile(filePath); + // Check BEFORE updating installed package info — after update the app + // will always appear as installed, losing the update/new-install distinction. + final wasInstalled = isInstalled(appId); // CRITICAL: Update installed package info DIRECTLY from target metadata. // We cannot rely on syncInstalledPackages() here because Android's package // database may not have committed yet, causing a race condition where // we get stale data and show "Update" instead of "Open". _updateInstalledPackage(appId, target); // Transition to Completed state (stays in map for batch progress tracking) - setOperation(appId, Completed(target: target)); + setOperation(appId, Completed(target: target, isUpdate: wasInstalled)); // Sync in background to get accurate info (signature hash, etc.) // but our state machine doesn't depend on it. unawaited(syncInstalledPackages()); @@ -716,12 +746,21 @@ final class AndroidPackageManager extends PackageManager { (installedV != null && installedV == targetV); // Only clear if we can establish completion reliably. + // Transition to Completed (not clearOperation) so batchProgressProvider + // counts this app. The Completed op stays until the user dismisses the + // banner or clearCompletedOperations runs. if (completed) { debugPrint( - '[PackageManager] Sync: clearing completed operation for $appId ' + '[PackageManager] Sync: completing operation for $appId ' '(installedVc=$installedVc, targetVc=$targetVc, installedV=$installedV, targetV=$targetV)', ); - clearOperation(appId); + // Sync fallback: state.installed was already overwritten with native + // data above (line 723), so we must NOT call _updateInstalledPackage + // here — that would replace accurate native info with target metadata. + // isUpdate defaults to true: sync mostly catches silent updates where + // the app was already installed. The impact of a wrong label is cosmetic. + setOperation(appId, Completed(target: op.target, isUpdate: true)); + clearInstallSlot(appId); } else { debugPrint( '[PackageManager] Sync: keeping operation for $appId ' diff --git a/lib/services/package_manager/install_operation.dart b/lib/services/package_manager/install_operation.dart index a3b2d41..7af1843 100644 --- a/lib/services/package_manager/install_operation.dart +++ b/lib/services/package_manager/install_operation.dart @@ -123,11 +123,18 @@ class AwaitingPermission extends InstallOperation { // INSTALL PHASE (No cancel - Android controls) // ═══════════════════════════════════════════════════════════════════════════════ -/// File verified, waiting for install to be triggered +/// File verified, waiting for install to be triggered. +/// [triggeredAt] is set when [triggerInstall] is actually called, so the +/// watchdog can detect native sessions that never respond. class ReadyToInstall extends InstallOperation { final String filePath; + final DateTime? triggeredAt; - const ReadyToInstall({required super.target, required this.filePath}); + const ReadyToInstall({ + required super.target, + required this.filePath, + this.triggeredAt, + }); } /// Native installation in progress @@ -182,7 +189,10 @@ class SystemProcessing extends InstallOperation { class Completed extends InstallOperation { final DateTime completedAt; - Completed({required super.target, DateTime? completedAt}) + /// Whether this was an update (app was already installed) or a new install. + final bool isUpdate; + + Completed({required super.target, DateTime? completedAt, this.isUpdate = false}) : completedAt = completedAt ?? DateTime.now(); } @@ -258,7 +268,8 @@ extension InstallOperationX on InstallOperation { this is Downloading || this is Verifying || this is Installing || - this is SystemProcessing; + this is SystemProcessing || + (this is ReadyToInstall && (this as ReadyToInstall).triggeredAt != null); /// Whether this operation is in the verification phase bool get isVerifying => this is Verifying; @@ -277,6 +288,7 @@ extension InstallOperationX on InstallOperation { Verifying(:final startedAt) => startedAt, Installing(:final startedAt) => startedAt, SystemProcessing(:final startedAt) => startedAt, + ReadyToInstall(:final triggeredAt) => triggeredAt, _ => null, }; diff --git a/lib/services/package_manager/package_manager.dart b/lib/services/package_manager/package_manager.dart index 54c31b6..0925a03 100644 --- a/lib/services/package_manager/package_manager.dart +++ b/lib/services/package_manager/package_manager.dart @@ -255,7 +255,7 @@ abstract class PackageManager extends StateNotifier { filePath: op.filePath, ), ); - if (op is Installing || op is SystemProcessing) { + if (op is Installing || op is SystemProcessing || op is ReadyToInstall) { clearInstallSlot(appId); needsQueueProcessing = true; } @@ -308,11 +308,15 @@ abstract class PackageManager extends StateNotifier { _updateWatchdogTimer(); } - /// Clear all completed and failed operations from the map. + /// Clear all terminal operations from the map. /// Called after the batch completion display timeout. + /// Terminal states: Completed, OperationFailed, InstallCancelled. void clearCompletedOperations() { final remaining = Map.of(state.operations) - ..removeWhere((_, op) => op is Completed || op is OperationFailed); + ..removeWhere( + (_, op) => + op is Completed || op is OperationFailed || op is InstallCancelled, + ); state = state.copyWith(operations: remaining); _updateWatchdogTimer(); } @@ -997,7 +1001,16 @@ abstract class PackageManager extends StateNotifier { if (op is ReadyToInstall) { activeInstall = appId; - debugPrint('[PackageManager] Starting install for $appId'); + // Mark triggeredAt so the watchdog can detect native sessions that + // never respond (e.g. Android PackageInstaller silently fails). + setOperation( + appId, + ReadyToInstall( + target: op.target, + filePath: op.filePath, + triggeredAt: DateTime.now(), + ), + ); unawaited(triggerInstall(appId)); } // If operation changed, it will be picked up on next process cycle @@ -1460,28 +1473,31 @@ enum BatchPhase { downloading, verifying, installing, completed, idle } /// Batch progress summary - ALL state derived from operations map. /// -/// Key insight: total = operations.length (includes Completed state). /// When an operation succeeds, it transitions to Completed instead of being removed. -/// This allows us to derive completed count without separate tracking. +/// InstallCancelled operations are excluded from totals (already resolved). class BatchProgress { const BatchProgress({ required this.total, required this.completed, + required this.completedLabel, required this.downloading, required this.verifying, required this.installing, required this.queued, required this.failed, - required this.cancelled, required this.phase, }); - /// Total operations (everything in the map, including completed) + /// Total operations (excludes InstallCancelled — those are already resolved + /// from the batch's perspective and should not inflate the count). final int total; /// Operations that completed successfully final int completed; + /// Label for completed operations: "updated", "installed", or "completed" (mixed) + final String completedLabel; + /// Operations currently downloading final int downloading; @@ -1497,14 +1513,11 @@ class BatchProgress { /// Operations that failed final int failed; - /// Operations cancelled by user (InstallCancelled - can retry individually) - final int cancelled; - /// Current dominant phase final BatchPhase phase; /// Whether any operations are in progress (not terminal) - /// Terminal states: Completed, OperationFailed, InstallCancelled + /// Terminal states: Completed, OperationFailed bool get hasInProgress => downloading > 0 || verifying > 0 || installing > 0 || queued > 0; @@ -1529,17 +1542,18 @@ final batchProgressProvider = Provider((ref) { // Count operations by type int completed = 0, + completedUpdates = 0, downloading = 0, verifying = 0, installing = 0, queued = 0, - failed = 0, - cancelled = 0; + failed = 0; for (final op in ops.values) { switch (op) { case Completed(): completed++; + if (op.isUpdate) completedUpdates++; case DownloadQueued() || ReadyToInstall(): queued++; case Downloading() || DownloadPaused(): @@ -1551,7 +1565,9 @@ final batchProgressProvider = Provider((ref) { case OperationFailed(): failed++; case InstallCancelled(): - cancelled++; // Terminal for batch - user can retry individually + // Not counted in batch — already resolved (user can retry individually). + // Stays in the operations map for the "Install (retry)" UI button. + break; case AwaitingPermission(): queued++; // Waiting for permission case Uninstalling(): @@ -1559,7 +1575,12 @@ final batchProgressProvider = Provider((ref) { } } - final total = ops.length; + // Total excludes InstallCancelled — those are already resolved from the + // batch's perspective and should not inflate the count. + final total = completed + downloading + verifying + installing + queued + failed; + + // If only InstallCancelled operations remain, no batch to show + if (total == 0) return null; // Determine current phase (priority: installing > verifying > downloading > completed) final phase = installing > 0 @@ -1572,15 +1593,23 @@ final batchProgressProvider = Provider((ref) { ? BatchPhase.completed : BatchPhase.idle; + // Derive label: "updated" if all updates, "installed" if all new, "completed" if mixed + final completedInstalls = completed - completedUpdates; + final completedLabel = completedUpdates > 0 && completedInstalls == 0 + ? 'updated' + : completedInstalls > 0 && completedUpdates == 0 + ? 'installed' + : 'completed'; + return BatchProgress( total: total, completed: completed, + completedLabel: completedLabel, downloading: downloading, verifying: verifying, installing: installing, queued: queued, failed: failed, - cancelled: cancelled, phase: phase, ); });