fix(upgrade): Make the FOSS supporter persist create-only-if-absent

persistUpgrade() wrote an unconditional record, so any sponsor return that got past the ViewModel's isPro guard replaced an existing supporter's upgradedAt - the "supporter since" date the status screen shows - and, for the legacy records every existing supporter has, their stored reason too. That guard reads a shareIn replay and can be stale, so it is not a race-free answer on its own. The write now happens inside the store transaction: an existing record is kept and reported back, only an absent one is created. The ViewModel thanks the user only for an actual unlock and stays quiet otherwise. It also no longer eats the sponsor visit when something fails - a failed entitlement read or a failed write restores the pending-launch marker so the next return can retry, while the error still travels the normal path. FossCache gains the same constructor test seam BillingCache has, so the transaction can be exercised against a real DataStore on a temp file.
2026-09-15 02:36:12 -04:00 · 2026-08-03 08:30:33 +02:00
parent d98c118f37
commit 26633b940d
6 changed files with 316 additions and 35 deletions
@@ -12,18 +12,25 @@ import kotlinx.serialization.json.Json
 import javax.inject.Inject
 import javax.inject.Singleton

+// Retained legacy migration: installs that predate the DataStore move still carry their upgrade
+// state in the "settings_foss" SharedPreferences file.
+private val Context.fossCacheDataStore by preferencesDataStore(
+    name = "settings_foss",
+    produceMigrations = { ctx -> listOf(SharedPreferencesMigration(ctx, "settings_foss")) },
+)
+
@Singleton
-class FossCache @Inject constructor(
-    @ApplicationContext context: Context,
-    @SerializationCapod json: Json,
+class FossCache internal constructor(
+    // Test seam: the store is handed in so a test can supply its own DataStore instead of the
+    // Context-bound production delegate. Same pattern as BillingCache.
+    private val dataStore: DataStore<Preferences>,
+    json: Json,
 ) {

-    private val Context.dataStore by preferencesDataStore(
-        name = "settings_foss",
-        produceMigrations = { ctx -> listOf(SharedPreferencesMigration(ctx, "settings_foss")) }
-    )
-
-    private val dataStore: DataStore<Preferences> = context.dataStore
+    @Inject constructor(
+        @ApplicationContext context: Context,
+        @SerializationCapod json: Json,
+    ) : this(context.fossCacheDataStore, json)

    val upgrade = dataStore.createValue<FossUpgrade?>(
        key = "foss.upgrade",
@@ -2,7 +2,7 @@ package eu.darken.capod.common.upgrade.core

 import eu.darken.capod.common.WebpageTool
 import eu.darken.capod.common.coroutine.AppScope
-import eu.darken.capod.common.datastore.value
+import eu.darken.capod.common.debug.logging.Logging.Priority.WARN
 import eu.darken.capod.common.debug.logging.log
 import eu.darken.capod.common.debug.logging.logTag
 import eu.darken.capod.common.flow.setupCommonEventHandlers
@@ -58,14 +58,35 @@ class UpgradeRepoFoss @Inject constructor(
    // Writes capod's RETAINED persistence schema: existing supporter records are serialized with
    // `reason` (foss.upgrade.reason.*). Adopting canonical's `upgradeType` schema would decode
    // every stored record as null and strip those supporters' entitlement.
-    internal suspend fun persistUpgrade() {
+    /**
+     * Create-only-if-absent inside the store transaction: an existing record (and its upgradedAt —
+     * the user-visible "supporter since" date) is never replaced. The VM-level isPro guard alone is
+     * not race-free: it reads a shareIn replay that can be stale. Note the kept record is still
+     * re-encoded through the current schema — decoded fields are preserved exactly.
+     *
+     * Caveat from [FossCache]'s `onErrorFallbackToDefault = true`: a stored record that fails to
+     * decode reads as null and therefore counts as ABSENT to this transaction, i.e. it gets
+     * replaced. That matches the pre-existing read behaviour — such a record already presents the
+     * user as free — and re-creating it on the next successful sponsor visit is the recovery path.
+     *
+     * @return true if a new record was created, false if an existing record was kept.
+     */
+    internal suspend fun persistUpgrade(): Boolean {
        log(TAG) { "persistUpgrade()" }
-        fossCache.upgrade.value(
-            FossUpgrade(
+        val updated = fossCache.upgrade.update { existing ->
+            existing ?: FossUpgrade(
                upgradedAt = Instant.now(),
                reason = FossUpgrade.Reason.DONATED,
            )
-        )
+        }
+        return if (updated.old == null) {
+            true
+        } else {
+            log(TAG, WARN) {
+                "persistUpgrade(): Record already exists (upgradedAt=${updated.old.upgradedAt}), keeping it"
+            }
+            false
+        }
    }

    override suspend fun refresh() {
@@ -143,24 +143,41 @@ class UpgradeViewModel @Inject constructor(
    fun checkSponsorReturn() = launch {
        val pressedAt = handle.remove<Long>(KEY_SPONSOR_PRESSED_AT) ?: return@launch

-        // Evaluated before the duration: an already upgraded supporter (recurring donation button)
-        // has nothing left to unlock, and persisting again would rewrite their upgradedAt — visibly
-        // resetting the "supporter since" date the status screen shows them.
-        if (upgradeRepo.upgradeInfo.first().isPro) {
-            log(TAG) { "checkSponsorReturn(): Already upgraded, staying quiet" }
-            return@launch
-        }
+        try {
+            // Evaluated before the duration: an already upgraded supporter (recurring donation
+            // button) has nothing left to unlock, so this fast path exists for the UX — return
+            // quietly, no redundant write attempt and no thanks toast for an unlock that already
+            // happened. Data integrity is not this guard's job: the repo's create-only transaction
+            // owns that.
+            if (upgradeRepo.upgradeInfo.first().isPro) {
+                log(TAG) { "checkSponsorReturn(): Already upgraded, staying quiet" }
+                return@launch
+            }

-        val elapsed = SystemClock.elapsedRealtime() - pressedAt
-        log(TAG) { "checkSponsorReturn(): elapsed=${elapsed}ms" }
+            val elapsed = SystemClock.elapsedRealtime() - pressedAt
+            log(TAG) { "checkSponsorReturn(): elapsed=${elapsed}ms" }

-        if (elapsed < SPONSOR_DELAY_MS) {
-            log(TAG) { "checkSponsorReturn(): Too quick, showing snackbar" }
-            snackbarEvents.tryEmit(R.string.upgrade_foss_sponsor_returned_early)
-        } else {
-            log(TAG) { "checkSponsorReturn(): Delay passed, persisting upgrade" }
-            upgradeRepo.persistUpgrade()
-            toastEvents.tryEmit(R.string.upgrade_foss_supporter_thanks)
+            if (elapsed < SPONSOR_DELAY_MS) {
+                log(TAG) { "checkSponsorReturn(): Too quick, showing snackbar" }
+                snackbarEvents.tryEmit(R.string.upgrade_foss_sponsor_returned_early)
+            } else {
+                log(TAG) { "checkSponsorReturn(): Delay passed, persisting upgrade" }
+                val created = upgradeRepo.persistUpgrade()
+                if (created) {
+                    toastEvents.tryEmit(R.string.upgrade_foss_supporter_thanks)
+                } else {
+                    // The isPro fast-path read a stale emission; the transaction kept the existing record.
+                    log(TAG) { "checkSponsorReturn(): Record already existed, staying quiet" }
+                }
+            }
+        } catch (e: Exception) {
+            // The marker was consumed above; neither a failed entitlement read nor a failed write may
+            // eat the user's valid sponsor visit — restore it so the next return/resume can retry the
+            // unlock. Rethrow unconditionally: cancellation is not swallowed, other errors surface via
+            // the normal error path. A restored marker after a successful persist is harmless — the
+            // next evaluation hits the quiet isPro path.
+            handle[KEY_SPONSOR_PRESSED_AT] = pressedAt
+            throw e
        }
    }

@@ -0,0 +1,141 @@
+package eu.darken.capod.common.upgrade.core
+
+import androidx.datastore.preferences.core.PreferenceDataStoreFactory
+import eu.darken.capod.common.WebpageTool
+import eu.darken.capod.common.datastore.value
+import eu.darken.capod.common.serialization.SerializationModule
+import io.kotest.matchers.shouldBe
+import io.kotest.matchers.shouldNotBe
+import io.mockk.mockk
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.async
+import kotlinx.coroutines.awaitAll
+import kotlinx.coroutines.cancel
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.test.TestScope
+import kotlinx.coroutines.test.runTest
+import org.junit.jupiter.api.AfterEach
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.io.TempDir
+import testhelpers.BaseTest
+import java.io.File
+import java.time.Instant
+import java.time.temporal.ChronoUnit
+
+/**
+ * The FOSS supporter record is create-only-if-absent: the sponsor-return heuristic can fire again
+ * for someone who is already a supporter (the recurring-donation button, or a stale entitlement
+ * replay), and a rewrite would move their "supporter since" date — and, for the legacy records
+ * every existing supporter has, replace their stored reason too.
+ *
+ * Driven through a real DataStore on a temp file via [FossCache]'s test seam, because the guarantee
+ * is the store transaction's, not the caller's.
+ */
+class UpgradeRepoFossPersistTest : BaseTest() {
+
+    @TempDir
+    lateinit var tempDir: File
+
+    // One store scope per test: the DataStore keeps its own actor alive on it.
+    private var storeScope: CoroutineScope? = null
+
+    @AfterEach
+    fun teardown() {
+        storeScope?.cancel()
+        storeScope = null
+    }
+
+    private class Harness(val cache: FossCache, val repo: UpgradeRepoFoss)
+
+    // Unique file name per test method: DataStore forbids two active instances on the same file.
+    private fun TestScope.buildHarness(storeName: String): Harness {
+        val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()).also { storeScope = it }
+        val dataStore = PreferenceDataStoreFactory.create(
+            scope = scope,
+            produceFile = { File(tempDir, "$storeName.preferences_pb") },
+        )
+        val cache = FossCache(dataStore, SerializationModule().json())
+        val repo = UpgradeRepoFoss(
+            // backgroundScope: the repo's shareIn keeps a collector alive for the scope's lifetime.
+            appScope = backgroundScope,
+            fossCache = cache,
+            webpageTool = mockk<WebpageTool>(relaxed = true),
+        )
+        return Harness(cache, repo)
+    }
+
+    @Test
+    fun `persistUpgrade keeps an existing legacy record`() = runTest {
+        val harness = buildHarness("legacy_record")
+        // A legacy 2022-schema value: a rewrite would corrupt BOTH the date and the reason.
+        harness.cache.upgrade.value(
+            FossUpgrade(
+                upgradedAt = Instant.EPOCH,
+                reason = FossUpgrade.Reason.NO_MONEY,
+            )
+        )
+
+        harness.repo.persistUpgrade() shouldBe false
+
+        harness.cache.upgrade.value() shouldBe FossUpgrade(
+            upgradedAt = Instant.EPOCH,
+            reason = FossUpgrade.Reason.NO_MONEY,
+        )
+        harness.repo.upgradeInfo.first().apply {
+            isPro shouldBe true
+            upgradedAt shouldBe Instant.EPOCH
+        }
+    }
+
+    @Test
+    fun `persistUpgrade creates on an empty store`() = runTest {
+        val harness = buildHarness("empty_store")
+        harness.cache.upgrade.value() shouldBe null
+
+        // Truncated: the record's serializer is epoch-millis, so a nanosecond lower bound can flake
+        // when the write lands within the same millisecond.
+        val before = Instant.now().truncatedTo(ChronoUnit.MILLIS)
+        harness.repo.persistUpgrade() shouldBe true
+        val after = Instant.now()
+
+        val created = harness.cache.upgrade.value()
+        created shouldNotBe null
+        created!!.reason shouldBe FossUpgrade.Reason.DONATED
+        (created.upgradedAt >= before) shouldBe true
+        (created.upgradedAt <= after) shouldBe true
+
+        // Boolean-proven keep: immune to a timestamp collision between the two writes.
+        harness.repo.persistUpgrade() shouldBe false
+        harness.cache.upgrade.value() shouldBe created
+    }
+
+    @Test
+    fun `concurrent persists elect exactly one creator`() = runTest {
+        val harness = buildHarness("concurrent")
+        harness.cache.upgrade.value() shouldBe null
+
+        val before = Instant.now().truncatedTo(ChronoUnit.MILLIS)
+        val gate = CompletableDeferred<Unit>()
+        val racers = List(2) {
+            async(Dispatchers.IO) {
+                gate.await()
+                harness.repo.persistUpgrade()
+            }
+        }
+        gate.complete(Unit)
+        val results = racers.awaitAll()
+        val after = Instant.now()
+
+        // Exactly one creator: the loser must report the record it found, not a second creation.
+        results.sorted() shouldBe listOf(false, true)
+
+        val record = harness.cache.upgrade.value()
+        record shouldNotBe null
+        record!!.reason shouldBe FossUpgrade.Reason.DONATED
+        (record.upgradedAt >= before) shouldBe true
+        (record.upgradedAt <= after) shouldBe true
+    }
+}
@@ -40,7 +40,7 @@ class FossUpgradeScreenHostTest : BaseTest() {
    private fun mockRepo(): UpgradeRepoFoss = mockk<UpgradeRepoFoss>(relaxed = true).apply {
        every { upgradeInfo } returns MutableStateFlow(UpgradeRepoFoss.Info())
        every { openGithubSponsorsPage() } returns true
-        coEvery { persistUpgrade() } answers { persisted++ }
+        coEvery { persistUpgrade() } answers { persisted++; true }
    }

    private fun buildVm(
@@ -8,6 +8,8 @@ import eu.darken.capod.common.upgrade.core.FossUpgrade
 import eu.darken.capod.common.upgrade.core.UpgradeRepoFoss
 import io.kotest.matchers.collections.shouldBeEmpty
 import io.kotest.matchers.shouldBe
+import io.kotest.matchers.types.shouldBeInstanceOf
+import io.mockk.coEvery
 import io.mockk.coVerify
 import io.mockk.every
 import io.mockk.mockk
@@ -17,6 +19,7 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.async
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.flow.flow
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.advanceUntilIdle
@@ -33,6 +36,7 @@ import testhelpers.BaseTest
 import testhelpers.TestApplication
 import testhelpers.coroutine.TestDispatcherProvider
 import testhelpers.coroutine.runTest2
+import java.io.IOException
 import java.time.Duration
 import java.time.Instant

@@ -63,6 +67,9 @@ class FossUpgradeViewModelTest : BaseTest() {
    ): UpgradeRepoFoss = mockk<UpgradeRepoFoss>(relaxed = true).apply {
        every { upgradeInfo } returns info
        every { openGithubSponsorsPage() } returns true
+        // Explicit: a relaxed mock would answer the Boolean with false, i.e. "record already
+        // existed", silently turning every thanks-toast assertion below into a no-op.
+        coEvery { persistUpgrade() } returns true
    }

    private fun buildVm(
@@ -262,9 +269,9 @@ class FossUpgradeViewModelTest : BaseTest() {
    fun `a recurring donation from the upgraded status keeps the supporter date`() = runTest2(
        context = testDispatcher,
    ) {
-        // The upgraded status screen's donate-again button runs the very same sponsor flow. A
-        // return past the delay must NOT persist again -- that would rewrite upgradedAt and
-        // visibly reset the "supporter since" date the screen shows.
+        // The upgraded status screen's donate-again button runs the very same sponsor flow. The
+        // store transaction keeps the existing record either way, so this is about the feedback:
+        // no redundant write attempt, and no thanks toast for an unlock that already happened.
        val repo = mockRepo(MutableStateFlow(upgradedInfo()))
        val vm = buildVm(repo = repo)

@@ -312,6 +319,94 @@ class FossUpgradeViewModelTest : BaseTest() {
        recreatedVm.hasPendingSponsorLaunch() shouldBe false
    }

+    @Test
+    fun `a sponsor return whose record already existed stays quiet`() = runTest2(context = testDispatcher) {
+        // The isPro fast path reads a shareIn replay that can be stale, so a supporter's return can
+        // get past it. Only the store transaction knows the record is already there — it keeps it
+        // and reports "not created", and there is no unlock to thank anyone for.
+        val repo = mockRepo()
+        coEvery { repo.persistUpgrade() } returns false
+        val vm = buildVm(repo = repo)
+
+        val nudges = mutableListOf<Int>()
+        val thanks = mutableListOf<Int>()
+        val snackbarCollector = launch(start = CoroutineStart.UNDISPATCHED) {
+            vm.snackbarEvents.collect { nudges.add(it) }
+        }
+        val toastCollector = launch(start = CoroutineStart.UNDISPATCHED) { vm.toastEvents.collect { thanks.add(it) } }
+
+        vm.goGithubSponsors()
+        ShadowSystemClock.advanceBy(Duration.ofSeconds(6))
+        vm.checkSponsorReturn()
+        advanceUntilIdle()
+
+        coVerify(exactly = 1) { repo.persistUpgrade() }
+        thanks.shouldBeEmpty()
+        nudges.shouldBeEmpty()
+        // Consumed: the visit was evaluated, there is nothing left to retry.
+        vm.hasPendingSponsorLaunch() shouldBe false
+
+        snackbarCollector.cancel()
+        toastCollector.cancel()
+    }
+
+    @Test
+    fun `a failed persist restores the pending sponsor launch`() = runTest2(context = testDispatcher) {
+        // The marker is consumed before the write. If the write then fails, dropping it would eat a
+        // valid sponsor visit for good — the next return/resume has to be able to retry the unlock.
+        val repo = mockRepo()
+        coEvery { repo.persistUpgrade() } throws IOException("write failed")
+        val vm = buildVm(repo = repo)
+
+        val thanks = mutableListOf<Int>()
+        val errors = mutableListOf<Throwable>()
+        val toastCollector = launch(start = CoroutineStart.UNDISPATCHED) { vm.toastEvents.collect { thanks.add(it) } }
+        val errorCollector = launch(start = CoroutineStart.UNDISPATCHED) { vm.errorEvents.collect { errors.add(it) } }
+
+        vm.goGithubSponsors()
+        ShadowSystemClock.advanceBy(Duration.ofSeconds(6))
+        vm.checkSponsorReturn()
+        advanceUntilIdle()
+
+        vm.hasPendingSponsorLaunch() shouldBe true
+        thanks.shouldBeEmpty()
+        // Rethrown, not swallowed: the failure still travels the normal error path.
+        errors.single().shouldBeInstanceOf<IOException>()
+
+        toastCollector.cancel()
+        errorCollector.cancel()
+    }
+
+    @Test
+    fun `a failed entitlement read restores the pending sponsor launch`() = runTest2(context = testDispatcher) {
+        // The guard's entitlement read happens after the marker was consumed, so it can eat the
+        // sponsor visit just as a failed write can. Installed after arming: the ViewModel's own init
+        // collectors already hold the working flow, so the only failing read is the guard's.
+        val repo = mockRepo()
+        val vm = buildVm(repo = repo)
+
+        val thanks = mutableListOf<Int>()
+        val errors = mutableListOf<Throwable>()
+        val toastCollector = launch(start = CoroutineStart.UNDISPATCHED) { vm.toastEvents.collect { thanks.add(it) } }
+        val errorCollector = launch(start = CoroutineStart.UNDISPATCHED) { vm.errorEvents.collect { errors.add(it) } }
+
+        vm.goGithubSponsors()
+        advanceUntilIdle()
+        every { repo.upgradeInfo } returns flow { throw IOException("read failed") }
+
+        ShadowSystemClock.advanceBy(Duration.ofSeconds(6))
+        vm.checkSponsorReturn()
+        advanceUntilIdle()
+
+        vm.hasPendingSponsorLaunch() shouldBe true
+        thanks.shouldBeEmpty()
+        coVerify(exactly = 0) { repo.persistUpgrade() }
+        errors.single().shouldBeInstanceOf<IOException>()
+
+        toastCollector.cancel()
+        errorCollector.cancel()
+    }
+
    @Test
    fun `a sponsor page that never opened arms nothing and a later retry still works`() = runTest2(
        context = testDispatcher,
@@ -354,7 +449,7 @@ class FossUpgradeViewModelTest : BaseTest() {
        context = testDispatcher,
    ) {
        // The status view's donate button is unarmed on purpose: a supporter browsing the sponsors
-        // page for a while must not run the unlock heuristic again and rewrite their upgrade date.
+        // page for a while must not run the unlock heuristic again — no write attempt, no toast.
        val repo = mockRepo(MutableStateFlow(upgradedInfo()))
        val vm = buildVm(repo = repo)