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 09b56a6..1d2bb01 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 @@ -86,6 +86,19 @@ class CollectionSyncer( /** Hrefs this run wrote, and which the sweep must therefore not remove. */ val touched = mutableSetOf() + /** + * Hrefs the write phase gave up on this run, and which must therefore + * not be downloaded. + * + * ⚠️ A resource deferred mid-create still has `href == null` locally, so + * [downloadPhase]'s `byHref` lookup cannot see it and its dirty-row guard + * cannot fire. Without this the same run downloads the server's copy, + * [apply] matches it by UID and overwrites the local edit — silently, + * with an empty `discardedEdits` and no failure. Deferring the write only + * helps if the read defers with it. + */ + val deferred = mutableSetOf() + /** * `uid -> parent uid`, resolved to row ids once every row exists. * @@ -424,8 +437,29 @@ class CollectionSyncer( // Either a previous run's PUT whose answer we never saw, // or an unrelated resource squatting the name. Only the // UID in the body can tell them apart. - val existing = remote.fetch(listOf(outcome.href)).getOrNull() - ?.resources?.firstOrNull() + // + // ⚠️ A fetch that *failed* is not evidence of a different + // task. This 412 is most often our own PUT from a run + // whose answer never arrived, so taking a fresh name on a + // timeout writes one UID to a second resource — forbidden + // by RFC 4791 §4.1, duplicated in every client, and a row + // whose href flips between the two on every later sync. + // Stay dirty and try again next run. A fetch that + // *succeeded* and returned nothing, or something else, is + // a name we may safely walk away from. + val fetched = remote.fetch(listOf(outcome.href)).getOrElse { + // `skip`, not `fail`: a transport failure is nobody's + // fault and the condition is re-evaluated next run. + // Counting it would spend a THRESHOLD budget that, + // for a resource with no href yet, nothing can ever + // refund — `uploadPhase` returns early once it is + // quarantined, so neither `stored` nor `purge` can + // reach it to clear the count again. + deferred += outcome.href.toString() + skip(local.key, "create verification failed: $it") + return + } + val existing = fetched.resources.firstOrNull() val sameTask = existing != null && uidOf(existing.iCalendar) == local.uid if (sameTask) { updateResource(local, outcome.href.toString(), body) @@ -540,6 +574,9 @@ class CollectionSyncer( remoteETags.forEach { (href, eTag) -> val local = byHref[href] when { + // The write phase gave up on it this run; it holds an edit + // that no local row can be matched to yet. + href in deferred -> Unit // Never seen it. local == null -> wanted += href // The write phase owns it this run. 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 4a54b5c..27c3eb9 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 @@ -281,7 +281,11 @@ class CollectionSyncerTest { @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") + .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", @@ -480,6 +484,54 @@ class CollectionSyncerTest { assertThat(store.rows.single().href).isEqualTo("http://server/dav/tasks/a.ics") } + @Test fun `a create whose verification fetch fails is not duplicated`() { + // Our own PUT from a run whose answer never arrived, and an edit the user + // made afterwards that only exists here. + remote.put("a.ics", vtodo("a", "Uploaded last time")) + store.rows += task(id = 1, uid = "a", title = "Edited after the lost PUT") + .copy(isDirty = true) + // Only the verification fetch fails. A blanket failure would take the + // download phase's multiget with it and pass for the wrong reason. + var failNext = true + remote.onFetch = { + if (failNext) { + failNext = false + Result.failure(IllegalStateException("timeout")) + } else { + null + } + } + + val report = sync() + + // Renaming on a failed fetch would put one UID in two resources, which + // RFC 4791 §4.1 forbids and which no later sync can clean up. + assertThat(remote.resources).hasSize(1) + assertThat(remote.log.count { it.startsWith("CREATE") }).isEqualTo(1) + // Deferring the write is only worth anything if the read defers too: the + // row has no href, so downloadPhase cannot match it and would otherwise + // overwrite the edit by UID with nothing in discardedEdits. + val row = store.rows.single() + assertThat(row.title).isEqualTo("Edited after the lost PUT") + assertThat(row.isDirty).isTrue() + assertThat(row.href).isNull() + // Deferred, not charged: nothing can refund a UID-keyed count once the + // resource is quarantined out of uploadPhase. + assertThat(quarantine).isEmpty() + assertThat(report.quarantined.single().reason).contains("verification failed") + assertThat(report.quarantined.single().failures).isEqualTo(0) + + // And the next run adopts the resource. + remote.onFetch = null + sync() + + assertThat(remote.resources).hasSize(1) + val adopted = store.rows.single() + assertThat(adopted.href).isEqualTo("http://server/dav/tasks/a.ics") + assertThat(adopted.title).isEqualTo("Edited after the lost PUT") + assertThat(quarantine).isEmpty() + } + // ------------------------------------------------------------ protection @Test fun `a read-only collection is never written to`() { diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt index 1b4478b..b197521 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt @@ -98,6 +98,15 @@ class FakeRemote(override val url: HttpUrl = "http://server/dav/tasks/".toHttpUr var onPut: ((HttpUrl) -> PutOutcome?)? = null var onDelete: ((HttpUrl) -> DeleteOutcome?)? = null + /** + * Returns non-null to pre-empt the default behaviour for that fetch. + * + * Unlike [fetchFailure], which breaks every fetch in the run, this can fail + * one and leave the rest working — the difference between "the server is + * down" and "this one request timed out". + */ + var onFetch: ((List) -> Result?)? = null + fun put(name: String, body: String, eTag: String? = "e-$name") { resources[url.newBuilder().addPathSegment(name).build().toString()] = Stored(eTag?.let { ETag(it, weak = false) }, body) @@ -138,6 +147,7 @@ class FakeRemote(override val url: HttpUrl = "http://server/dav/tasks/".toHttpUr override fun fetch(hrefs: List): Result { log += "FETCH ${hrefs.joinToString(",") { it.pathSegments.last() }}" + onFetch?.invoke(hrefs)?.let { return it } fetchFailure?.let { return Result.failure(IllegalStateException(it)) } val found = hrefs.mapNotNull { href -> resources[href.toString()]?.let { RemoteResource(href, it.eTag, it.body) }