fix(upgrade): Invalidate a dead billing connection from refreshStrict too

refreshStrict() throws its incomplete-result error AFTER useConnection already returned: refreshPurchases hands back a partial result instead of throwing, so useConnection's dead-binder detection never sees it. A gate that ran against a connection whose binder died mid-query (partialError cause chain carrying SERVICE_DISCONNECTED / SERVICE_TIMEOUT) left that connection installed, and every later purchase check kept talking to the corpse until something else tore it down. processReconciliation() already compensated for the refresh()/connect-loop paths; the strict path had no such call. Extract that dead-connection block into invalidateOnDeadConnection() and call it from both. The strict path deliberately does NOT feed the episode clock — a gate the user aborted mid-purchase is not a reconciliation outcome.
2026-09-14 18:26:11 -04:00 · 2026-08-16 17:44:02 +02:00
parent a357d77b00
commit 563c6e71f7
2 changed files with 69 additions and 11 deletions
@@ -418,17 +418,12 @@ class BillingManager @Inject constructor(
        }
    }

-    // Everything a COMPLETED refresh owes the rest of the app, in one place: both the connect loop's
-    // initial refresh and manual refresh() calls run through it, so a Restore tap during an outage
-    // feeds the same bookkeeping the connect loop does. Only reached when refreshPurchases returned
-    // (it still throws when it found nothing AND a query failed — that path is the connect loop's /
-    // useConnection's).
-    private fun processReconciliation(refresh: BillingConnection.PurchaseRefresh) {
-        // A partial refresh no longer reaches useConnection's dead-binder detection (it returns
-        // instead of throwing), so the teardown that used to ride the throw path happens here.
-        // Cause chain, not the exception itself: the failure arrives user-friendly-mapped.
-        // Deliberately no holder CAS: the failing connection may already have been replaced, and
-        // the accepted cost of that rare race is one extra failed action while the loop reconnects.
+    // A partial refresh no longer reaches useConnection's dead-binder detection (it returns instead
+    // of throwing), so the teardown that used to ride the throw path happens here. Cause chain, not
+    // the exception itself: the failure arrives user-friendly-mapped. Deliberately no holder CAS:
+    // the failing connection may already have been replaced, and the accepted cost of that rare
+    // race is one extra failed action while the loop reconnects.
+    private fun invalidateOnDeadConnection(refresh: BillingConnection.PurchaseRefresh) {
        val clientError = refresh.partialError?.let {
            (it as? BillingClientException) ?: (it.cause as? BillingClientException)
        }
@@ -436,6 +431,15 @@ class BillingManager @Inject constructor(
            log(TAG, WARN) { "Refresh reported the connection dead (${clientError.result.responseCode}), invalidating." }
            invalidations.trySend(Unit)
        }
+    }
+
+    // Everything a COMPLETED refresh owes the rest of the app, in one place: both the connect loop's
+    // initial refresh and manual refresh() calls run through it, so a Restore tap during an outage
+    // feeds the same bookkeeping the connect loop does. Only reached when refreshPurchases returned
+    // (it still throws when it found nothing AND a query failed — that path is the connect loop's /
+    // useConnection's).
+    private fun processReconciliation(refresh: BillingConnection.PurchaseRefresh) {
+        invalidateOnDeadConnection(refresh)

        if (!refresh.isComplete && !refresh.hasConfirmedProPurchase) {
            // A reconciliation that couldn't confirm Pro. Stamped with the refresh's COMMIT time,
@@ -535,6 +539,11 @@ class BillingManager @Inject constructor(
    suspend fun refreshStrict(): BillingData {
        log(TAG) { "refreshStrict()" }
        val fresh = useConnection { refreshPurchases() }
+        // A gate that hit a dying connection must still trigger the reconnect (the throw below
+        // bypasses useConnection's detection, which already returned), or every later purchase
+        // check keeps reusing the corpse. No-op on a complete refresh. The episode clock stays out
+        // of this path: an aborted gate is not a reconciliation outcome.
+        invalidateOnDeadConnection(fresh)
        if (!fresh.isComplete) {
            // partialError is set for every incomplete refresh; the fallback only exists so a
            // future incompleteness without a captured cause still fails closed instead of passing.
@@ -14,6 +14,7 @@ import eu.darken.capod.common.upgrade.core.billing.client.BillingClientException
 import eu.darken.capod.common.upgrade.core.billing.client.BillingConnection
 import eu.darken.capod.common.upgrade.core.billing.client.BillingConnectionProvider
 import io.kotest.assertions.throwables.shouldThrow
+import io.kotest.matchers.collections.shouldNotContain
 import io.kotest.matchers.longs.shouldBeLessThan
 import io.kotest.matchers.shouldBe
 import io.mockk.coEvery
@@ -730,6 +731,54 @@ class BillingManagerTest : BaseTest() {
        failures shouldBe emptyList()
    }

+    @Test fun `a strict gate failure on a dead connection still tears the connection down`() = runTest2 {
+        // The gate's throw happens after useConnection already returned, so its dead-binder
+        // detection can't fire — without the explicit invalidation connection 1 stays installed and
+        // every later purchase check runs against the corpse. Feeding the episode clock still stays
+        // off this path.
+        val owned = purchase()
+        val dead = connection(
+            refreshes = listOf(
+                completeRefresh(),
+                partialRefresh(
+                    occurredAt = 4242L,
+                    error = GplayServiceUnavailableException(
+                        BillingClientException(result(BillingResponseCode.SERVICE_DISCONNECTED))
+                    ),
+                ),
+            ),
+        )
+        val good = connection(refreshResults = listOf(emptyList(), listOf(owned)))
+        var attempts = 0
+        val provider = mockk<BillingConnectionProvider>().apply {
+            every { this@apply.connection } returns flow {
+                attempts++
+                emit(if (attempts == 1) dead else good)
+                awaitCancellation()
+            }
+        }
+        val manager = manager(provider)
+        val failures = collectFailures(manager)
+        runCurrent() // connection 1 established
+
+        shouldThrow<Exception> { manager.refreshStrict() }
+        // Teardown takes several dispatches and runs on backgroundScope — runCurrent, never
+        // advanceUntilIdle, which would leave the connect loop untouched.
+        runCurrent()
+
+        // The next action is fresh demand: it skips the reconnect backoff and lands on connection 2.
+        val refreshed = async { manager.refresh() }
+        advanceUntilIdle()
+        refreshed.await() shouldBe BillingData(listOf(owned))
+        attempts shouldBe 2
+
+        runCurrent()
+        // The torn-down connection is a failed loop iteration (wall-clock stamped), but the gate's
+        // own partial refresh must never reach the feed with its commit time.
+        failures.isNotEmpty() shouldBe true
+        failures shouldNotContain 4242L
+    }
+
    @Test fun `a partial reconciliation without a confirmed pro purchase signals its commit time`() = runTest2 {
        // The pending-only cold start: Play answered for one type with a payment in progress, the
        // other failed. Nothing confirms Pro, so the grace episode clock must advance — stamped