diff --git a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt index 69f4e33..1c922fe 100644 --- a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt +++ b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt @@ -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")) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt index 8ef5c03..25a174b 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt @@ -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 = rows.filterNot { it.isDeleted } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt index 5220723..7164711 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt @@ -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() diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt index a496df0..62889a7 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt @@ -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()) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt index 9057a9f..b42ce56 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt @@ -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 /** diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriter.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriter.kt index 5e81d33..66ed181 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriter.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriter.kt @@ -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 diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt index a6aefcb..c53709f 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt @@ -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`() { diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriterTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriterTest.kt index f5c55bd..7a1ed8b 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriterTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskFormWriterTest.kt @@ -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) + } }