sync: do not write blind when the validator fetch fails

A row with an href but no usable ETag asks the server for one before
writing. That fetch was read with getOrNull(), so a timeout looked the
same as a server with no validator to offer: we PUT unconditionally over
whatever a concurrent edit had left there, and booked it under
unconditionalWrites, which by its own definition means the server had
none. The report asserted something false. Only Result.failure defers
now; a fetch that succeeds and yields nothing usable still writes and is
still counted.

The deferral also has to hold on the incremental path. downloadChanged
matches by href like downloadPhase does, so a row deferred mid-create --
still without an href -- is invisible to it and past its dirty-row
guard, and apply then matches by UID and overwrites the edit. The
explicit refetch list is filtered too.

deferred is set on both call sites of updateResource. It is redundant on
the direct one, where the row keeps its href, and load-bearing on the
adoption from createResource, where it does not.

app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt

@@ -162,7 +162,7 @@ class CollectionSyncer(
                 // The write phase asked for fresh copies the change log may never
                 // mention — a server does not have to echo our own writes back to
                 // us — so they are fetched explicitly rather than hoped for.
-                fetchAndApply(refetch.filterNot { isQuarantined(it) })
+                fetchAndApply(refetch.filterNot { isQuarantined(it) || it in deferred })
             }
 
             resolveParents()
@@ -315,6 +315,11 @@ class CollectionSyncer(
             val byHref = localResources().filter { it.href != null }.associateBy { it.href!! }
             val wanted = changed.filterNot { ref ->
                 val href = ref.href.toString()
+                // The write phase gave up on it this run. Checked before the
+                // `byHref` lookup, because the case it exists for is a row whose
+                // href is still null — invisible to that map, and so past the
+                // dirty-row guard below.
+                if (href in deferred) return@filterNot true
                 val local = byHref[href] ?: return@filterNot false
                 // ⚠️ A row still dirty after the write phase is an edit that never
                 // reached the server — the collection was demoted to read-only, or
@@ -491,8 +496,25 @@ class CollectionSyncer(
             if (eTag == null) {
                 // No usable validator on record. Ask for one before writing,
                 // rather than writing blind.
-                eTag = remote.fetch(listOf(url)).getOrNull()
-                    ?.resources?.firstOrNull()
+                //
+                // ⚠️ A fetch that *failed* says we do not know, not that the
+                // server has none to offer. Falling through would PUT
+                // unconditionally over whatever a concurrent edit had left
+                // there, and book it under `unconditionalWrites` — whose whole
+                // meaning is that the server offered no validator. A success
+                // that yields nothing usable does mean that, and still writes.
+                //
+                // `deferred` is redundant on the direct call, where the row
+                // keeps its href and the dirty-row guards can see it. It is
+                // load-bearing on the delegated one from `createResource`, where
+                // the row's href is still null and only this suppresses a
+                // download that would overwrite the edit by UID.
+                val fetched = remote.fetch(listOf(url)).getOrElse {
+                    deferred += href
+                    skip(local.key, "update validator fetch failed: $it")
+                    return
+                }
+                eTag = fetched.resources.firstOrNull()
                     ?.eTag?.takeIf { it.usable }?.value
                 if (eTag == null) {
                     report = report.copy(unconditionalWrites = report.unconditionalWrites + 1)

app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt

@@ -139,6 +139,59 @@ class CollectionSyncerTest {
         assertThat(remote.resources.values.single().body).contains("SUMMARY:New")
     }
 
+    @Test fun `a failed validator fetch defers the update instead of writing blind`() {
+        remote.put("one.ics", vtodo("a", "Server"))
+        // No local etag: what StoredNeedsRefetch, and any server that serves none,
+        // leave behind.
+        store.rows += task(id = 1, uid = "a", title = "Mine")
+            .copy(href = "http://server/dav/tasks/one.ics", isDirty = true)
+        var failNext = true
+        remote.onFetch = {
+            if (failNext) {
+                failNext = false
+                Result.failure(IllegalStateException("timeout"))
+            } else {
+                null
+            }
+        }
+
+        val report = sync()
+
+        // Not knowing the validator is not the same as the server having none.
+        assertThat(remote.log.none { it.startsWith("UPDATE") }).isTrue()
+        assertThat(remote.resources.values.single().body).contains("SUMMARY:Server")
+        assertThat(report.unconditionalWrites).isEqualTo(0)
+        val row = store.rows.single()
+        assertThat(row.title).isEqualTo("Mine")
+        assertThat(row.isDirty).isTrue()
+        assertThat(quarantine).isEmpty()
+        assertThat(report.quarantined.single().failures).isEqualTo(0)
+        assertThat(report.quarantined.single().reason).contains("validator fetch failed")
+
+        remote.onFetch = null
+        sync()
+
+        assertThat(remote.log).contains("UPDATE one.ics if-match=e-one.ics")
+        assertThat(remote.resources.values.single().body).contains("SUMMARY:Mine")
+        assertThat(store.rows.single().isDirty).isFalse()
+        assertThat(quarantine).isEmpty()
+    }
+
+    @Test fun `a server with no validator to offer is still written to`() {
+        remote.put("one.ics", vtodo("a", "Server"), eTag = null)
+        store.rows += task(id = 1, uid = "a", title = "Mine")
+            .copy(href = "http://server/dav/tasks/one.ics", isDirty = true)
+
+        val report = sync()
+
+        // The fetch succeeded and there was simply no validator — the case
+        // unconditionalWrites exists to count. Deferring here instead would stop
+        // syncing to every etag-less server.
+        assertThat(remote.log).contains("UPDATE one.ics if-match=null")
+        assertThat(report.unconditionalWrites).isEqualTo(1)
+        assertThat(remote.resources.values.single().body).contains("SUMMARY:Mine")
+    }
+
     @Test fun `a PUT that returns no usable etag triggers a refetch`() {
         store.rows += task(id = 1, uid = "a", title = "New").copy(isDirty = true)
         remote.onPut = { href -> PutOutcome.StoredNeedsRefetch(href) }
@@ -532,6 +585,30 @@ class CollectionSyncerTest {
         assertThat(quarantine).isEmpty()
     }
 
+    @Test fun `an adoption whose validator fetch fails keeps the local edit`() {
+        // Create 412s, the first fetch proves the resource is ours, and the
+        // validator fetch inside the adoption then fails.
+        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)
+        var fetches = 0
+        remote.onFetch = {
+            fetches += 1
+            if (fetches == 2) Result.failure(IllegalStateException("timeout")) else null
+        }
+
+        sync()
+
+        // The row still has no href here, so downloadPhase cannot match it and
+        // only `deferred` keeps apply from overwriting the edit by UID. This is
+        // the call site that makes updateResource's `deferred` load-bearing.
+        assertThat(remote.resources).hasSize(1)
+        val row = store.rows.single()
+        assertThat(row.title).isEqualTo("Edited after the lost PUT")
+        assertThat(row.isDirty).isTrue()
+        assertThat(row.href).isNull()
+    }
+
     // ------------------------------------------------------------ protection
 
     @Test fun `a read-only collection is never written to`() {

app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt

@@ -291,6 +291,34 @@ class IncrementalSyncTest {
 
     // --------------------------------------------------------------- setup
 
+    @Test fun `a deferred create is not overwritten by the change log`() {
+        // A previous run's PUT landed but its answer was lost; the user has since
+        // edited the task, so the row is dirty with no href.
+        remote.put("a.ics", vtodo("a", "On the server"))
+        store.rows += task(id = 1, uid = "a", title = "Edited after the lost PUT")
+            .copy(isDirty = true)
+        remote.changePages += page(changed = listOf("a.ics"), token = "urn:x:2")
+        var failNext = true
+        remote.onFetch = {
+            if (failNext) {
+                failNext = false
+                Result.failure(IllegalStateException("timeout"))
+            } else {
+                null
+            }
+        }
+
+        sync()
+
+        // downloadChanged matches by href too, so a row with none is past its
+        // dirty-row guard: without honouring `deferred` here, apply matches by
+        // UID and the edit is gone 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()
+    }
+
     private fun page(
         changed: List<String> = emptyList(),
         removed: List<String> = emptyList(),