From 7850bfc2024011407cd7488b4e84f1162ed9ec53 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 7 Sep 2026 22:26:36 +0200 Subject: [PATCH] sync: check the reply's origin instead of keying on it Three corrections to 7fac4bd, all found in review. Folding the origin into the match key was wrong: the REPORT body carries only our paths, so a reply's href resolves against the collection's post-redirect location while our stored hrefs still hold the origin we had before it. Every resource of a redirected collection would have landed in both buckets -- the failure the commit was meant to remove. The key is the decoded path; the origin is checked separately, and a reply may come from the host we asked or from the collection's own. Two request hrefs can decode to one identity, and keeping the last of them silently reported the other as missing and applied one row's body to the other. Only identities naming exactly one href are matched loosely now; where we cannot tell two spellings apart, the exact one is the only honest answer. And the syncer's amnesty was excused by any stray at all, though servers volunteer siblings as a matter of course -- so a genuinely omitted resource would never have been counted and would be re-requested for ever. Only a stray naming the same path can be this href under a spelling we failed to read. --- .../agendula/data/sync/CollectionSyncer.kt | 15 ++++- .../data/sync/CollectionSyncerTest.kt | 28 ++++++++- .../caldav/CalendarCollection.kt | 49 +++++++++++++--- .../caldav/CalendarCollectionTest.kt | 58 +++++++++++++++++++ 4 files changed, 135 insertions(+), 15 deletions(-) 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 d8e538b..a06cf32 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 @@ -662,11 +662,20 @@ class CollectionSyncer( // no refund: at THRESHOLD the href is stripped before the fetch, // so `apply` can never run to clear it. A wasteful re-request is // recoverable; a permanent silent drop of a good task is not. - val unmatched = fetched.unsolicited.isNotEmpty() + // ⚠️ Paired by path, not excused wholesale. Real servers answer + // with hrefs nobody asked for as a matter of course — a sibling, + // something from another collection — so "any stray at all" + // would mean a genuinely omitted resource is never counted and + // is re-requested for ever, which is the loop the count exists + // to break. Only a stray naming the *same path* is plausibly + // this href under a spelling we failed to recognise, and that is + // the one case where counting could permanently drop a resource + // that is really there. + val strayPaths = fetched.unsolicited.map { it.pathSegments }.toSet() fetched.missing.forEach { val href = it.toString() - if (unmatched) { - skip(href, "listed but not returned, among hrefs we did not ask for") + if (it.pathSegments in strayPaths) { + skip(href, "answered under a spelling we did not recognise") } else { fail(href, "listed but not returned") } 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 91dda04..f5a6067 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 @@ -524,9 +524,9 @@ class CollectionSyncerTest { resources = emptyList(), missing = hrefs, failed = emptyList(), - // The shape an href we failed to recognise takes: one - // resource in both buckets. - unsolicited = listOf("http://server/dav/tasks/one.ics".toHttpUrl()), + // The shape an href we failed to recognise takes: the same + // path, in both buckets. + unsolicited = listOf("http://elsewhere/dav/tasks/one.ics".toHttpUrl()), ), ) } @@ -539,6 +539,28 @@ class CollectionSyncerTest { assertThat(report.quarantined.single().failures).isEqualTo(0) } + @Test fun `a stray for another path does not excuse a missing one`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = hrefs, + failed = emptyList(), + // Servers volunteer siblings routinely. Excusing every + // missing href on that basis would mean nothing is ever + // counted, and the loop never breaks. + unsolicited = listOf("http://server/dav/tasks/somebody-else.ics".toHttpUrl()), + ), + ) + } + + val report = sync() + + assertThat(quarantine.values.single()).isEqualTo(1) + assertThat(report.quarantined.single().failures).isEqualTo(1) + } + @Test fun `a missing href is still counted when nothing was unmatched`() { remote.put("one.ics", vtodo("a", "Fine")) remote.onFetch = { hrefs -> diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index 501c8b8..ba0ae42 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -190,13 +190,31 @@ class CalendarCollection( * on this side can refund the count. * * `pathSegments` is decoded, so both spellings agree. Keeping it a list, not - * a joined string, stops `/a%2Fb` colliding with `/a/b`. Scheme, host and - * port ride along because a path-only match would accept a body from another - * host entirely. The trailing slash still distinguishes, exactly as - * `UrlUtils.equals` refuses to normalise it. + * a joined string, stops `/a%2Fb` colliding with `/a/b`. The trailing slash + * still distinguishes, exactly as `UrlUtils.equals` refuses to normalise it. + * + * ⚠️ The origin is deliberately *not* part of this key, and is checked + * separately by [answersFor]. The two sides do not share one by + * construction: the REPORT body carries only our paths, so the reply's href + * resolves against the collection's *post-redirect* location, while our + * stored hrefs still carry the origin we had before the redirect. Keying on + * the origin would put every resource of a redirected collection into both + * `unsolicited` and `missing`. */ - private fun identityOf(url: HttpUrl): List = - listOf(url.scheme, url.host, url.port.toString()) + url.pathSegments + private fun identityOf(url: HttpUrl): List = url.pathSegments + + /** + * Whether a reply about [answered] can be the resource we asked for at + * [asked]. + * + * Same host as the href we asked for, or as the collection we asked it of — + * the latter covering a redirect we followed. Anything else is a body from + * somewhere we never spoke to, and applying it would write one host's + * content into another host's row. + */ + private fun answersFor(answered: HttpUrl, asked: HttpUrl, collection: HttpUrl): Boolean = + answered.host.equals(asked.host, ignoreCase = true) || + answered.host.equals(collection.host, ignoreCase = true) /** [batchSize] is a seam for tests; production always uses [MULTIGET_BATCH]. */ internal fun fetch(hrefs: List, batchSize: Int): Result = @@ -207,10 +225,23 @@ class CalendarCollection( val seen = mutableSetOf() hrefs.chunked(batchSize).forEach { batch -> - val wanted = batch.associateBy { identityOf(it) } - dav.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> + val byExactPath = batch.associateBy { it.encodedPath } + // ⚠️ Only identities that name exactly one request href. Two + // spellings of one path — `/my%40dav.ics` and `/my@dav.ics` — + // decode alike but are separate rows here, and silently keeping + // the last would report the other as missing and eventually + // quarantine it. Where we cannot tell them apart, the exact + // spelling is the only honest match. + val byPath = batch.groupBy { identityOf(it) } + .filterValues { it.size == 1 } + .mapValues { (_, matches) -> matches.single() } + val calendar = dav + calendar.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> if (relation == Response.HrefRelation.SELF) return@multiget - val asked = wanted[identityOf(response.href)] + val candidate = byExactPath[response.href.encodedPath] + ?: byPath[identityOf(response.href)] + val asked = candidate + ?.takeIf { answersFor(response.href, it, calendar.location) } if (asked == null) { unsolicited += response.href return@multiget diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt index 04f46ef..583afc6 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -2,6 +2,7 @@ package de.jeanlucmakiola.caldav import com.google.common.truth.Truth.assertThat import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer @@ -132,6 +133,34 @@ END:VCALENDAR assertThat(result.resources.single().href).isEqualTo(asked) } + @Test fun `an href stored before a redirect still matches the reply`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + // ⚠️ The REPORT body carries only paths, so the reply resolves against + // the collection we asked — while a stored href can still carry the + // origin we held before a redirect moved us. Keying on the origin would + // put every resource of a redirected collection into both buckets. + val stored = "https://old.example.com/dav/tasks/one.ics".toHttpUrl() + val result = collection.fetch(listOf(stored)).getOrThrow() + + assertThat(result.resources.map { it.href }).containsExactly(stored) + assertThat(result.missing).isEmpty() + assertThat(result.unsolicited).isEmpty() + } + @Test fun `the same path on another host is not our resource`() { server.enqueue( multistatus( @@ -157,6 +186,35 @@ END:VCALENDAR assertThat(result.missing).hasSize(1) } + @Test fun `an ambiguous spelling matches nothing rather than the wrong row`() { + // A third spelling: matches neither request exactly, decodes like both. + server.enqueue( + multistatus( + """ + + /dav/tasks/%6Dy@dav.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val plain = href("my@dav.ics") + val encoded = server.url("/dav/tasks/my%40dav.ics") + val result = collection.fetch(listOf(plain, encoded)).getOrThrow() + + // ⚠️ Two rows decode alike, so no answer can say which it means. Guessing + // applies one row's body to the other; both are reported instead, and the + // syncer will not spend a quarantine count on a stray of the same path. + assertThat(result.resources).isEmpty() + assertThat(result.unsolicited).hasSize(1) + assertThat(result.missing).containsExactly(plain, encoded) + } + @Test fun `a trailing slash is still a different resource`() { server.enqueue( multistatus(