makiolaj/agendula
1
0
Fork 0
Code Issues Pull Requests Actions Releases 8 Activity

sync: report a refused resource instead of losing it

Browse Source 
A multiget answer that was neither a body nor an omission fell through
every bucket: seen, so not missing; no body, so not a resource. Nothing
counted it, the sweep left it alone because the listing still named it,
and downloadPhase asked for it again on every sync for ever -- the loop
quarantine exists to break.

FetchResult now carries `failed`, with the status the server actually
gave. Kept apart from `missing` because "refused" and "not mentioned"
are different facts: 404 and 410 are skipped, since the next listing
drops the href and the sweep purges the row, and counting them would
quarantine the resource out of that very sweep; 5xx and a bodiless
success are skipped as the server's own trouble; any other 4xx is
counted, being a judgement about this resource that repeats forever.

A per-property refusal -- a propstat with 403 around calendar-data --
is the usual shape of "you may not read this one object", and
Response.properties drops non-2xx propstats, so the code is read back
out of them rather than reported as a blank.

The two write-side guards had to learn this too. Both read a fetch that
came back Result.success as an answer, so a refusal now looked like
"somebody else's resource" (a second resource under one UID) and like
"the server has no validator" (an unconditional PUT over a concurrent
edit) -- the two things those guards were added to prevent.
This commit is contained in:
makiolaj
2026-09-07 21:26:17 +02:00
parent 83b4fb8e6e
commit d5ce24b673
6 changed files with 317 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt
+43
View File
@@ -464,6 +464,17 @@ class CollectionSyncer(
skip(local.key, "create verification failed: $it")
return
}
// A refusal is not an answer either. It arrives as a
// *successful* multiget carrying a per-resource error, so
// without this the empty `resources` reads as "somebody
// else's" and we take a fresh name — the duplicate UID
// this branch exists to avoid.
if (fetched.failed.isNotEmpty()) {
deferred += outcome.href.toString()
val code = fetched.failed.first().code
skip(local.key, "create verification refused: $code")
return
}
val existing = fetched.resources.firstOrNull()
val sameTask = existing != null && uidOf(existing.iCalendar) == local.uid
if (sameTask) {
@@ -514,6 +525,12 @@ class CollectionSyncer(
skip(local.key, "update validator fetch failed: $it")
return
}
// Same distinction: refused is not "has none to offer".
if (fetched.failed.isNotEmpty()) {
deferred += href
skip(local.key, "validator fetch refused: ${fetched.failed.first().code}")
return
}
eTag = fetched.resources.firstOrNull()
?.eTag?.takeIf { it.usable }?.value
if (eTag == null) {
@@ -639,6 +656,29 @@ class CollectionSyncer(
// on every sync for ever, which is the loop quarantine exists to
// break.
fetched.missing.forEach { fail(it.toString(), "listed but not returned") }
fetched.failed.forEach { failure ->
// ⚠️ Quarantining here also silences the upload phase and the
// sweep, because a row's quarantine key *is* its href. So the
// verdict has to distinguish what the server actually said.
val href = failure.href.toString()
when (failure.code) {
// Gone. The next listing omits it and the sweep purges the
// row — counting it would quarantine the resource out of
// the very sweep that would have cleaned it up.
HTTP_NOT_FOUND, HTTP_GONE ->
skip(href, "the server no longer has this resource: ${failure.code}")
// The server failing on its own stored object, or on the
// batch. A multiget carries no body of ours, so there is
// nothing here for a server to reject permanently.
0, in 500..599 ->
skip(href, "the server could not return this resource: ${failure.code}")
// A judgement about this resource — a per-object ACL, or a
// share hiding one object. Deterministic, and nothing else
// breaks the loop.
else ->
fail(href, "the server refused this resource: ${failure.code}")
}
}
}
}
@@ -940,6 +980,9 @@ class CollectionSyncer(
}
private companion object {
const val HTTP_NOT_FOUND = 404
const val HTTP_GONE = 410
/** RFC 5545 `CLASS:CONFIDENTIAL`, as `tasks.classification` stores it. */
const val CLASS_CONFIDENTIAL = 2
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt
+129
View File
@@ -3,7 +3,12 @@ package de.jeanlucmakiola.agendula.data.sync
import com.google.common.truth.Truth.assertThat
import de.jeanlucmakiola.agendula.data.tasks.room.TaskEntity
import de.jeanlucmakiola.agendula.data.tasks.room.TaskListEntity
import de.jeanlucmakiola.caldav.FetchFailure
import de.jeanlucmakiola.caldav.FetchResult
import de.jeanlucmakiola.caldav.PutOutcome
import de.jeanlucmakiola.caldav.RemoteResource
import okhttp3.HttpUrl
import okhttp3.HttpUrl.Companion.toHttpUrl
import org.junit.jupiter.api.Test
import kotlin.time.Instant
@@ -471,6 +476,60 @@ class CollectionSyncerTest {
assertThat(report.deletedLocally).isEqualTo(1)
}
@Test fun `a resource the server refuses is counted, and the batch survives`() {
remote.put("one.ics", vtodo("a", "Fine"))
remote.put("two.ics", vtodo("b", "Refused"))
remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 403) }
val report = sync()
// A refusal on one member says nothing about the others.
assertThat(store.rows.map { it.uid }).containsExactly("a")
assertThat(quarantine.values.single()).isEqualTo(1)
assertThat(report.quarantined.single().failures).isEqualTo(1)
assertThat(report.quarantined.single().reason).contains("403")
}
@Test fun `a resource the server says is gone is not counted`() {
remote.put("one.ics", vtodo("a", "Fine"))
remote.put("two.ics", vtodo("b", "Gone"))
remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 404) }
val report = sync()
// Counting it would quarantine the resource out of the sweep that is
// about to clean it up once the listing drops it.
assertThat(quarantine).isEmpty()
assertThat(report.quarantined.single().failures).isEqualTo(0)
}
@Test fun `a resource the server fails on is not counted`() {
remote.put("one.ics", vtodo("a", "Fine"))
remote.put("two.ics", vtodo("b", "Broken"))
remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 500) }
val report = sync()
// A multiget carries no body of ours, so there is nothing here a server
// could be permanently right to reject.
assertThat(quarantine).isEmpty()
assertThat(report.quarantined.single().failures).isEqualTo(0)
}
@Test fun `a resource the server keeps refusing stops being requested`() {
remote.put("two.ics", vtodo("b", "Refused"))
remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 403) }
repeat(QuarantineStore.THRESHOLD) { sync() }
remote.log.clear()
sync()
// 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.
assertThat(remote.log).containsExactly("LIST")
}
@Test fun `a create the server keeps refusing is eventually quarantined`() {
store.rows += task(id = 1, uid = "a", title = "Never acceptable").copy(isDirty = true)
remote.onPut = { PutOutcome.Rejected(415, "Unsupported Media Type") }
@@ -609,6 +668,55 @@ class CollectionSyncerTest {
assertThat(row.href).isNull()
}
@Test fun `a refused verification fetch does not rename the create`() {
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)
// A per-resource refusal is a *successful* multiget. Without inspecting
// `failed`, the empty resources list reads as somebody else's resource.
remote.onFetch = { hrefs ->
Result.success(
FetchResult(
resources = emptyList(),
missing = emptyList(),
failed = hrefs.map { FetchFailure(it, 403) },
unsolicited = emptyList(),
),
)
}
sync()
assertThat(remote.resources).hasSize(1)
assertThat(remote.log.count { it.startsWith("CREATE") }).isEqualTo(1)
val row = store.rows.single()
assertThat(row.title).isEqualTo("Edited after the lost PUT")
assertThat(row.isDirty).isTrue()
}
@Test fun `a refused validator fetch does not write unconditionally`() {
remote.put("one.ics", vtodo("a", "Server"))
store.rows += task(id = 1, uid = "a", title = "Mine")
.copy(href = "http://server/dav/tasks/one.ics", isDirty = true)
remote.onFetch = { hrefs ->
Result.success(
FetchResult(
resources = emptyList(),
missing = emptyList(),
failed = hrefs.map { FetchFailure(it, 500) },
unsolicited = emptyList(),
),
)
}
val report = sync()
assertThat(remote.log.none { it.startsWith("UPDATE") }).isTrue()
assertThat(report.unconditionalWrites).isEqualTo(0)
assertThat(remote.resources.values.single().body).contains("SUMMARY:Server")
assertThat(store.rows.single().isDirty).isTrue()
}
// ------------------------------------------------------------ protection
@Test fun `a read-only collection is never written to`() {
@@ -673,6 +781,27 @@ class CollectionSyncerTest {
// ----------------------------------------------------------------- setup
/** A multiget in which `two.ics` alone comes back refused. */
private fun refuseTwoIcs(hrefs: List<HttpUrl>, code: Int): Result<FetchResult> {
val refused = hrefs.filter { it.pathSegments.last() == "two.ics" }
val rest = hrefs - refused.toSet()
val served = remote.resources.filterKeys { key -> rest.any { it.toString() == key } }
return Result.success(
FetchResult(
resources = served.map { (href, stored) ->
RemoteResource(
href = href.toHttpUrl(),
eTag = stored.eTag,
iCalendar = stored.body,
)
},
missing = emptyList(),
failed = refused.map { FetchFailure(it, code) },
unsolicited = emptyList(),
),
)
}
private fun task(
id: Long,
uid: String,
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt
+1
View File
@@ -156,6 +156,7 @@ class FakeRemote(override val url: HttpUrl = "http://server/dav/tasks/".toHttpUr
FetchResult(
resources = found,
missing = hrefs.filterNot { it.toString() in resources },
failed = emptyList(),
unsolicited = emptyList(),
),
)
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt
+26 -2
View File
@@ -183,6 +183,7 @@ class CalendarCollection(
runCatching {
val resources = mutableListOf<RemoteResource>()
val unsolicited = mutableListOf<HttpUrl>()
val failed = mutableListOf<FetchFailure>()
val seen = mutableSetOf<HttpUrl>()
hrefs.chunked(batchSize).forEach { batch ->
@@ -194,9 +195,26 @@ class CalendarCollection(
unsolicited += response.href
return@multiget
}
// Before the checks below, and deliberately: the server did
// mention this href, so whatever it said, the href is not
// one the server omitted.
seen += asked
if (!response.isSuccess()) return@multiget
val body = response[CalendarData::class.java]?.iCalendar ?: return@multiget
if (!response.isSuccess()) {
failed += FetchFailure(asked, response.status?.code ?: 0)
return@multiget
}
val body = response[CalendarData::class.java]?.iCalendar
if (body == null) {
// Success at the response level, no body. The usual shape
// is a per-property refusal — a `propstat` carrying 403
// around `calendar-data` — and `Response.properties` drops
// non-2xx propstats silently, so the verdict has to be
// read back out of them or a deterministic refusal
// arrives looking like a transient blank.
val refusal = response.propstat.firstOrNull { !it.isSuccess() }
failed += FetchFailure(asked, refusal?.status?.code ?: 0)
return@multiget
}
resources += RemoteResource(
href = asked,
// The tag from *this* response, paired with *this* body.
@@ -209,6 +227,12 @@ class CalendarCollection(
FetchResult(
resources = resources,
missing = hrefs.filterNot { it in seen },
// A server that repeats a response element must not spend two
// thirds of the quarantine threshold in one run — nor count
// against a resource it also answered properly.
failed = failed
.filterNot { failure -> resources.any { it.href == failure.href } }
.distinctBy { it.href },
unsolicited = unsolicited,
)
}
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt
+17
View File
@@ -64,11 +64,28 @@ data class RemoteRef(val href: HttpUrl, val eTag: ETag?)
*/
data class RemoteResource(val href: HttpUrl, val eTag: ETag?, val iCalendar: String)
/**
* A resource the server answered for, but did not hand over.
*
* @param code the response-level status, or `0` when the server reported
* success and supplied no `calendar-data` — the same "no HTTP judgement to
* report" convention [PutOutcome.Rejected] uses.
*/
data class FetchFailure(val href: HttpUrl, val code: Int)
/** What a multiget actually returned, and what it did not. */
data class FetchResult(
val resources: List<RemoteResource>,
/** Asked for, not answered — the server simply omitted them. */
val missing: List<HttpUrl>,
/**
* Asked for, answered, and refused or empty.
*
* Separate from [missing] because "the server said 403" and "the server said
* nothing" are different facts with different right answers — merging them
* is the same mistake as merging the three meanings of a 412.
*/
val failed: List<FetchFailure>,
/**
* Answered without being asked for.
*
caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt
+101
View File
@@ -101,6 +101,107 @@ END:VCALENDAR</C:calendar-data>
assertThat(result.missing.map { it.encodedPath }).containsExactly("/dav/tasks/two.ics")
}
@Test fun `a refused resource is reported as failed, not as missing`() {
server.enqueue(
multistatus(
"""
<response>
<href>/dav/tasks/one.ics</href>
<propstat><prop>
<getetag>"e1"</getetag>
<C:calendar-data xmlns:C="urn:ietf:params:xml:ns:caldav">BEGIN:VCALENDAR
END:VCALENDAR</C:calendar-data>
</prop><status>HTTP/1.1 200 OK</status></propstat>
</response>
<response>
<href>/dav/tasks/two.ics</href>
<status>HTTP/1.1 403 Forbidden</status>
</response>
""",
),
)
val result = collection.fetch(listOf(href("one.ics"), href("two.ics"))).getOrThrow()
// The server did mention two.ics, so it is not missing. Merging the two
// costs the caller the difference between "refused" and "omitted", which
// are not the same fact and do not deserve the same answer.
assertThat(result.missing).isEmpty()
assertThat(result.failed.map { it.href.encodedPath to it.code })
.containsExactly("/dav/tasks/two.ics" to 403)
// One bad member does not cost the batch its good ones.
assertThat(result.resources.map { it.href.encodedPath })
.containsExactly("/dav/tasks/one.ics")
}
@Test fun `a success carrying no calendar-data is reported as failed`() {
server.enqueue(
multistatus(
"""
<response>
<href>/dav/tasks/one.ics</href>
<propstat><prop>
<getetag>"e1"</getetag>
</prop><status>HTTP/1.1 200 OK</status></propstat>
</response>
""",
),
)
val result = collection.fetch(listOf(href("one.ics"))).getOrThrow()
// No HTTP judgement to report: the server said yes and sent nothing.
assertThat(result.failed.map { it.href.encodedPath to it.code })
.containsExactly("/dav/tasks/one.ics" to 0)
assertThat(result.resources).isEmpty()
assertThat(result.missing).isEmpty()
}
@Test fun `a propstat refusal carries its own status, not a blank`() {
server.enqueue(
multistatus(
"""
<response>
<href>/dav/tasks/one.ics</href>
<propstat><prop>
<getetag>"e1"</getetag>
</prop><status>HTTP/1.1 200 OK</status></propstat>
<propstat><prop>
<C:calendar-data xmlns:C="urn:ietf:params:xml:ns:caldav"/>
</prop><status>HTTP/1.1 403 Forbidden</status></propstat>
</response>
""",
),
)
val result = collection.fetch(listOf(href("one.ics"))).getOrThrow()
// A per-property refusal is the usual shape for "you may not read this
// one object". Response.properties drops non-2xx propstats, so reporting
// 0 here would make a deterministic refusal look like a transient blank.
assertThat(result.failed.map { it.code }).containsExactly(403)
}
@Test fun `a resource the server says is gone carries its status`() {
server.enqueue(
multistatus(
"""
<response>
<href>/dav/tasks/one.ics</href>
<status>HTTP/1.1 404 Not Found</status>
</response>
""",
),
)
val result = collection.fetch(listOf(href("one.ics"))).getOrThrow()
// The code has to survive the trip: the engine treats 404 and 403 as
// opposite verdicts.
assertThat(result.failed.map { it.code }).containsExactly(404)
assertThat(result.missing).isEmpty()
}
@Test fun `fetch batches`() {
repeat(2) { server.enqueue(multistatus("")) }
collection.fetch((1..3).map { href("$it.ics") }, batchSize = 2).getOrThrow()