Widget: fix "complete works once, then stops" race between overlapping completions

Root cause traced from server logs, not guessed: every completion request
was succeeding server-side (100% 200s, including a 5-tap burst spanning
different tasks), and each successful completion's response payload was
correctly shrinking. So the failure wasn't dispatch or network -- it was
that a successful completion could still get silently undone client-side.

fetchAndPersist does an unconditional full overwrite of the cached item
list on every successful GET. CompleteWorker/DeferWorker run one instance
per task id with no ordering guarantee between different ids' workers
(different unique work names, no KEEP protection across them -- that
protection only ever covered same-task double-taps). So: 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; A's slower GET
response, captured before B's completion landed, then overwrites the
cache and silently resurrects B.

Fix: track locally-optimistic removals with a timestamp (PendingRemovals.kt)
and filter them out of every fetchAndPersist write for a bounded TTL (2
min), regardless of which worker's fetch is doing the writing. The TTL
means a completion that never actually confirms (permanent network
failure) still self-heals via the next periodic refresh, matching an
existing self-healing property already relied on elsewhere in this
codebase, instead of hiding the task forever.

Added PendingRemovalsTest.kt (pure-function unit tests, no Android
runtime needed) covering the exact race scenario plus TTL expiry and
edge cases. Verified the tests actually catch a regression by deliberately
reverting the fix to a no-op against a real backup, confirming 3 tests
failed with the exact expected assertion, then restoring and confirming
green again.

Built, tested, and published as doot-widget.apk.

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"))
+    }
+}