diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/database/CachingApplicationDao.kt b/app/src/main/java/com/greenart7c3/nostrsigner/database/CachingApplicationDao.kt index 2f7516cb..cb6a2f8a 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/database/CachingApplicationDao.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/database/CachingApplicationDao.kt @@ -17,6 +17,16 @@ import androidx.paging.PagingSource * (coarse-grained per-app). Every mutation in [ApplicationDao] is scoped to a * single app key, so this matches the natural granularity without forcing * callers to know which exact (type, kind, relay) tuple they touched. + * + * [getAll] results are cached per account pubKey in a separate small LRU. + * That query decrypts every application row (AndroidKeyStore AES-GCM, a binder + * hop to keystore2 per field) and [com.greenart7c3.nostrsigner.service.NotificationSubscription] + * re-runs it on every ~30s relay-refresh cycle, which made it the app's + * dominant native allocator. Any mutation of the `application` table evicts + * the affected account's entry (or all entries when only the app key — not + * the owning account — is known). Cached lists are handed out as defensive + * copies; callers must not expect in-place edits of returned entities to be + * visible (all current call sites are read-only consumers). */ class CachingApplicationDao( private val delegate: ApplicationDao, @@ -38,12 +48,22 @@ class CachingApplicationDao( private val cache = LruCache(MAX_ENTRIES) + // One entry per account (key = account pubKey); a handful at most. + private val getAllCache = LruCache>(GET_ALL_MAX_ENTRIES) + private fun lookup(key: Key): Value? = synchronized(cache) { cache.get(key) } private fun store(key: Key, value: Value) { synchronized(cache) { cache.put(key, value) } } + /** Evicts the cached [getAll] list for [pubKey], or every list when null. */ + private fun invalidateGetAll(pubKey: String?) { + synchronized(getAllCache) { + if (pubKey != null) getAllCache.remove(pubKey) else getAllCache.evictAll() + } + } + private fun invalidateApp(pkKey: String) { synchronized(cache) { val toRemove = cache.snapshot().keys.filter { it.pkKey == pkKey } @@ -122,6 +142,7 @@ class CachingApplicationDao( override suspend fun insertApplicationWithPermissions(application: ApplicationWithPermissions) { delegate.insertApplicationWithPermissions(application) invalidateApp(application.application.key) + invalidateGetAll(application.application.pubKey) } override suspend fun deletePermissions(key: String) { @@ -157,11 +178,15 @@ class CachingApplicationDao( override suspend fun delete(entity: ApplicationEntity) { delegate.delete(entity) invalidateApp(entity.key) + invalidateGetAll(entity.pubKey) } override suspend fun delete(key: String) { delegate.delete(key) invalidateApp(key) + // Only the app key is known here, not the owning account, so drop every + // getAll entry — there is at most one per account. + invalidateGetAll(null) } // Affects many apps at once (timestamp predicate), so wipe the whole cache. @@ -172,13 +197,21 @@ class CachingApplicationDao( override suspend fun deleteOldApplications(time: Long): Int { val n = delegate.deleteOldApplications(time) - if (n > 0) invalidateAll() + if (n > 0) { + invalidateAll() + invalidateGetAll(null) + } return n } // -------- pass-through (not cached, not invalidating) -------- - override suspend fun getAll(pubKey: String): List = delegate.getAll(pubKey) + override suspend fun getAll(pubKey: String): List { + synchronized(getAllCache) { getAllCache.get(pubKey) }?.let { return it.toList() } + val result = delegate.getAll(pubKey) + synchronized(getAllCache) { getAllCache.put(pubKey, result) } + return result.toList() + } override suspend fun getAllNotConnected(): List = delegate.getAllNotConnected() @@ -211,23 +244,28 @@ class CachingApplicationDao( // ApplicationEntity columns like signPolicy and lastUsed feed cached reads // (getSignPolicy), so invalidate the affected app's entries. invalidateApp(event.key) + invalidateGetAll(event.pubKey) return result } override fun insertAll(events: List): List? { val result = delegate.insertAll(events) events.forEach { invalidateApp(it.key) } + events.asSequence().map { it.pubKey }.distinct().forEach(::invalidateGetAll) return result } override suspend fun updateLastUsed(key: String, time: Long) { delegate.updateLastUsed(key, time) - // lastUsed doesn't affect any cached read, so no invalidation needed. + // lastUsed doesn't affect any cached read of the permission cache, but it + // is a column of getAll's projection, so the per-account list goes stale. + invalidateGetAll(null) } override suspend fun updateNameAndIcon(key: String, name: String, icon: String) { delegate.updateNameAndIcon(key, name, icon) invalidateApp(key) + invalidateGetAll(null) } // -------- *Raw delegations (envelope-encrypted persistence boundary) -------- @@ -255,9 +293,14 @@ class CachingApplicationDao( override suspend fun getAllWithLocalKeyRaw(pubKey: String): List = delegate.getAllWithLocalKeyRaw(pubKey) - // Key-rotation write path: secret/localKey never feed a cached read, so a - // plain delegation with no invalidation is correct. - override suspend fun updateEncryptedColumnsRaw(key: String, secret: String, localKey: String) = delegate.updateEncryptedColumnsRaw(key, secret, localKey) + // Key-rotation write path: secret/localKey never feed a cached read of the + // permission cache, but they ARE columns of getAll's projection, so evict + // the per-account lists (SecureCryptoHelper.rotateKey calls this through + // the caching wrapper, ciphertext in / ciphertext out). + override suspend fun updateEncryptedColumnsRaw(key: String, secret: String, localKey: String) { + delegate.updateEncryptedColumnsRaw(key, secret, localKey) + invalidateGetAll(null) + } override suspend fun insertApplicationRaw(event: ApplicationEntity): Long? = delegate.insertApplicationRaw(event) @@ -271,5 +314,6 @@ class CachingApplicationDao( companion object { private const val MAX_ENTRIES = 512 + private const val GET_ALL_MAX_ENTRIES = 8 } } diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/database/HistoryDao.kt b/app/src/main/java/com/greenart7c3/nostrsigner/database/HistoryDao.kt index c4146df5..72b65ba0 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/database/HistoryDao.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/database/HistoryDao.kt @@ -129,7 +129,9 @@ interface HistoryDao { val lastUsed = entities.maxByOrNull { it.time }?.time ?: TimeUtils.now() val pkKey = entities.firstOrNull()?.pkKey if (pkKey != null) { - Amber.instance.getDatabase(npub).dao().updateLastUsed(pkKey, lastUsed) + // Cached dao so the write invalidates CachingApplicationDao's + // getAll entry (lastUsed is a column of that projection). + Amber.instance.dao(npub).updateLastUsed(pkKey, lastUsed) } } } catch (e: Exception) { diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/service/ConnectivityService.kt b/app/src/main/java/com/greenart7c3/nostrsigner/service/ConnectivityService.kt index 65b21f40..f5cf84bc 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/service/ConnectivityService.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/service/ConnectivityService.kt @@ -113,10 +113,13 @@ class ConnectivityService : Service() { LocalPreferences.allSavedAccounts(this).forEach { Amber.instance.databases[it.npub] = Amber.instance.getDatabase(it.npub) Amber.instance.applicationIOScope.launch { - Amber.instance.databases[it.npub]?.dao()?.getAllNotConnected()?.forEach { app -> + // Cached dao so the write invalidates CachingApplicationDao's getAll + // entry for this account (the service can restart in a live process). + val dao = Amber.instance.dao(it.npub) + dao.getAllNotConnected()?.forEach { app -> if (app.application.secret.isNotEmpty() && app.application.secret != app.application.key) { app.application.isConnected = true - Amber.instance.databases[it.npub]?.dao()?.insertApplicationWithPermissions(app) + dao.insertApplicationWithPermissions(app) } } } diff --git a/app/src/main/java/com/greenart7c3/nostrsigner/service/NotificationSubscription.kt b/app/src/main/java/com/greenart7c3/nostrsigner/service/NotificationSubscription.kt index 8a56146b..c7bba10a 100644 --- a/app/src/main/java/com/greenart7c3/nostrsigner/service/NotificationSubscription.kt +++ b/app/src/main/java/com/greenart7c3/nostrsigner/service/NotificationSubscription.kt @@ -86,7 +86,12 @@ class NotificationSubscription( LocalPreferences.allAccounts(appContext).forEach { account -> val since = computeSince() - val allConnections = Amber.instance.getDatabase(account.npub).dao().getAll(account.hexKey) + // Cached dao: getAll() is served from CachingApplicationDao's per-account + // LRU. updateFilter re-runs on every ~30s relay refresh, and an uncached + // getAll re-decrypts every application row through AndroidKeyStore + // (a keystore2 binder round-trip per encrypted field) each cycle — + // previously the app's dominant native allocator. + val allConnections = Amber.instance.dao(account.npub).getAll(account.hexKey) val connectionsWithLocalKey = allConnections.filter { it.localKey.isNotEmpty() } val hasLegacyConnections = allConnections.any { it.localKey.isEmpty() && it.relays.isNotEmpty() } diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/database/CachingApplicationDaoTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/database/CachingApplicationDaoTest.kt new file mode 100644 index 00000000..3986f3a8 --- /dev/null +++ b/app/src/test/java/com/greenart7c3/nostrsigner/database/CachingApplicationDaoTest.kt @@ -0,0 +1,200 @@ +package com.greenart7c3.nostrsigner.database + +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotSame +import org.junit.Before +import org.junit.Test + +/** + * Verifies the per-account getAll() read-through cache introduced because + * NotificationSubscription.updateFilter() re-runs getAll on every ~30s relay + * refresh and each call used to re-decrypt every application row through + * AndroidKeyStore. The delegate must be hit once per cache lifetime, and every + * application-table mutation must evict the cached list. + */ +class CachingApplicationDaoTest { + private lateinit var delegate: ApplicationDao + private lateinit var dao: CachingApplicationDao + + private val accountKey = "accounthexpubkey" + + private fun entity(key: String = "app1") = ApplicationEntity.empty().copy( + key = key, + pubKey = accountKey, + localKey = "ab".repeat(32), + ) + + @Before + fun setUp() { + delegate = mockk(relaxed = true) + dao = CachingApplicationDao(delegate) + } + + private suspend fun stubGetAll(vararg entities: ApplicationEntity) { + coEvery { delegate.getAll(accountKey) } returns entities.toList() + } + + @Test + fun `getAll hits the delegate once per cache lifetime`() = runBlocking { + stubGetAll(entity()) + + dao.getAll(accountKey) + dao.getAll(accountKey) + dao.getAll(accountKey) + + coVerify(exactly = 1) { delegate.getAll(accountKey) } + } + + @Test + fun `getAll returns equal lists on cache hits`() = runBlocking { + val first = entity() + stubGetAll(first) + + assertEquals(listOf(first), dao.getAll(accountKey)) + assertEquals(listOf(first), dao.getAll(accountKey)) + } + + @Test + fun `returned list is a defensive copy of the cached list`() = runBlocking { + stubGetAll(entity()) + + val first = dao.getAll(accountKey) + val second = dao.getAll(accountKey) + + // Cache hits must not hand out the same mutable list instance callers + // could alias; equality stays intact. + assertNotSame(first, second) + assertEquals(first, second) + } + + @Test + fun `insertApplication evicts the owning account's cached list`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.insertApplication(entity()) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `insertAll evicts the owning account's cached list`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.insertAll(listOf(entity())) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `insertApplicationWithPermissions evicts the owning account's cached list`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + val withPermissions = ApplicationWithPermissions(entity(), mutableListOf()) + dao.insertApplicationWithPermissions(withPermissions) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `delete by entity evicts the owning account's cached list`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.delete(entity()) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `delete by key evicts all cached lists`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.delete("app1") + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `updateLastUsed evicts all cached lists`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.updateLastUsed("app1", 123L) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `updateNameAndIcon evicts all cached lists`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.updateNameAndIcon("app1", "name", "icon") + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `updateEncryptedColumnsRaw evicts all cached lists`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.updateEncryptedColumnsRaw("app1", "secret", "localKey") + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `deleteOldApplications evicts cached lists when rows are removed`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + coEvery { delegate.deleteOldApplications(any()) } returns 1 + dao.deleteOldApplications(0L) + + dao.getAll(accountKey) + coVerify(exactly = 2) { delegate.getAll(accountKey) } + } + + @Test + fun `permission-only writes do not evict the cached list`() = runBlocking { + stubGetAll(entity()) + dao.getAll(accountKey) + + dao.deletePermissions("app1") + + dao.getAll(accountKey) + coVerify(exactly = 1) { delegate.getAll(accountKey) } + } + + @Test + fun `accounts are cached independently`() = runBlocking { + val otherAccount = "otheraccounthexkey" + coEvery { delegate.getAll(accountKey) } returns listOf(entity()) + coEvery { delegate.getAll(otherAccount) } returns emptyList() + + dao.getAll(accountKey) + dao.getAll(otherAccount) + dao.getAll(accountKey) + dao.getAll(otherAccount) + + coVerify(exactly = 1) { delegate.getAll(accountKey) } + coVerify(exactly = 1) { delegate.getAll(otherAccount) } + } +} diff --git a/app/src/test/java/com/greenart7c3/nostrsigner/service/NotificationSubscriptionTest.kt b/app/src/test/java/com/greenart7c3/nostrsigner/service/NotificationSubscriptionTest.kt index d2ae22c5..4b471cef 100644 --- a/app/src/test/java/com/greenart7c3/nostrsigner/service/NotificationSubscriptionTest.kt +++ b/app/src/test/java/com/greenart7c3/nostrsigner/service/NotificationSubscriptionTest.kt @@ -7,6 +7,7 @@ import com.greenart7c3.nostrsigner.LocalPreferences import com.greenart7c3.nostrsigner.database.AppDatabase import com.greenart7c3.nostrsigner.database.ApplicationDao import com.greenart7c3.nostrsigner.database.ApplicationEntity +import com.greenart7c3.nostrsigner.database.CachingApplicationDao import com.greenart7c3.nostrsigner.models.Account import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.NostrClient @@ -51,6 +52,9 @@ class NotificationSubscriptionTest { val amber = mockk(relaxed = true) every { amber.getDatabase(any()) } returns database + // updateFilter reads through the caching wrapper; a fresh wrapper per call + // mirrors per-test dao state without cross-test cache leakage. + every { amber.dao(any()) } answers { CachingApplicationDao(dao) } every { amber.notificationCache } returns LruCache(512) installAmberInstance(amber)