Merge pull request #65 from garfiec/fix/repo-cache-mutex

fix(data): mutex guard repository caches against suspension races

core/data/src/androidUnitTest/kotlin/com/garfiec/librechat/core/data/repository/RepositoryCacheMutexTest.kt

@@ -0,0 +1,130 @@
+package com.garfiec.librechat.core.data.repository
+
+import com.garfiec.librechat.core.common.result.Result
+import com.garfiec.librechat.core.model.Agent
+import com.garfiec.librechat.core.model.Preset
+import com.garfiec.librechat.core.model.response.AgentListResponse
+import com.garfiec.librechat.core.network.api.AgentsApi
+import com.garfiec.librechat.core.network.api.PresetsApi
+import com.google.common.truth.Truth.assertThat
+import io.mockk.coEvery
+import io.mockk.coVerify
+import io.mockk.mockk
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.async
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.test.advanceUntilIdle
+import kotlinx.coroutines.test.runTest
+import org.junit.Test
+
+@OptIn(ExperimentalCoroutinesApi::class)
+class RepositoryCacheMutexTest {
+
+    @Test
+    fun `AgentRepositoryImpl - two concurrent getAgents on cold cache hit the API exactly once`() =
+        runTest {
+            val agentsApi = mockk<AgentsApi>()
+            val responseAgents = listOf(Agent(id = "a1"), Agent(id = "a2"))
+            coEvery { agentsApi.getAgents(null) } coAnswers {
+                delay(50)
+                AgentListResponse(data = responseAgents)
+            }
+
+            val repository = AgentRepositoryImpl(agentsApi = agentsApi)
+
+            val first = async { repository.getAgents(category = null) }
+            val second = async { repository.getAgents(category = null) }
+            advanceUntilIdle()
+
+            val firstResult = first.await()
+            val secondResult = second.await()
+
+            assertThat(firstResult).isInstanceOf(Result.Success::class.java)
+            assertThat(secondResult).isInstanceOf(Result.Success::class.java)
+            assertThat((firstResult as Result.Success).data).isEqualTo(responseAgents)
+            assertThat((secondResult as Result.Success).data).isEqualTo(responseAgents)
+            coVerify(exactly = 1) { agentsApi.getAgents(null) }
+        }
+
+    @Test
+    fun `AgentRepositoryImpl - second call after cache hydrated does not re-fetch`() = runTest {
+        val agentsApi = mockk<AgentsApi>()
+        val responseAgents = listOf(Agent(id = "a1"))
+        coEvery { agentsApi.getAgents(null) } returns AgentListResponse(data = responseAgents)
+
+        val repository = AgentRepositoryImpl(agentsApi = agentsApi)
+        repository.getAgents(category = null)
+        repository.getAgents(category = null)
+        repository.getAgents(category = null)
+
+        coVerify(exactly = 1) { agentsApi.getAgents(null) }
+    }
+
+    @Test
+    fun `AgentRepositoryImpl - categorised getAgents bypasses cache and does not populate it`() =
+        runTest {
+            val agentsApi = mockk<AgentsApi>()
+            val filteredAgents = listOf(Agent(id = "cat1"))
+            val allAgents = listOf(Agent(id = "a1"), Agent(id = "a2"))
+            coEvery { agentsApi.getAgents("writing") } returns AgentListResponse(data = filteredAgents)
+            coEvery { agentsApi.getAgents(null) } returns AgentListResponse(data = allAgents)
+
+            val repository = AgentRepositoryImpl(agentsApi = agentsApi)
+            repository.getAgents(category = "writing")
+            // Cache must still be cold — next call with category=null must hit the API.
+            repository.getAgents(category = null)
+            repository.getAgents(category = null)
+
+            coVerify(exactly = 1) { agentsApi.getAgents("writing") }
+            coVerify(exactly = 1) { agentsApi.getAgents(null) }
+        }
+
+    @Test
+    fun `PresetRepositoryImpl - two concurrent getAll on cold cache hit the API exactly once`() =
+        runTest {
+            val presetsApi = mockk<PresetsApi>()
+            val responsePresets = listOf(
+                Preset(presetId = "p1", title = "One"),
+                Preset(presetId = "p2", title = "Two"),
+            )
+            coEvery { presetsApi.getPresets() } coAnswers {
+                delay(50)
+                responsePresets
+            }
+
+            val repository = PresetRepositoryImpl(presetsApi = presetsApi)
+
+            val first = async { repository.getAll() }
+            val second = async { repository.getAll() }
+            advanceUntilIdle()
+
+            val firstResult = first.await()
+            val secondResult = second.await()
+
+            assertThat(firstResult).isInstanceOf(Result.Success::class.java)
+            assertThat(secondResult).isInstanceOf(Result.Success::class.java)
+            assertThat((firstResult as Result.Success).data).isEqualTo(responsePresets)
+            assertThat((secondResult as Result.Success).data).isEqualTo(responsePresets)
+            coVerify(exactly = 1) { presetsApi.getPresets() }
+        }
+
+    @Test
+    fun `PresetRepositoryImpl - mutation invalidates cache so next getAll re-fetches`() = runTest {
+        val presetsApi = mockk<PresetsApi>()
+        val first = listOf(Preset(presetId = "p1", title = "One"))
+        val second = listOf(
+            Preset(presetId = "p1", title = "One"),
+            Preset(presetId = "p2", title = "Two"),
+        )
+        coEvery { presetsApi.getPresets() } returnsMany listOf(first, second)
+        coEvery { presetsApi.createPreset(any()) } returns Preset(presetId = "p2", title = "Two")
+
+        val repository = PresetRepositoryImpl(presetsApi = presetsApi)
+        repository.getAll()
+        repository.create(Preset(presetId = "p2", title = "Two"))
+        val result = repository.getAll()
+
+        assertThat((result as Result.Success).data).isEqualTo(second)
+        coVerify(exactly = 2) { presetsApi.getPresets() }
+    }
+}

