Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ interface FileSystemDao {
)
suspend fun getBySyncedFolderId(syncedFolderId: String): List<FilesystemEntity>

@Query(
"""
DELETE FROM ${ProviderMeta.ProviderTableMeta.FILESYSTEM_TABLE_NAME}
WHERE ${ProviderMeta.ProviderTableMeta.FILESYSTEM_SYNCED_FOLDER_ID} = :syncedFolderId
"""
)
suspend fun deleteBySyncedFolderId(syncedFolderId: String)

@Query(
"""
SELECT COUNT(*) > 0 FROM ${ProviderMeta.ProviderTableMeta.FILESYSTEM_TABLE_NAME}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,10 @@ class FileUploadHelper {
}
}

suspend fun clearAutoUploadScanCache(id: Long) {
uploadsStorageManager.fileSystemDao.deleteBySyncedFolderId(id.toString())
}

/**
* Splits a list of files into:
* 1. Files that have an auto-upload folder configured.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ class AutoUploadDisplayItemFactory(
isSubfolderByDate = syncedFolder.isSubfolderByDate
uploadAction = syncedFolder.uploadAction
setNameCollisionPolicy(syncedFolder.nameCollisionPolicy.serialize())
setEnabled(syncedFolder.isEnabled, clock.currentTime)
if (syncedFolder.isEnabled != isEnabled) {
setEnabled(syncedFolder.isEnabled, clock.currentTime)
}
Comment on lines +125 to +127

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6758178. updateExistingFolder now does the same as the list toggle: switching a folder off in its settings runs the upload entity cleanup, and switching it on clears the scan cache before saving and starting the scan. I have not added a test for the handler path, it needs the fragment and adapter; the factory part is covered by AutoUploadDisplayItemFactoryTest.

Claude Code on behalf of @Blackclaws

setSubFolderRule(syncedFolder.subFolderRule)
setExcludeHidden(syncedFolder.isExcludeHidden)
}
Expand All @@ -146,7 +148,7 @@ class AutoUploadDisplayItemFactory(
syncedFolder.uploadAction,
syncedFolder.nameCollisionPolicyInt,
syncedFolder.isEnabled,
clock.currentTime,
syncedFolder.enabledTimestampMs,
filePaths,
folderName,
numberOfFiles,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class AutoUploadClickHandler(

private val fileUploadHelper = FileUploadHelper.instance()

private val folderStateDispatcher = Dispatchers.IO.limitedParallelism(1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to limit parallelism to 1? Also how come click handler can be triggered from multiple threads at the same time?

Additionally, please remove the comment. It makes hard to follow code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment in e209985.

The handler is only called on the main thread. The concern is the coroutines it launches: every click starts one on Dispatchers.IO, a thread pool, so the cleanup launched by "off" and the clear-and-start launched by a quick "on" right after can run at the same time or in reverse order. If "on" runs first, it clears the filesystem entities and starts the worker, and the "off" cleanup then finds no entities, so it leaves the upload entities behind. A leftover UPLOADED entity with the skip policy makes the worker treat the file as uploaded again, which brings back the bug this PR fixes. If the cleanup instead runs while the worker is uploading, it deletes the upload entities the worker just created.

limitedParallelism(1) runs these coroutines one at a time, in the order of the clicks. The window is small, since it needs off and on within the time the cleanup takes. If you consider that negligible, I can drop the dispatcher and use Dispatchers.IO as before.

Claude Code on behalf of @Blackclaws

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limitedParallelism(1) serializes the wrong things
It only orders two coroutines inside AutoUploadClickHandler (disable cleanup vs. enable step).
Both start from the main thread milliseconds apart; a user toggle can't outrun Dispatchers.IO starting a coroutine. The real concurrent actor is AutoUploadWorker, which the dispatcher doesn't cover:
Disabling a folder does not cancel its worker. The worker checks isEnabled only at start (AutoUploadWorker.kt:97-98).
Re-enabling calls startAutoUpload with ExistingWorkPolicy.KEEP (BackgroundJobManagerImpl.kt:560). If the old worker is still running, no new one is queued.
clearAutoUploadScanCachethen deletesfilesystemrows while that worker is paging through them viagetFilePathsWithIds(lastId)`.

The dispatcher is a field on AutoUploadClickHandler, created in AutoUploadFragment.setupHelpers (AutoUploadFragment.kt:185).
Fragment recreation → new handler → new dispatcher → no ordering across instances.
Everything runs in lifecycleScope; toggle then leave the screen → cleanup can be cancelled. Serial execution doesn't help there.

Also Timestamp changes in AutoUploadDisplayItemFactory should be separate PR.


fun onCreateCustomFolderClick(activity: AppCompatActivity, accountName: String) {
Log_OC.d(TAG, "Show custom folder dialog")
if (!PermissionUtil.checkStoragePermission(activity)) {
Expand All @@ -60,14 +62,21 @@ class AutoUploadClickHandler(

if (item.isEnabled) {
Log_OC.d(TAG, "auto-upload configuration sync status is enabled: " + item.remotePath)
backgroundJobManager.startAutoUpload(item, overridePowerSaving = false)
fragment.lifecycleScope.launch(folderStateDispatcher) {
fileUploadHelper.clearAutoUploadScanCache(item.id)
backgroundJobManager.startAutoUpload(item, overridePowerSaving = false)
}
dialogs.showBatteryOptimizationDialogIfNeeded()
return
}

Log_OC.d(TAG, "auto-upload configuration sync status is disabled: " + item.remotePath)
fragment.lifecycleScope.launch(Dispatchers.IO) {
fileUploadHelper.removeEntityFromUploadEntities(item.id)
removeUploadEntities(item.id)
}

private fun removeUploadEntities(syncedFolderId: Long) {
fragment.lifecycleScope.launch(folderStateDispatcher) {
fileUploadHelper.removeEntityFromUploadEntities(syncedFolderId)
}
}

Expand Down Expand Up @@ -135,8 +144,23 @@ class AutoUploadClickHandler(
private fun updateExistingFolder(syncedFolder: SyncedFolderParcelable): Boolean {
val adapter = getAdapter()
val item = adapter.get(syncedFolder.section) ?: return false
val wasEnabled = item.isEnabled
itemFactory.updateAutoUploadFolderItem(item, syncedFolder)
persistence.saveOrUpdateAutoUploadFolder(item)

when {
!wasEnabled && item.isEnabled -> fragment.lifecycleScope.launch(folderStateDispatcher) {
fileUploadHelper.clearAutoUploadScanCache(item.id)
persistence.saveOrUpdateAutoUploadFolder(item)
}

wasEnabled && !item.isEnabled -> {
persistence.saveOrUpdateAutoUploadFolder(item)
removeUploadEntities(item.id)
}

else -> persistence.saveOrUpdateAutoUploadFolder(item)
}

adapter.notifyItemChanged(adapter.getSectionHeaderIndex(syncedFolder.section))
return true
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Nextcloud - Android Client
*
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
package com.owncloud.android.ui.fragment.autoUpload.data

import com.nextcloud.client.core.Clock
import com.nextcloud.client.jobs.upload.FileUploadWorker
import com.nextcloud.client.preferences.SubFolderRule
import com.owncloud.android.datamodel.MediaFolder
import com.owncloud.android.datamodel.MediaFolderType
import com.owncloud.android.datamodel.SyncedFolder
import com.owncloud.android.datamodel.SyncedFolderDisplayItem
import com.owncloud.android.files.services.NameCollisionPolicy
import com.owncloud.android.ui.dialog.parcel.SyncedFolderParcelable
import io.mockk.every
import io.mockk.mockk
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test

@Suppress("MagicNumber")
class AutoUploadDisplayItemFactoryTest {

private val enabledAt = 1_000_000_000_000L
private val now = 2_000_000_000_000L

private val clock: Clock = mockk()
private lateinit var factory: AutoUploadDisplayItemFactory

@Before
fun setup() {
every { clock.currentTime } returns now
factory = AutoUploadDisplayItemFactory(clock, 4, "/InstantUpload")
}

private fun createItem(enabled: Boolean, timestampMs: Long = enabledAt) = SyncedFolderDisplayItem(
1L,
"/storage/emulated/0/DCIM/Camera",
"/InstantUpload/Camera",
true,
false,
false,
false,
"account",
FileUploadWorker.LOCAL_BEHAVIOUR_FORGET,
NameCollisionPolicy.ASK_USER.serialize(),
enabled,
timestampMs,
"Camera",
MediaFolderType.CUSTOM,
false,
SubFolderRule.YEAR_MONTH,
false,
SyncedFolder.NOT_SCANNED_YET
)

@Test
fun `saving settings of an enabled folder keeps its enabled timestamp`() {
val item = createItem(enabled = true)
val parcelable = SyncedFolderParcelable(item, 0).apply { isWifiOnly = false }

factory.updateAutoUploadFolderItem(item, parcelable)

assertEquals(enabledAt, item.enabledTimestampMs)
}

@Test
fun `enabling a folder through its settings starts a new enabled period`() {
val item = createItem(enabled = false)
val parcelable = SyncedFolderParcelable(item, 0).apply { isEnabled = true }

factory.updateAutoUploadFolderItem(item, parcelable)

assertEquals(now, item.enabledTimestampMs)
}

@Test
fun `disabling a folder through its settings clears its enabled timestamp`() {
val item = createItem(enabled = true)
val parcelable = SyncedFolderParcelable(item, 0).apply { isEnabled = false }

factory.updateAutoUploadFolderItem(item, parcelable)

assertEquals(SyncedFolder.EMPTY_ENABLED_TIMESTAMP_MS, item.enabledTimestampMs)
}

@Test
fun `display item of a stored folder keeps the stored enabled timestamp`() {
val stored = createItem(enabled = true)
val mediaFolder = MediaFolder().apply {
folderName = "Camera"
absolutePath = stored.localPath
filePaths = emptyList()
type = MediaFolderType.IMAGE
}

val item = factory.createAutoUploadFolder(stored, mediaFolder)

assertEquals(enabledAt, item.enabledTimestampMs)
}
}