fix(reports): finish a submission even when its caller is cancelled

Lookup submits from a composition scope, so leaving the screen cancels the
submission. The send wraps a blocking OkHttp call, which runs to the end, and
the cancellation then threw away its 200: the report stayed in the outbox and
went out again two minutes later. The deployed Worker has no id dedup, so it
stores that twice. Cancelled just after the claim, a report was marked sent
for a day with nothing queued.

CommunityReportSubmitter.submit() now runs claim, enqueue, send and dequeue
under NonCancellable. A send torn down by its own transport with no queued
copy gives the claim back, since nothing will deliver it. The report client
gains a 60 s call timeout, so the direct attempt, now uncancellable, always
ends inside the outbox's two-minute hold.

The CHANGELOG bullets that described the Worker as deployed with its
duplicate marker and id support now say the id is used by the updated Worker,
which isn't deployed yet.

Tests: CommunityReportSubmitterTest gains a report delivered after its
caller was cancelled (dequeued), a claim whose caller was cancelled (still
queued and sent), a torn-down send with no queue (claim released), and the
call timeout staying under the outbox hold. Three mutants are each killed by
name.

Author: SysAdminDoc

CHANGELOG.md

@@ -165,12 +165,14 @@ All notable changes to CallShield will be documented in this file.
   as a robocall counts as one vote a day, not two. A report the server turns
   down, or one the outbox gives up on, can be made again the same day. Lookup
   now says when a report was queued or already made today instead of always
-  saying it was sent. Each report carries an id, so a resend can be told apart
-  from a second report.
+  saying it was sent. Leaving the screen mid-send doesn't send a report twice
+  either, since one that got through takes its queued copy back out. Each
+  report now carries an id, which the updated Worker below uses to tell a
+  resend from a second report.
 - The app treats the Worker's "already submitted" answer as delivered only when
-  the Worker says it stored the report. The deployed Worker writes that marker
-  before storing, so its bare answer can mean the report was lost, and the app
-  now tries again later instead of calling it sent.
+  the Worker says it stored the report. A bare "already submitted" from an
+  older Worker can mean the report was never stored, so the app now tries again
+  later instead of calling it sent.
 - The app's own unit tests no longer send reports. One of them had posted two
   fictional numbers, +1 555-123-4567 and +1 555-987-6543, to the report service
   on every run since July, and each landed in this repository's report queue.

app/src/main/java/com/sysadmindoc/callshield/data/CommunityContributor.kt

