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 a06cf32..8ef5c03 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 @@ -235,8 +235,19 @@ class CollectionSyncer( if (!removalsArePlausible(page.removed)) return false - applyRemovals(page.removed) + // ⚠️ Bodies before removals, unlike the phase order above. A + // delete-and-recreate arrives as `removed: one.ics` + + // `changed: two.ics` in one page, and purging first destroys the + // row the download would have re-pointed — it comes back with + // `sortOrder = 0`, no colour and no parent, and `deletedLocally` + // is reported for a task nobody deleted. Downloading first lets + // `apply` move the row, after which `one.ics` names nothing and + // the removal is the no-op it should be. Nothing is lost the + // other way: RFC 6578 reports each resource once, so a page + // cannot both change and remove the same href, and a href this + // run wrote is already guarded by `touched`. downloadChanged(page.changed) + applyRemovals(page.removed) // ⚠️ After the bodies, and only if they actually landed. A // network drop mid-multiget would otherwise commit a cursor past @@ -626,7 +637,15 @@ class CollectionSyncer( } } wanted += refetch - wanted.removeAll { isQuarantined(it) } + // ⚠️ Not on a full reconciliation. The download side has no refund: + // at THRESHOLD the href is stripped before the fetch, so `apply` — + // and with it `succeeded` — can never run to clear the count again, + // and a per-object ACL wrong for an afternoon hides that task for the + // life of the install. The upload side needs no equivalent, because + // `stored` and `purge` refund it there. One probe per periodic + // reconciliation is the expiry: it costs a single resource on a slow + // cadence, and a resource that answers this time clears its count. + if (!fullReconciliationDue) wanted.removeAll { isQuarantined(it) } fetchAndApply(wanted) } @@ -775,6 +794,16 @@ class CollectionSyncer( // Keep the hoisted index honest for the resources still to come. index.byUid[uid] = written index.byHref[href] = written + // ⚠️ Including the href these rows just left. A delete-and-recreate + // keeps the UID and changes the filename, so the old entry still + // names rows that now live here — and a later resource in the same + // batch reading that entry as `displaced` deletes them. The + // in-memory twin of the sweep's pre-download snapshot. + val moved = written.map { it.id }.toSet() + existing.values.mapNotNull { it.href }.toSet().minus(href).forEach { vacated -> + val left = index.byHref[vacated].orEmpty().filterNot { it.id in moved } + if (left.isEmpty()) index.byHref.remove(vacated) else index.byHref[vacated] = left + } if (displaced.isNotEmpty()) { val ids = displaced.map { it.id } 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 f5a6067..a6aefcb 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 @@ -25,7 +25,8 @@ class CollectionSyncerTest { href = "http://server/dav/tasks/", ) - private fun sync() = CollectionSyncer(store) { NOW }.sync(list, remote, quarantine) + private fun sync(fullDue: Boolean = true) = + CollectionSyncer(store) { NOW }.sync(list, remote, quarantine, fullDue) // -------------------------------------------------------------- download @@ -588,11 +589,12 @@ class CollectionSyncerTest { repeat(QuarantineStore.THRESHOLD) { sync() } remote.log.clear() - sync() + sync(fullDue = false) // The whole point. It is in the listing, so the sweep leaves it alone, // and it never reaches apply, so without a count it is re-requested on - // every sync for ever. + // every sync for ever. The daily reconciliation still probes it once — + // otherwise nothing on this side could ever clear the count. assertThat(remote.log).containsExactly("LIST") } @@ -845,6 +847,36 @@ class CollectionSyncerTest { assertThat(report.failure).contains("revoked") } + @Test fun `a rename into a name another task took keeps both rows`() { + store.rows += task(id = 1, uid = "a", title = "Moved") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-old") + // Fetched in insertion order, so the rename is applied first and leaves + // the index entry for `one.ics` behind. + remote.put("two.ics", vtodo("a", "Moved")) + remote.put("one.ics", vtodo("b", "A different task at the old name")) + + sync() + + // ⚠️ `one.ics` now holds somebody else, and row 1 no longer lives there + // — reading it out of a stale index entry deletes the task the rename + // just repaired. + assertThat(store.rows.map { it.uid }).containsExactly("a", "b") + } + + @Test fun `a full reconciliation probes a resource the download gave up on`() { + remote.put("one.ics", vtodo("a", "Buy milk")) + quarantine[QuarantineStore.key(list.id, "http://server/dav/tasks/one.ics")] = + QuarantineStore.THRESHOLD + + sync() + + // ⚠️ The download side has no refund — at THRESHOLD the href is stripped + // before the fetch, so nothing can ever clear the count again. One probe + // per periodic reconciliation is the expiry. + assertThat(store.rows.single().uid).isEqualTo("a") + assertThat(quarantine).isEmpty() + } + // ----------------------------------------------------------------- setup /** A multiget in which `two.ics` alone comes back refused. */ diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt index dad3709..1ab9e05 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt @@ -165,6 +165,35 @@ class IncrementalSyncTest { assertThat(store.rows).hasSize(20) } + @Test fun `a delete-and-recreate in one page keeps the row's local columns`() { + store.rows += task(id = 1, uid = "a", title = "Old") + .copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-old", + sortOrder = 5, + color = 0x00FF00, + ) + remote.put("two.ics", vtodo("a", "Same task, new file"), eTag = "e-new") + remote.changePages += page( + changed = listOf("two.ics"), + removed = listOf("one.ics"), + token = "urn:x:2", + eTag = "e-new", + ) + + val report = sync() + + // ⚠️ Same UID, new filename. Purging `one.ics` before `two.ics` is + // fetched re-inserts the task with sortOrder 0 and no colour, and + // reports a deletion nobody asked for. + val row = store.rows.single() + assertThat(row.id).isEqualTo(1) + assertThat(row.href).isEqualTo("http://server/dav/tasks/two.ics") + assertThat(row.sortOrder).isEqualTo(5) + assertThat(row.color).isEqualTo(0x00FF00) + assertThat(report.deletedLocally).isEqualTo(0) + } + @Test fun `an un-uploaded local edit is never downloaded over`() { remote.readOnly = true store.rows += task(id = 1, uid = "a", title = "Mine, unsent")