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. --- .../org/terst/doot/widget/PendingRemovalsTest.kt | 107 +++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 android/app/src/test/java/org/terst/doot/widget/PendingRemovalsTest.kt (limited to 'android/app/src/test/java/org/terst/doot/widget') 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(), decodePendingRemovals(null)) + assertEquals(emptyMap(), decodePendingRemovals("not json")) + } +} -- cgit v1.2.3