@@ -47,13 +47,20 @@ object CommunityContributor {
     private val reportDomainPattern = Regex("^[a-z0-9](?:[a-z0-9.-]{0,251}[a-z0-9])?$")
     private val urlIndicatorPattern = Regex("^[a-z_]{3,40}$")
 
-    private val client =
+    // The call timeout bounds the whole attempt (DNS, every route tried, the
+    // write, OkHttp's own retry), so it always ends before the outbox's queued
+    // copy is due to go out, and a submission that can't be cancelled can't
+    // hang either.
+    internal val client =
         HttpClient.shared
             .newBuilder()
             .connectTimeout(10, TimeUnit.SECONDS)
             .readTimeout(10, TimeUnit.SECONDS)
+            .callTimeout(REPORT_CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS)
             .build()
 
+    internal const val REPORT_CALL_TIMEOUT_SECONDS = 60L
+
     /**
      * Typed outcome so the UI can localize and color-code without sniffing
      * English substrings out of [ContributeResult.message] (which is

app/src/main/java/com/sysadmindoc/callshield/data/CommunityReportSubmitter.kt

@@ -27,10 +27,17 @@ internal data class CommunityReport(
  *
  * The claim comes first, so a second tap while the first is in flight is
  * suppressed too. The report goes into the outbox before the direct attempt:
- * if the caller is cancelled or the process dies mid-send, the queued copy
- * still goes out, carrying the same id. A delivered report, or one refused
- * for good, takes its queued copy back out, and a refusal also releases the
- * claim so the user isn't told it was already sent.
+ * if the process dies mid-send, the queued copy still goes out, carrying the
+ * same id. A delivered report, or one refused for good, takes its queued copy
+ * back out, and a refusal also releases the claim so the user isn't told it
+ * was already sent.
+ *
+ * Once started, a submission runs to the end even if its caller is cancelled,
+ * the way a user leaving Lookup mid-send cancels it. Cancelled after the send,
+ * a delivered report would keep its queued copy, and the outbox would send it
+ * again, which a Worker without id dedup stores twice. Cancelled after the
+ * claim, the report would be marked sent for a day with nothing queued. Every
+ * step is bounded, the send by the report client's call timeout.
  */
 internal class CommunityReportSubmitter(
     private val claim: suspend (number: String, vote: String, now: Long) -> Boolean,
@@ -41,11 +48,17 @@ internal class CommunityReportSubmitter(
     private val clock: () -> Long = System::currentTimeMillis,
     private val newId: () -> String = { UUID.randomUUID().toString() },
 ) {
-    @Suppress("TooGenericExceptionCaught")
     suspend fun submit(
         number: String,
         type: String,
         indicators: SmsReportIndicators?,
+    ): ContributeResult = withContext(NonCancellable) { submitToTheEnd(number, type, indicators) }
+
+    @Suppress("TooGenericExceptionCaught")
+    private suspend fun submitToTheEnd(
+        number: String,
+        type: String,
+        indicators: SmsReportIndicators?,
     ): ContributeResult {
         val normalized =
             CommunityContributor.normalizeForReport(number)
@@ -61,26 +74,29 @@ internal class CommunityReportSubmitter(
             try {
                 enqueue(report)
                 true
-            } catch (e: CancellationException) {
-                withContext(NonCancellable) { release(normalized, vote) }
-                throw e
             } catch (_: Exception) {
                 // WorkManager couldn't take it. The direct attempt below still
                 // runs; it just has no safety net.
                 false
             }
-        val result = transport(report)
+        val result =
+            try {
+                transport(report)
+            } catch (e: CancellationException) {
+                // The send itself was torn down. The queued copy goes out
+                // later with the same id, so the claim stands.
+                if (!queued) release(normalized, vote)
+                throw e
+            }
         return when {
             result.success -> {
-                withContext(NonCancellable) { if (queued) dequeue(report) }
+                if (queued) dequeue(report)
                 result
             }
 
             !result.outcome.isTransient || !queued -> {
-                withContext(NonCancellable) {
-                    if (queued) dequeue(report)
-                    release(normalized, vote)
-                }
+                if (queued) dequeue(report)
+                release(normalized, vote)
                 result
             }
 

app/src/main/java/com/sysadmindoc/callshield/service/CommunityReportWorker.kt

@@ -75,7 +75,7 @@ class CommunityReportWorker internal constructor(
         private const val KEY_SMS_URL_INDICATORS = "sms_url_indicators"
         private const val BACKOFF_MINUTES = 5L
 
-        /** Longer than the direct attempt's connect and read timeouts together. */
+        /** Longer than the direct attempt's whole-call timeout, so the two never overlap. */
         internal const val QUEUED_DELAY_MINUTES = 2L
 
         /** Attempts that ran with a network; waiting offline doesn't use them up. */

app/src/test/java/com/sysadmindoc/callshield/data/CommunityReportSubmitterTest.kt

@@ -5,11 +5,14 @@ import androidx.datastore.preferences.core.PreferenceDataStoreFactory
 import com.sysadmindoc.callshield.data.CommunityContributor.ContributeOutcome
 import com.sysadmindoc.callshield.data.CommunityContributor.ContributeResult
 import com.sysadmindoc.callshield.data.repository.SettingsRepository
+import com.sysadmindoc.callshield.service.CommunityReportWorker
 import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.cancelAndJoin
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
 import org.junit.After
 import org.junit.Assert.assertEquals
@@ -185,6 +188,104 @@ class CommunityReportSubmitterTest {
         assertEquals(ContributeOutcome.ALREADY_SUBMITTED, store.submit("+12122340101").outcome)
     }
 
+    @Test
+    fun `a report delivered after its caller was cancelled still takes its queued copy back out`() {
+        // Leaving Lookup mid-send used to throw the Worker's 200 away, and the
+        // outbox then sent the report again two minutes later.
+        val store = store()
+        val sending = CompletableDeferred<Unit>()
+        val answered = CompletableDeferred<Unit>()
+        val submitter =
+            gatedSubmitter(store, transport = { report ->
+                sent += report
+                sending.complete(Unit)
+                answered.await()
+                ContributeResult(true, "stored", ContributeOutcome.REPORTED_SPAM)
+            })
+
+        runBlocking {
+            val job = launch(Dispatchers.Default) { submitter.submit("+12122340101", "spam", null) }
+            sending.await()
+            job.cancel()
+            answered.complete(Unit)
+            job.join()
+        }
+
+        assertEquals(listOf("report-1"), queued.map { it.id })
+        assertEquals(listOf("report-1"), dequeued.map { it.id })
+    }
+
+    @Test
+    fun `a claim is never left without its queued copy when the caller is cancelled`() {
+        val store = store()
+        val claimed = CompletableDeferred<Unit>()
+        val resume = CompletableDeferred<Unit>()
+        val submitter =
+            gatedSubmitter(store, claim = { number, vote, time ->
+                store.settings.claimCommunityReport(number, vote, time).also {
+                    claimed.complete(Unit)
+                    resume.await()
+                }
+            })
+
+        runBlocking {
+            val job = launch(Dispatchers.Default) { submitter.submit("+12122340101", "spam", null) }
+            claimed.await()
+            job.cancel()
+            resume.complete(Unit)
+            job.join()
+        }
+
+        assertEquals(listOf("report-1"), queued.map { it.id })
+        assertEquals(listOf("report-1"), sent.map { it.id })
+    }
+
+    @Test
+    fun `a send torn down with no queued copy gives the claim back`() {
+        val store = store()
+        cancelSend = true
+        val submitter = gatedSubmitter(store, enqueue = { error("WorkManager is unavailable") })
+
+        runBlocking {
+            try {
+                submitter.submit("+12122340101", "spam", null)
+                fail("the cancellation should reach the caller")
+            } catch (_: CancellationException) {
+                // Nothing will deliver it now, so it mustn't read as sent.
+            }
+        }
+
+        cancelSend = false
+        assertEquals(ContributeOutcome.REPORTED_SPAM, store.submit("+12122340101").outcome)
+    }
+
+    @Test
+    fun `the direct attempt always ends before the queued copy is due`() {
+        val callTimeout = TimeUnit.SECONDS.toMillis(CommunityContributor.REPORT_CALL_TIMEOUT_SECONDS)
+
+        assertEquals(callTimeout.toInt(), CommunityContributor.client.callTimeoutMillis)
+        assertTrue(callTimeout < TimeUnit.MINUTES.toMillis(CommunityReportWorker.QUEUED_DELAY_MINUTES))
+    }
+
+    private fun gatedSubmitter(
+        store: Store,
+        claim: suspend (String, String, Long) -> Boolean = store.settings::claimCommunityReport,
+        enqueue: suspend (CommunityReport) -> Unit = { report -> queued += report },
+        transport: suspend (CommunityReport) -> ContributeResult = { report ->
+            sent += report
+            if (cancelSend) throw CancellationException("the send was torn down")
+            ContributeResult(true, "stored", ContributeOutcome.REPORTED_SPAM)
+        },
+    ) = CommunityReportSubmitter(
+        claim = claim,
+        release = store.settings::releaseCommunityReport,
+        transport = transport,
+        enqueue = enqueue,
+        dequeue = { report -> dequeued += report },
+        clock = { now },
+        newId = { "report-${++nextId}" },
+    )
+
     @Test
     fun `a refused report leaves nothing queued and can be made again`() {
         val store = store()