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

sync: do not rename a create when its verification fetch fails

Browse Source 
A 412 on create means the name is taken, and only the UID in the body
says whether we took it ourselves on a run whose answer never arrived.
That check read the fetch with getOrNull(), so a timeout looked exactly
like "somebody else's resource" and we PUT the same body under a fresh
random name: one UID in two resources, which RFC 4791 4.1 forbids, the
task duplicated in every client, and a row whose href flips between the
two on every later sync. Only a Result.failure aborts; a fetch that
succeeded and returned nothing, or something else, still renames.

Aborting the write is not enough on its own. The row still has no href,
so downloadPhase cannot match it, its dirty-row guard cannot fire, and
apply then overwrites the local edit by UID with nothing in the report.
The href is deferred for the rest of the run so the read gives up with
the write.

Deferral is skipped rather than failed: a transport failure is nobody's
fault, and a UID-keyed quarantine count is unrefundable once it passes
the threshold, since uploadPhase then returns before anything can clear
it.
This commit is contained in:
makiolaj
2026-09-07 20:58:20 +02:00
parent 09656f6aa7
commit 21f56a78ae
3 changed files with 102 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt
+39 -2
View File
@@ -86,6 +86,19 @@ class CollectionSyncer(
/** Hrefs this run wrote, and which the sweep must therefore not remove. */
val touched = mutableSetOf<String>()
/**
* 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<String>()
/**
* `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.
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt
+53 -1
View File
@@ -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`() {
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt
+10
View File
@@ -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<HttpUrl>) -> Result<FetchResult>?)? = 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<HttpUrl>): Result<FetchResult> {
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) }