From 7b9eb14db9b953b3c5882b3b85c819d9baa2dd2c Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 5 Aug 2026 00:36:18 +0000 Subject: 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. --- .../java/org/terst/doot/widget/data/DataStore.kt | 1 + .../org/terst/doot/widget/data/PendingRemovals.kt | 51 ++++++++++++++++++++++ .../org/terst/doot/widget/data/WidgetRepository.kt | 23 +++++++++- .../org/terst/doot/widget/work/CompleteWorker.kt | 24 +++++++++- .../java/org/terst/doot/widget/work/DeferWorker.kt | 9 +++- 5 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 android/app/src/main/java/org/terst/doot/widget/data/PendingRemovals.kt (limited to 'android/app/src/main') 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 = + raw?.let { runCatching { json.decodeFromString>(it) }.getOrNull() } ?: emptyMap() + +fun encodePendingRemovals(pending: Map): String = json.encodeToString(pending) + +fun Map.pruneExpired(now: Long): Map = + 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.excludingUnexpiredPendingRemovals(pending: Map, now: Long): List { + 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>(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 { 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() 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() }, -- cgit v1.2.3