Merge pull request #15872 from nextcloud/fix/choose-correct-upload-ids-for-retry

fix: choose correct upload ids for retry

Author: alperozturk96

app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt

@@ -71,6 +71,15 @@ interface UploadDao {
     )
     suspend fun updateStatus(remotePath: String, accountName: String, status: Int): Int
 
+    @Query(
+        """
+    SELECT * FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME}
+    WHERE ${ProviderTableMeta.UPLOADS_STATUS} = :status
+      AND (:nameCollisionPolicy IS NULL OR ${ProviderTableMeta.UPLOADS_NAME_COLLISION_POLICY} = :nameCollisionPolicy)
+"""
+    )
+    suspend fun getUploadsByStatus(status: Int, nameCollisionPolicy: Int? = null): List<UploadEntity>
+
     @Query(
         """
     SELECT * FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME}
@@ -79,7 +88,7 @@ interface UploadDao {
       AND (:nameCollisionPolicy IS NULL OR ${ProviderTableMeta.UPLOADS_NAME_COLLISION_POLICY} = :nameCollisionPolicy)
 """
     )
-    suspend fun getUploadsByStatus(
+    suspend fun getUploadsByAccountNameAndStatus(
         accountName: String,
         status: Int,
         nameCollisionPolicy: Int? = null

app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt

@@ -88,33 +88,57 @@ class FileUploadHelper {
         fun buildRemoteName(accountName: String, remotePath: String): String = accountName + remotePath
     }
 
+    /**
+     * Retries all failed uploads across all user accounts.
+     *
+     * This function retrieves all uploads with the status [UploadStatus.UPLOAD_FAILED], including both
+     * manual uploads and auto uploads. It runs in a background thread (Dispatcher.IO) and ensures
+     * that only one retry operation runs at a time by using a semaphore to prevent concurrent execution.
+     *
+     * Once the failed uploads are retrieved, it calls [retryUploads], which triggers the corresponding
+     * upload workers for each failed upload.
+     *
+     * The function returns `true` if there were any failed uploads to retry and the retry process was
+     * started, or `false` if no uploads were retried.
+     *
+     * @param uploadsStorageManager Provides access to upload data and persistence.
+     * @param connectivityService Checks the current network connectivity state.
+     * @param accountManager Handles user account authentication and selection.
+     * @param powerManagementService Ensures uploads respect power constraints.
+     * @return `true` if any failed uploads were found and retried; `false` otherwise.
+     */
     fun retryFailedUploads(
         uploadsStorageManager: UploadsStorageManager,
         connectivityService: ConnectivityService,
         accountManager: UserAccountManager,
         powerManagementService: PowerManagementService
-    ) {
-        if (retryFailedUploadsSemaphore.tryAcquire()) {
-            try {
-                val failedUploads = uploadsStorageManager.failedUploads
-                if (failedUploads == null || failedUploads.isEmpty()) {
-                    Log_OC.d(TAG, "Failed uploads are empty or null")
-                    return
+    ): Boolean {
+        if (!retryFailedUploadsSemaphore.tryAcquire()) {
+            Log_OC.d(TAG, "skipping retryFailedUploads, already running")
+            return true
+        }
+
+        var isUploadStarted = false
+
+        try {
+            getUploadsByStatus(null, UploadStatus.UPLOAD_FAILED) {
+                if (it.isNotEmpty()) {
+                    isUploadStarted = true
                 }
 
                 retryUploads(
                     uploadsStorageManager,
                     connectivityService,
                     accountManager,
                     powerManagementService,
-                    failedUploads
+                    uploads = it
                 )
-            } finally {
-                retryFailedUploadsSemaphore.release()
             }
-        } else {
-            Log_OC.d(TAG, "Skip retryFailedUploads since it is already running")
+        } finally {
+            retryFailedUploadsSemaphore.release()
         }
+
+        return isUploadStarted
     }
 
     fun retryCancelledUploads(
@@ -123,18 +147,18 @@ class FileUploadHelper {
         accountManager: UserAccountManager,
         powerManagementService: PowerManagementService
     ): Boolean {
-        val cancelledUploads = uploadsStorageManager.cancelledUploadsForCurrentAccount
-        if (cancelledUploads == null || cancelledUploads.isEmpty()) {
-            return false
+        var result = false
+        getUploadsByStatus(accountManager.user.accountName, UploadStatus.UPLOAD_CANCELLED) {
+            result = retryUploads(
+                uploadsStorageManager,
+                connectivityService,
+                accountManager,
+                powerManagementService,
+                it
+            )
         }
 
-        return retryUploads(
-            uploadsStorageManager,
-            connectivityService,
-            accountManager,
-            powerManagementService,
-            cancelledUploads
-        )
+        return result
     }
 
     @Suppress("ComplexCondition")
@@ -143,51 +167,51 @@ class FileUploadHelper {
         connectivityService: ConnectivityService,
         accountManager: UserAccountManager,
         powerManagementService: PowerManagementService,
-        failedUploads: Array<OCUpload>
+        uploads: Array<OCUpload>
     ): Boolean {
         var showNotExistMessage = false
         val isOnline = checkConnectivity(connectivityService)
         val connectivity = connectivityService.connectivity
         val batteryStatus = powerManagementService.battery
-        val accountNames = accountManager.accounts.filter { account ->
-            accountManager.getUser(account.name).isPresent
-        }.map { account ->
-            account.name
-        }.toHashSet()
-
-        for (failedUpload in failedUploads) {
-            if (!accountNames.contains(failedUpload.accountName)) {
-                uploadsStorageManager.removeUpload(failedUpload)
-                continue
-            }
 
-            val uploadResult =
-                checkUploadConditions(failedUpload, connectivity, batteryStatus, powerManagementService, isOnline)
+        val uploadsToRetry = mutableListOf<Long>()
+
+        for (upload in uploads) {
+            val uploadResult = checkUploadConditions(
+                upload,
+                connectivity,
+                batteryStatus,
+                powerManagementService,
+                isOnline
+            )
 
             if (uploadResult != UploadResult.UPLOADED) {
-                if (failedUpload.lastResult != uploadResult) {
+                if (upload.lastResult != uploadResult) {
                     // Setting Upload status else cancelled uploads will behave wrong, when retrying
                     // Needs to happen first since lastResult wil be overwritten by setter
-                    failedUpload.uploadStatus = UploadStatus.UPLOAD_FAILED
+                    upload.uploadStatus = UploadStatus.UPLOAD_FAILED
 
-                    failedUpload.lastResult = uploadResult
-                    uploadsStorageManager.updateUpload(failedUpload)
+                    upload.lastResult = uploadResult
+                    uploadsStorageManager.updateUpload(upload)
                 }
                 if (uploadResult == UploadResult.FILE_NOT_FOUND) {
                     showNotExistMessage = true
                 }
                 continue
             }
 
-            failedUpload.uploadStatus = UploadStatus.UPLOAD_IN_PROGRESS
-            uploadsStorageManager.updateUpload(failedUpload)
+            // Only uploads that passed checks get marked in progress and are collected for scheduling
+            upload.uploadStatus = UploadStatus.UPLOAD_IN_PROGRESS
+            uploadsStorageManager.updateUpload(upload)
+            uploadsToRetry.add(upload.uploadId)
         }
 
-        accountNames.forEach { accountName ->
-            val user = accountManager.getUser(accountName)
-            if (user.isPresent) {
-                backgroundJobManager.startFilesUploadJob(user.get(), failedUploads.getUploadIds(), false)
-            }
+        if (uploadsToRetry.isNotEmpty()) {
+            backgroundJobManager.startFilesUploadJob(
+                accountManager.user,
+                uploadsToRetry.toLongArray(),
+                false
+            )
         }
 
         return showNotExistMessage
@@ -235,21 +259,35 @@ class FileUploadHelper {
         }
     }
 
+    /**
+     * Retrieves uploads filtered by their status, optionally for a specific account.
+     *
+     * This function queries the uploads database asynchronously to obtain a list of uploads
+     * that match the specified [status]. If an [accountName] is provided, only uploads
+     * belonging to that account are retrieved. If [accountName] is `null`, uploads with the
+     * given [status] from **all user accounts** are returned.
+     *
+     * Once the uploads are fetched, the [onCompleted] callback is invoked with the resulting array.
+     *
+     * @param accountName The name of the account to filter uploads by.
+     * If `null`, uploads matching the given [status] from all accounts are returned.
+     * @param status The [UploadStatus] to filter uploads by (e.g., `UPLOAD_FAILED`).
+     * @param nameCollisionPolicy The [NameCollisionPolicy] to filter uploads by (e.g., `SKIP`).
+     * @param onCompleted A callback invoked with the resulting array of [OCUpload] objects.
+     */
     fun getUploadsByStatus(
-        accountName: String,
+        accountName: String?,
         status: UploadStatus,
         nameCollisionPolicy: NameCollisionPolicy? = null,
         onCompleted: (Array<OCUpload>) -> Unit
     ) {
         ioScope.launch {
-            val result = uploadsStorageManager.uploadDao
-                .getUploadsByStatus(
-                    accountName,
-                    status.value,
-                    nameCollisionPolicy?.serialize()
-                )
-                .map { it.toOCUpload(null) }
-                .toTypedArray()
+            val dao = uploadsStorageManager.uploadDao
+            val result = if (accountName != null) {
+                dao.getUploadsByAccountNameAndStatus(accountName, status.value, nameCollisionPolicy?.serialize())
+            } else {
+                dao.getUploadsByStatus(status.value, nameCollisionPolicy?.serialize())
+            }.map { it.toOCUpload(null) }.toTypedArray()
             onCompleted(result)
         }
     }

app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java

@@ -483,35 +483,10 @@ public long[] getCurrentUploadIds(final @NonNull String accountName) {
             .toArray();
     }
 
-    /**
-     * Get all failed uploads.
-     */
-    public OCUpload[] getFailedUploads() {
-        return getUploads("(" + ProviderTableMeta.UPLOADS_STATUS + IS_EQUAL +
-                              OR + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                              EQUAL + UploadResult.DELAYED_FOR_WIFI.getValue() +
-                              OR + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                              EQUAL + UploadResult.LOCK_FAILED.getValue() +
-                              OR + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                              EQUAL + UploadResult.DELAYED_FOR_CHARGING.getValue() +
-                              OR + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                              EQUAL + UploadResult.DELAYED_IN_POWER_SAVE_MODE.getValue() +
-                              " ) AND " + ProviderTableMeta.UPLOADS_LAST_RESULT +
-                              "!= " + UploadResult.VIRUS_DETECTED.getValue()
-            , String.valueOf(UploadStatus.UPLOAD_FAILED.value));
-    }
-
     public OCUpload[] getUploadsForAccount(final @NonNull String accountName) {
         return getUploads(ProviderTableMeta.UPLOADS_ACCOUNT_NAME + IS_EQUAL, accountName);
     }
 
-    public OCUpload[] getCancelledUploadsForCurrentAccount() {
-        User user = currentAccountProvider.getUser();
-
-        return getUploads(ProviderTableMeta.UPLOADS_STATUS + EQUAL + UploadStatus.UPLOAD_CANCELLED.value + AND +
-                              ProviderTableMeta.UPLOADS_ACCOUNT_NAME + IS_EQUAL, user.getAccountName());
-    }
-
     private ContentResolver getDB() {
         return contentResolver;
     }

app/src/main/java/com/owncloud/android/ui/activity/UploadListActivity.java

@@ -41,7 +41,6 @@
 import com.owncloud.android.operations.CheckCurrentCredentialsOperation;
 import com.owncloud.android.ui.adapter.UploadListAdapter;
 import com.owncloud.android.ui.decoration.MediaGridItemDecoration;
-import com.owncloud.android.utils.DisplayUtils;
 import com.owncloud.android.utils.FilesSyncHelper;
 
 import javax.inject.Inject;
@@ -133,15 +132,13 @@ private void observeWorkerState() {
         WorkerStateLiveData.Companion.instance().observe(this, state -> {
             if (state instanceof WorkerState.UploadStarted) {
                 Log_OC.d(TAG, "Upload worker started");
-                handleUploadWorkerState();
+                uploadListAdapter.loadUploadItemsFromDb();
+            } else if (state instanceof WorkerState.UploadFinished) {
+                uploadListAdapter.loadUploadItemsFromDb(() -> swipeListRefreshLayout.setRefreshing(false));
             }
         });
     }
 
-    private void handleUploadWorkerState() {
-        uploadListAdapter.loadUploadItemsFromDb();
-    }
-
     private void setupContent() {
         binding.list.setEmptyView(binding.emptyList.getRoot());
         binding.emptyList.getRoot().setVisibility(View.GONE);
@@ -182,25 +179,15 @@ private void loadItems() {
     }
 
     private void refresh() {
-        FilesSyncHelper.startAutoUploadImmediately(syncedFolderProvider,
-                                                    backgroundJobManager,
-                                                    true);
-
-        if (uploadsStorageManager.getFailedUploads().length > 0) {
-            new Thread(() -> {
-                FileUploadHelper.Companion.instance().retryFailedUploads(
-                    uploadsStorageManager,
-                    connectivityService,
-                    accountManager,
-                    powerManagementService);
-                uploadListAdapter.loadUploadItemsFromDb();
-            }).start();
-            DisplayUtils.showSnackMessage(this, R.string.uploader_local_files_uploaded);
+        boolean isUploadStarted = FileUploadHelper.Companion.instance().retryFailedUploads(
+            uploadsStorageManager,
+            connectivityService,
+            accountManager,
+            powerManagementService);
+
+        if (!isUploadStarted) {
+            uploadListAdapter.loadUploadItemsFromDb(() -> swipeListRefreshLayout.setRefreshing(false));
         }
-
-
-        // update UI
-        uploadListAdapter.loadUploadItemsFromDb(() -> swipeListRefreshLayout.setRefreshing(false));
     }
 
     @Override

app/src/main/java/com/owncloud/android/ui/adapter/UploadListAdapter.java

@@ -264,16 +264,12 @@ private void showFailedPopupMenu(HeaderViewHolder headerViewHolder) {
                 clearTempEncryptedFolder();
                 loadUploadItemsFromDb();
             } else if (itemId == R.id.action_upload_list_failed_retry) {
-
-                // FIXME For e2e resume is not working
-                new Thread(() -> {
-                    uploadHelper.retryFailedUploads(
-                        uploadsStorageManager,
-                        connectivityService,
-                        accountManager,
-                        powerManagementService);
-                    loadUploadItemsFromDb();
-                }).start();
+                uploadHelper.retryFailedUploads(
+                    uploadsStorageManager,
+                    connectivityService,
+                    accountManager,
+                    powerManagementService);
+                loadUploadItemsFromDb();
             }
 
             return true;

app/src/main/java/com/owncloud/android/utils/FilesSyncHelper.java

@@ -24,7 +24,6 @@
 import com.nextcloud.client.network.ConnectivityService;
 import com.nextcloud.utils.extensions.UriExtensionsKt;
 import com.owncloud.android.MainApp;
-import com.owncloud.android.datamodel.FileDataStorageManager;
 import com.owncloud.android.datamodel.FilesystemDataProvider;
 import com.owncloud.android.datamodel.MediaFolderType;
 import com.owncloud.android.datamodel.SyncedFolder;
@@ -222,11 +221,11 @@ public static void restartUploadsIfNeeded(final UploadsStorageManager uploadsSto
                                               final ConnectivityService connectivityService,
                                               final PowerManagementService powerManagementService) {
         Log_OC.d(TAG, "restartUploadsIfNeeded, called");
-        new Thread(() -> FileUploadHelper.Companion.instance().retryFailedUploads(
+        FileUploadHelper.Companion.instance().retryFailedUploads(
             uploadsStorageManager,
             connectivityService,
             accountManager,
-            powerManagementService)).start();
+            powerManagementService);
     }
 
     public static void scheduleFilesSyncForAllFoldersIfNeeded(Context context, SyncedFolderProvider syncedFolderProvider, BackgroundJobManager jobManager) {