diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt index 0c10e51..09b56a6 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt @@ -503,7 +503,22 @@ class CollectionSyncer( private fun stored(local: LocalResource, href: HttpUrl, eTag: String?) { val key = href.toString() - store.markSynced(local.rows.map { it.id }, key, eTag) + // ⚠️ Only the rows that were in the body. `serialize` writes + // `local.live`, so a tombstoned override was left out — the PUT *is* + // its deletion, and the row has no job left. Marking it synced + // instead strands `is_deleted = 1` behind a cleared `is_dirty`, + // where no phase can reach it: the ETag we just recorded matches the + // server's, so nothing re-downloads it and `apply`'s stale-override + // sweep never runs. The unique index on + // (list_id, uid, recurrence_id) then makes re-adding that occurrence + // throw for good. + // + // Overrides only, for now. A tombstoned *master* still reaches here + // because `markDeleted` does not tombstone a series' overrides, and + // deleting it would cascade the live ones away via `master_id`. + val (buried, kept) = local.rows.partition { it.isDeleted && it.recurrenceId != null } + if (buried.isNotEmpty()) store.deleteAll(buried.map { it.id }) + store.markSynced(kept.map { it.id }, key, eTag) // Both, because a resource that failed as a create was counted under // its UID and is now counted under its href. Clearing one would leak // the other into the store forever. diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt index 5b0a7ad..b3940c5 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt @@ -24,7 +24,12 @@ interface SyncStore { fun deleteAll(taskIds: List) - /** Records href and ETag on a whole resource, clearing `is_dirty`. */ + /** + * Records href and ETag on a resource's rows, clearing `is_dirty`. + * + * The caller excludes any row it left out of the body — that row is deleted, + * not marked synced. + */ fun markSynced(taskIds: List, href: String?, eTag: String?) fun setParent(taskId: Long, parentId: Long?) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt index fb75cb3..9057a9f 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt @@ -160,10 +160,13 @@ interface TaskDao { /** * Records where a resource lives and which version we hold. * - * Applied to every row of a resource at once, because a master and its + * Applied to a resource's rows at once, because a master and its * `RECURRENCE-ID` overrides share one href and one ETag — they are one file - * on the server. `is_dirty` is cleared **explicitly** rather than left to a - * default: a downstream write that leaves the flag set uploads what was just + * on the server. A tombstoned override is excluded by the caller: it was + * left out of the body, so the caller deletes the row instead. + * + * `is_dirty` is cleared **explicitly** rather than left to a default: a + * downstream write that leaves the flag set uploads what was just * downloaded, which is how a sync loop starts. */ @Query("UPDATE tasks SET href = :href, etag = :etag, is_dirty = 0 WHERE id IN (:taskIds)") diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt index 3dd9da8..4a54b5c 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt @@ -278,6 +278,66 @@ class CollectionSyncerTest { assertThat(store.rows).isEmpty() } + @Test fun `a deleted occurrence is dropped from the store once the PUT lands`() { + remote.put("one.ics", vtodo("a", "Weekly")) + store.rows += task(id = 1, uid = "a", title = "Weekly") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-one.ics", rrule = "FREQ=WEEKLY") + store.rows += task(id = 2, uid = "a", title = "Weekly") + .copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + masterId = 1, + recurrenceId = NOW, + isDeleted = true, + isDirty = true, + ) + + sync() + + // The occurrence left the body, so the PUT is its deletion. Marking the + // tombstone synced instead strands is_deleted = 1 with is_dirty = 0, + // which no phase can reach and which the unique index on + // (list_id, uid, recurrence_id) turns into a permanent failure to re-add. + assertThat(remote.log.any { it.startsWith("UPDATE one.ics") }).isTrue() + assertThat(remote.log.none { it.startsWith("DELETE") }).isTrue() + assertThat(store.rows.none { it.isDeleted }).isTrue() + assertThat(store.rows.map { it.id }).containsExactly(1L) + // FakeStore models neither the unique index on (list_id, uid, + // recurrence_id) nor the master_id cascade, so the permanent-failure-to- + // re-add consequence is reasoned about, not asserted here. + assertThat(store.rows.single().isDirty).isFalse() + } + + @Test fun `a tombstoned master with a live override keeps its row`() { + remote.put("one.ics", vtodo("a", "Series")) + store.rows += task(id = 1, uid = "a", title = "Series").copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + isDeleted = true, + isDirty = true, + ) + store.rows += task(id = 2, uid = "a", title = "Series").copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + masterId = 1, + recurrenceId = NOW, + ) + + sync() + + // Pins today's behaviour, which is wrong and is not this fix's to correct: + // markDeleted tombstones only the master, so isDeleted (rows.all) is false, + // the resource goes to uploadPhase rather than deletePhase, and no DELETE + // is ever sent. Dropping the master row here instead would cascade the live + // override away via master_id and leave the server holding a resource + // nothing here has rows for — so only overrides are dropped for now. + assertThat(remote.log.none { it.startsWith("DELETE") }).isTrue() + assertThat(store.rows.map { it.id }).containsExactly(1L, 2L) + val master = store.rows.single { it.id == 1L } + assertThat(master.isDeleted).isTrue() + assertThat(master.isDirty).isFalse() + } + @Test fun `the sweep does not remove what this run just created`() { store.rows += task(id = 1, uid = "a", title = "Fresh").copy(isDirty = true)