Make the legacy rounding safe for reads, look up one occurrence exactly (#135)

Rounding no longer fails a read, waits for itself across threads, goes in one
batch and is only remembered once every row took it. Occurrence lookups prefer
an exact anchor, enrich only the row they return and remember a view without
original_instance_id. A series-wide save keeps an all-day due on its day, and
overrides deleted through the published provider take the occurrence along.

app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderRecurrenceTest.kt

@@ -189,6 +189,27 @@ class ExternalProviderRecurrenceTest {
         assertThat(source.task(override.taskId, override.occurrenceStart)?.taskId).isEqualTo(override.taskId)
     }
 
+    @Test
+    fun anAllDayDueAddedToTheSeriesStaysOnItsDay() {
+        val day = Clock.System.now().toEpochMilliseconds() / 86_400_000 * 86_400_000
+        val id = source.insertTask(
+            TaskForm(title = "All day", listId = listId, start = Instant.fromEpochMilliseconds(day), isAllDay = true, rrule = "FREQ=DAILY"),
+            reminders = null,
+        )
+        val task = source.task(id)!!
+        // Local midnight the next day, as the date picker hands it over.
+        val localMidnight = Instant.fromEpochMilliseconds(day + 86_400_000 - java.util.TimeZone.getDefault().getOffset(day))
+
+        source.updateSeries(
+            id,
+            task.occurrenceStart!!,
+            TaskForm(title = "All day", listId = listId, start = task.start, due = localMidnight, isAllDay = true, rrule = "FREQ=DAILY"),
+            reminders = null,
+        )
+
+        assertThat(storedColumn(id, Tasks.DUE)!!.toLong() % 86_400_000).isEqualTo(0L)
+    }
+
     @Test
     fun savingAllFromAnOverrideKeepsTheSeriesWhereItIs() {
         val id = dailyTask()

app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt

@@ -12,6 +12,7 @@ import android.database.sqlite.SQLiteException
 import android.net.Uri
 import android.os.Handler
 import android.os.Looper
+import android.util.Log
 import dagger.hilt.android.qualifiers.ApplicationContext
 import de.jeanlucmakiola.agendula.data.tasks.TasksContract.Instances
 import de.jeanlucmakiola.agendula.data.tasks.TasksContract.Lists
@@ -75,16 +76,20 @@ class AndroidTasksDataSource @Inject constructor(
     }
 
     override fun task(taskId: Long): Task? {
-        val rows = queryInstances("${Instances.TASK_ID} = ?", arrayOf(taskId.toString()))
-        return rows.firstOrNull { it.distanceFromCurrent == 0 } ?: rows.firstOrNull()
+        roundLegacyAnchors()
+        val rows = instanceRows(resolver.query(TasksContract.instancesUri(authority()), null, "${Instances.TASK_ID} = ?", arrayOf(taskId.toString()), null))
+        return (rows.firstOrNull { it.task.distanceFromCurrent == 0 } ?: rows.firstOrNull())?.let { enrich(listOf(it)).single() }
     }
 
     override fun task(taskId: Long, occurrenceStart: Instant?): Task? {
         if (occurrenceStart == null) return task(taskId)
-        val rows = instanceTasks(queryOccurrences(taskId))
-        return rows.firstOrNull { it.occurrenceStart?.let { start -> sameSecond(start, occurrenceStart) } == true }
-            ?: rows.firstOrNull { it.taskId == taskId && it.distanceFromCurrent == 0 }
-            ?: rows.firstOrNull { it.taskId == taskId }
+        roundLegacyAnchors()
+        val rows = instanceRows(queryOccurrences(taskId))
+        val chosen = rows.firstOrNull { it.anchor == occurrenceStart }
+            ?: rows.firstOrNull { it.anchor?.let { a -> sameSecond(a, occurrenceStart) } == true }
+            ?: rows.firstOrNull { it.task.taskId == taskId && it.task.distanceFromCurrent == 0 }
+            ?: rows.firstOrNull { it.task.taskId == taskId }
+        return chosen?.let { enrich(listOf(it)).single() }
     }
 
     override fun subtasks(parentTaskId: Long): List<Task> =
@@ -122,15 +127,24 @@ class AndroidTasksDataSource @Inject constructor(
     }
 
     /** Maps [cursor]'s instance rows, with the rule each series keeps on its `tasks` row, and their tags. */
-    private fun instanceTasks(cursor: Cursor?): List<Task> {
+    private fun instanceTasks(cursor: Cursor?): List<Task> = enrich(instanceRows(cursor))
+
+    /** [cursor]'s instance rows, before their rules and tags are looked up. */
+    private fun instanceRows(cursor: Cursor?): List<InstanceRow> {
         var flagged = false
         val rows = cursor?.use { c ->
             flagged = c.getColumnIndex(Instances.IS_RECURRING) >= 0
             val reader = CursorColumnReader(c)
             buildList { while (c.moveToNext()) add(TaskMapper.instance(reader)) }
-        }
-        if (rows.isNullOrEmpty()) return emptyList()
-        val rules = seriesRules(rows.filter { !flagged || it.task.isRecurring }.map { it.ruleOwner })
+        }.orEmpty()
+        // Where the provider flags recurrence, only flagged rows need their rule looked up.
+        return if (flagged) rows.map { if (it.task.isRecurring) it else it.copy(ruleOwner = NO_RULE) } else rows
+    }
+
+    /** [rows] completed with the rule each series keeps on its `tasks` row, and their tags. */
+    private fun enrich(rows: List<InstanceRow>): List<Task> {
+        if (rows.isEmpty()) return emptyList()
+        val rules = seriesRules(rows.map { it.ruleOwner }.filter { it != NO_RULE })
         val tasks = rows.map { TaskMapper.withRule(it, rules[it.ruleOwner]) }
         val tags = categories(tasks.map { it.taskId })
         return tasks.map { task -> tags[task.taskId]?.let { task.copy(tags = it) } ?: task }
@@ -248,14 +262,14 @@ class AndroidTasksDataSource @Inject constructor(
             val r = CursorColumnReader(c)
             r.getLong(Tasks.DTSTART) to r.getLong(Tasks.DUE)
         }
-        fun shifted(anchor: Long?, was: Instant?, now: Instant?): Long? = when {
-            now == null -> null
-            anchor == null || was == null -> now.toEpochMilliseconds()
-            else -> anchor + (now - was).inWholeMilliseconds
-        }?.let(::providerMillis)
-        val values = TaskWriteMapper.seriesValues(form, ZoneId.systemDefault().id) +
-            (Tasks.DTSTART to shifted(anchors?.first, occurrence?.start, form.start)) +
-            (Tasks.DUE to shifted(anchors?.second, occurrence?.due, form.due))
+        val base = TaskWriteMapper.seriesValues(form, ZoneId.systemDefault().id)
+        // Without an anchor to shift, the form's own value stands — already pinned for an all-day series.
+        fun shifted(column: String, anchor: Long?, was: Instant?, now: Instant?): Any? =
+            if (now == null || anchor == null || was == null) base[column]
+            else providerMillis(anchor + (now - was).inWholeMilliseconds)
+        val values = base +
+            (Tasks.DTSTART to shifted(Tasks.DTSTART, anchors?.first, occurrence?.start, form.start)) +
+            (Tasks.DUE to shifted(Tasks.DUE, anchors?.second, occurrence?.due, form.due))
         val rows = resolver.update(taskUri(authority(), seriesId), values.toContentValues(), null, null)
         if (rows == 0) throw TaskWriteFailedException("update series $seriesId")
         setCategories(seriesId, form.tags)
@@ -322,8 +336,7 @@ class AndroidTasksDataSource @Inject constructor(
         val values = mapOf<String, Any?>(Tasks.RRULE to rule)
         val rows = resolver.update(taskUri(authority(), seriesId), values.toContentValues(), null, null)
         if (rows == 0) throw TaskWriteFailedException("end series $seriesId")
-        // Plain deletes: the instances URI would add EXDATEs past the new end.
-        exceptionsFrom(seriesId, occurrenceStart).forEach { resolver.delete(taskUri(authority(), it), null, null) }
+        exceptionsFrom(seriesId, occurrenceStart).forEach(::deleteTask)
     }
 
     /** Override rows of [seriesId] standing for [from] or a later occurrence. */
@@ -438,11 +451,14 @@ class AndroidTasksDataSource @Inject constructor(
     private fun <T> occurrence(taskId: Long, occurrenceStart: Instant, read: (ColumnReader) -> T): T? =
         queryOccurrences(taskId)?.use { c ->
             val reader = CursorColumnReader(c)
+            // An exact match first: a row of the same second is a fallback, not a peer.
+            var near: Int? = null
             while (c.moveToNext()) {
                 val anchor = TaskMapper.occurrenceAnchor(reader) ?: continue
-                if (sameSecond(anchor, occurrenceStart)) return@use read(reader)
+                if (anchor == occurrenceStart) return@use read(reader)
+                if (near == null && sameSecond(anchor, occurrenceStart)) near = c.position
             }
-            null
+            near?.let { c.moveToPosition(it); read(reader) }
         }
 
     /**
@@ -451,35 +467,60 @@ class AndroidTasksDataSource @Inject constructor(
      * task's own rows only, rather than a failed query.
      */
     private fun queryOccurrences(taskId: Long): Cursor? {
-        val uri = TasksContract.instancesUri(authority())
+        val authority = authority()
+        val uri = TasksContract.instancesUri(authority)
         val id = taskId.toString()
-        return try {
-            resolver.query(uri, null, OCCURRENCE_OF, arrayOf(id, id), null)
-        } catch (_: SQLiteException) {
-            resolver.query(uri, null, "${Instances.TASK_ID} = ?", arrayOf(id), null)
+        if (ownRowsOnlyFor != authority) {
+            try {
+                return resolver.query(uri, null, OCCURRENCE_OF, arrayOf(id, id), null)
+            } catch (_: SQLiteException) {
+                ownRowsOnlyFor = authority
+            } catch (_: IllegalArgumentException) {
+                ownRowsOnlyFor = authority
+            }
         }
+        return resolver.query(uri, null, "${Instances.TASK_ID} = ?", arrayOf(id), null)
     }
 
+    /** The provider whose instances view turned out to lack `original_instance_id`. */
+    @Volatile
+    private var ownRowsOnlyFor: String? = null
+
     @Volatile
     private var anchorsRoundedFor: String? = null
 
+    private val roundingLock = Any()
+
     /**
      * Rounds the DTSTART/DUE a build before [providerMillis] stored with
      * milliseconds, which the provider cannot move past a completed occurrence.
-     * Once per provider, before the first read; written as the rows' sync adapter,
-     * since the server only ever had whole seconds, so nothing is dirtied or
-     * re-uploaded. Overrides need nothing: the provider forks them at its own
-     * whole-second instance times.
+     * Once per provider, before the first read: concurrent readers wait for it.
+     * Written as the rows' sync adapter, since the server only ever had whole
+     * seconds, so nothing is dirtied or re-uploaded. Overrides need nothing: the
+     * provider forks them at its own whole-second instance times.
+     *
+     * Never fails a read. A row the provider refuses is left as it was and tried
+     * again on the next start; only a pass that rounded everything is remembered.
      */
     private fun roundLegacyAnchors() {
         val authority = authority()
         if (anchorsRoundedFor == authority) return
-        anchorsRoundedFor = authority
-        val prefs = context.getSharedPreferences(MIGRATIONS, Context.MODE_PRIVATE)
-        val key = "anchors_rounded:$authority"
-        if (prefs.getBoolean(key, false)) return
+        synchronized(roundingLock) {
+            if (anchorsRoundedFor == authority) return
+            val prefs = context.getSharedPreferences(MIGRATIONS, Context.MODE_PRIVATE)
+            val key = "anchors_rounded:$authority"
+            val done = prefs.getBoolean(key, false) || runCatching { roundAll(authority) }
+                .onFailure { Log.w(TAG, "Rounding legacy anchors failed", it) }
+                .getOrDefault(false)
+            if (done) prefs.edit().putBoolean(key, true).apply()
+            anchorsRoundedFor = authority
+        }
+    }
+
+    /** True when every stale row was rounded. */
+    private fun roundAll(authority: String): Boolean {
         val columns = listOf(Tasks.DTSTART, Tasks.DUE)
-        val stale = resolver.query(
+        val ops = resolver.query(
             TasksContract.tasksUri(authority),
             arrayOf(Tasks.ID, Tasks.ACCOUNT_NAME, Tasks.ACCOUNT_TYPE) + columns,
             columns.joinToString(" OR ") { "$it % 1000 != 0" },
@@ -487,7 +528,7 @@ class AndroidTasksDataSource @Inject constructor(
             null,
         )?.use { c ->
             val r = CursorColumnReader(c)
-            buildList {
+            buildList<ContentProviderOperation> {
                 while (c.moveToNext()) {
                     val id = r.getLong(Tasks.ID) ?: continue
                     val values = buildMap<String, Any?> {
@@ -496,17 +537,20 @@ class AndroidTasksDataSource @Inject constructor(
                             if (providerMillis(millis) != millis) put(column, providerMillis(millis))
                         }
                     }
-                    val uri = TasksContract.asSyncAdapter(
-                        taskUri(authority, id),
-                        r.getString(Tasks.ACCOUNT_NAME).orEmpty(),
-                        r.getString(Tasks.ACCOUNT_TYPE).orEmpty(),
-                    )
-                    add(uri to values)
+                    val name = r.getString(Tasks.ACCOUNT_NAME)
+                    val type = r.getString(Tasks.ACCOUNT_TYPE)
+                    val uri = if (name.isNullOrEmpty() || type.isNullOrEmpty()) taskUri(authority, id)
+                    else TasksContract.asSyncAdapter(taskUri(authority, id), name, type)
+                    add(ContentProviderOperation.newUpdate(uri).withValues(values.toContentValues()).build())
                 }
             }
-        }.orEmpty()
-        stale.forEach { (uri, values) -> resolver.update(uri, values.toContentValues(), null, null) }
-        prefs.edit().putBoolean(key, true).apply()
+        } ?: return false
+        if (ops.isEmpty()) return true
+        val batch = runCatching { resolver.applyBatch(authority, ArrayList(ops)) }.getOrNull()
+        if (batch != null) return batch.all { (it.count ?: 0) > 0 }
+        // One refused row fails the whole batch; row by row, the rest still go.
+        return ops.map { op -> runCatching { resolver.applyBatch(authority, arrayListOf(op)).single().count ?: 0 }.getOrDefault(0) > 0 }
+            .all { it }
     }
 
     override fun setReminders(taskId: Long, reminders: List<TaskReminder>) {
@@ -751,5 +795,10 @@ private fun sameSecond(a: Instant, b: Instant): Boolean =
 
 private const val MIGRATIONS = "provider_migrations"
 
+private const val TAG = "AndroidTasksDataSource"
+
+/** No rule to look up: the provider says the row does not recur. */
+private const val NO_RULE = -1L
+
 /** SQLite's default bound-argument limit on older Android releases. */
 private const val MAX_SQL_ARGS = 999

app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt

@@ -124,7 +124,8 @@ interface TasksDataSource {
      *
      * Implementations fall back to [setCompleted] when the row turns out not to
      * be a series master — an override, or a plain task the caller happened to
-     * hand an anchor for — so the routing above cannot get this wrong.
+     * hand an anchor for. A series whose occurrence cannot be found fails with
+     * [TaskWriteFailedException] instead: the plain write would close all of it.
      */
     fun setCompletedInstance(taskId: Long, occurrenceStart: Instant, completed: Boolean)
 

app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriter.kt

@@ -192,7 +192,8 @@ class ProviderWriter internal constructor(
         val current = database.tasks().entity(taskId)?.takeUnless { it.isDeleted } ?: return false
         requireWritableList(current.listId)
         // An override deletes itself into an EXDATE on its master.
-        if (current.masterId != null) store.deleteTask(taskId) else store.deleteWithSubtasks(taskId)
+        val occurrence = current.recurrenceId
+        if (current.masterId != null && occurrence != null) store.deleteOverride(taskId, occurrence) else store.deleteWithSubtasks(taskId)
         return true
     }