diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/core/FossCache.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/core/FossCache.kt index 30f693bf..b2575f2c 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/core/FossCache.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/core/FossCache.kt @@ -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, + json: Json, ) { - private val Context.dataStore by preferencesDataStore( - name = "settings_foss", - produceMigrations = { ctx -> listOf(SharedPreferencesMigration(ctx, "settings_foss")) } - ) - - private val dataStore: DataStore = context.dataStore + @Inject constructor( + @ApplicationContext context: Context, + @SerializationCapod json: Json, + ) : this(context.fossCacheDataStore, json) val upgrade = dataStore.createValue( key = "foss.upgrade", diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt index 40f48184..f5f64cca 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt @@ -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() { diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt index 880e76a7..c6d5f418 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt @@ -143,24 +143,41 @@ class UpgradeViewModel @Inject constructor( fun checkSponsorReturn() = launch { val pressedAt = handle.remove(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 } } diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFossPersistTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFossPersistTest.kt new file mode 100644 index 00000000..cead3af0 --- /dev/null +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFossPersistTest.kt @@ -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(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() + 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 + } +} diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt index 9b870d7d..09a4e6a1 100644 --- a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt @@ -40,7 +40,7 @@ class FossUpgradeScreenHostTest : BaseTest() { private fun mockRepo(): UpgradeRepoFoss = mockk(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( diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt index c358e658..4d5ab6bb 100644 --- a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt @@ -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(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() + val thanks = mutableListOf() + 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() + val errors = mutableListOf() + 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() + + 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() + val errors = mutableListOf() + 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() + + 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)