Make player history failures non-fatal

2026-07-26 19:40:58 -04:00
parent 1ec3dd1276
commit 843957d154
3 changed files with 262 additions and 53 deletions
@@ -72,7 +72,9 @@ export JAVA_HOME="/Applications/Android Studio.app/Contents/jbr/Contents/Home"
   version, setup preference, session/hand counts, baseline agreement, and stable
   review-category counters. It never stores hole cards, boards, opponents, or
   raw action histories. Storage mutations are serialised because hand completion
-   and coach analysis run on different coroutines.
+   and coach analysis run on different coroutines. History is non-critical: an
+   unreadable/newer schema or failed commit falls back to in-memory counters and
+   disables persistence for that process rather than crashing or overwriting data.

 ## Testing notes

@@ -142,7 +144,9 @@ export JAVA_HOME="/Applications/Android Studio.app/Contents/jbr/Contents/Home"
 - Versioned player history survives process restarts in a small aggregate
  `SharedPreferences` store. The setup screen restores the last player/coach
  preference and shows cross-session agreement/review trends only after at least
-  ten coached decisions; smaller samples are labelled as insufficient.
+  ten coached decisions; smaller samples are labelled as insufficient. Schema
+  reads go through an explicit migration dispatcher; add the migration branch
+  before incrementing `PLAYER_HISTORY_SCHEMA_VERSION`.
 - Gradle emits an `archives` deprecation from the Kotlin Multiplatform plugin's
  own `jvm()` target registration — upstream in Kotlin 2.2.10, not our build.
 - Not built yet: LLM persona layer.
@@ -3,6 +3,7 @@ package com.jsjdesigns.poker
 import android.annotation.SuppressLint
 import android.content.Context
 import android.content.SharedPreferences
+import android.util.Log
 import com.jsjdesigns.poker.bot.CoachingReview
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.sync.Mutex
@@ -16,20 +17,29 @@ interface PlayerHistoryStore {
    suspend fun recordCoachReview(review: CoachingReview): PlayerHistory
 }

+internal interface PlayerHistoryPersistence {
+    fun read(): PlayerHistory
+    fun write(history: PlayerHistory): Boolean
+}
+
 /**
- * Small versioned aggregate store.
+ * Serialised read-modify-write with a deliberately non-fatal failure policy.
 *
- * SharedPreferences is appropriate here because the complete schema is a few
- * scalar counters. Mutations are serialised and committed on Dispatchers.IO so a
- * hand completion and an asynchronous coach review cannot overwrite each other.
+ * History is coaching metadata, never game-critical state. If data is corrupt,
+ * belongs to a newer app schema, or cannot be written, the current process keeps
+ * a fresh in-memory history and stops touching durable storage. In particular,
+ * an older sideloaded APK neither crashes nor overwrites a newer schema.
 */
