Skip to content

fix: prevent and clean up duplicate file entries from concurrent folder refreshes - #17885

Open
6801318d8d wants to merge 2 commits into
nextcloud:masterfrom
6801318d8d:fix/duplicate-file-rows
Open

6801318d8d wants to merge 2 commits into
nextcloud:masterfrom
6801318d8d:fix/duplicate-file-rows

Conversation

@6801318d8d

Copy link
Copy Markdown

Probably the same bug as #17856 (there in an external storage and the share chooser; here in the root folder and the system file picker), which I could not check.

What happens

On a phone with app 35.0.1 (server 34.0.5), the Android file picker (DocumentsUI → Nextcloud) listed three top-level folders twice (Butterfly, Photos, zotero) while the server has one of each. Notes and two external-storage mounts were listed once. Both copies open the same folder.

The duplicates are two rows in filelist with the same path and parent:

  1. Two refreshes of the same folder run at the same time. On launch, FileDisplayActivity.onCreate() queues MetadataWorker (refreshes the root, then each subfolder) and onResume() calls syncAndUpdateFolder(ignoreETag = true) on the root. Both run a RefreshFolderOperation on the root. DocumentsStorageProvider does the same when the picker queries an expired folder: every queryChildDocuments() starts a ReloadFolderDocumentTask on a cached thread pool.
  2. saveFolder() checks, then inserts. For each child it checks fileExists(id) || fileExists(path) and inserts if neither matches. filelist has no unique constraint on (file_owner, path) (so the CONFLICT_IGNORE insert in FileContentProvider.upsertSingleFile() never conflicts on the path either). If both refreshes read the folder before either writes, both insert every child that is new to the local database. Children that already have a row are just updated twice, which is why only folders created on the server since the last sync were doubled.
  3. A refresh never removes the extra row. prefillLocalFilesMap() keys the local children by remote path, so one of two rows with the same path overwrites the other in the map. The hidden row is neither matched with a server entry nor left over in filesToRemove, so every later refresh keeps it.

Evidence from the server's access log (Apache in the nextcloud Docker image, user agent Nextcloud-android/35.0.1; times in UTC):

  • 13:49:03, the app was launched: 4 PROPFIND /remote.php/dav/files/<user>/ in the same second (two pairs with identical response sizes = two refreshes, each a check plus a listing), then each top-level folder fetched in turn (MetadataWorker).
  • 13:59:18, DocumentsUI started the app's DocumentsStorageProvider: again 4 root PROPFINDs in the same second.
  • A fresh login on another device (account added via Login Flow v2) showed the same pattern right after the login.

Steps to reproduce

  1. Create a new folder in the root of the account on the server.
  2. Force-stop the app, so that the provider's cached listing is older than CACHE_EXPIRATION (1 minute).
  3. Open the system file picker (e.g. ACTION_OPEN_DOCUMENT from any app) and tap Nextcloud.

The provider starts two reloads of the root in parallel and the new folder is listed twice. With the release build of 35.0.1 this doubled every new folder in 3 of 4 picker reloads.

Changes

  • fix(database): serialize file row inserts. saveFile() and saveFolder() now hold a static lock across their existence checks and the write, so the second writer sees the rows of the first one and updates them instead of inserting. I put the lock in FileDataStorageManager and not in RefreshFolderOperation because these two methods are the only places that insert into filelist, and they are called from many paths besides refreshes (SynchronizeFolderOperation, uploads via saveFileWithParent(), createDirectoryTree, ...). The app is single-process for this table (only :crash runs in another process). saveFolder() now also sets the id on children that were matched by path, so callers get valid ids.
  • fix(refresh): remove duplicate file entries. prefillLocalFilesMap() gets an overload that reports rows sharing a remote path with another row. It 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 uses FileDao.deleteFolderWithDescendants() by id: it removes the database rows only (with their child rows for folders) and never deletes local files, because those belong to the row that is kept. In end-to-end encrypted folders the map key is built from the encrypted name, so only rows with the same path count as duplicates. The existing two-argument prefillLocalFilesMap() now keeps the same row instead of the last one.

Testing

  • New JVM unit test PrefillLocalFilesMapTest (duplicate detection, which row is kept, no false duplicates in encrypted folders).
  • ./gradlew :app:testGenericDebugUnitTest --tests com.owncloud.android.operations.PrefillLocalFilesMapTest on JDK 21: 5 tests, 0 failures.
  • ./gradlew :app:compileGenericDebugJavaWithJavac on the first commit alone: builds.
  • ./gradlew :app:spotlessCheck :app:detekt: pass.
  • On a device (Xiaomi POCO M4 5G, Android 14, server 34.0.5), with qaDebug builds installed next to the release app (35.0.1) and the steps to reproduce above, run by script (adb + uiautomator), one new folder per attempt:
    • master (71c112b0d4): the new folder was listed twice in 3 of 4 attempts.
    • this branch, installed over the master build with the same data: the first reload removed the three existing duplicates (app log: Removing duplicate entry 4118 of /0dup-10/, same for /0dup-11/ and /0dup-12/), and 0 of 5 new folders were listed twice. The app's filelist database, copied with run-as afterwards, has 4124 rows and no (file_owner, path) duplicates.
  • There is no automated test for the race itself.

🖼️ Screenshots

N/A (no UI change).

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

The diagnosis, code, tests and this description were produced with Claude Code (Claude Opus 5.5). The commits carry an Assisted-by trailer.

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
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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant