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

sync: three corrections to the reconciler, from the review pass

Browse Source 
The incremental path applied a page's removals before downloading its
changes, so a delete-and-recreate reported as removed one.ics + changed
two.ics purged the row before two.ics was ever fetched: the task came
back with sortOrder 0, no colour and no parent, and deletedLocally was
reported for a task nobody deleted. The same damage the sweep's
pre-download snapshot used to do, on the fast path. Bodies now go first,
after which the vacated href names nothing and the removal is the no-op
it should be — RFC 6578 reports each resource once, so a page cannot
both change and remove one href, and touched already guards what this
run wrote.

apply re-pointed a row's href in the hoisted index but left the old
entry naming those rows, so a later resource in the same batch read them
as displaced and deleted them: server holds two.ics with UID a (was at
one.ics) and one.ics now with UID b, and the rename is destroyed by the
resource that took its name. The in-memory twin of the same bug.

The download-side quarantine had no way back. At THRESHOLD the href is
stripped before the fetch, so apply — and with it succeeded — can never
run to clear the count, and a per-object ACL wrong for an afternoon hid
that task for the life of the install. The upload side is fine, since
stored and purge refund it there. The daily full reconciliation now
probes a quarantined href once: it costs one resource a day, and one
that answers clears its count.
This commit is contained in:
makiolaj
2026-09-09 11:17:21 +02:00
parent 41ffc75fc5
commit ddcffa61b3
3 changed files with 95 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt
+31 -2
View File
@@ -235,8 +235,19 @@ class CollectionSyncer(
if (!removalsArePlausible(page.removed)) return false
applyRemovals(page.removed)
// ⚠️ Bodies before removals, unlike the phase order above. A
// delete-and-recreate arrives as `removed: one.ics` +
// `changed: two.ics` in one page, and purging first destroys the
// row the download would have re-pointed — it comes back with
// `sortOrder = 0`, no colour and no parent, and `deletedLocally`
// is reported for a task nobody deleted. Downloading first lets
// `apply` move the row, after which `one.ics` names nothing and
// the removal is the no-op it should be. Nothing is lost the
// other way: RFC 6578 reports each resource once, so a page
// cannot both change and remove the same href, and a href this
// run wrote is already guarded by `touched`.
downloadChanged(page.changed)
applyRemovals(page.removed)
// ⚠️ After the bodies, and only if they actually landed. A
// network drop mid-multiget would otherwise commit a cursor past
@@ -626,7 +637,15 @@ class CollectionSyncer(
}
}
wanted += refetch
wanted.removeAll { isQuarantined(it) }
// ⚠️ Not on a full reconciliation. The download side has no refund:
// at THRESHOLD the href is stripped before the fetch, so `apply` —
// and with it `succeeded` — can never run to clear the count again,
// and a per-object ACL wrong for an afternoon hides that task for the
// life of the install. The upload side needs no equivalent, because
// `stored` and `purge` refund it there. One probe per periodic
// reconciliation is the expiry: it costs a single resource on a slow
// cadence, and a resource that answers this time clears its count.
if (!fullReconciliationDue) wanted.removeAll { isQuarantined(it) }
fetchAndApply(wanted)
}
@@ -775,6 +794,16 @@ class CollectionSyncer(
// Keep the hoisted index honest for the resources still to come.
index.byUid[uid] = written
index.byHref[href] = written
// ⚠️ Including the href these rows just left. A delete-and-recreate
// keeps the UID and changes the filename, so the old entry still
// names rows that now live here — and a later resource in the same
// batch reading that entry as `displaced` deletes them. The
// in-memory twin of the sweep's pre-download snapshot.
val moved = written.map { it.id }.toSet()
existing.values.mapNotNull { it.href }.toSet().minus(href).forEach { vacated ->
val left = index.byHref[vacated].orEmpty().filterNot { it.id in moved }
if (left.isEmpty()) index.byHref.remove(vacated) else index.byHref[vacated] = left
}
if (displaced.isNotEmpty()) {
val ids = displaced.map { it.id }
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt
+35 -3
View File
@@ -25,7 +25,8 @@ class CollectionSyncerTest {
href = "http://server/dav/tasks/",
)
private fun sync() = CollectionSyncer(store) { NOW }.sync(list, remote, quarantine)
private fun sync(fullDue: Boolean = true) =
CollectionSyncer(store) { NOW }.sync(list, remote, quarantine, fullDue)
// -------------------------------------------------------------- download
@@ -588,11 +589,12 @@ class CollectionSyncerTest {
repeat(QuarantineStore.THRESHOLD) { sync() }
remote.log.clear()
sync()
sync(fullDue = false)
// 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.
// every sync for ever. The daily reconciliation still probes it once —
// otherwise nothing on this side could ever clear the count.
assertThat(remote.log).containsExactly("LIST")
}
@@ -845,6 +847,36 @@ class CollectionSyncerTest {
assertThat(report.failure).contains("revoked")
}
@Test fun `a rename into a name another task took keeps both rows`() {
store.rows += task(id = 1, uid = "a", title = "Moved")
.copy(href = "http://server/dav/tasks/one.ics", etag = "e-old")
// Fetched in insertion order, so the rename is applied first and leaves
// the index entry for `one.ics` behind.
remote.put("two.ics", vtodo("a", "Moved"))
remote.put("one.ics", vtodo("b", "A different task at the old name"))
sync()
// ⚠️ `one.ics` now holds somebody else, and row 1 no longer lives there
// — reading it out of a stale index entry deletes the task the rename
// just repaired.
assertThat(store.rows.map { it.uid }).containsExactly("a", "b")
}
@Test fun `a full reconciliation probes a resource the download gave up on`() {
remote.put("one.ics", vtodo("a", "Buy milk"))
quarantine[QuarantineStore.key(list.id, "http://server/dav/tasks/one.ics")] =
QuarantineStore.THRESHOLD
sync()
// ⚠️ The download side has no refund — at THRESHOLD the href is stripped
// before the fetch, so nothing can ever clear the count again. One probe
// per periodic reconciliation is the expiry.
assertThat(store.rows.single().uid).isEqualTo("a")
assertThat(quarantine).isEmpty()
}
// ----------------------------------------------------------------- setup
/** A multiget in which `two.ics` alone comes back refused. */
app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt
+29
View File
@@ -165,6 +165,35 @@ class IncrementalSyncTest {
assertThat(store.rows).hasSize(20)
}
@Test fun `a delete-and-recreate in one page keeps the row's local columns`() {
store.rows += task(id = 1, uid = "a", title = "Old")
.copy(
href = "http://server/dav/tasks/one.ics",
etag = "e-old",
sortOrder = 5,
color = 0x00FF00,
)
remote.put("two.ics", vtodo("a", "Same task, new file"), eTag = "e-new")
remote.changePages += page(
changed = listOf("two.ics"),
removed = listOf("one.ics"),
token = "urn:x:2",
eTag = "e-new",
)
val report = sync()
// ⚠️ Same UID, new filename. Purging `one.ics` before `two.ics` is
// fetched re-inserts the task with sortOrder 0 and no colour, and
// reports a deletion nobody asked for.
val row = store.rows.single()
assertThat(row.id).isEqualTo(1)
assertThat(row.href).isEqualTo("http://server/dav/tasks/two.ics")
assertThat(row.sortOrder).isEqualTo(5)
assertThat(row.color).isEqualTo(0x00FF00)
assertThat(report.deletedLocally).isEqualTo(0)
}
@Test fun `an un-uploaded local edit is never downloaded over`() {
remote.readOnly = true
store.rows += task(id = 1, uid = "a", title = "Mine, unsent")