core/data/src/commonMain/kotlin/com/garfiec/librechat/core/data/repository/AgentRepositoryImpl.kt

@@ -17,28 +17,30 @@ import com.garfiec.librechat.core.model.request.RevertAgentRequest
 import com.garfiec.librechat.core.model.request.ToolCallRequest
 import com.garfiec.librechat.core.model.request.UpdateAgentRequest
 import com.garfiec.librechat.core.network.api.AgentsApi
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import kotlinx.serialization.json.Json
-import kotlin.concurrent.Volatile
 
 class AgentRepositoryImpl(
     private val agentsApi: AgentsApi,
 ) : AgentRepository {
 
-    @Volatile
+    private val cacheMutex = Mutex()
     private var cachedAgents: List<Agent>? = null
 
     // --- Agent CRUD ---
 
     override suspend fun getAgents(category: String?): Result<List<Agent>> {
         return safeApiCall {
-            if (category == null && cachedAgents != null) {
+            if (category != null) {
-                return@safeApiCall cachedAgents!!
+                return@safeApiCall agentsApi.getAgents(category).data
             }
-            val agents = agentsApi.getAgents(category).data
+            cacheMutex.withLock {
-            if (category == null) {
+                cachedAgents?.let { return@withLock it }
+                val agents = agentsApi.getAgents(null).data
                 cachedAgents = agents
+                agents
             }
-            agents
         }
     }
 
@@ -65,7 +67,8 @@ class AgentRepositoryImpl(
 
     override suspend fun getAgent(id: String): Result<Agent> {
         return safeApiCall {
-            cachedAgents?.find { it.id == id }?.let { return@safeApiCall it }
+            cacheMutex.withLock { cachedAgents?.find { it.id == id } }
+                ?.let { return@safeApiCall it }
             agentsApi.getAgent(id)
         }
     }
@@ -201,7 +204,7 @@ class AgentRepositoryImpl(
         }
     }
 
-    private fun invalidateCache() {
+    private suspend fun invalidateCache() {
-        cachedAgents = null
+        cacheMutex.withLock { cachedAgents = null }
     }
 }

core/data/src/commonMain/kotlin/com/garfiec/librechat/core/data/repository/PresetRepositoryImpl.kt

@@ -4,28 +4,31 @@ import com.garfiec.librechat.core.common.result.Result
 import com.garfiec.librechat.core.common.result.safeApiCall
 import com.garfiec.librechat.core.model.Preset
 import com.garfiec.librechat.core.network.api.PresetsApi
-import kotlin.concurrent.Volatile
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 
 class PresetRepositoryImpl(
     private val presetsApi: PresetsApi,
 ) : PresetRepository {
 
-    @Volatile
+    private val cacheMutex = Mutex()
     private var cachedPresets: List<Preset>? = null
 
     override suspend fun getAll(): Result<List<Preset>> {
         return safeApiCall {
-            if (cachedPresets != null) return@safeApiCall cachedPresets!!
+            cacheMutex.withLock {
-            val presets = presetsApi.getPresets()
+                cachedPresets?.let { return@withLock it }
-            cachedPresets = presets
+                val presets = presetsApi.getPresets()
-            presets
+                cachedPresets = presets
+                presets
+            }
         }
     }
 
     override suspend fun create(preset: Preset): Result<Preset> {
         return safeApiCall {
             val created = presetsApi.createPreset(preset)
-            cachedPresets = null
+            cacheMutex.withLock { cachedPresets = null }
             created
         }
     }
@@ -33,7 +36,7 @@ class PresetRepositoryImpl(
     override suspend fun update(preset: Preset): Result<Preset> {
         return safeApiCall {
             val updated = presetsApi.updatePreset(preset)
-            cachedPresets = null
+            cacheMutex.withLock { cachedPresets = null }
             updated
         }
     }
@@ -41,7 +44,7 @@ class PresetRepositoryImpl(
     override suspend fun delete(presetId: String): Result<Unit> {
         return safeApiCall {
             presetsApi.deletePreset(presetId)
-            cachedPresets = null
+            cacheMutex.withLock { cachedPresets = null }
         }
     }
 }