From f55ba7af2ee2c0d591df7eb74aed9945cb778e3e Mon Sep 17 00:00:00 2001 From: Eiichi Yoshikawa Date: Sat, 24 Jan 2026 16:35:47 +0900 Subject: [PATCH 1/2] feat: enable auto-sync for file access via Storage Access Framework (SAF). --- .../DocumentsStorageProvider.kt | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt b/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt index 45d8a58f3..7a6b4d01f 100644 --- a/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt +++ b/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt @@ -63,7 +63,6 @@ import eu.opencloud.android.presentation.documentsprovider.cursors.SpaceCursor import eu.opencloud.android.presentation.settings.security.SettingsSecurityFragment.Companion.PREFERENCE_LOCK_ACCESS_FROM_DOCUMENT_PROVIDER import eu.opencloud.android.usecases.synchronization.SynchronizeFileUseCase import eu.opencloud.android.usecases.synchronization.SynchronizeFolderUseCase -import eu.opencloud.android.usecases.transfers.downloads.DownloadFileUseCase import eu.opencloud.android.usecases.transfers.uploads.UploadFilesFromSystemUseCase import eu.opencloud.android.utils.FileStorageUtils import eu.opencloud.android.utils.NotificationUtils @@ -107,11 +106,8 @@ class DocumentsStorageProvider : DocumentsProvider() { if (!uploadOnly) { ocFile = getFileByIdOrException(documentId.toInt()) - if (!ocFile.isAvailableLocally) { - val downloadFileUseCase: DownloadFileUseCase by inject() - - downloadFileUseCase(DownloadFileUseCase.Params(accountName = ocFile.owner, file = ocFile)) - + if (!ocFile.isAvailableLocally || !isWrite) { + syncFileWithServer(ocFile) do { if (!waitOrGetCancelled(signal)) { return null @@ -150,23 +146,7 @@ class DocumentsStorageProvider : DocumentsProvider() { uploadFilesUseCase(uploadFilesUseCaseParams) } } else { - Thread { - val synchronizeFileUseCase: SynchronizeFileUseCase by inject() - val result = synchronizeFileUseCase( - SynchronizeFileUseCase.Params( - fileToSynchronize = ocFile, - ) - ) - Timber.d("Synced ${ocFile.remotePath} from ${ocFile.owner} with result: $result") - if (result.getDataOrNull() is SynchronizeFileUseCase.SyncType.ConflictDetected) { - context?.let { - NotificationUtils.notifyConflict( - fileInConflict = ocFile, - context = it - ) - } - } - }.start() + syncFileWithServer(ocFile) } } } catch (e: IOException) { @@ -488,6 +468,28 @@ class DocumentsStorageProvider : DocumentsProvider() { return NONEXISTENT_DOCUMENT_ID } + private fun syncFileWithServer(fileToSync: OCFile) { + Timber.d("Trying to sync a file ${fileToSync.id} with server") + + val synchronizeFileUseCase : SynchronizeFileUseCase by inject() + val synchronizeFileUseCaseParam = SynchronizeFileUseCase.Params( + fileToSynchronize = fileToSync + ) + CoroutineScope(Dispatchers.IO).launch { + val useCaseResult = synchronizeFileUseCase(synchronizeFileUseCaseParam) + Timber.d("${fileToSync.remotePath} from ${fileToSync.owner} was synced with server with result: $useCaseResult") + + if (useCaseResult.getDataOrNull() is SynchronizeFileUseCase.SyncType.ConflictDetected) { + context?.let { + NotificationUtils.notifyConflict( + fileInConflict = fileToSync, + context = it + ) + } + } + } + } + private fun syncDirectoryWithServer(parentDocumentId: String) { Timber.d("Trying to sync $parentDocumentId with server") val folderToSync = getFileByIdOrException(parentDocumentId.toInt()) From 8d83217608474cf00d24660aad6bd76147d96692 Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Tue, 10 Mar 2026 18:28:23 +0100 Subject: [PATCH 2/2] fix: serve up-to-date files when accessed via SAF, not stale local copies The previous implementation fired SynchronizeFileUseCase asynchronously and polled isAvailableLocally. For files that were already downloaded but outdated on the server, isAvailableLocally was already true from the old copy, so the poll exited immediately and served the stale file. Call SynchronizeFileUseCase synchronously and act on its result: when a download is enqueued, poll the WorkManager job state until it completes. This also detects download failures (FAILED/CANCELLED) and returns early instead of polling indefinitely. --- .../DocumentsStorageProvider.kt | 226 ++++++++++++++++-- .../android/workers/DownloadFileWorker.kt | 14 +- 2 files changed, 219 insertions(+), 21 deletions(-) diff --git a/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt b/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt index 7a6b4d01f..4e1a9bc73 100644 --- a/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt +++ b/opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt @@ -62,10 +62,13 @@ import eu.opencloud.android.presentation.documentsprovider.cursors.RootCursor import eu.opencloud.android.presentation.documentsprovider.cursors.SpaceCursor import eu.opencloud.android.presentation.settings.security.SettingsSecurityFragment.Companion.PREFERENCE_LOCK_ACCESS_FROM_DOCUMENT_PROVIDER import eu.opencloud.android.usecases.synchronization.SynchronizeFileUseCase +import eu.opencloud.android.usecases.transfers.downloads.DownloadFileUseCase import eu.opencloud.android.usecases.synchronization.SynchronizeFolderUseCase import eu.opencloud.android.usecases.transfers.uploads.UploadFilesFromSystemUseCase import eu.opencloud.android.utils.FileStorageUtils import eu.opencloud.android.utils.NotificationUtils +import androidx.work.WorkInfo +import androidx.work.WorkManager import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch @@ -74,7 +77,10 @@ import timber.log.Timber import java.io.File import java.io.FileNotFoundException import java.io.IOException +import java.util.UUID import java.util.Vector +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CompletableFuture class DocumentsStorageProvider : DocumentsProvider() { /** @@ -89,6 +95,16 @@ class DocumentsStorageProvider : DocumentsProvider() { private lateinit var fileToUpload: OCFile + // Cache to avoid redundant PROPFINDs when apps (e.g. Google Photos) call + // openDocument many times for the same file. Two layers: + // 1. In-flight dedup: concurrent calls for the same file share one PROPFIND via + // a CompletableFuture. The first caller does the actual work, others wait. + // 2. TTL cache: after a sync completes, skip re-checking the same file for a + // few seconds to handle rapid sequential calls. + private val inFlightSyncs = ConcurrentHashMap>() + private var propfindCacheFileId: Long? = null + private var propfindCacheTimestamp: Long = 0 + override fun openDocument( documentId: String, mode: String, @@ -106,15 +122,80 @@ class DocumentsStorageProvider : DocumentsProvider() { if (!uploadOnly) { ocFile = getFileByIdOrException(documentId.toInt()) - if (!ocFile.isAvailableLocally || !isWrite) { - syncFileWithServer(ocFile) - do { - if (!waitOrGetCancelled(signal)) { - return null + if (!ocFile.isAvailableLocally) { + // File has never been downloaded. Enqueue the download directly — + // no need for a PROPFIND since we already know we need the file. + val downloadFileUseCase: DownloadFileUseCase by inject() + val workerId = downloadFileUseCase( + DownloadFileUseCase.Params(accountName = ocFile.owner, file = ocFile) + ) + if (!waitForDownload(workerId, documentId.toInt(), signal)) { + return null + } + ocFile = getFileByIdOrException(documentId.toInt()) + if (!ocFile.isAvailableLocally) { + return null + } + // Seed the TTL cache — the file was just downloaded, so there's no need + // for a PROPFIND if Google Photos immediately calls openDocument again. + propfindCacheFileId = ocFile.id + propfindCacheTimestamp = System.currentTimeMillis() + } else if (!isWrite) { + // File is available locally and opened for reading. Check with the server + // (PROPFIND) whether a newer version exists, and download it if so. + // + // Apps like Google Photos call openDocument many times concurrently for + // the same file. Without dedup, each call does its own PROPFIND, and due + // to the synchronized lock in OpenCloudClient.executeHttpMethod they + // serialize — causing 10+ second waits per extra call. We handle this + // with two layers: + // 1. TTL cache: skip if we just confirmed this file is up-to-date. + // 2. In-flight dedup: concurrent calls share one PROPFIND result. + val fileId = ocFile.id!! + val now = System.currentTimeMillis() + if (fileId == propfindCacheFileId && now - propfindCacheTimestamp <= PROPFIND_CACHE_TTL_MS) { + Timber.d("Skipping PROPFIND for file $fileId, recently synced ${now - propfindCacheTimestamp}ms ago") + } else { + val syncResult = syncFileWithServerCoalesced(ocFile) + + when (syncResult) { + is SynchronizeFileUseCase.SyncType.AlreadySynchronized -> { + // File is up to date, nothing to wait for. + } + is SynchronizeFileUseCase.SyncType.DownloadEnqueued -> { + // A newer version exists. SynchronizeFileUseCase only enqueues + // a WorkManager download, it does not wait for it to finish. + if (!waitForDownload(syncResult.workerId, documentId.toInt(), signal)) { + return null + } + } + is SynchronizeFileUseCase.SyncType.ConflictDetected -> { + // File changed both locally and remotely. Notify the user and + // serve the local version (same behavior as before). + context?.let { + NotificationUtils.notifyConflict(fileInConflict = ocFile, context = it) + } + } + is SynchronizeFileUseCase.SyncType.FileNotFound -> { + return null + } + is SynchronizeFileUseCase.SyncType.UploadEnqueued -> { + // Local file is newer, upload was enqueued. Serve the local version. + } + null -> { + // Sync failed, serve the local version anyway. + } } - ocFile = getFileByIdOrException(documentId.toInt()) - } while (!ocFile.isAvailableLocally) + propfindCacheFileId = fileId + propfindCacheTimestamp = System.currentTimeMillis() + + // Re-read the file from DB to get the updated state after download. + ocFile = getFileByIdOrException(documentId.toInt()) + if (!ocFile.isAvailableLocally) { + return null + } + } } } else { ocFile = fileToUpload @@ -146,7 +227,7 @@ class DocumentsStorageProvider : DocumentsProvider() { uploadFilesUseCase(uploadFilesUseCaseParams) } } else { - syncFileWithServer(ocFile) + syncFileWithServerAsync(ocFile) } } } catch (e: IOException) { @@ -468,28 +549,134 @@ class DocumentsStorageProvider : DocumentsProvider() { return NONEXISTENT_DOCUMENT_ID } - private fun syncFileWithServer(fileToSync: OCFile) { - Timber.d("Trying to sync a file ${fileToSync.id} with server") + /** + * Synchronize a file with the server, coalescing concurrent requests. + * + * If another thread is already syncing this file, we wait for its result instead of + * starting a second PROPFIND. This avoids the serialized lock contention in + * OpenCloudClient.executeHttpMethod when multiple binder threads call openDocument + * for the same file simultaneously. + * + * The future is always removed from [inFlightSyncs] when done (via finally), + * so errors or timeouts cannot leave stale entries that would block future syncs. + */ + private fun syncFileWithServerCoalesced(fileToSync: OCFile): SynchronizeFileUseCase.SyncType? { + val fileId = fileToSync.id!! + val newFuture = CompletableFuture() + val existingFuture = inFlightSyncs.putIfAbsent(fileId, newFuture) + + if (existingFuture != null) { + // Another thread is already syncing this file. Wait for its result. + Timber.d("Sync for file $fileId already in flight, waiting for result") + return try { + existingFuture.get() + } catch (e: Exception) { + Timber.w(e, "In-flight sync for file $fileId failed, serving local version") + null + } + } - val synchronizeFileUseCase : SynchronizeFileUseCase by inject() - val synchronizeFileUseCaseParam = SynchronizeFileUseCase.Params( - fileToSynchronize = fileToSync + // We are the first thread — do the actual PROPFIND. + return try { + val result = syncFileWithServerBlocking(fileToSync) + newFuture.complete(result) + result + } catch (e: Exception) { + newFuture.completeExceptionally(e) + throw e + } finally { + inFlightSyncs.remove(fileId) + } + } + + /** + * Synchronize a file with the server and return the result. + * Runs synchronously on the calling thread (blocks until the PROPFIND completes). + * Note: if a download is needed, this only *enqueues* it — use [waitForDownload] to + * wait for the actual download to finish. + */ + private fun syncFileWithServerBlocking(fileToSync: OCFile): SynchronizeFileUseCase.SyncType? { + Timber.d("Trying to sync file ${fileToSync.id} with server (blocking)") + + val synchronizeFileUseCase: SynchronizeFileUseCase by inject() + val useCaseResult = synchronizeFileUseCase( + SynchronizeFileUseCase.Params(fileToSynchronize = fileToSync) ) + Timber.d("${fileToSync.remotePath} from ${fileToSync.owner} synced with result: $useCaseResult") + + return useCaseResult.getDataOrNull() + } + + /** + * Fire-and-forget sync: used in the close handler after writes, + * where we don't need to wait for the result. + */ + private fun syncFileWithServerAsync(fileToSync: OCFile) { + Timber.d("Trying to sync file ${fileToSync.id} with server (async)") + + val synchronizeFileUseCase: SynchronizeFileUseCase by inject() CoroutineScope(Dispatchers.IO).launch { - val useCaseResult = synchronizeFileUseCase(synchronizeFileUseCaseParam) - Timber.d("${fileToSync.remotePath} from ${fileToSync.owner} was synced with server with result: $useCaseResult") + val useCaseResult = synchronizeFileUseCase( + SynchronizeFileUseCase.Params(fileToSynchronize = fileToSync) + ) + Timber.d("${fileToSync.remotePath} from ${fileToSync.owner} synced with result: $useCaseResult") if (useCaseResult.getDataOrNull() is SynchronizeFileUseCase.SyncType.ConflictDetected) { context?.let { - NotificationUtils.notifyConflict( - fileInConflict = fileToSync, - context = it - ) + NotificationUtils.notifyConflict(fileInConflict = fileToSync, context = it) } } } } + /** + * Wait for a download to finish. + * + * If [workerId] is non-null, we use WorkManager to wait directly for that specific job. + * If [workerId] is null, it means a download for this file was already in progress + * (enqueued by a previous call), so we fall back to polling the DB until the file + * becomes available locally. + * + * Note: openDocument can be called concurrently on multiple binder threads for the + * same file (e.g. the calling app retries or requests the file multiple times). + * The first call enqueues the download and gets a workerId; subsequent concurrent + * calls get null (DownloadFileUseCase deduplicates) and use the polling fallback. + * + * @return true if the file is ready, false if cancelled. + */ + private fun waitForDownload(workerId: UUID?, fileId: Int, signal: CancellationSignal?): Boolean { + if (workerId != null) { + // Poll WorkManager until this specific job reaches a terminal state. + // Note: getWorkInfoById().get() returns the *current* state immediately, + // it does NOT block until the work finishes. + Timber.d("Waiting for download worker $workerId to finish") + val workManager = WorkManager.getInstance(context!!) + do { + if (!waitOrGetCancelled(signal)) { + return false + } + val workInfo = workManager.getWorkInfoById(workerId).get() + Timber.d("Download worker $workerId state: ${workInfo.state}") + when (workInfo.state) { + WorkInfo.State.SUCCEEDED -> return true + WorkInfo.State.FAILED, WorkInfo.State.CANCELLED -> return false + else -> { /* ENQUEUED, RUNNING, BLOCKED — keep waiting */ } + } + } while (true) + } + + // workerId is null — a download was already in progress from a previous request. + // Poll until the file appears locally, checking for cancellation each second. + Timber.d("Download already in progress for file $fileId, polling until available") + do { + if (!waitOrGetCancelled(signal)) { + return false + } + val file = getFileByIdOrException(fileId) + if (file.isAvailableLocally) return true + } while (true) + } + private fun syncDirectoryWithServer(parentDocumentId: String) { Timber.d("Trying to sync $parentDocumentId with server") val folderToSync = getFileByIdOrException(parentDocumentId.toInt()) @@ -585,5 +772,6 @@ class DocumentsStorageProvider : DocumentsProvider() { companion object { const val NONEXISTENT_DOCUMENT_ID = "-1" + const val PROPFIND_CACHE_TTL_MS = 3000L } } diff --git a/opencloudApp/src/main/java/eu/opencloud/android/workers/DownloadFileWorker.kt b/opencloudApp/src/main/java/eu/opencloud/android/workers/DownloadFileWorker.kt index f5b0e0258..e588f16b3 100644 --- a/opencloudApp/src/main/java/eu/opencloud/android/workers/DownloadFileWorker.kt +++ b/opencloudApp/src/main/java/eu/opencloud/android/workers/DownloadFileWorker.kt @@ -205,14 +205,24 @@ class DownloadFileWorker( * We will update info about local storage (where it was stored and its size) */ private fun updateDatabaseWithLatestInfoForThisFile() { + val finalFile = File(finalLocationForFile) val currentTime = System.currentTimeMillis() ocFile.apply { needsToUpdateThumbnail = true modificationTimestamp = downloadRemoteFileOperation.modificationTimestamp etag = downloadRemoteFileOperation.etag storagePath = finalLocationForFile - length = (File(finalLocationForFile).length()) - lastSyncDateForData = currentTime + length = finalFile.length() + // Use the file's actual mtime, not the current time. SynchronizeFileUseCase + // compares lastSyncDateForData against the file's filesystem mtime to detect + // local modifications. Using currentTime here can be slightly earlier than the + // file's mtime (due to write time and second-precision rounding), which causes + // a false "changed locally" detection and an unnecessary upload. + // This mainly affects SAF (DocumentsStorageProvider) where apps like Google + // Photos call openDocument repeatedly right after download, triggering the + // false positive immediately. In normal app usage, the next sync is typically + // much later and the small timestamp difference doesn't cause issues. + lastSyncDateForData = finalFile.lastModified() modifiedAtLastSyncForData = downloadRemoteFileOperation.modificationTimestamp lastUsage = currentTime }