-class SharedPreferencesPlayerHistoryStore(context: Context) : PlayerHistoryStore {
-    private val preferences =
-        context.getSharedPreferences(PREFERENCES_NAME, Context.MODE_PRIVATE)
+internal class ResilientPlayerHistoryStore(
+    private val persistence: PlayerHistoryPersistence,
+    private val failureReporter: (message: String, error: RuntimeException?) -> Unit = { _, _ -> },
+) : PlayerHistoryStore {
    private val mutex = Mutex()
+    private var cachedHistory: PlayerHistory? = null
+    private var persistenceAvailable = true

    override suspend fun load(): PlayerHistory = withContext(Dispatchers.IO) {
-        mutex.withLock { preferences.readHistory() }
+        mutex.withLock { currentHistory() }
    }

    override suspend fun recordSession(
@@ -46,55 +56,128 @@ class SharedPreferencesPlayerHistoryStore(context: Context) : PlayerHistoryStore
    private suspend fun mutate(block: (PlayerHistory) -> PlayerHistory): PlayerHistory =
        withContext(Dispatchers.IO) {
            mutex.withLock {
-                val updated = block(preferences.readHistory())
-                check(preferences.writeHistory(updated)) { "could not persist player history" }
+                val updated = block(currentHistory())
+                cachedHistory = updated
+                if (persistenceAvailable) {
+                    try {
+                        if (!persistence.write(updated)) {
+                            disablePersistence("Could not commit player history", null)
+                        }
+                    } catch (error: RuntimeException) {
+                        disablePersistence("Could not write player history", error)
+                    }
+                }
                updated
            }
        }

-    private companion object {
-        const val PREFERENCES_NAME = "player_history"
-        const val KEY_SCHEMA = "schema_version"
-        const val KEY_PLAYER_NAME = "player_name"
-        const val KEY_COACH_ENABLED = "coach_enabled"
-        const val KEY_SESSIONS = "sessions_started"
-        const val KEY_HANDS = "hands_completed"
-        const val KEY_DECISIONS = "decisions_reviewed"
-        const val KEY_ALIGNED = "baseline_aligned"
-
-        fun SharedPreferences.readHistory(): PlayerHistory {
-            val storedSchema = getInt(KEY_SCHEMA, PLAYER_HISTORY_SCHEMA_VERSION)
-            require(storedSchema <= PLAYER_HISTORY_SCHEMA_VERSION) {
-                "player history schema $storedSchema is newer than supported " +
-                    PLAYER_HISTORY_SCHEMA_VERSION
+    private fun currentHistory(): PlayerHistory {
+        cachedHistory?.let { return it }
+        val loaded = if (persistenceAvailable) {
+            try {
+                persistence.read()
+            } catch (error: RuntimeException) {
+                disablePersistence("Could not read player history", error)
+                PlayerHistory()
            }
-            return PlayerHistory(
-                playerName = getString(KEY_PLAYER_NAME, "").orEmpty(),
-                coachEnabled = getBoolean(KEY_COACH_ENABLED, false),
-                sessionsStarted = getLong(KEY_SESSIONS, 0),
-                handsCompleted = getLong(KEY_HANDS, 0),
-                decisionsReviewed = getLong(KEY_DECISIONS, 0),
-                baselineAligned = getLong(KEY_ALIGNED, 0),
-                leakCounts = LeakKind.entries.associateWith { kind ->
-                    getLong("leak_${kind.id}", 0)
-                },
-            )
+        } else {
+            PlayerHistory()
        }
+        cachedHistory = loaded
+        return loaded
+    }

-        @SuppressLint("UseKtx") // KTX edit discards commit(), so durability failures cannot be reported.
-        fun SharedPreferences.writeHistory(history: PlayerHistory): Boolean {
-            val editor = edit()
-                .putInt(KEY_SCHEMA, history.schemaVersion)
-                .putString(KEY_PLAYER_NAME, history.playerName)
-                .putBoolean(KEY_COACH_ENABLED, history.coachEnabled)
-                .putLong(KEY_SESSIONS, history.sessionsStarted)
-                .putLong(KEY_HANDS, history.handsCompleted)
-                .putLong(KEY_DECISIONS, history.decisionsReviewed)
-                .putLong(KEY_ALIGNED, history.baselineAligned)
-            for (kind in LeakKind.entries) {
-                editor.putLong("leak_${kind.id}", history.leakCount(kind))
-            }
-            return editor.commit()
-        }
+    private fun disablePersistence(message: String, error: RuntimeException?) {
+        persistenceAvailable = false
+        // Reporting must not turn a handled storage failure back into a crash.
+        runCatching { failureReporter(message, error) }
    }
 }
+
+class SharedPreferencesPlayerHistoryStore(context: Context) : PlayerHistoryStore by
+    ResilientPlayerHistoryStore(
+        persistence = SharedPreferencesHistoryPersistence(
+            context.getSharedPreferences(PREFERENCES_NAME, Context.MODE_PRIVATE),
+        ),
+        failureReporter = { message, error ->
+            if (error == null) Log.e(LOG_TAG, message) else Log.e(LOG_TAG, message, error)
+        },
+    )
+
+internal enum class HistorySchemaPlan {
+    FRESH,
+    READ_VERSION_1,
+    UNSUPPORTED,
+}
+
+/**
+ * Explicit migration dispatch. Add a branch and migration before incrementing
+ * [PLAYER_HISTORY_SCHEMA_VERSION]; never reinterpret a non-additive schema.
+ */
+internal fun historySchemaPlan(storedVersion: Int): HistorySchemaPlan = when (storedVersion) {
+    0 -> HistorySchemaPlan.FRESH
+    1 -> HistorySchemaPlan.READ_VERSION_1
+    else -> HistorySchemaPlan.UNSUPPORTED
+}
+
+internal class UnsupportedPlayerHistorySchema(storedVersion: Int) :
+    IllegalStateException(
+        "player history schema $storedVersion is not supported by schema " +
+            PLAYER_HISTORY_SCHEMA_VERSION,
+    )
+
+private class SharedPreferencesHistoryPersistence(
+    private val preferences: SharedPreferences,
+) : PlayerHistoryPersistence {
+    override fun read(): PlayerHistory {
+        val storedSchema = if (preferences.contains(KEY_SCHEMA)) {
+            preferences.getInt(KEY_SCHEMA, 0)
+        } else {
+            0
+        }
+        return when (historySchemaPlan(storedSchema)) {
+            HistorySchemaPlan.FRESH -> PlayerHistory()
+            HistorySchemaPlan.READ_VERSION_1 -> preferences.readVersion1()
+            HistorySchemaPlan.UNSUPPORTED ->
+                throw UnsupportedPlayerHistorySchema(storedSchema)
+        }
+    }
+
+    @SuppressLint("UseKtx") // KTX edit discards commit(), so failures cannot be reported.
+    override fun write(history: PlayerHistory): Boolean {
+        val editor = preferences.edit()
+            .putInt(KEY_SCHEMA, history.schemaVersion)
+            .putString(KEY_PLAYER_NAME, history.playerName)
+            .putBoolean(KEY_COACH_ENABLED, history.coachEnabled)
+            .putLong(KEY_SESSIONS, history.sessionsStarted)
+            .putLong(KEY_HANDS, history.handsCompleted)
+            .putLong(KEY_DECISIONS, history.decisionsReviewed)
+            .putLong(KEY_ALIGNED, history.baselineAligned)
+        for (kind in LeakKind.entries) {
+            editor.putLong("leak_${kind.id}", history.leakCount(kind))
+        }
+        return editor.commit()
+    }
+
+    private fun SharedPreferences.readVersion1(): PlayerHistory = PlayerHistory(
+        playerName = getString(KEY_PLAYER_NAME, "").orEmpty(),
+        coachEnabled = getBoolean(KEY_COACH_ENABLED, false),
+        sessionsStarted = getLong(KEY_SESSIONS, 0),
+        handsCompleted = getLong(KEY_HANDS, 0),
+        decisionsReviewed = getLong(KEY_DECISIONS, 0),
+        baselineAligned = getLong(KEY_ALIGNED, 0),
+        leakCounts = LeakKind.entries.associateWith { kind ->
+            getLong("leak_${kind.id}", 0)
+        },
+    )
+}
+
+private const val PREFERENCES_NAME = "player_history"
+private const val LOG_TAG = "PlayerHistory"
+private const val KEY_SCHEMA = "schema_version"
+private const val KEY_PLAYER_NAME = "player_name"
+private const val KEY_COACH_ENABLED = "coach_enabled"
+private const val KEY_SESSIONS = "sessions_started"
+private const val KEY_HANDS = "hands_completed"
+private const val KEY_DECISIONS = "decisions_reviewed"
+private const val KEY_ALIGNED = "baseline_aligned"
@@ -0,0 +1,122 @@
+package com.jsjdesigns.poker
+
+import kotlinx.coroutines.coroutineScope
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class PlayerHistoryStoreTest {
+
+    private class FakePersistence(
+        var stored: PlayerHistory = PlayerHistory(),
+        var readFailure: RuntimeException? = null,
+        var writeResult: Boolean = true,
+        var writeFailure: RuntimeException? = null,
+    ) : PlayerHistoryPersistence {
+        var reads = 0
+        var writes = 0
+
+        override fun read(): PlayerHistory {
+            reads++
+            readFailure?.let { throw it }
+            return stored
+        }
+
+        override fun write(history: PlayerHistory): Boolean {
+            writes++
+            writeFailure?.let { throw it }
+            if (writeResult) stored = history
+            return writeResult
+        }
+    }
+
+    @Test
+    fun `schema dispatch has an explicit migration branch`() {
+        assertEquals(HistorySchemaPlan.FRESH, historySchemaPlan(0))
+        assertEquals(HistorySchemaPlan.READ_VERSION_1, historySchemaPlan(1))
+        assertEquals(HistorySchemaPlan.UNSUPPORTED, historySchemaPlan(2))
+        assertEquals(HistorySchemaPlan.UNSUPPORTED, historySchemaPlan(99))
+        assertEquals(HistorySchemaPlan.UNSUPPORTED, historySchemaPlan(-1))
+    }
+
+    @Test
+    fun `newer schema falls back in memory without overwrite or crash`() = runTest {
+        val persistence = FakePersistence(
+            readFailure = UnsupportedPlayerHistorySchema(99),
+        )
+        val failures = mutableListOf<String>()
+        val store = ResilientPlayerHistoryStore(persistence) { message, _ ->
+            failures += message
+        }
+
+        assertEquals(PlayerHistory(), store.load())
+        val currentSession = store.recordSession("Jay", coachEnabled = true)
+
+        assertEquals("Jay", currentSession.playerName)
+        assertTrue(currentSession.coachEnabled)
+        assertEquals(1L, currentSession.sessionsStarted)
+        assertEquals("fallback remains useful in this process", currentSession, store.load())
+        assertEquals(1, persistence.reads)
+        assertEquals("an older build must not overwrite newer data", 0, persistence.writes)
+        assertEquals(1, failures.size)
+    }
+
+    @Test
+    fun `corrupt history also degrades to an in-memory session`() = runTest {
+        val persistence = FakePersistence(
+            readFailure = IllegalArgumentException("corrupt counter"),
+        )
+        val store = ResilientPlayerHistoryStore(persistence)
+
+        assertEquals(PlayerHistory(), store.load())
+        assertEquals(1L, store.recordCompletedHand().handsCompleted)
+        assertEquals(0, persistence.writes)
+    }
+
+    @Test
+    fun `failed commit disables further writes but keeps current counters`() = runTest {
+        val persistence = FakePersistence(writeResult = false)
+        val failures = mutableListOf<String>()
+        val store = ResilientPlayerHistoryStore(persistence) { message, _ ->
+            failures += message
+        }
+
+        assertEquals(1L, store.recordCompletedHand().handsCompleted)
+        assertEquals(2L, store.recordCompletedHand().handsCompleted)
+
+        assertEquals("do not repeatedly hammer failed storage", 1, persistence.writes)
+        assertEquals(1, failures.size)
+        assertTrue("commit" in failures.single())
+    }
+
+    @Test
+    fun `write exception is contained like a false commit`() = runTest {
+        val persistence = FakePersistence(
+            writeFailure = IllegalStateException("disk unavailable"),
+        )
+        val store = ResilientPlayerHistoryStore(persistence)
+
+        assertEquals(1L, store.recordCompletedHand().handsCompleted)
+        assertEquals(2L, store.recordCompletedHand().handsCompleted)
+        assertEquals(1, persistence.writes)
+    }
+
+    @Test
+    fun `concurrent hand updates cannot clobber one another`() = runTest {
+        val persistence = FakePersistence()
+        val store = ResilientPlayerHistoryStore(persistence)
+
+        coroutineScope {
+            repeat(100) {
+                launch { store.recordCompletedHand() }
+            }
+        }
+
+        assertEquals(100L, store.load().handsCompleted)
+        assertEquals(100, persistence.writes)
+        assertFalse(persistence.stored.coachEnabled)
+    }
+}