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..66a2f5d56e8f --- /dev/null +++ b/app/src/androidTest/java/com/nextcloud/client/database/dao/UploadDaoUnfinishedUploadsTest.kt @@ -0,0 +1,116 @@ +/* + * 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.Assert.assertTrue +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()) + } + + @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, + 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 + ) + + 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 bf1631108716..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 @@ -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,39 @@ 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) + } + + @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 653dc03c9050..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( @@ -36,6 +42,34 @@ 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 $UNFINISHED_UPLOAD_CONDITION)" + ) + fun existsUnfinishedByAccountAndRemotePath( + accountName: String, + remotePath: String, + inProgressStatus: Int, + failedStatus: Int, + 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/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..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 @@ -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,29 @@ 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 { + val uploadDao = uploadsStorageManager.uploadDao + val existsUnfinished = if (file.isFolder) { + uploadDao::existsUnfinishedByAccountInFolder + } else { + uploadDao::existsUnfinishedByAccountAndRemotePath + } + return existsUnfinished( + 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 +524,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 +561,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..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 @@ -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,27 @@ 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 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 +59,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 +80,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 +112,7 @@ internal class DocumentEditor( return null } + awaitPendingUploads(document, sourceFile) MoveFileOperation(sourceFile.remotePath, targetFile.remotePath, document.storageManager) .execute(document.requireClient(context)) .requireSuccess(TAG) { @@ -109,6 +124,47 @@ 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 { 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) { @@ -119,5 +175,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..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 @@ -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, document.user.accountName) 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..0a92bdb5efd7 --- /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, 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 new file mode 100644 index 000000000000..725414d1ca26 --- /dev/null +++ b/app/src/main/java/com/owncloud/android/providers/documentstorage/open/PendingDocumentWrites.kt @@ -0,0 +1,31 @@ +/* + * 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, accountName: String): PendingDocumentWrite = + PendingDocumentWrite(fileId, accountName).also(writes::add) + + fun closed(write: PendingDocumentWrite) { + write.state = PendingWriteState.CLOSED_UPLOAD_NOT_QUEUED + } + + fun end(write: PendingDocumentWrite) { + writes.remove(write) + } + + fun states(isAffected: (PendingDocumentWrite) -> Boolean): Set = + writes.filter(isAffected).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..03e1969b8075 --- /dev/null +++ b/app/src/test/java/com/owncloud/android/providers/documentstorage/modify/DocumentEditorPendingUploadTest.kt @@ -0,0 +1,289 @@ +/* + * 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, ACCOUNT_NAME) + 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, ACCOUNT_NAME) + + 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, ACCOUNT_NAME) + val second = pendingWrites.begin(FILE_ID, ACCOUNT_NAME) + 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) + } + + @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 { + 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 { + 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 = + 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 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 + private val CLOSE_GRACE_PERIOD = 5.seconds.inWholeMilliseconds + } +}