diff options
Diffstat (limited to 'android/app/src')
6 files changed, 211 insertions, 4 deletions
diff --git a/android/app/src/main/java/org/terst/doot/widget/data/DataStore.kt b/android/app/src/main/java/org/terst/doot/widget/data/DataStore.kt index 23ee566..4b17687 100644 --- a/android/app/src/main/java/org/terst/doot/widget/data/DataStore.kt +++ b/android/app/src/main/java/org/terst/doot/widget/data/DataStore.kt @@ -19,4 +19,5 @@ object Keys { val IS_REFRESHING = booleanPreferencesKey("is_refreshing") val TEXT_SIZE = stringPreferencesKey("text_size") val BUDGET_STATUS_JSON = stringPreferencesKey("budget_status_json") + val PENDING_REMOVALS_JSON = stringPreferencesKey("pending_removals_json") } diff --git a/android/app/src/main/java/org/terst/doot/widget/data/PendingRemovals.kt b/android/app/src/main/java/org/terst/doot/widget/data/PendingRemovals.kt new file mode 100644 index 0000000..31e85cf --- /dev/null +++ b/android/app/src/main/java/org/terst/doot/widget/data/PendingRemovals.kt @@ -0,0 +1,51 @@ +package org.terst.doot.widget.data + +import kotlinx.serialization.decodeFromString +import kotlinx.serialization.encodeToString +import kotlinx.serialization.json.Json + +private val json = Json { ignoreUnknownKeys = true } + +// How long a locally-optimistic removal is trusted over a possibly-stale +// server fetch before it's allowed to expire. Bounded, not permanent: if a +// completion never actually confirms (e.g. the device goes offline and +// CompleteWorker/DeferWorker retry forever), the item must still eventually +// reappear rather than staying hidden forever -- the existing periodic +// RefreshWorker tick already relies on exactly this kind of self-healing +// (see the "Widget periodic-refresh self-healing fix" worklog entry), and a +// non-expiring pending-removal would silently break that property. +const val PENDING_REMOVAL_TTL_MS = 2 * 60 * 1000L + +fun pendingKey(id: String, source: String) = "$id|$source" + +fun decodePendingRemovals(raw: String?): Map<String, Long> = + raw?.let { runCatching { json.decodeFromString<Map<String, Long>>(it) }.getOrNull() } ?: emptyMap() + +fun encodePendingRemovals(pending: Map<String, Long>): String = json.encodeToString(pending) + +fun Map<String, Long>.pruneExpired(now: Long): Map<String, Long> = + filterValues { now - it < PENDING_REMOVAL_TTL_MS } + +/** + * Excludes items whose (id, source) has an unexpired pending local removal. + * + * This exists to close a race in fetchAndPersist: it does a full overwrite + * of the cached item list on every successful fetch, and CompleteWorker / + * DeferWorker run one instance per task with no ordering guarantee between + * different tasks' workers. Tapping complete on task A starts a GET that's + * still in flight; tapping complete on task B before A's GET returns + * optimistically removes B locally -- but A's GET response was captured + * before B's completion landed server-side, so if A's slower write lands + * after B's, it would silently resurrect B. This is the root cause of the + * 2026-08-05 report: "check to complete works once, then doesn't work + * anymore" -- it wasn't that later taps failed (server logs confirmed every + * completion request succeeded), it's that a slower, earlier-started fetch + * kept clobbering faster, later optimistic removals. + */ +fun List<WidgetItem>.excludingUnexpiredPendingRemovals(pending: Map<String, Long>, now: Long): List<WidgetItem> { + if (pending.isEmpty()) return this + return filterNot { item -> + val removedAt = pending[pendingKey(item.id, item.source)] ?: return@filterNot false + now - removedAt < PENDING_REMOVAL_TTL_MS + } +} diff --git a/android/app/src/main/java/org/terst/doot/widget/data/WidgetRepository.kt b/android/app/src/main/java/org/terst/doot/widget/data/WidgetRepository.kt index d630247..c8936e8 100644 --- a/android/app/src/main/java/org/terst/doot/widget/data/WidgetRepository.kt +++ b/android/app/src/main/java/org/terst/doot/widget/data/WidgetRepository.kt @@ -26,12 +26,31 @@ private data class WidgetAddResponse(val id: String) /** * Optimistically drops an item from the cached list so the widget can update instantly on * completion, ahead of the authoritative refresh a worker performs once the API call succeeds. + * + * Also records the removal in PENDING_REMOVALS_JSON so a slower, already-in-flight fetch + * (from another concurrent completion, or a periodic refresh) can't silently resurrect this + * item by overwriting ITEMS_JSON with a stale snapshot captured before this removal happened + * -- see PendingRemovals.kt for the full mechanism and the incident this fixes. */ suspend fun removeWidgetItemLocally(context: Context, id: String, source: String) { context.dataStore.edit { prefs -> val raw = prefs[Keys.ITEMS_JSON] ?: return@edit val items = runCatching { json.decodeFromString<List<WidgetItem>>(raw) }.getOrNull() ?: return@edit prefs[Keys.ITEMS_JSON] = json.encodeToString(items.filterNot { it.id == id && it.source == source }) + + val now = System.currentTimeMillis() + val pending = decodePendingRemovals(prefs[Keys.PENDING_REMOVALS_JSON]).pruneExpired(now) + prefs[Keys.PENDING_REMOVALS_JSON] = encodePendingRemovals(pending + (pendingKey(id, source) to now)) + } +} + +/** Clears a pending removal once its own completion has been confirmed server-side by a + * fetch known to have started after it landed -- tightens the race window below, but the + * TTL in PendingRemovals.kt is what actually guarantees it can't get stuck forever. */ +suspend fun clearPendingRemoval(context: Context, id: String, source: String) { + context.dataStore.edit { prefs -> + val pending = decodePendingRemovals(prefs[Keys.PENDING_REMOVALS_JSON]).pruneExpired(System.currentTimeMillis()) + prefs[Keys.PENDING_REMOVALS_JSON] = encodePendingRemovals(pending - pendingKey(id, source)) } } @@ -58,7 +77,9 @@ class WidgetRepository( suspend fun fetchAndPersist(context: Context): Result<WidgetResponse> { return fetchRaw().onSuccess { resp -> context.dataStore.edit { prefs -> - prefs[Keys.ITEMS_JSON] = json.encodeToString(resp.items) + val now = System.currentTimeMillis() + val pending = decodePendingRemovals(prefs[Keys.PENDING_REMOVALS_JSON]).pruneExpired(now) + prefs[Keys.ITEMS_JSON] = json.encodeToString(resp.items.excludingUnexpiredPendingRemovals(pending, now)) prefs[Keys.NOW] = resp.now prefs[Keys.LAST_UPDATED] = System.currentTimeMillis() if (resp.budgetStatus != null) { diff --git a/android/app/src/main/java/org/terst/doot/widget/work/CompleteWorker.kt b/android/app/src/main/java/org/terst/doot/widget/work/CompleteWorker.kt index 51ad727..e6b6069 100644 --- a/android/app/src/main/java/org/terst/doot/widget/work/CompleteWorker.kt +++ b/android/app/src/main/java/org/terst/doot/widget/work/CompleteWorker.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.flow.first import org.terst.doot.widget.ui.DootWidget import org.terst.doot.widget.data.Keys import org.terst.doot.widget.data.WidgetRepository +import org.terst.doot.widget.data.clearPendingRemoval import org.terst.doot.widget.data.dataStore class CompleteWorker(context: Context, params: WorkerParameters) : @@ -24,6 +25,11 @@ class CompleteWorker(context: Context, params: WorkerParameters) : return repo.complete(applicationContext, id, source).fold( onSuccess = { repo.fetchAndPersist(applicationContext) + // Confirmed server-side and reflected by our own fetch above -- safe to + // stop protecting this id from being overwritten by another in-flight + // fetch now (PendingRemovals.kt's TTL is the actual backstop if this + // never runs, e.g. process death between complete() and here). + clearPendingRemoval(applicationContext, id, source) DootWidget().updateAll(applicationContext) Result.success() }, @@ -48,8 +54,22 @@ class CompleteWorker(context: Context, params: WorkerParameters) : // already has a completion in flight is simply dropped -- the first // request's result (including its fetchAndPersist/updateAll) is what // takes effect, with no race between two workers touching the same - // task. Different task ids still run independently/concurrently, - // which is fine since they don't share a row. + // task. + // + // Different task ids still run independently/concurrently by design + // (they don't share a row, no reason to serialize them) -- but that + // meant the identical race this comment describes for same-task + // double-taps was wide open ACROSS different tasks too: completing + // task A starts a fetch that's still in flight; completing task B + // (different id, different unique work name, no KEEP protection + // between them) optimistically removes B locally, and if A's slower + // fetch (captured before B's completion landed) writes after B's + // removal, it silently resurrects B. This was the actual root cause + // of the 2026-08-05 "works once, then doesn't work anymore" report + // -- server logs showed every tap's completion request succeeding, + // it just kept getting undone by a slower sibling. See + // PendingRemovals.kt / WidgetRepository.fetchAndPersist for the fix, + // which covers both the same-task and cross-task cases uniformly. fun enqueue(context: Context, id: String, source: String) { val data = workDataOf(KEY_ID to id, KEY_SOURCE to source) val request = OneTimeWorkRequestBuilder<CompleteWorker>() diff --git a/android/app/src/main/java/org/terst/doot/widget/work/DeferWorker.kt b/android/app/src/main/java/org/terst/doot/widget/work/DeferWorker.kt index 03ed1ab..5df5254 100644 --- a/android/app/src/main/java/org/terst/doot/widget/work/DeferWorker.kt +++ b/android/app/src/main/java/org/terst/doot/widget/work/DeferWorker.kt @@ -7,10 +7,15 @@ import kotlinx.coroutines.flow.first import org.terst.doot.widget.ui.DootWidget import org.terst.doot.widget.data.Keys import org.terst.doot.widget.data.WidgetRepository +import org.terst.doot.widget.data.clearPendingRemoval import org.terst.doot.widget.data.dataStore // Mirrors CompleteWorker's shape -- see that file for the enqueueUniqueWork -// rationale (same rapid-double-tap race applies here). +// rationale (same rapid-double-tap race applies here) and PendingRemovals.kt +// for the cross-task race both workers are equally exposed to (fixed +// 2026-08-05: fetchAndPersist's full-overwrite could resurrect an item this +// worker just optimistically removed if a slower sibling's fetch, from a +// different task's worker, landed after this one's). class DeferWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) { @@ -25,6 +30,8 @@ class DeferWorker(context: Context, params: WorkerParameters) : return repo.defer(id).fold( onSuccess = { repo.fetchAndPersist(applicationContext) + // "doot" is the only source DeferWorker is ever used for (see Actions.kt). + clearPendingRemoval(applicationContext, id, "doot") DootWidget().updateAll(applicationContext) Result.success() }, diff --git a/android/app/src/test/java/org/terst/doot/widget/PendingRemovalsTest.kt b/android/app/src/test/java/org/terst/doot/widget/PendingRemovalsTest.kt new file mode 100644 index 0000000..415bfd1 --- /dev/null +++ b/android/app/src/test/java/org/terst/doot/widget/PendingRemovalsTest.kt @@ -0,0 +1,107 @@ +package org.terst.doot.widget + +import org.junit.Assert.* +import org.junit.Test +import org.terst.doot.widget.data.PENDING_REMOVAL_TTL_MS +import org.terst.doot.widget.data.WidgetItem +import org.terst.doot.widget.data.decodePendingRemovals +import org.terst.doot.widget.data.encodePendingRemovals +import org.terst.doot.widget.data.excludingUnexpiredPendingRemovals +import org.terst.doot.widget.data.pendingKey +import org.terst.doot.widget.data.pruneExpired + +class PendingRemovalsTest { + + private fun taskItem(id: String, source: String = "doot") = + WidgetItem(id = id, title = "Task $id", source = source, type = "task") + + @Test + fun `excludingUnexpiredPendingRemovals filters out a pending item`() { + val items = listOf(taskItem("a"), taskItem("b")) + val now = 1_000_000L + val pending = mapOf(pendingKey("b", "doot") to now) + + val result = items.excludingUnexpiredPendingRemovals(pending, now) + + assertEquals(listOf("a"), result.map { it.id }) + } + + @Test + fun `excludingUnexpiredPendingRemovals is a no-op with an empty pending map`() { + val items = listOf(taskItem("a"), taskItem("b")) + assertEquals(items, items.excludingUnexpiredPendingRemovals(emptyMap(), 1_000_000L)) + } + + // This is the actual regression: a slower, earlier-started fetch's response still + // includes an item that a faster, later-started completion has since removed. Without + // pending-removal protection, persisting that stale snapshot resurrects the item -- + // this is what made "check to complete" look like it stopped working after the first + // tap (2026-08-05), even though every completion request was independently succeeding + // server-side. + @Test + fun `a stale fetch snapshot captured before a completion cannot resurrect it`() { + val staleSnapshotStillContainingB = listOf(taskItem("a"), taskItem("b")) + val completedAt = 1_000_000L + val pendingAfterCompletingB = mapOf(pendingKey("b", "doot") to completedAt) + + // The stale fetch's write happens *after* b was completed, wall-clock, even though + // its data was captured before -- exactly the race: slower fetch, later write. + val persisted = staleSnapshotStillContainingB + .excludingUnexpiredPendingRemovals(pendingAfterCompletingB, completedAt + 500) + + assertEquals(listOf("a"), persisted.map { it.id }) + } + + @Test + fun `a pending removal expires and stops protecting after the TTL`() { + val items = listOf(taskItem("a"), taskItem("b")) + val removedAt = 1_000_000L + val pending = mapOf(pendingKey("b", "doot") to removedAt) + + // Just before expiry: still protected. + val stillProtected = items.excludingUnexpiredPendingRemovals(pending, removedAt + PENDING_REMOVAL_TTL_MS - 1) + assertEquals(listOf("a"), stillProtected.map { it.id }) + + // At/after expiry: self-healing kicks in -- a permanently-failed completion must + // not hide its task forever, matching the existing periodic-refresh self-healing + // property elsewhere in this codebase. + val expired = items.excludingUnexpiredPendingRemovals(pending, removedAt + PENDING_REMOVAL_TTL_MS) + assertEquals(listOf("a", "b"), expired.map { it.id }) + } + + @Test + fun `different source with the same id is not confused for the pending item`() { + val items = listOf(taskItem("shared-id", source = "trello")) + val pending = mapOf(pendingKey("shared-id", "doot") to 1_000_000L) + + val result = items.excludingUnexpiredPendingRemovals(pending, 1_000_000L) + + assertEquals(listOf("shared-id"), result.map { it.id }) + } + + @Test + fun `pruneExpired drops old entries and keeps fresh ones`() { + val now = 1_000_000L + val pending = mapOf( + "fresh" to now, + "stale" to now - PENDING_REMOVAL_TTL_MS + ) + + val pruned = pending.pruneExpired(now) + + assertEquals(setOf("fresh"), pruned.keys) + } + + @Test + fun `encode and decode round-trip`() { + val pending = mapOf(pendingKey("a", "doot") to 42L) + val decoded = decodePendingRemovals(encodePendingRemovals(pending)) + assertEquals(pending, decoded) + } + + @Test + fun `decodePendingRemovals treats null and malformed input as empty`() { + assertEquals(emptyMap<String, Long>(), decodePendingRemovals(null)) + assertEquals(emptyMap<String, Long>(), decodePendingRemovals("not json")) + } +} |