From cfbf6dd3650397ff24e870f23f1491d8a43a1acc Mon Sep 17 00:00:00 2001 From: 6801318d8d <144167388+6801318d8d@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:37:59 +0100 Subject: [PATCH 1/2] fix(database): serialize file row inserts to avoid duplicate entries saveFile() and saveFolder() check whether a row exists (by id or remote path) and insert one otherwise. The filelist table has no unique constraint on the remote path, so two writers running at the same time both insert the same new file. This happens on app launch, where FileDisplayActivity.onCreate() queues MetadataWorker and onResume() refreshes the root folder, and in DocumentsStorageProvider, which runs one ReloadFolderDocumentTask per children query on a cached thread pool: every folder that is new to the local database ends up twice in the list. Hold a static lock across the existence checks and the write, so the second writer sees the rows of the first one and updates them. The lock is in FileDataStorageManager rather than in RefreshFolderOperation because these two methods are the only places that insert into the table and they are also called by SynchronizeFolderOperation, uploads and directory creation. When saveFolder() updates a child that it found by path, also set the id on the OCFile so callers get a valid id. Signed-off-by: 6801318d8d <144167388+6801318d8d@users.noreply.github.com> Assisted-by: ClaudeCode:claude-opus-5-5 --- .../datamodel/FileDataStorageManager.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java index 04055f375549..8debbd21ace2 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java @@ -112,6 +112,13 @@ public class FileDataStorageManager { private static final String JSON_EMPTY_ARRAY = "[]"; private static final long INTERNAL_TWO_WAY_SYNC_NEVER_SYNCED = 0L; + /** + * Serializes the "update if the row exists, insert otherwise" sequences of {@link #saveFile(OCFile)} and + * {@link #saveFolder(OCFile, List, Collection)}. The file table has no unique constraint on the remote path, so + * two concurrent writers (e.g. two refreshes of the same folder) would otherwise both insert the same new file. + */ + private static final Object FILE_TABLE_WRITE_LOCK = new Object(); + private final ContentResolver contentResolver; private final ContentProviderClient contentProviderClient; private final User user; @@ -572,6 +579,12 @@ public List getFolderImagesAndVideos(OCFile folder, boolean onlyOnDevice } public boolean saveFile(OCFile ocFile) { + synchronized (FILE_TABLE_WRITE_LOCK) { + return saveFileLocked(ocFile); + } + } + + private boolean saveFileLocked(OCFile ocFile) { Log_OC.d(TAG, "saving file " + ocFile.getFileName() + " into " + ocFile.getRemotePath()); boolean overridden = false; @@ -754,6 +767,12 @@ public void saveNewFile(OCFile newFile) { * @param filesToRemove */ public void saveFolder(OCFile folder, List updatedFiles, Collection filesToRemove) { + synchronized (FILE_TABLE_WRITE_LOCK) { + saveFolderLocked(folder, updatedFiles, filesToRemove); + } + } + + private void saveFolderLocked(OCFile folder, List updatedFiles, Collection filesToRemove) { Log_OC.d(TAG, "Saving folder " + folder.getRemotePath() + " with " + updatedFiles.size() + " children and " + filesToRemove.size() + " files to remove"); @@ -770,6 +789,7 @@ public void saveFolder(OCFile folder, List updatedFiles, Collection Date: Mon, 26 Oct 2026 15:38:01 +0100 Subject: [PATCH 2/2] fix(refresh): remove duplicate file entries when refreshing a folder prefillLocalFilesMap() keys the local children of a folder by remote path. When two rows share a path, one of them is hidden by the other: it is never matched with a server entry and never left over for removal, so it stays in the list forever and every refresh keeps it. Add an overload that reports the duplicates and keeps the row whose local copy exists on the device, otherwise the oldest one (lowest id). RefreshFolderOperation and SynchronizeFolderOperation delete the reported rows with removeDuplicateEntries(), which removes the database rows by id (with their child rows for folders) and never deletes local files, since those belong to the row that is kept. Rows that only share the map key in an end-to-end encrypted folder but have different remote paths are not treated as duplicates. Signed-off-by: 6801318d8d <144167388+6801318d8d@users.noreply.github.com> Assisted-by: ClaudeCode:claude-opus-5-5 --- .../datamodel/FileDataStorageManager.java | 11 +++ .../operations/RefreshFolderOperation.java | 49 +++++++++- .../SynchronizeFolderOperation.java | 8 +- .../operations/PrefillLocalFilesMapTest.kt | 90 +++++++++++++++++++ 4 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt diff --git a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java index 8debbd21ace2..248ea0d7f681 100644 --- a/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java +++ b/app/src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java @@ -1085,6 +1085,17 @@ private boolean removeFolderInDb(OCFile folder) { return fileDao.deleteFolderWithDescendants(user.getAccountName(), folder.getFileId()) > 0; } + /** + * Removes duplicate database entries, with the entries of their contents for folders. Local files are not + * touched: they belong to the entry that is kept for the same remote path. + */ + public void removeDuplicateEntries(Collection duplicates) { + for (OCFile duplicate : duplicates) { + Log_OC.w(TAG, "Removing duplicate entry " + duplicate.getFileId() + " of " + duplicate.getRemotePath()); + fileDao.deleteFolderWithDescendants(user.getAccountName(), duplicate.getFileId()); + } + } + private boolean removeLocalFolder(OCFile folder) { if (folder == null) { Log_OC.d(TAG, "removeLocalFolder: folder is null"); diff --git a/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java b/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java index dc27a5f236a3..49bfa7ecf475 100644 --- a/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java +++ b/app/src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java @@ -58,6 +58,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import androidx.localbroadcastmanager.content.LocalBroadcastManager; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -557,16 +558,24 @@ private void synchronizeData(List folderAndFiles) { // get current data about local contents of the folder to synchronize Map localFilesMap; + List duplicateLocalFiles = new ArrayList<>(); E2EVersion e2EVersion = fileDataStorageManager.getE2EEVersionObject(user); if (object instanceof DecryptedFolderMetadataFileV1 metadataFileV1) { - localFilesMap = prefillLocalFilesMap(metadataFileV1, fileDataStorageManager.getFolderContent(mLocalFolder, false)); + localFilesMap = prefillLocalFilesMap(metadataFileV1, + fileDataStorageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); } else { - localFilesMap = prefillLocalFilesMap(object, fileDataStorageManager.getFolderContent(mLocalFolder, false)); + localFilesMap = prefillLocalFilesMap(object, + fileDataStorageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); if (object instanceof DecryptedFolderMetadataFile metadataFile) { mLocalFolder.setE2eCounter(metadataFile.getMetadata().getCounter()); } } + // duplicate entries are never matched with a remote file, so they would stay forever + fileDataStorageManager.removeDuplicateEntries(duplicateLocalFiles); + // loop to update every child OCFile remoteFile; OCFile localFile; @@ -792,6 +801,19 @@ private void setLocalFileDataOnUpdatedFile(OCFile remoteFile, OCFile localFile, @NonNull @SuppressFBWarnings("OCP") public static Map prefillLocalFilesMap(Object metadata, List localFiles) { + return prefillLocalFilesMap(metadata, localFiles, new ArrayList<>()); + } + + /** + * Maps the remote path of every local file to the file. Local files with the same remote path as another one are + * duplicate database entries: only the one chosen by {@link #preferredLocalFile(OCFile, OCFile)} is mapped, the + * others are added to {@code duplicates}. + */ + @NonNull + @SuppressFBWarnings("OCP") + public static Map prefillLocalFilesMap(Object metadata, + List localFiles, + List duplicates) { Map localFilesMap = Maps.newHashMapWithExpectedSize(localFiles.size()); for (OCFile file : localFiles) { @@ -803,11 +825,32 @@ public static Map prefillLocalFilesMap(Object metadata, List folderAndFiles) throws OperationCancel } // get current data about local contents of the folder to synchronize - Map localFilesMap = RefreshFolderOperation.prefillLocalFilesMap(object,storageManager.getFolderContent(mLocalFolder, false)); + List duplicateLocalFiles = new ArrayList<>(); + Map localFilesMap = RefreshFolderOperation.prefillLocalFilesMap(object, + storageManager.getFolderContent(mLocalFolder, false), + duplicateLocalFiles); + + // duplicate entries are never matched with a remote file, so they would stay forever + storageManager.removeDuplicateEntries(duplicateLocalFiles); // loop to synchronize every child List updatedFiles = new ArrayList<>(folderAndFiles.size() - 1); diff --git a/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt b/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt new file mode 100644 index 000000000000..fb88ee976bd5 --- /dev/null +++ b/app/src/test/java/com/owncloud/android/operations/PrefillLocalFilesMapTest.kt @@ -0,0 +1,90 @@ +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +package com.owncloud.android.operations + +import com.owncloud.android.datamodel.OCFile +import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock + +class PrefillLocalFilesMapTest { + + private fun localFile(id: Long, path: String, onDevice: Boolean = false): OCFile = mock { + on { fileId } doReturn id + on { remotePath } doReturn path + on { existsOnDevice() } doReturn onDevice + } + + @Test + fun withoutDuplicates_mapsEveryFile() { + val photos = localFile(1, "/Photos/") + val notes = localFile(2, "/Notes/") + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(photos, notes), duplicates) + + assertEquals(mapOf("/Photos/" to photos, "/Notes/" to notes), map) + assertTrue(duplicates.isEmpty()) + } + + @Test + fun withDuplicates_keepsLowestIdAndReportsTheOthers() { + val newer = localFile(7, "/Photos/") + val oldest = localFile(3, "/Photos/") + val middle = localFile(5, "/Photos/") + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(newer, oldest, middle), duplicates) + + assertSame(oldest, map["/Photos/"]) + assertEquals(setOf(newer, middle), duplicates.toSet()) + assertEquals(2, duplicates.size) + } + + @Test + fun withDuplicates_keepsEntryWithLocalCopy() { + val oldest = localFile(3, "/Documents/report.pdf") + val downloaded = localFile(9, "/Documents/report.pdf", onDevice = true) + val duplicates = mutableListOf() + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(oldest, downloaded), duplicates) + + assertSame(downloaded, map["/Documents/report.pdf"]) + assertEquals(listOf(oldest), duplicates) + } + + @Test + fun encryptedFolder_sameKeyButDifferentRemotePath_isNoDuplicate() { + val first: OCFile = mock { + on { remotePath } doReturn "/secret/aaa" + on { parentRemotePath } doReturn "/secret/" + } + val second: OCFile = mock { + on { remotePath } doReturn "/secret/bbb" + on { parentRemotePath } doReturn "/secret/" + } + val duplicates = mutableListOf() + + RefreshFolderOperation.prefillLocalFilesMap(Any(), listOf(first, second), duplicates) + + assertTrue(duplicates.isEmpty()) + } + + @Test + fun withoutDuplicatesList_mapsTheSameEntry() { + val newer = localFile(7, "/zotero/") + val oldest = localFile(3, "/zotero/") + + val map = RefreshFolderOperation.prefillLocalFilesMap(null, listOf(newer, oldest)) + + assertSame(oldest, map["/zotero/"]) + } +}