From 7dca538633ac938ed2d5b9cfcb91081d36036bc5 Mon Sep 17 00:00:00 2001 From: Kilian <19181985+kilrau@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:02:41 +0100 Subject: [PATCH 1/2] fix(document-provider): wait for pending uploads before rename, move and copy Content written through the documents provider is uploaded asynchronously after the file descriptor is closed, and the queued upload is bound to the remote path at that moment. A rename, move or copy issued right after the write ran on the server against the empty placeholder created by createDocument, and the upload was left pointing at a path that no longer existed. Apps that write to a temporary name and rename it when done (e.g. Signal's local backups) ended up with 0 byte files on the server. - Track every write in openForWrite before the file descriptor is handed out, as OPEN and then CLOSED_UPLOAD_NOT_QUEUED once the close is reported. The mark ends when the upload job is enqueued: startFilesUploadJob now returns its Job, which completes once WorkManager accepted the chain or enqueueing failed. - Rename, move and copy wait while a write is CLOSED_UPLOAD_NOT_QUEUED or the scheduled upload job still has to process the document: its upload row is in progress, or failed with a result the job retries. Rows are matched by account and exact path. An OPEN write is waited for only for a short grace period, since the close is reported asynchronously; a document that stays open is not blocked on. - The wait gives up only when no upload bytes of the account have been transferred for 10 minutes, so uploads that take hours are not cut short. Refs #7846 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Kilian <19181985+kilrau@users.noreply.github.com> --- .../dao/UploadDaoUnfinishedUploadsTest.kt | 91 +++++++ .../DocumentsStorageProviderIT.kt | 20 ++ .../client/database/dao/UploadDao.kt | 17 ++ .../client/jobs/BackgroundJobManager.kt | 7 +- .../client/jobs/BackgroundJobManagerImpl.kt | 16 +- .../client/jobs/upload/FileUploadHelper.kt | 24 +- .../client/jobs/upload/FileUploadWorker.kt | 6 + .../DocumentsStorageProvider.kt | 8 +- .../documentstorage/modify/DocumentEditor.kt | 52 +++- .../documentstorage/open/DocumentOpener.kt | 21 +- .../open/PendingDocumentWrite.kt | 12 + .../open/PendingDocumentWrites.kt | 30 +++ .../documentstorage/open/PendingWriteState.kt | 12 + .../modify/DocumentEditorPendingUploadTest.kt | 243 ++++++++++++++++++ 14 files changed, 546 insertions(+), 13 deletions(-) create mode 100644 app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt create mode 100644 app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt create mode 100644 app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt create mode 100644 app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingWriteState.kt create mode 100644 app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt diff --git a/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt b/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt new file mode 100644 index 000000000000..6b13921c585c --- /dev/null +++ b/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt @@ -0,0 +1,91 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.nextcloud.client.database.dao + +import androidx.room.Room +import androidx.test.platform.app.InstrumentationRegistry +import com.nextcloud.client.database.NextcloudDatabase +import com.nextcloud.client.database.entity.toUploadEntity +import com.nextcloud.client.database.typeConverter.OfflineOperationTypeConverter +import com.nextcloud.utils.extensions.isNonRetryable +import com.owncloud.android.datamodel.UploadsStorageManager.UploadStatus +import com.owncloud.android.db.OCUpload +import com.owncloud.android.db.UploadResult +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Test + +class UploadDaoUnfinishedUploadsTest { + + private val database = Room.inMemoryDatabaseBuilder( + InstrumentationRegistry.getInstrumentation().targetContext, + NextcloudDatabase::class.java + ) + .addTypeConverter(OfflineOperationTypeConverter()) + .allowMainThreadQueries() + .build() + private val uploadDao = database.uploadDao() + private val nonRetryableResults = UploadResult.entries.filter { it.isNonRetryable() }.map { it.value } + + @After + fun tearDown() { + database.close() + } + + @Test + fun uploadIsUnfinishedWhileTheUploadJobStillProcessesIt() { + val cases = listOf( + Triple(UploadStatus.UPLOAD_IN_PROGRESS, UploadResult.UNKNOWN, true), + Triple(UploadStatus.UPLOAD_FAILED, UploadResult.NETWORK_CONNECTION, true), + Triple(UploadStatus.UPLOAD_FAILED, null, true), + Triple(UploadStatus.UPLOAD_FAILED, UploadResult.FILE_NOT_FOUND, false), + Triple(UploadStatus.UPLOAD_SUCCEEDED, UploadResult.UPLOADED, false) + ) + + cases.forEachIndexed { index, (status, result, unfinished) -> + val remotePath = "$FILE_PATH.$index" + insert(remotePath, status, result) + + assertEquals("$status $result", unfinished, isFileUnfinished(remotePath)) + } + } + + @Test + fun uploadOfOtherAccountOrPathIsIgnored() { + insert(FILE_PATH, UploadStatus.UPLOAD_IN_PROGRESS, accountName = OTHER_ACCOUNT_NAME) + insert("$FILE_PATH.bak", UploadStatus.UPLOAD_IN_PROGRESS) + + assertFalse(isFileUnfinished()) + } + + private fun insert( + remotePath: String, + status: UploadStatus, + result: UploadResult? = UploadResult.UNKNOWN, + accountName: String = ACCOUNT_NAME + ) { + val upload = OCUpload(LOCAL_PATH, remotePath, accountName).apply { uploadStatus = status } + uploadDao.insertOrReplace(upload.toUploadEntity().copy(lastResult = result?.value)) + } + + private fun isFileUnfinished(remotePath: String = FILE_PATH): Boolean = + uploadDao.existsUnfinishedByAccountAndRemotePath( + ACCOUNT_NAME, + remotePath, + UploadStatus.UPLOAD_IN_PROGRESS.value, + UploadStatus.UPLOAD_FAILED.value, + nonRetryableResults + ) + + companion object { + private const val ACCOUNT_NAME = "user@server" + private const val OTHER_ACCOUNT_NAME = "other@server" + private const val LOCAL_PATH = "/local/document.bin" + private const val FILE_PATH = "/folder/document.bin" + } +} diff --git a/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt b/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt index bf1631108716..08d162b599ba 100644 --- a/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt +++ b/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt @@ -20,6 +20,8 @@ import com.owncloud.android.datamodel.UploadsStorageManager.UploadStatus import com.owncloud.android.db.OCUpload import com.owncloud.android.db.UploadResult import com.owncloud.android.lib.common.utils.Log_OC +import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation +import com.owncloud.android.lib.resources.files.model.RemoteFile import com.owncloud.android.providers.documentstorage.DocumentsProviderUtils.assertExistsOnServer import com.owncloud.android.providers.documentstorage.DocumentsProviderUtils.assertListFilesEquals import com.owncloud.android.providers.documentstorage.DocumentsProviderUtils.assertReadEquals @@ -48,6 +50,8 @@ private const val MAX_FILE_NAME_LENGTH = 225 // the provider can only enqueue the upload worker, so give it a generous budget before giving up private const val UPLOAD_POLL_ATTEMPTS = 45 +private const val RENAME_AFTER_WRITE_DATA_SIZE = 64 * 1024 + class DocumentsStorageProviderIT : AbstractOnServerIT() { private val context = targetContext @@ -281,6 +285,22 @@ class DocumentsStorageProviderIT : AbstractOnServerIT() { assertEquals(String(content2), String(bytes)) } + @Test + fun testRenameRightAfterWriteKeepsContent() { + val file1 = rootDir.createFile("application/octet-stream", RandomStringGenerator.make())!! + val data = Random.nextBytes(RENAME_AFTER_WRITE_DATA_SIZE) + contentResolver.openOutputStream(file1.uri, "wt").use { + it!!.write(data) + } + + DocumentsContract.renameDocument(contentResolver, file1.uri, RandomStringGenerator.make()) + + val remotePath = file1.getOCFile(storageManager)!!.remotePath + val result = ReadFileRemoteOperation(remotePath).execute(client) + assertTrue(result.isSuccess) + assertEquals(data.size.toLong(), (result.data[0] as RemoteFile).length) + } + private fun awaitUploadedToServer(file: DocumentFile) { val remotePath = file.getOCFile(storageManager)!!.remotePath diff --git a/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt b/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt index 653dc03c9050..338d01b3eac6 100644 --- a/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt +++ b/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt @@ -36,6 +36,23 @@ interface UploadDao { ) fun getByRemotePath(remotePath: String): UploadEntity? + @Query( + "SELECT EXISTS(SELECT 1 FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME} " + + "WHERE ${ProviderTableMeta.UPLOADS_ACCOUNT_NAME} = :accountName " + + "AND ${ProviderTableMeta.UPLOADS_REMOTE_PATH} = :remotePath " + + "AND (${ProviderTableMeta.UPLOADS_STATUS} = :inProgressStatus " + + "OR (${ProviderTableMeta.UPLOADS_STATUS} = :failedStatus " + + "AND (${ProviderTableMeta.UPLOADS_LAST_RESULT} IS NULL " + + "OR ${ProviderTableMeta.UPLOADS_LAST_RESULT} NOT IN (:nonRetryableResults)))))" + ) + fun existsUnfinishedByAccountAndRemotePath( + accountName: String, + remotePath: String, + inProgressStatus: Int, + failedStatus: Int, + nonRetryableResults: List + ): Boolean + @Query( "DELETE FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME} " + "WHERE ${ProviderTableMeta.UPLOADS_ACCOUNT_NAME} = :accountName " + diff --git a/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManager.kt b/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManager.kt index 7f4a0a3184cc..fc342e7a8769 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManager.kt +++ b/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManager.kt @@ -13,6 +13,7 @@ import com.nextcloud.client.account.User import com.owncloud.android.datamodel.OCFile import com.owncloud.android.datamodel.SyncedFolder import com.owncloud.android.operations.DownloadType +import kotlinx.coroutines.Job /** * This interface allows to control, schedule and monitor all application @@ -135,12 +136,16 @@ interface BackgroundJobManager { fun startNotificationJob(subject: String, signature: String) fun startAccountRemovalJob(accountName: String, remoteWipe: Boolean) + + /** + * @return completes once enqueueing the upload job finished or failed + */ fun startFilesUploadJob( user: User, uploadIds: LongArray, showSameFileAlreadyExistsNotification: Boolean, skipAutoUploadCheck: Boolean = false - ) + ): Job fun startAlbumFilesUploadJob(user: User, uploadIds: LongArray, albumName: String) fun getFileUploads(user: User): LiveData> fun cancelFilesUploadJob(user: User) diff --git a/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt b/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt index 54f3852d8763..cd13deaabb92 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt +++ b/app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt @@ -23,6 +23,7 @@ import androidx.work.OutOfQuotaPolicy import androidx.work.PeriodicWorkRequest import androidx.work.WorkInfo import androidx.work.WorkManager +import androidx.work.await import androidx.work.workDataOf import com.nextcloud.client.account.User import com.nextcloud.client.core.Clock @@ -46,8 +47,10 @@ import com.owncloud.android.datamodel.OCFile import com.owncloud.android.datamodel.SyncedFolder import com.owncloud.android.lib.common.utils.Log_OC import com.owncloud.android.operations.DownloadType +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import java.time.Duration import java.util.Date @@ -679,8 +682,8 @@ internal class BackgroundJobManagerImpl( uploadIds: LongArray, showSameFileAlreadyExistsNotification: Boolean, skipAutoUploadCheck: Boolean - ) { - defaultDispatcherScope.launch { + ): Job { + val enqueueJob = defaultDispatcherScope.launch { val batchSize = FileUploadHelper.MAX_FILE_COUNT val batches = uploadIds.toList().chunked(batchSize) val tag = startFileUploadJobTag(user.accountName) @@ -721,9 +724,16 @@ internal class BackgroundJobManagerImpl( workChain = workChain.then(request) } - workChain.enqueue() + try { + workChain.enqueue().await() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Log_OC.e(TAG, "Failed to enqueue upload job", e) + } } } + return enqueueJob } private fun startAlbumsFileUploadJobTag(accountName: String): String = ALBUM_JOB_FILES_UPLOAD + accountName diff --git a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt index 8d404bdf8f82..36918fa036e8 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt +++ b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt @@ -30,6 +30,7 @@ import com.nextcloud.utils.extensions.checkWCFRestrictions import com.nextcloud.utils.extensions.createOwncloudClient import com.nextcloud.utils.extensions.getUploadIds import com.nextcloud.utils.extensions.isLastResultConflictError +import com.nextcloud.utils.extensions.isNonRetryable import com.nextcloud.utils.extensions.isSame import com.owncloud.android.MainApp import com.owncloud.android.R @@ -54,6 +55,7 @@ import com.owncloud.android.ui.adapter.uploadList.helper.ConflictHandlingResult import com.owncloud.android.ui.adapter.uploadList.helper.UploadListAdapterActionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import java.io.File @@ -91,6 +93,8 @@ class FileUploadHelper { const val MAX_UPLOADS_PER_QUERY = 500 + private val nonRetryableResults = UploadResult.entries.filter { it.isNonRetryable() }.map { it.value } + val mBoundListeners = HashMap() private val retryInProgress = AtomicBoolean(false) @@ -429,6 +433,22 @@ class FileUploadHelper { FileUploadWorker.isUploading(remotePath, accountName) } + /** + * Mirrors the rows [FileUploadWorker] still processes; rows left behind without a scheduled upload job never + * finish. Rows are matched by path, so a retryable failed row also counts while another batch of the account runs. + */ + fun isUploadPending(file: OCFile, accountName: String): Boolean = + uploadsStorageManager.uploadDao.existsUnfinishedByAccountAndRemotePath( + accountName, + file.remotePath, + UploadStatus.UPLOAD_IN_PROGRESS.value, + UploadStatus.UPLOAD_FAILED.value, + nonRetryableResults + ) && + backgroundJobManager.isStartFileUploadJobScheduled(accountName) + + fun getLastUploadProgress(accountName: String): Long = FileUploadWorker.getLastTransferProgress(accountName) + private fun checkConnectivity(connectivityService: ConnectivityService): Boolean { // check that connection isn't walled off and that the server is reachable return connectivityService.connectivity.isConnected && !connectivityService.isInternetWalled() @@ -497,7 +517,7 @@ class FileUploadHelper { behaviour: Int, nameCollisionPolicy: NameCollisionPolicy, skipAutoUploadCheck: Boolean = false - ) { + ): Job { Log_OC.d(this, "upload updated file") val uploads = existingFiles.map { file -> @@ -534,7 +554,7 @@ class FileUploadHelper { } } val uploadIds: LongArray = uploads.filterNotNull().map { it.uploadId }.toLongArray() - backgroundJobManager.startFilesUploadJob( + return backgroundJobManager.startFilesUploadJob( user, uploadIds, true, diff --git a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt index 3d7872b7f4cb..b7cb71ba2dc5 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt +++ b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt @@ -9,6 +9,7 @@ package com.nextcloud.client.jobs.upload import android.app.Notification import android.content.Context +import android.os.SystemClock import androidx.core.app.NotificationCompat import androidx.localbroadcastmanager.content.LocalBroadcastManager import androidx.work.CoroutineWorker @@ -133,6 +134,10 @@ class FileUploadWorker( it.remotePath == remotePath && it.user.accountName == accountName } + private val lastTransferProgress = ConcurrentHashMap() + + fun getLastTransferProgress(accountName: String): Long = lastTransferProgress[accountName] ?: 0L + /** * [activeOperations] tracks every upload running in this process, not only the ones this worker started. * Auto upload has to register here too, otherwise a retry pass sees its in-progress row as abandoned and @@ -497,6 +502,7 @@ class FileUploadWorker( totalToTransfer: Long, fileAbsoluteName: String ) { + inputData.getString(ACCOUNT)?.let { lastTransferProgress[it] = SystemClock.elapsedRealtime() } val percent = getPercent(totalTransferredSoFar, totalToTransfer) val currentTime = System.currentTimeMillis() diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProvider.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProvider.kt index 5000c009cbf8..0c7dd8da48fe 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProvider.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProvider.kt @@ -37,6 +37,7 @@ import com.owncloud.android.providers.documentstorage.modify.DocumentCreator import com.owncloud.android.providers.documentstorage.modify.DocumentEditor import com.owncloud.android.providers.documentstorage.modify.DocumentNameValidator import com.owncloud.android.providers.documentstorage.open.DocumentOpener +import com.owncloud.android.providers.documentstorage.open.PendingDocumentWrites import com.owncloud.android.providers.documentstorage.query.DocumentLister import com.owncloud.android.providers.documentstorage.sync.DocumentNotifier import com.owncloud.android.providers.documentstorage.sync.FolderRefresher @@ -71,11 +72,14 @@ class DocumentsStorageProvider : DocumentsProvider() { private val folderRefresher by lazy { FolderRefresher(appContext) } private val nameValidator by lazy { DocumentNameValidator(appContext, accountManager) } private val lister by lazy { DocumentLister(appContext, scope, notifier, folderRefresher) } - private val opener by lazy { DocumentOpener(appContext, scope, fileUploadHelper) } + private val pendingWrites = PendingDocumentWrites() + private val opener by lazy { DocumentOpener(appContext, scope, fileUploadHelper, pendingWrites) } private val creator by lazy { DocumentCreator(appContext, accountManager, resolver, nameValidator, folderRefresher, notifier) } - private val editor by lazy { DocumentEditor(appContext, resolver, notifier, folderRefresher, nameValidator) } + private val editor by lazy { + DocumentEditor(appContext, resolver, notifier, folderRefresher, nameValidator, fileUploadHelper, pendingWrites) + } private val isAppLocked: Boolean get() = preferences.lockPreference in diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt index b38149b228b1..b03f64fd6e84 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt @@ -7,6 +7,8 @@ package com.owncloud.android.providers.documentstorage.modify import android.content.Context +import android.os.SystemClock +import com.nextcloud.client.jobs.upload.FileUploadHelper import com.nextcloud.utils.extensions.showToast import com.owncloud.android.R import com.owncloud.android.datamodel.OCFile @@ -15,17 +17,26 @@ import com.owncloud.android.operations.MoveFileOperation import com.owncloud.android.operations.RenameFileOperation import com.owncloud.android.providers.documentstorage.model.Document import com.owncloud.android.providers.documentstorage.model.DocumentResolver +import com.owncloud.android.providers.documentstorage.open.PendingDocumentWrites +import com.owncloud.android.providers.documentstorage.open.PendingWriteState import com.owncloud.android.providers.documentstorage.sync.DocumentNotifier import com.owncloud.android.providers.documentstorage.sync.FolderRefresher import com.owncloud.android.providers.documentstorage.util.requireSuccess +import dagger.Lazy import java.io.FileNotFoundException +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds internal class DocumentEditor( private val context: Context, private val resolver: DocumentResolver, private val notifier: DocumentNotifier, private val folderRefresher: FolderRefresher, - private val nameValidator: DocumentNameValidator + private val nameValidator: DocumentNameValidator, + private val fileUploadHelper: Lazy, + private val pendingWrites: PendingDocumentWrites, + private val elapsedRealtime: () -> Long = SystemClock::elapsedRealtime, + private val sleep: (Long) -> Unit = Thread::sleep ) { @Volatile @@ -47,6 +58,7 @@ internal class DocumentEditor( return } + awaitPendingUploads(document, file) val result = RenameFileOperation(file.remotePath, displayName, document.storageManager) .execute(document.requireClient(context)) result.requireSuccess(TAG) { "Failed to rename document with documentId $documentId: ${result.exception}" } @@ -67,6 +79,7 @@ internal class DocumentEditor( val storageManager = document.storageManager val failureMessage = "Failed to copy document with documentId $sourceDocumentId to $targetParentDocumentId" + awaitPendingUploads(document, sourceFile) CopyFileOperation(sourceFile.remotePath, targetFile.remotePath, storageManager) .execute(document.requireClient(context)) .requireSuccess(TAG) { failureMessage } @@ -98,6 +111,7 @@ internal class DocumentEditor( return null } + awaitPendingUploads(document, sourceFile) MoveFileOperation(sourceFile.remotePath, targetFile.remotePath, document.storageManager) .execute(document.requireClient(context)) .requireSuccess(TAG) { @@ -109,6 +123,39 @@ internal class DocumentEditor( return sourceDocumentId } + /** + * The upload starts asynchronously after close() and targets the remote path the document had then, so acting on + * the server first would hit the stale copy and strand the upload. Large uploads can take hours, hence an idle + * timeout instead of a total cap. close() also reaches the provider asynchronously, hence [CLOSE_GRACE_PERIOD]. + */ + @Throws(FileNotFoundException::class) + private fun awaitPendingUploads(document: Document, file: OCFile) { + val uploadHelper = fileUploadHelper.get() + val accountName = document.user.accountName + val waitStart = elapsedRealtime() + var lastProgress = waitStart + + while (hasPendingUpload(document, file, uploadHelper, elapsedRealtime() - waitStart)) { + lastProgress = maxOf(lastProgress, uploadHelper.getLastUploadProgress(accountName)) + if (elapsedRealtime() - lastProgress > PENDING_UPLOAD_IDLE_TIMEOUT.inWholeMilliseconds) { + throw FileNotFoundException("Pending upload of ${file.remotePath} made no progress") + } + sleep(PENDING_UPLOAD_POLL_INTERVAL.inWholeMilliseconds) + } + } + + private fun hasPendingUpload( + document: Document, + file: OCFile, + uploadHelper: FileUploadHelper, + waitedMillis: Long + ): Boolean { + val writeStates = pendingWrites.states { it == file.fileId } + return PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED in writeStates || + (PendingWriteState.OPEN in writeStates && waitedMillis < CLOSE_GRACE_PERIOD.inWholeMilliseconds) || + uploadHelper.isUploadPending(file, document.user.accountName) + } + private fun isValidTargetFolder(targetFolder: OCFile): Boolean { isFolderPathValid = nameValidator.isValidFolderPath(targetFolder.fileName) if (!isFolderPathValid) { @@ -119,5 +166,8 @@ internal class DocumentEditor( companion object { private val TAG = DocumentEditor::class.java.simpleName + private val PENDING_UPLOAD_IDLE_TIMEOUT = 10.minutes + private val PENDING_UPLOAD_POLL_INTERVAL = 1.seconds + private val CLOSE_GRACE_PERIOD = 5.seconds } } diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt index c453468a61f2..586245099721 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt @@ -39,7 +39,8 @@ import java.nio.file.StandardCopyOption internal class DocumentOpener( private val context: Context, private val scope: CoroutineScope, - private val fileUploadHelper: Lazy + private val fileUploadHelper: Lazy, + private val pendingWrites: PendingDocumentWrites ) { @Throws(FileNotFoundException::class) @@ -214,19 +215,30 @@ internal class DocumentOpener( openedFile: OpenedDocumentFile, accessMode: AccessMode ): ParcelFileDescriptor { + // before handing out the descriptor: close() reaches the provider asynchronously, possibly after a rename + val write = pendingWrites.begin(ocFile.fileId) val onClose = ParcelFileDescriptor.OnCloseListener { error -> if (error != null) { Log_OC.e(TAG, "File was closed with an error: ${ocFile.fileName}", error) openedFile.deleteCopy() + pendingWrites.end(write) return@OnCloseListener } - scope.launch { uploadWrittenDocument(document, ocFile, openedFile) } + pendingWrites.closed(write) + scope.launch { + try { + uploadWrittenDocument(document, ocFile, openedFile) + } finally { + pendingWrites.end(write) + } + } } return try { ParcelFileDescriptor.open(openedFile.descriptorFile, accessMode.bits, Handler(context.mainLooper), onClose) } catch (e: IOException) { + pendingWrites.end(write) throw fileNotFoundException( "Failed to open document for writing ${openedFile.descriptorFile.absolutePath}", e @@ -234,7 +246,7 @@ internal class DocumentOpener( } } - private fun uploadWrittenDocument(document: Document, ocFile: OCFile, openedFile: OpenedDocumentFile) { + private suspend fun uploadWrittenDocument(document: Document, ocFile: OCFile, openedFile: OpenedDocumentFile) { val file = openedFile.localFile try { openedFile.moveCopyToLocalFile() @@ -249,12 +261,13 @@ internal class DocumentOpener( ocFile.modificationTimestamp = System.currentTimeMillis() document.storageManager.saveFile(ocFile) + // joined so the write stays pending until isUploadPending can see the upload fileUploadHelper.get().uploadUpdatedFile( document.user, arrayOf(ocFile), FileUploadWorker.LOCAL_BEHAVIOUR_DELETE, NameCollisionPolicy.OVERWRITE - ) + ).join() } companion object { diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt new file mode 100644 index 000000000000..8143552f55a7 --- /dev/null +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt @@ -0,0 +1,12 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.providers.documentstorage.open + +internal class PendingDocumentWrite(val fileId: Long) { + @Volatile + var state = PendingWriteState.OPEN +} diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt new file mode 100644 index 000000000000..0224c6df9c6e --- /dev/null +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt @@ -0,0 +1,30 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.providers.documentstorage.open + +import java.util.concurrent.ConcurrentHashMap + +/** + * Tracked per write, not per document, because a document can be open for writing more than once at the same time. + */ +internal class PendingDocumentWrites { + + private val writes: MutableSet = ConcurrentHashMap.newKeySet() + + fun begin(fileId: Long): PendingDocumentWrite = PendingDocumentWrite(fileId).also(writes::add) + + fun closed(write: PendingDocumentWrite) { + write.state = PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED + } + + fun end(write: PendingDocumentWrite) { + writes.remove(write) + } + + fun states(isAffected: (fileId: Long) -> Boolean): Set = + writes.filter { isAffected(it.fileId) }.mapTo(mutableSetOf()) { it.state } +} diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingWriteState.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingWriteState.kt new file mode 100644 index 000000000000..03ae1991bd60 --- /dev/null +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingWriteState.kt @@ -0,0 +1,12 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.providers.documentstorage.open + +internal enum class PendingWriteState { + OPEN, + CLOSED_UPLOAD_NOT_QUEUED +} diff --git a/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt b/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt new file mode 100644 index 000000000000..66ea9b12b5d6 --- /dev/null +++ b/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt @@ -0,0 +1,243 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.providers.documentstorage.modify + +import android.content.Context +import com.nextcloud.client.account.User +import com.nextcloud.client.jobs.upload.FileUploadHelper +import com.owncloud.android.datamodel.FileDataStorageManager +import com.owncloud.android.datamodel.OCFile +import com.owncloud.android.lib.common.OwnCloudClient +import com.owncloud.android.lib.common.operations.RemoteOperation +import com.owncloud.android.lib.common.operations.RemoteOperationResult +import com.owncloud.android.operations.CopyFileOperation +import com.owncloud.android.operations.MoveFileOperation +import com.owncloud.android.operations.RenameFileOperation +import com.owncloud.android.providers.documentstorage.model.Document +import com.owncloud.android.providers.documentstorage.model.DocumentResolver +import com.owncloud.android.providers.documentstorage.open.PendingDocumentWrites +import com.owncloud.android.providers.documentstorage.open.PendingWriteState +import com.owncloud.android.providers.documentstorage.sync.FolderRefresher +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Before +import org.junit.Test +import org.mockito.MockedConstruction +import org.mockito.Mockito.mockConstruction +import org.mockito.kotlin.any +import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import java.io.FileNotFoundException +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds + +class DocumentEditorPendingUploadTest { + + private val context = mock() + private val client = mock() + private val user = mock { on { accountName } doReturn ACCOUNT_NAME } + private val storageManager = mock { on { user } doReturn user } + private val resolver = mock() + private val nameValidator = mock { + on { isValidFolderPath(any()) } doReturn true + } + private val success = mock> { on { isSuccess } doReturn true } + private val folderRefresher = mock { + on { refresh(any(), any(), any(), any()) } doReturn success + } + private val pendingWrites = PendingDocumentWrites() + + private var uploadPending = false + private var lastUploadProgress = 0L + private var now = 0L + private var polls = 0 + private var onPoll: () -> Unit = {} + private val operationsRunWhilePending = mutableListOf() + + private val uploadHelper = mock { + on { isUploadPending(any(), any()) } doAnswer { uploadPending } + on { getLastUploadProgress(any()) } doAnswer { lastUploadProgress } + } + + private val editor = DocumentEditor( + context, + resolver, + mock(), + folderRefresher, + nameValidator, + { uploadHelper }, + pendingWrites, + elapsedRealtime = { now }, + sleep = { millis -> + now += millis + polls++ + onPoll() + } + ) + + private val constructions = mutableListOf>() + + @Before + fun setUp() { + givenDocument(SOURCE_ID, FILE_ID, FILE_PATH, isFolder = false) + givenDocument(TARGET_FOLDER_ID, TARGET_FOLDER_FILE_ID, TARGET_FOLDER_PATH, isFolder = true) + givenDocument(SOURCE_PARENT_ID, SOURCE_PARENT_FILE_ID, SOURCE_PARENT_PATH, isFolder = true) + whenever(storageManager.getFileByEncryptedRemotePath(any())).thenReturn(mock()) + + constructions += recordExecution(RenameFileOperation::class.java) + constructions += recordExecution(MoveFileOperation::class.java) + constructions += recordExecution(CopyFileOperation::class.java) + } + + @After + fun tearDown() { + constructions.forEach { it.close() } + } + + @Test + fun `rename without pending upload renames immediately`() { + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(0, polls) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `rename, move and copy wait for queued upload`() { + val operations = listOf( + { editor.rename(SOURCE_ID, NEW_NAME) }, + { editor.move(SOURCE_ID, SOURCE_PARENT_ID, TARGET_FOLDER_ID) }, + { editor.copy(SOURCE_ID, TARGET_FOLDER_ID) } + ) + + operations.forEach { operation -> + uploadPending = true + polls = 0 + onPoll = { if (polls == 2) uploadPending = false } + + operation() + + assertEquals(2, polls) + } + assertEquals(listOf(false, false, false), operationsRunWhilePending) + } + + @Test + fun `rename waits for close that is not reported yet and then for the upload`() { + val write = pendingWrites.begin(FILE_ID) + onPoll = { + when (polls) { + 1 -> pendingWrites.closed(write) + 2 -> pendingWrites.end(write) + } + } + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(2, polls) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `rename of document that stays open proceeds after close grace period`() { + pendingWrites.begin(FILE_ID) + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(CLOSE_GRACE_PERIOD, now) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `rename waits for every concurrent write of the document`() { + val first = pendingWrites.begin(FILE_ID) + val second = pendingWrites.begin(FILE_ID) + pendingWrites.closed(first) + pendingWrites.closed(second) + onPoll = { + when (polls) { + 1 -> pendingWrites.end(first) + 2 -> pendingWrites.end(second) + } + } + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(2, polls) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `waiting continues past idle timeout while upload bytes are transferred`() { + uploadPending = true + onPoll = { + lastUploadProgress = now + if (now > 3 * IDLE_TIMEOUT) uploadPending = false + } + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `rename fails without renaming when no upload bytes are transferred within idle timeout`() { + uploadPending = true + + assertThrows(FileNotFoundException::class.java) { editor.rename(SOURCE_ID, NEW_NAME) } + + assertEquals(emptyList(), operationsRunWhilePending) + } + + private fun givenDocument(documentId: String, fileId: Long, remotePath: String, isFolder: Boolean) { + val file = givenFile(fileId, remotePath, isFolder) + val document = mock { + on { requireFile() } doReturn file + on { user } doReturn user + on { storageManager } doReturn storageManager + on { requireClient(context) } doReturn client + } + whenever(resolver.toDocument(documentId)).thenReturn(document) + } + + private fun givenFile(fileId: Long, remotePath: String, isFolder: Boolean): OCFile = mock { + on { this.fileId } doReturn fileId + on { this.remotePath } doReturn remotePath + on { this.isFolder } doReturn isFolder + on { fileName } doReturn remotePath.trimEnd('/').substringAfterLast('/') + on { canRename() } doReturn true + on { canMove() } doReturn true + } + + private fun > recordExecution(type: Class): MockedConstruction = + mockConstruction(type) { operation, _ -> + whenever(operation.execute(client)).thenAnswer { + operationsRunWhilePending += uploadPending || + PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED in pendingWrites.states { true } + success + } + } + + companion object { + private const val ACCOUNT_NAME = "test@server" + private const val NEW_NAME = "renamed.bin" + private const val SOURCE_ID = "root;1" + private const val TARGET_FOLDER_ID = "root;2" + private const val SOURCE_PARENT_ID = "root;3" + private const val FILE_ID = 1L + private const val TARGET_FOLDER_FILE_ID = 2L + private const val SOURCE_PARENT_FILE_ID = 3L + private const val FILE_PATH = "/folder/.document.tmp" + private const val TARGET_FOLDER_PATH = "/target/" + private const val SOURCE_PARENT_PATH = "/folder/" + private val IDLE_TIMEOUT = 10.minutes.inWholeMilliseconds + private val CLOSE_GRACE_PERIOD = 5.seconds.inWholeMilliseconds + } +} From 06c2a63cd86d8ddd1343eb2abca04c308914dd77 Mon Sep 17 00:00:00 2001 From: Kilian <19181985+kilrau@users.noreply.github.com> Date: Sat, 7 Nov 2026 11:21:13 +0100 Subject: [PATCH 2/2] fix(document-provider): also wait for pending uploads inside renamed, moved or copied folders Renaming, moving or copying a folder right after writing a file inside it has the same problem as doing it to the file itself: the server-side operation runs against the empty placeholder and the queued upload targets the old path. Signal's current local backups write each snapshot into a signal-backup--tmp folder and rename the folder when done, so they still end up with 0 byte files without this. Wait for writes and unfinished uploads of every file below the folder as well, matched by account and remote path prefix. Refs #7846 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Kilian <19181985+kilrau@users.noreply.github.com> --- .../dao/UploadDaoUnfinishedUploadsTest.kt | 25 +++++++ .../DocumentsStorageProviderIT.kt | 17 +++++ .../client/database/dao/UploadDao.kt | 25 +++++-- .../client/jobs/upload/FileUploadHelper.kt | 11 ++- .../documentstorage/modify/DocumentEditor.kt | 11 ++- .../documentstorage/open/DocumentOpener.kt | 2 +- .../open/PendingDocumentWrite.kt | 2 +- .../open/PendingDocumentWrites.kt | 7 +- .../modify/DocumentEditorPendingUploadTest.kt | 68 ++++++++++++++++--- 9 files changed, 145 insertions(+), 23 deletions(-) diff --git a/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt b/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt index 6b13921c585c..66a2f5d56e8f 100644 --- a/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt +++ b/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt @@ -18,6 +18,7 @@ import com.owncloud.android.db.UploadResult import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test class UploadDaoUnfinishedUploadsTest { @@ -63,6 +64,21 @@ class UploadDaoUnfinishedUploadsTest { assertFalse(isFileUnfinished()) } + @Test + fun unfinishedUploadInsideFolderIsUnfinished() { + insert("${FOLDER_PATH}sub/document.bin", UploadStatus.UPLOAD_FAILED, UploadResult.NETWORK_CONNECTION) + + assertTrue(isFolderUnfinished()) + } + + @Test + fun folderIgnoresFinishedChildrenAndSiblingWithSamePrefix() { + insert("${FOLDER_PATH}document.bin", UploadStatus.UPLOAD_FAILED, UploadResult.FILE_NOT_FOUND) + insert("${FOLDER_PATH.trimEnd('/')}-other/document.bin", UploadStatus.UPLOAD_IN_PROGRESS) + + assertFalse(isFolderUnfinished()) + } + private fun insert( remotePath: String, status: UploadStatus, @@ -82,10 +98,19 @@ class UploadDaoUnfinishedUploadsTest { nonRetryableResults ) + private fun isFolderUnfinished(): Boolean = uploadDao.existsUnfinishedByAccountInFolder( + ACCOUNT_NAME, + FOLDER_PATH, + UploadStatus.UPLOAD_IN_PROGRESS.value, + UploadStatus.UPLOAD_FAILED.value, + nonRetryableResults + ) + companion object { private const val ACCOUNT_NAME = "user@server" private const val OTHER_ACCOUNT_NAME = "other@server" private const val LOCAL_PATH = "/local/document.bin" private const val FILE_PATH = "/folder/document.bin" + private const val FOLDER_PATH = "/snapshot/" } } diff --git a/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt b/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt index 08d162b599ba..835c2a53a785 100644 --- a/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt +++ b/app/src/androidTest/java/com/owncloud/android/providers/documentstorage/DocumentsStorageProviderIT.kt @@ -301,6 +301,23 @@ class DocumentsStorageProviderIT : AbstractOnServerIT() { assertEquals(data.size.toLong(), (result.data[0] as RemoteFile).length) } + @Test + fun testRenameFolderRightAfterWriteKeepsContent() { + val folder = rootDir.createDirectory(RandomStringGenerator.make())!! + val file1 = folder.createFile("application/octet-stream", RandomStringGenerator.make())!! + val data = Random.nextBytes(RENAME_AFTER_WRITE_DATA_SIZE) + contentResolver.openOutputStream(file1.uri, "wt").use { + it!!.write(data) + } + + DocumentsContract.renameDocument(contentResolver, folder.uri, RandomStringGenerator.make()) + + val remotePath = file1.getOCFile(storageManager)!!.remotePath + val result = ReadFileRemoteOperation(remotePath).execute(client) + assertTrue(result.isSuccess) + assertEquals(data.size.toLong(), (result.data[0] as RemoteFile).length) + } + private fun awaitUploadedToServer(file: DocumentFile) { val remotePath = file.getOCFile(storageManager)!!.remotePath diff --git a/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt b/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt index 338d01b3eac6..b5a5bf194179 100644 --- a/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt +++ b/app/src/main/java/com/nextcloud/client/database/dao/UploadDao.kt @@ -14,6 +14,12 @@ import androidx.room.Query import com.nextcloud.client.database.entity.UploadEntity import com.owncloud.android.db.ProviderMeta.ProviderTableMeta +private const val UNFINISHED_UPLOAD_CONDITION = + "(${ProviderTableMeta.UPLOADS_STATUS} = :inProgressStatus " + + "OR (${ProviderTableMeta.UPLOADS_STATUS} = :failedStatus " + + "AND (${ProviderTableMeta.UPLOADS_LAST_RESULT} IS NULL " + + "OR ${ProviderTableMeta.UPLOADS_LAST_RESULT} NOT IN (:nonRetryableResults))))" + @Dao interface UploadDao { @Query( @@ -40,10 +46,7 @@ interface UploadDao { "SELECT EXISTS(SELECT 1 FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME} " + "WHERE ${ProviderTableMeta.UPLOADS_ACCOUNT_NAME} = :accountName " + "AND ${ProviderTableMeta.UPLOADS_REMOTE_PATH} = :remotePath " + - "AND (${ProviderTableMeta.UPLOADS_STATUS} = :inProgressStatus " + - "OR (${ProviderTableMeta.UPLOADS_STATUS} = :failedStatus " + - "AND (${ProviderTableMeta.UPLOADS_LAST_RESULT} IS NULL " + - "OR ${ProviderTableMeta.UPLOADS_LAST_RESULT} NOT IN (:nonRetryableResults)))))" + "AND $UNFINISHED_UPLOAD_CONDITION)" ) fun existsUnfinishedByAccountAndRemotePath( accountName: String, @@ -53,6 +56,20 @@ interface UploadDao { nonRetryableResults: List ): Boolean + @Query( + "SELECT EXISTS(SELECT 1 FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME} " + + "WHERE ${ProviderTableMeta.UPLOADS_ACCOUNT_NAME} = :accountName " + + "AND substr(${ProviderTableMeta.UPLOADS_REMOTE_PATH}, 1, length(:folderPath)) = :folderPath " + + "AND $UNFINISHED_UPLOAD_CONDITION)" + ) + fun existsUnfinishedByAccountInFolder( + accountName: String, + folderPath: String, + inProgressStatus: Int, + failedStatus: Int, + nonRetryableResults: List + ): Boolean + @Query( "DELETE FROM ${ProviderTableMeta.UPLOADS_TABLE_NAME} " + "WHERE ${ProviderTableMeta.UPLOADS_ACCOUNT_NAME} = :accountName " + diff --git a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt index 36918fa036e8..018efbd766a0 100644 --- a/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt +++ b/app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadHelper.kt @@ -437,8 +437,14 @@ class FileUploadHelper { * Mirrors the rows [FileUploadWorker] still processes; rows left behind without a scheduled upload job never * finish. Rows are matched by path, so a retryable failed row also counts while another batch of the account runs. */ - fun isUploadPending(file: OCFile, accountName: String): Boolean = - uploadsStorageManager.uploadDao.existsUnfinishedByAccountAndRemotePath( + fun isUploadPending(file: OCFile, accountName: String): Boolean { + val uploadDao = uploadsStorageManager.uploadDao + val existsUnfinished = if (file.isFolder) { + uploadDao::existsUnfinishedByAccountInFolder + } else { + uploadDao::existsUnfinishedByAccountAndRemotePath + } + return existsUnfinished( accountName, file.remotePath, UploadStatus.UPLOAD_IN_PROGRESS.value, @@ -446,6 +452,7 @@ class FileUploadHelper { nonRetryableResults ) && backgroundJobManager.isStartFileUploadJobScheduled(accountName) + } fun getLastUploadProgress(accountName: String): Long = FileUploadWorker.getLastTransferProgress(accountName) diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt index b03f64fd6e84..ac818a42c65f 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditor.kt @@ -17,6 +17,7 @@ import com.owncloud.android.operations.MoveFileOperation import com.owncloud.android.operations.RenameFileOperation import com.owncloud.android.providers.documentstorage.model.Document import com.owncloud.android.providers.documentstorage.model.DocumentResolver +import com.owncloud.android.providers.documentstorage.open.PendingDocumentWrite import com.owncloud.android.providers.documentstorage.open.PendingDocumentWrites import com.owncloud.android.providers.documentstorage.open.PendingWriteState import com.owncloud.android.providers.documentstorage.sync.DocumentNotifier @@ -150,12 +151,20 @@ internal class DocumentEditor( uploadHelper: FileUploadHelper, waitedMillis: Long ): Boolean { - val writeStates = pendingWrites.states { it == file.fileId } + val writeStates = pendingWrites.states { isAffected(document, file, it) } return PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED in writeStates || (PendingWriteState.OPEN in writeStates && waitedMillis < CLOSE_GRACE_PERIOD.inWholeMilliseconds) || uploadHelper.isUploadPending(file, document.user.accountName) } + private fun isAffected(document: Document, file: OCFile, write: PendingDocumentWrite): Boolean = + if (file.isFolder) { + write.accountName == document.user.accountName && + document.storageManager.getFileById(write.fileId)?.remotePath?.startsWith(file.remotePath) == true + } else { + write.fileId == file.fileId + } + private fun isValidTargetFolder(targetFolder: OCFile): Boolean { isFolderPathValid = nameValidator.isValidFolderPath(targetFolder.fileName) if (!isFolderPathValid) { diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt index 586245099721..cdc42fc373ce 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/DocumentOpener.kt @@ -216,7 +216,7 @@ internal class DocumentOpener( accessMode: AccessMode ): ParcelFileDescriptor { // before handing out the descriptor: close() reaches the provider asynchronously, possibly after a rename - val write = pendingWrites.begin(ocFile.fileId) + val write = pendingWrites.begin(ocFile.fileId, document.user.accountName) val onClose = ParcelFileDescriptor.OnCloseListener { error -> if (error != null) { Log_OC.e(TAG, "File was closed with an error: ${ocFile.fileName}", error) diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt index 8143552f55a7..0a92bdb5efd7 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrite.kt @@ -6,7 +6,7 @@ */ package com.owncloud.android.providers.documentstorage.open -internal class PendingDocumentWrite(val fileId: Long) { +internal class PendingDocumentWrite(val fileId: Long, val accountName: String) { @Volatile var state = PendingWriteState.OPEN } diff --git a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt index 0224c6df9c6e..725414d1ca26 100644 --- a/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt @@ -15,7 +15,8 @@ internal class PendingDocumentWrites { private val writes: MutableSet = ConcurrentHashMap.newKeySet() - fun begin(fileId: Long): PendingDocumentWrite = PendingDocumentWrite(fileId).also(writes::add) + fun begin(fileId: Long, accountName: String): PendingDocumentWrite = + PendingDocumentWrite(fileId, accountName).also(writes::add) fun closed(write: PendingDocumentWrite) { write.state = PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED @@ -25,6 +26,6 @@ internal class PendingDocumentWrites { writes.remove(write) } - fun states(isAffected: (fileId: Long) -> Boolean): Set = - writes.filter { isAffected(it.fileId) }.mapTo(mutableSetOf()) { it.state } + fun states(isAffected: (PendingDocumentWrite) -> Boolean): Set = + writes.filter(isAffected).mapTo(mutableSetOf()) { it.state } } diff --git a/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt b/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt index 66ea9b12b5d6..03e1969b8075 100644 --- a/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt +++ b/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt @@ -131,7 +131,7 @@ class DocumentEditorPendingUploadTest { @Test fun `rename waits for close that is not reported yet and then for the upload`() { - val write = pendingWrites.begin(FILE_ID) + val write = pendingWrites.begin(FILE_ID, ACCOUNT_NAME) onPoll = { when (polls) { 1 -> pendingWrites.closed(write) @@ -147,7 +147,7 @@ class DocumentEditorPendingUploadTest { @Test fun `rename of document that stays open proceeds after close grace period`() { - pendingWrites.begin(FILE_ID) + pendingWrites.begin(FILE_ID, ACCOUNT_NAME) editor.rename(SOURCE_ID, NEW_NAME) @@ -157,8 +157,8 @@ class DocumentEditorPendingUploadTest { @Test fun `rename waits for every concurrent write of the document`() { - val first = pendingWrites.begin(FILE_ID) - val second = pendingWrites.begin(FILE_ID) + val first = pendingWrites.begin(FILE_ID, ACCOUNT_NAME) + val second = pendingWrites.begin(FILE_ID, ACCOUNT_NAME) pendingWrites.closed(first) pendingWrites.closed(second) onPoll = { @@ -196,6 +196,45 @@ class DocumentEditorPendingUploadTest { assertEquals(emptyList(), operationsRunWhilePending) } + @Test + fun `folder rename waits for queued upload inside the folder`() { + givenDocument(SOURCE_ID, FILE_ID, FOLDER_PATH, isFolder = true) + uploadPending = true + onPoll = { uploadPending = false } + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(1, polls) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `folder rename waits for closed child whose upload is not queued yet`() { + givenDocument(SOURCE_ID, FILE_ID, FOLDER_PATH, isFolder = true) + givenFile(CHILD_FILE_ID, "${FOLDER_PATH}child.bin", isFolder = false) + val write = pendingWrites.begin(CHILD_FILE_ID, ACCOUNT_NAME) + pendingWrites.closed(write) + onPoll = { pendingWrites.end(write) } + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(1, polls) + assertEquals(listOf(false), operationsRunWhilePending) + } + + @Test + fun `folder rename ignores writes outside the folder or of another account`() { + givenDocument(SOURCE_ID, FILE_ID, FOLDER_PATH, isFolder = true) + givenFile(CHILD_FILE_ID, "/elsewhere/child.bin", isFolder = false) + givenFile(OTHER_ACCOUNT_CHILD_FILE_ID, "${FOLDER_PATH}child.bin", isFolder = false) + pendingWrites.closed(pendingWrites.begin(CHILD_FILE_ID, ACCOUNT_NAME)) + pendingWrites.closed(pendingWrites.begin(OTHER_ACCOUNT_CHILD_FILE_ID, "other@server")) + + editor.rename(SOURCE_ID, NEW_NAME) + + assertEquals(0, polls) + } + private fun givenDocument(documentId: String, fileId: Long, remotePath: String, isFolder: Boolean) { val file = givenFile(fileId, remotePath, isFolder) val document = mock { @@ -207,13 +246,17 @@ class DocumentEditorPendingUploadTest { whenever(resolver.toDocument(documentId)).thenReturn(document) } - private fun givenFile(fileId: Long, remotePath: String, isFolder: Boolean): OCFile = mock { - on { this.fileId } doReturn fileId - on { this.remotePath } doReturn remotePath - on { this.isFolder } doReturn isFolder - on { fileName } doReturn remotePath.trimEnd('/').substringAfterLast('/') - on { canRename() } doReturn true - on { canMove() } doReturn true + private fun givenFile(fileId: Long, remotePath: String, isFolder: Boolean): OCFile { + val file = mock { + on { this.fileId } doReturn fileId + on { this.remotePath } doReturn remotePath + on { this.isFolder } doReturn isFolder + on { fileName } doReturn remotePath.trimEnd('/').substringAfterLast('/') + on { canRename() } doReturn true + on { canMove() } doReturn true + } + whenever(storageManager.getFileById(fileId)).thenReturn(file) + return file } private fun > recordExecution(type: Class): MockedConstruction = @@ -234,7 +277,10 @@ class DocumentEditorPendingUploadTest { private const val FILE_ID = 1L private const val TARGET_FOLDER_FILE_ID = 2L private const val SOURCE_PARENT_FILE_ID = 3L + private const val CHILD_FILE_ID = 4L + private const val OTHER_ACCOUNT_CHILD_FILE_ID = 5L private const val FILE_PATH = "/folder/.document.tmp" + private const val FOLDER_PATH = "/folder/snapshot-tmp/" private const val TARGET_FOLDER_PATH = "/target/" private const val SOURCE_PARENT_PATH = "/folder/" private val IDLE_TIMEOUT = 10.minutes.inWholeMilliseconds