fix: resolve infinite dirty-flag loop when local task has duplicate UID by mvanhorn · Pull Request #2486 · bitfireAT/davx5-ose

mvanhorn · 2026-06-13T08:35:18Z

Purpose

Fixes an infinite dirty-flag loop that prevents the "unsynchronized change" badge from clearing when a locally created task shares a UID with a task already on the server (issue #2226, root cause confirmed by @rfc2822).

Short description

Added findAllTasksWithSyncId(syncId) to DmfsRecurringTaskList that returns all main tasks matching a given _SYNC_ID by reusing the existing iterateTaskAndExceptions.
Overrode LocalTaskList.findByName to call findAllTasksWithSyncId instead of findTaskAndExceptions. When exactly one match is found, behavior is unchanged. When multiple tasks share the same _SYNC_ID, any task that is both dirty and has no eTag (never successfully uploaded) is assigned a fresh UUID so the sync algorithm can upload it as a new resource, while the already-synced task (with an eTag) is returned as before.
Added three instrumented tests for findAllTasksWithSyncId in DmfsRecurringTaskListTest.

Checklist

The PR has a proper title, description and label.
I have self-reviewed the PR.
I have added documentation to complex functions and functions that can be used by other modules.
I have added reasonable tests or consciously decided to not add tests.

Fixes #2226

CLAassistant · 2026-06-13T08:35:25Z

All committers have signed the CLA.

ArnyminerZ

Just some minor comments. Waiting for Copilot's review, and @rfc2822's approval

ArnyminerZ · 2026-06-16T09:57:44Z

+        // locally with a UID that already exists on the server: one entry is already synced (has
+        // an eTag) and another was never successfully uploaded (dirty, no eTag). Reassign the
+        // dirty/no-eTag duplicate a fresh UUID so it can be uploaded as a genuinely new resource.
+        val logger = Logger.getLogger(javaClass.name)


polish: When using loggers, use the same pattern like in the rest of the repo please.

Example:

davx5-ose/core/src/main/kotlin/at/bitfire/davdroid/servicedetection/DavResourceFinder.kt

Line 76 in ba69786

val log: Logger = Logger.getLogger(javaClass.name)

I mean, define it globally at class-level instead of in-line

ArnyminerZ · 2026-06-16T10:00:02Z

+     * @return list of all matching tasks (each with their exceptions); may be empty
+     */
+    fun findAllTasksWithSyncId(syncId: String): List<TaskAndExceptions> {
+        val result = mutableListOf<TaskAndExceptions>()


suggestion: Please use buildList with iteration as in #2509 since that is the new method of building lists, instead of creating mutable lists and appending.

ArnyminerZ · 2026-06-16T10:07:42Z

Also please @mvanhorn sign the CLA

Copilot

Pull request overview

This PR fixes an infinite “dirty/unsynchronized change” loop when the local task database contains multiple main tasks sharing the same _SYNC_ID (e.g., a locally created task used a UID already present on the server). It does so by enabling discovery of all tasks with a given sync ID and resolving the collision by reassigning _SYNC_ID for dirty, never-uploaded duplicates.

Changes:

Add findAllTasksWithSyncId(syncId) to DmfsRecurringTaskList to return all matching main tasks (with their exceptions).
Update LocalTaskList.findByName() to handle duplicate _SYNC_ID rows by reassigning _SYNC_ID for dirty/no-eTag duplicates so they can be uploaded as distinct resources.
Add instrumented tests covering findAllTasksWithSyncId() behavior for 0/1/multiple matches.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
synctools/src/main/kotlin/at/bitfire/synctools/storage/tasks/DmfsRecurringTaskList.kt	Adds API to fetch all main tasks matching a `_SYNC_ID` (with exceptions).
synctools/src/androidTest/kotlin/at/bitfire/synctools/storage/tasks/DmfsRecurringTaskListTest.kt	Adds instrumentation tests for the new multi-match lookup.
core/src/main/kotlin/at/bitfire/davdroid/resource/LocalTaskList.kt	Adjusts lookup-by-name to resolve duplicate `_SYNC_ID` collisions by reassigning new IDs to dirty/no-eTag duplicates.

+        // Return the first match that still has the original name (i.e. the synced one with an eTag).
+        val synced = matches.firstOrNull { task ->
+            val values = task.main.entityValues
+            val hasETag = values.getAsString(DmfsTasksContract.COLUMN_ETAG) != null
+            hasETag
+        } ?: matches.first()
+        return LocalTask(recurringTaskList, synced)


+            contentValuesOf(
+                Tasks.LIST_ID to taskList.id,
+                Tasks._SYNC_ID to syncId,
+                Tasks.TITLE to "Dirty Local Task",
+                Tasks.DTSTART to now + 3600000,
+                Tasks.TZ to timeZoneId


+        // There are multiple tasks with the same _SYNC_ID. This happens when a task was created
+        // locally with a UID that already exists on the server: one entry is already synced (has
+        // an eTag) and another was never successfully uploaded (dirty, no eTag). Reassign the
+        // dirty/no-eTag duplicate a fresh UUID so it can be uploaded as a genuinely new resource.
+        val logger = Logger.getLogger(javaClass.name)


rfc2822

Blocking this for now because this change affects the core sync algorithm and could have a lot of unknown side effects.

We should probably have a look later and properly understand the problem and the solution.

mvanhorn · 2026-07-09T17:53:53Z

recheck

mvanhorn · 2026-07-09T17:56:03Z

Completely understand the caution - this touches the core sync path and deserves scrutiny. If it helps de-risk: I can add focused regression tests around the duplicate-UID case (duplicate-UID task stays clean after a round-trip, single-UID behavior unchanged), or split the loop-breaking guard from the rest so it lands separately. Happy to wait for your call either way. CLA signed.

When a task is created locally with a UID that already exists on the server, DmfsRecurringTaskList.findTaskAndExceptions returns only the first match causing an endless dirty-flag loop (HTTP 412 ignored, server copy never re-downloaded, _dirty=1 forever). Add findAllTasksWithSyncId() to return all main tasks with a given _SYNC_ID. Override LocalTaskList.findByName() to use it: on a duplicate, any task that is dirty and has no eTag is assigned a fresh UUID so it can be uploaded as a genuinely new resource, while the already-synced copy is returned to the sync algorithm as before. Fixes bitfireAT#2226

Address review feedback: - findByName now re-queries findAllTasksWithSyncId after reassigning the dirty/no-eTag duplicate's _SYNC_ID, and selects from the remaining rows (preferring an eTag, else null), so it never returns a LocalTask backed by a _SYNC_ID that no longer exists in the provider. - Move the logger to a class-level property. - Build the result of findAllTasksWithSyncId with buildList. - Set _DIRTY on the dirty task in the recurring-task-list test. - Add a LocalTaskListTest regression test for the duplicate _SYNC_ID case.

mvanhorn · 2026-07-17T04:12:59Z

Fixed the blocking one first: findByName now re-queries findAllTasksWithSyncId after the reassignment loop and picks from the rows that still match the name (preferring an eTag, null if none remain), so it no longer returns a LocalTask whose _SYNC_ID was just reassigned out from under it. Alongside that (ac74f7c): moved the logger to a class-level property, switched findAllTasksWithSyncId to buildList per #2509, set _DIRTY on the dirty task in the recurring-task-list test, and added a LocalTaskListTest for the duplicate _SYNC_ID case (two tasks sharing a sync id, one eTag'd and one dirty/no-eTag, asserting the dirty one is reassigned and the returned task is the surviving eTag'd one). I could not run the Gradle/Robolectric suite locally since AGP would not resolve offline in my environment, so I am leaning on CI for the full run; the new test passed standalone ktlint.

rfc2822 · 2026-07-17T07:07:07Z

Thanks for the update. We unfortunately have to fix a few other things before we can have a look.

mvanhorn · 2026-07-25T17:02:36Z

Following up on this one. The changes you requested went in at ac74f7c and all five review threads are resolved, so I think this is waiting on a re-review rather than on me.

I re-checked the branch today: the requested changes are all present and the diff is still scoped to the four expected files. I was not able to run the Gradle build or tests locally in my sandbox, so if CI is happy that is the stronger signal here.

Whenever you have a moment.

Resolved conflicts, preserving this PR's change on top of upstream's restructuring. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

mvanhorn · 2026-08-02T22:22:02Z

Merged current main into the branch (99d8a0d). It had drifted 137 commits and picked up conflicts in LocalTaskList.kt and the two DmfsRecurringTaskList files. Those are resolved and the PR is mergeable again.

Nothing under review changed, it's still the same fix from ac74f7c plus the merge.

ArnyminerZ added the pr-bugfix Fixes something that isn't working (only used for PRs) label Jun 16, 2026

ArnyminerZ requested a review from Copilot June 16, 2026 10:04

Copilot started reviewing on behalf of ArnyminerZ June 16, 2026 10:05 View session

ArnyminerZ requested changes Jun 16, 2026

View reviewed changes

Copilot AI reviewed Jun 16, 2026

View reviewed changes

rfc2822 requested changes Jun 16, 2026

View reviewed changes

rfc2822 added the needs info Further information needed to continue label Jun 16, 2026

rfc2822 force-pushed the main branch from 6df847a to 9601d12 Compare July 9, 2026 12:00

mvanhorn force-pushed the fix/2226-duplicate-uid-task-dirty-loop branch from c14974b to 78233d6 Compare July 10, 2026 16:10

Merge upstream/main into fix/2226-duplicate-uid-task-dirty-loop

99d8a0d

Resolved conflicts, preserving this PR's change on top of upstream's restructuring. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

Uh oh!

Uh oh!

Conversation

mvanhorn commented Jun 13, 2026

Purpose

Short description

Checklist

Uh oh!

CLAassistant commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArnyminerZ left a comment

Choose a reason for hiding this comment

Uh oh!

ArnyminerZ Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

ArnyminerZ Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

ArnyminerZ commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

rfc2822 left a comment

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Jul 9, 2026

Uh oh!

mvanhorn commented Jul 9, 2026

Uh oh!

mvanhorn commented Jul 17, 2026

Uh oh!

rfc2822 commented Jul 17, 2026

Uh oh!

mvanhorn commented Jul 25, 2026

Uh oh!

mvanhorn commented Aug 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Jun 13, 2026 •

edited

Loading