sync: a deleted series and a deleted occurrence now reach the server

Deleting a recurring task never sent a DELETE. markDeleted tombstoned only the row it was handed, and master_id's CASCADE never fired because nothing was actually deleted — so a series with any override left LocalResource.isDeleted false, went to the upload phase, and ended is_deleted = 1, is_dirty = 0 with a matching ETag: beyond every phase's reach. Other clients kept the task; here it was gone. markDeleted now tombstones the series whole, and a resource is read as deleted from its master rather than from all of its rows, which also repairs the rows an older version left in that state instead of leaving them stuck. Deleting a single occurrence wrote no EXDATE. Removing the override does not delete the occurrence, it un-overrides it — per RFC 5545 the master's RRULE regenerates it as a plain instance, on the server and in every other client. It was invisible locally only because the override queries filter tombstones out. The exception is written onto the master now and the override row is dropped, which is also what hides the occurrence in a device-only list, where there is no tombstone to do it. The value takes the shape the list already has: lib-recur parses the whole EXDATE list or none of it, so a UTC date-time appended to a run of DATEs would drop every exception the series had. ResourceValidator gains the rule that would have named the old bug: a body of overrides with no master describes instances of something that is not in the resource.
2026-09-09 11:26:06 +02:00
parent 80133d5cc8
commit 9c84b49fc9
8 changed files with 256 additions and 20 deletions
@@ -49,6 +49,16 @@ class RoomTasksDataSourceTest {
        percentComplete: Int? = null,
    ) = TaskForm(title = title, listId = listId, due = due, percentComplete = percentComplete)

+    /** A list that belongs to an account, so writes owe a server something. */
+    private fun syncedList(): Long {
+        val accountId = db.accounts().insert(
+            AccountEntity(displayName = "me@example.com", username = "me"),
+        )
+        return db.taskLists().insert(
+            TaskListEntity(name = "Work", color = 0, accountId = accountId, href = "https://s/w/"),
+        )
+    }
+
    /** Turns [taskId] into a weekly series anchored at [anchor]. */
    private fun makeRecurring(taskId: Long, anchor: Instant, rule: String = "FREQ=WEEKLY") {
        val entity = db.tasks().entity(taskId)!!
@@ -359,6 +369,46 @@ class RoomTasksDataSourceTest {
        assertThat(db.tasks().allOverrides(listId)).isEmpty()
    }

+    @Test
+    fun deletingASyncedSeriesTombstonesItsOverridesToo() {
+        val syncedList = syncedList()
+        val id = source.insertTask(TaskForm(title = "Standup", listId = syncedList))
+        makeRecurring(id, now)
+        val target = source.tasks(TaskQuery(listId = syncedList))
+            .filter { it.taskId == id }
+            .first { it.distanceFromCurrent == 1 }
+        source.updateInstance(id, target.occurrenceStart!!, TaskForm(title = "moved", listId = syncedList))
+
+        source.deleteTask(id)
+
+        // ⚠️ master_id cascades on delete, and a tombstone deletes nothing — so
+        // a master marked alone left the resource reading as partly deleted, the
+        // DELETE was never sent, and the task stayed on the server for ever.
+        val rows = db.tasks().allIn(syncedList)
+        assertThat(rows).hasSize(2)
+        assertThat(rows.all { it.isDeleted && it.isDirty }).isTrue()
+    }
+
+    @Test
+    fun deletingOneOccurrenceExceptsItOnTheMaster() {
+        val id = source.insertTask(form())
+        makeRecurring(id, now)
+        val target = source.tasks(TaskQuery(listId = listId))
+            .filter { it.taskId == id }
+            .first { it.distanceFromCurrent == 1 }
+        source.updateInstance(id, target.occurrenceStart!!, form(title = "moved"))
+        val override = db.tasks().override(id, target.occurrenceStart)!!
+
+        source.deleteTask(override.id)
+
+        // ⚠️ Dropping the override row un-overrides the occurrence, and the
+        // master's RRULE regenerates it. The EXDATE is the deletion.
+        assertThat(db.tasks().entity(override.id)).isNull()
+        assertThat(db.tasks().entity(id)!!.exdate).isNotEmpty()
+        assertThat(source.tasks(TaskQuery(listId = listId)).map { it.occurrenceStart })
+            .doesNotContain(target.occurrenceStart)
+    }
+
    @Test
    fun subtasksReadBackUnderTheirParent() {
        val parent = source.insertTask(form(title = "Prepare invoice"))
@@ -597,9 +597,12 @@ class CollectionSyncer(
            // (list_id, uid, recurrence_id) then makes re-adding that occurrence
            // throw for good.
            //
-            // Overrides only, for now. A tombstoned *master* still reaches here
-            // because `markDeleted` does not tombstone a series' overrides, and
-            // deleting it would cascade the live ones away via `master_id`.
+            // Overrides only. A tombstoned master no longer reaches this phase
+            // at all — `markDeleted` tombstones the whole series, so the
+            // resource reads as deleted and goes to `deletePhase` — but rows
+            // tombstoned by an older version of the app are still out there, and
+            // hard-deleting a master here would cascade its live overrides away
+            // via `master_id`.
            val (buried, kept) = local.rows.partition { it.isDeleted && it.recurrenceId != null }
            if (buried.isNotEmpty()) store.deleteAll(buried.map { it.id })
            store.markSynced(kept.map { it.id }, key, eTag)
@@ -1025,8 +1028,17 @@ class CollectionSyncer(
         */
        val key: String = href ?: "uid:$uid"

-        /** Only when the whole resource is gone; a deleted override is an edit. */
-        val isDeleted: Boolean = rows.all { it.isDeleted }
+        /**
+         * The master's tombstone is the resource's: an override cannot outlive
+         * the series it belongs to. A deleted *override* is an edit.
+         *
+         * ⚠️ Not `rows.all { … }`. `markDeleted` now tombstones a series whole,
+         * but rows tombstoned by an older version left the master marked and its
+         * overrides live — which read as "partly deleted", went to the upload
+         * phase, and never sent the DELETE the user asked for. Reading the
+         * master repairs those on the next sync instead of leaving them stuck.
+         */
+        val isDeleted: Boolean = master.isDeleted

        /** What gets serialised: a deleted override is simply absent. */
        val live: List<TaskEntity> = rows.filterNot { it.isDeleted }
@@ -41,6 +41,16 @@ object ResourceValidator {
            return Rejection("mixes VTODO with ${foreign.joinToString(", ")}")
        }

+        // ⚠️ Overrides with no master. A `RECURRENCE-ID` names an instance *of*
+        // a series, so a body holding only overrides describes instances of
+        // something that is not in the resource — RFC 4791 §4.1 asks for the
+        // recurring component and its overridden instances, not the instances
+        // alone. This is the shape a partly tombstoned series used to serialise
+        // to, and no server rejects it in a way that names the cause.
+        if (todos.all { it.property("RECURRENCE-ID") != null }) {
+            return Rejection("holds overridden instances but not the task they override")
+        }
+
        // One resource, one UID — the constraint that makes "fork the conflicting
        // edit into the same resource" impossible, and the one servers enforce.
        val uids = todos.map { it.property("UID")?.value.orEmpty() }.distinct()
@@ -240,12 +240,39 @@ class RoomTasksDataSource @Inject constructor(

    /**
     * Hard delete for a row no server knows about, tombstone for one that is
-     * still owed to a collection. `master_id` cascades, so deleting a series
-     * takes its overrides with it.
+     * still owed to a collection — and an `EXDATE` when what is being deleted is
+     * a single occurrence of a series.
+     *
+     * ⚠️ Removing an override does not delete the occurrence, it *un-overrides*
+     * it: per RFC 5545 the master's `RRULE` regenerates it as a plain instance,
+     * on the server and on every other client. The exception has to be written
+     * down on the master, which is also what keeps the occurrence hidden in a
+     * device-only list, where there is no tombstone to hide it.
     */
    override fun deleteTask(taskId: Long) {
        val current = tasks.entity(taskId) ?: return
        val listAccount = lists.entity(current.listId)?.accountId
+        val masterId = current.masterId
+        val occurrence = current.recurrenceId
+        if (masterId != null && occurrence != null) {
+            tasks.entity(masterId)?.let { master ->
+                tasks.update(
+                    TaskFormWriter.excepting(
+                        master,
+                        occurrence,
+                        clock.now(),
+                        zone(),
+                        // A device-only list owes nobody a PUT.
+                        dirty = listAccount != null,
+                    ),
+                )
+            }
+            // The master's EXDATE *is* the deletion, so the override has no job
+            // left. Leaving a tombstone behind would upload a body with the
+            // occurrence merely absent, which says the opposite.
+            tasks.delete(taskId)
+            return
+        }
        if (listAccount == null) tasks.delete(taskId) else tasks.markDeleted(taskId, clock.now())
    }

@@ -138,8 +138,24 @@ interface TaskDao {
    @Query("DELETE FROM tasks WHERE id = :taskId")
    fun delete(taskId: Long): Int

-    /** Tombstone, for a row a server still knows about. */
-    @Query("UPDATE tasks SET is_deleted = 1, is_dirty = 1, last_modified = :at WHERE id = :taskId")
+    /**
+     * Tombstone, for a row a server still knows about — and for the whole series
+     * when [taskId] is a master.
+     *
+     * ⚠️ `master_id` cascades on *delete*, and a tombstone deletes nothing, so
+     * tombstoning the master alone left its overrides live. The resource then
+     * read as partly deleted: `LocalResource.isDeleted` is `rows.all { … }`, so
+     * it went to the upload phase instead of the delete phase, no DELETE was
+     * ever sent, and the master ended `is_deleted = 1, is_dirty = 0` with a
+     * matching ETag — beyond the reach of every phase. Other clients kept the
+     * task; here it was gone.
+     */
+    @Query(
+        """
+        UPDATE tasks SET is_deleted = 1, is_dirty = 1, last_modified = :at
+        WHERE id = :taskId OR master_id = :taskId
+        """
+    )
    fun markDeleted(taskId: Long, at: Instant?): Int

    /**
@@ -2,6 +2,7 @@ package de.jeanlucmakiola.agendula.data.tasks.room

 import de.jeanlucmakiola.agendula.domain.TaskForm
 import de.jeanlucmakiola.agendula.domain.TaskStatus
+import de.jeanlucmakiola.agendula.domain.ical.ICalValues
 import de.jeanlucmakiola.agendula.domain.toICal
 import kotlin.time.Instant

@@ -70,6 +71,62 @@ object TaskFormWriter {
        isDirty = true,
    )

+    /**
+     * [master] with [occurrence] added to its `EXDATE` — how a single occurrence
+     * of a series is deleted.
+     *
+     * ⚠️ Removing the override row is not a deletion. RFC 5545 reads an absent
+     * `RECURRENCE-ID` component as "not overridden", so the master's `RRULE`
+     * regenerates that instance; only an `EXDATE` takes it out of the set.
+     *
+     * @param floatingZone what a series with no `TZID` means by its wall times —
+     *   the same zone [de.jeanlucmakiola.agendula.domain.recurrence
+     *   .RecurrenceExpander] resolves it in.
+     */
+    fun excepting(
+        master: TaskEntity,
+        occurrence: Instant,
+        now: Instant,
+        floatingZone: String,
+        dirty: Boolean,
+    ): TaskEntity {
+        val existing = master.exdate?.trim()?.ifEmpty { null }
+        val value = exceptionValue(master, occurrence, existing, floatingZone)
+        if (existing != null && value in existing.split(',').map(String::trim)) return master
+        return master.copy(
+            exdate = if (existing == null) value else "$existing,$value",
+            lastModified = now,
+            isDirty = master.isDirty || dirty,
+        )
+    }
+
+    /**
+     * ⚠️ Written in the shape the list is already in, not in ours.
+     *
+     * `EXDATE` is stored as the bare property value, so a list is a run of one
+     * value type — and lib-recur parses the whole list or none of it. Appending
+     * a UTC `…Z` to a run of `DATE`s would drop every exception the series
+     * already had, this one included.
+     */
+    private fun exceptionValue(
+        master: TaskEntity,
+        occurrence: Instant,
+        existing: String?,
+        floatingZone: String,
+    ): String {
+        val sample = existing?.substringBefore(',')?.trim()
+        return when {
+            sample == null ->
+                if (master.isAllDay) ICalValues.formatDate(occurrence)
+                else ICalValues.formatDateTime(occurrence, null)
+            !sample.contains('T') -> ICalValues.formatDate(occurrence)
+            sample.endsWith("Z") -> ICalValues.formatDateTime(occurrence, null)
+            // Local wall time: the TZID parameter was dropped on the way in, so
+            // the series' own zone is what those values mean.
+            else -> ICalValues.formatDateTime(occurrence, master.timezone ?: floatingZone)
+        }
+    }
+
    /**
     * A form carrying no percent leaves status alone — the standalone toggle stays
     * authoritative. Otherwise progress and status move together in both
@@ -388,17 +388,15 @@ class CollectionSyncerTest {

        sync()

-        // Pins today's behaviour, which is wrong and is not this fix's to correct:
-        // markDeleted tombstones only the master, so isDeleted (rows.all) is false,
-        // the resource goes to uploadPhase rather than deletePhase, and no DELETE
-        // is ever sent. Dropping the master row here instead would cascade the live
-        // override away via master_id and leave the server holding a resource
-        // nothing here has rows for — so only overrides are dropped for now.
-        assertThat(remote.log.none { it.startsWith("DELETE") }).isTrue()
-        assertThat(store.rows.map { it.id }).containsExactly(1L, 2L)
-        val master = store.rows.single { it.id == 1L }
-        assertThat(master.isDeleted).isTrue()
-        assertThat(master.isDirty).isFalse()
+        // ⚠️ The shape an older version of markDeleted left behind: the master
+        // tombstoned, its overrides live. Read as rows.all it was "partly
+        // deleted", so it went to the upload phase, no DELETE was ever sent, and
+        // it ended is_deleted = 1, is_dirty = 0 with a matching ETag — beyond
+        // every phase's reach, gone here and still there for everyone else. The
+        // master's tombstone is the resource's, so the next sync finishes what
+        // the user asked for.
+        assertThat(remote.log).contains("DELETE one.ics if-match=e-one.ics")
+        assertThat(store.rows).isEmpty()
    }

    @Test fun `the sweep does not remove what this run just created`() {
@@ -149,4 +149,70 @@ class TaskFormWriterTest {
        assertThat(TaskFormWriter.apply(task(), form().copy(priority = Priority.NONE), NOW, ZONE).priority)
            .isEqualTo(0)
    }
+
+    @Test
+    fun `deleting one occurrence writes the exception onto the master`() {
+        val master = task().copy(rrule = "FREQ=DAILY", timezone = ZONE)
+        val occurrence = Instant.parse("2026-03-01T09:00:00Z")
+
+        val excepted = TaskFormWriter.excepting(master, occurrence, NOW, ZONE, dirty = true)
+
+        // ⚠️ Dropping the override row un-overrides the occurrence; the RRULE
+        // then regenerates it. Only an EXDATE takes it out of the set.
+        assertThat(excepted.exdate).isEqualTo("20260301T090000Z")
+        assertThat(excepted.isDirty).isTrue()
+        assertThat(excepted.lastModified).isEqualTo(NOW)
+    }
+
+    @Test
+    fun `a device-only list gets the exception without being made dirty`() {
+        val master = task().copy(rrule = "FREQ=DAILY")
+        val excepted = TaskFormWriter.excepting(
+            master,
+            Instant.parse("2026-03-01T09:00:00Z"),
+            NOW,
+            ZONE,
+            dirty = false,
+        )
+
+        // The occurrence still has to be hidden — there is no tombstone doing it.
+        assertThat(excepted.exdate).isNotNull()
+        assertThat(excepted.isDirty).isFalse()
+    }
+
+    @Test
+    fun `an exception is written in the shape the list already has`() {
+        val allDay = task().copy(isAllDay = true, exdate = "20260228")
+        val occurrence = Instant.parse("2026-03-01T00:00:00Z")
+
+        // ⚠️ lib-recur parses the whole EXDATE list or none of it, so a UTC
+        // date-time appended to a run of DATEs drops every exception the series
+        // had, this one included.
+        assertThat(TaskFormWriter.excepting(allDay, occurrence, NOW, ZONE, dirty = true).exdate)
+            .isEqualTo("20260228,20260301")
+
+        val floating = task().copy(timezone = ZONE, exdate = "20260228T100000")
+        assertThat(
+            TaskFormWriter.excepting(
+                floating,
+                Instant.parse("2026-03-01T09:00:00Z"),
+                NOW,
+                ZONE,
+                dirty = true,
+            ).exdate,
+        ).isEqualTo("20260228T100000,20260301T100000")
+    }
+
+    @Test
+    fun `an occurrence already excepted is not added twice`() {
+        val master = task().copy(exdate = "20260301T090000Z")
+        val same = TaskFormWriter.excepting(
+            master,
+            Instant.parse("2026-03-01T09:00:00Z"),
+            NOW,
+            ZONE,
+            dirty = true,
+        )
+        assertThat(same).isSameInstanceAs(master)
+    }
 }