Merge pull request #430 from greenart7c3/claude/fix-performance-leaks-DKhGr

Fix memory leaks and hot-path inefficiencies
2026-10-05 19:08:23 +00:00 · 2026-05-11 07:41:01 -03:00
parent 1bdbdc0208 da8351b7d5
commit 2426ca453d
10 changed files with 139 additions and 63 deletions
@@ -43,6 +43,7 @@ import com.greenart7c3.nostrsigner.okhttp.HttpClientManager
 import com.greenart7c3.nostrsigner.okhttp.OkHttpWebSocket
 import com.greenart7c3.nostrsigner.relays.AmberRelayStats
 import com.greenart7c3.nostrsigner.relays.NostrClientLoggerListener
+import com.greenart7c3.nostrsigner.service.ApplicationNameCache
 import com.greenart7c3.nostrsigner.service.ClearLogsWorker
 import com.greenart7c3.nostrsigner.service.ConnectivityService
 import com.greenart7c3.nostrsigner.service.NotificationSubscription
@@ -147,6 +148,36 @@ class Amber :
    private var logDatabases = ConcurrentHashMap<String, LogDatabase>()
    private var historyDatabases = ConcurrentHashMap<String, HistoryDatabase>()

+    // Hoisted out of runMigrations() so re-entry (e.g. test re-init) can't
+    // double-register and double-fire foreground/background callbacks.
+    private var processLifecycleObserverRegistered = false
+    private val processLifecycleObserver = object : DefaultLifecycleObserver {
+        override fun onStart(owner: LifecycleOwner) {
+            Log.d("ProcessLifecycleOwner", "App in foreground")
+            isAppInForeground = true
+
+            // activates the profile filter only when the app is in the foreground
+            if (!settings.killSwitch.value) {
+                applicationIOScope.launch {
+                    profileSubscription.updateFilter()
+                    if (settings.autoCheckUpdates) {
+                        maybeCheckForUpdates()
+                    }
+                }
+            }
+        }
+
+        override fun onStop(owner: LifecycleOwner) {
+            Log.d("ProcessLifecycleOwner", "App in background")
+            isAppInForeground = false
+
+            // closes the filter when in the background
+            applicationIOScope.launch {
+                profileSubscription.closeSub()
+            }
+        }
+    }
+
    val isOnMobileDataState = mutableStateOf(false)
    val isOnWifiDataState = mutableStateOf(false)
    val isOnOfflineState = mutableStateOf(false)
@@ -155,7 +186,11 @@ class Amber :
    val keystoreFailedAccounts = MutableStateFlow<List<String>>(emptyList())

    @Volatile var intentionalDisconnectTime = 0L
-    val notificationCache = LruCache<String, Long>(10)
+
+    // Capacity 10 was too small under bursty NIP-46 traffic — duplicate-detection
+    // started missing recent events. 512 covers high-volume relays without being
+    // a meaningful memory cost (event-id hex strings + Long).
+    val notificationCache = LruCache<String, Long>(512)

    fun isSocksProxyAlive(proxyHost: String, proxyPort: Int): Boolean {
        if (settings.torMode == TorMode.BUILTIN) {
@@ -294,32 +329,10 @@ class Amber :
                }

                launch(Dispatchers.Main) {
-                    ProcessLifecycleOwner.get().lifecycle.addObserver(object : DefaultLifecycleObserver {
-                        override fun onStart(owner: LifecycleOwner) {
-                            Log.d("ProcessLifecycleOwner", "App in foreground")
-                            isAppInForeground = true
-
-                            // activates the profile filter only when the app is in the foreground
-                            if (!settings.killSwitch.value) {
-                                applicationIOScope.launch {
-                                    profileSubscription.updateFilter()
-                                    if (settings.autoCheckUpdates) {
-                                        maybeCheckForUpdates()
-                                    }
-                                }
-                            }
-                        }
-
-                        override fun onStop(owner: LifecycleOwner) {
-                            Log.d("ProcessLifecycleOwner", "App in background")
-                            isAppInForeground = false
-
-                            // closes the filter when in the background
-                            applicationIOScope.launch {
-                                profileSubscription.closeSub()
-                            }
-                        }
-                    })
+                    if (!processLifecycleObserverRegistered) {
+                        ProcessLifecycleOwner.get().lifecycle.addObserver(processLifecycleObserver)
+                        processLifecycleObserverRegistered = true
+                    }
                }

                // Wait for Tor to be ready before establishing relay connections
@@ -422,6 +435,18 @@ class Amber :
        return historyDatabases[npub]!!
    }

+    /**
+     * Closes and evicts every cached Room handle for [npub] so file locks and
+     * worker threads are released. Called from the logout path; without it each
+     * removed account leaks 3 open Room connections forever.
+     */
+    fun closeDatabasesFor(npub: String) {
+        databases.remove(npub)?.runCatching { close() }
+        logDatabases.remove(npub)?.runCatching { close() }
+        historyDatabases.remove(npub)?.runCatching { close() }
+        ApplicationNameCache.clearForAccount(npub)
+    }
+
    fun getSavedRelays(account: Account): Set<NormalizedRelayUrl> {
        val database = getDatabase(account.npub)
        val savedRelays = buildSet {
@@ -374,6 +374,10 @@ object LocalPreferences {
    @SuppressLint("ApplySharedPref")
    fun updatePrefsForLogout(npub: String, context: Context): Boolean {
        accountCache.remove(npub)
+        // Close Room handles BEFORE deleting on-disk preference/data files so
+        // we release file locks and worker-pool threads instead of leaking
+        // them for the lifetime of the process.
+        Amber.instance.closeDatabasesFor(npub)
        val userPrefs = sharedPrefs(context, npub)
        userPrefs.edit(commit = true) { clear() }
        removeAccount(context, npub)
@@ -1,7 +1,28 @@
 package com.greenart7c3.nostrsigner.service

-import java.util.concurrent.ConcurrentHashMap
+import androidx.collection.LruCache

+/**
+ * Bounded cache of "$npub-$key" -> display name. Previously an unbounded
+ * ConcurrentHashMap; long-running sessions with many connecting apps were
+ * leaking memory here. The size cap is conservative — names are short, but
+ * the cache grows with every external app the user has ever interacted with.
+ */
 object ApplicationNameCache {
-    val names = ConcurrentHashMap<String, String>()
+    private const val MAX_ENTRIES = 256
+
+    private val cache = LruCache<String, String>(MAX_ENTRIES)
+
+    operator fun get(key: String): String? = synchronized(cache) { cache.get(key) }
+
+    operator fun set(key: String, value: String) {
+        synchronized(cache) { cache.put(key, value) }
+    }
+
+    fun clearForAccount(npub: String) {
+        synchronized(cache) {
+            val toRemove = cache.snapshot().keys.filter { it.startsWith("$npub-") }
+            toRemove.forEach { cache.remove(it) }
+        }
+    }
 }
@@ -54,7 +54,6 @@ import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
 import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal
 import com.vitorpamplona.quartz.nip01Core.tags.people.taggedUsers
 import com.vitorpamplona.quartz.nip04Dm.crypto.EncryptedInfo
-import com.vitorpamplona.quartz.nip19Bech32.toNpub
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect
 import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip04Decrypt
@@ -108,7 +107,7 @@ class EventNotificationConsumer(private val applicationContext: Context) {
        }
    }

-    fun consume(
+    suspend fun consume(
        event: Event,
        relay: NormalizedRelayUrl,
    ) {
@@ -144,24 +143,18 @@ class EventNotificationConsumer(private val applicationContext: Context) {
            saveLog("Direct account match for ${acc.npub}", relay.url)
        }

-        // If not a direct account match, search for a connection with this local pubkey
+        // Connection-based match via the in-memory index populated by
+        // NotificationSubscription.updateFilter. This replaces an O(accounts × connections)
+        // synchronous scan that previously ran inside runBlocking on the relay event thread.
        if (acc == null) {
-            val accounts = LocalPreferences.allSavedAccounts(applicationContext)
-            outer@ for (accountInfo in accounts) {
-                val account = LocalPreferences.loadFromEncryptedStorageSync(applicationContext, accountInfo.npub)
-                    ?: continue
-                val connections =
-                    kotlinx.coroutines.runBlocking {
-                        Amber.instance.getDatabase(account.npub).dao().getAllWithLocalKey(account.hexKey)
-                    }
-                val taggedNpub = taggedKey.toNPub()
-                for (conn in connections) {
-                    if (conn.localKey.isNotEmpty() && conn.localPubKey.hexToByteArray().toNpub() == taggedNpub) {
-                        acc = account
-                        connectionPrivKey = conn.localKey
-                        saveLog("Found connection for ${acc.npub} with local pubkey ${conn.localPubKey}", relay.url)
-                        break@outer
-                    }
+            val taggedHex = taggedKey.pubKey
+            val match = LocalKeyAccountIndex.lookup(taggedHex)
+            if (match != null) {
+                val resolved = LocalPreferences.loadFromEncryptedStorageSync(applicationContext, match.npub)
+                if (resolved != null) {
+                    acc = resolved
+                    connectionPrivKey = match.localPrivKey
+                    saveLog("Found connection for ${acc.npub} with local pubkey $taggedHex", relay.url)
                }
            }
        }
@@ -17,8 +17,11 @@ import java.util.concurrent.ConcurrentHashMap
 object IntentRateLimiter {
    const val UNKNOWN_PACKAGE = "<unknown>"
    private const val UNKNOWN_PACKAGE_LIMIT = 3
-    private const val CLEANUP_WHEN_SIZE_EXCEEDS = 256
-    private const val CLEANUP_STALE_MS = 60L * 60L * 1000L
+
+    // Cleanup runs at every checkAndRecord; tighter bounds make it harder for a
+    // misbehaving caller to balloon the bucket map before reclamation kicks in.
+    private const val CLEANUP_WHEN_SIZE_EXCEEDS = 64
+    private const val CLEANUP_STALE_MS = 5L * 60L * 1000L

    data class BucketKey(
        val pkg: String,
@@ -0,0 +1,22 @@
+package com.greenart7c3.nostrsigner.service
+
+import java.util.concurrent.ConcurrentHashMap
+
+/**
+ * Maps a connection's local public key (hex) to the account npub it belongs to
+ * and the connection's local private key. Maintained by [NotificationSubscription.updateFilter];
+ * read by [EventNotificationConsumer.consume] to avoid an O(accounts × connections)
+ * scan with synchronous DB calls and key encoding on every incoming bunker event.
+ */
+object LocalKeyAccountIndex {
+    data class Match(val npub: String, val localPrivKey: String)
+
+    private val byLocalPubKey = ConcurrentHashMap<String, Match>()
+
+    fun lookup(localPubKey: String): Match? = byLocalPubKey[localPubKey]
+
+    fun replaceAll(entries: Map<String, Match>) {
+        byLocalPubKey.keys.retainAll(entries.keys)
+        byLocalPubKey.putAll(entries)
+    }
+}
@@ -35,6 +35,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
 import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent
 import com.vitorpamplona.quartz.utils.TimeUtils
 import java.util.UUID
+import kotlinx.coroutines.launch

 class NotificationSubscription(
    val client: NostrClient,
@@ -51,7 +52,9 @@ class NotificationSubscription(
    override fun onIncomingMessage(relay: IRelayClient, msgStr: String, msg: Message) {
        if (msg is EventMessage) {
            if (subIds.containsValue(msg.subId)) {
-                eventNotificationConsumer.consume(msg.event, relay.url)
+                Amber.instance.applicationIOScope.launch {
+                    eventNotificationConsumer.consume(msg.event, relay.url)
+                }
            }
        }
        super.onIncomingMessage(relay, msgStr, msg)
@@ -73,6 +76,7 @@ class NotificationSubscription(
    suspend fun updateFilter() {
        if (BuildFlavorChecker.isOfflineFlavor()) return
        val activeSubKeys = mutableSetOf<String>()
+        val indexEntries = mutableMapOf<String, LocalKeyAccountIndex.Match>()

        LocalPreferences.allAccounts(appContext).forEach { account ->
            val since = computeSince()
@@ -84,6 +88,7 @@ class NotificationSubscription(
            // Per-connection subscription on each connection's own relays
            for (conn in connectionsWithLocalKey) {
                val connPubKey = conn.localPubKey
+                indexEntries[connPubKey] = LocalKeyAccountIndex.Match(account.npub, conn.localKey)
                val subKey = "${account.hexKey}_$connPubKey"
                activeSubKeys.add(subKey)
                if (!subIds.containsKey(subKey)) {
@@ -135,6 +140,9 @@ class NotificationSubscription(
                client.unsubscribe(subId)
            }
        }
+
+        // Refresh the localPubKey -> account index used by EventNotificationConsumer.
+        LocalKeyAccountIndex.replaceAll(indexEntries)
    }

    private fun computeSince(): Long {
@@ -327,7 +327,7 @@ fun ApplicationName(

    LaunchedEffect(key) {
        val cacheKey = "${account.npub.toShortenHex()}-$key"
-        val cached = ApplicationNameCache.names[cacheKey]
+        val cached = ApplicationNameCache[cacheKey]
        if (cached != null) {
            name = cached
        } else {
@@ -336,7 +336,7 @@ fun ApplicationName(
            }
            if (appName != null) {
                name = appName
-                ApplicationNameCache.names[cacheKey] = appName
+                ApplicationNameCache[cacheKey] = appName
            }
        }
    }
@@ -1,10 +1,8 @@
 package com.greenart7c3.nostrsigner.ui

 import androidx.compose.runtime.Immutable
-import com.greenart7c3.nostrsigner.Amber
 import kotlinx.coroutines.channels.BufferOverflow
 import kotlinx.coroutines.flow.MutableSharedFlow
-import kotlinx.coroutines.launch

@Immutable
 open class ToastMsg
@@ -31,17 +29,19 @@ class ResourceToastMsg(
 ) : ToastMsg()

 object ToastManager {
+    // DROP_OLDEST means tryEmit always succeeds, so callers no longer need to
+    // spawn a coroutine on the application scope just to push a message.
    val toasts = MutableSharedFlow<ToastMsg?>(0, 3, onBufferOverflow = BufferOverflow.DROP_OLDEST)

    fun clearToasts() {
-        Amber.instance.applicationIOScope.launch { toasts.emit(null) }
+        toasts.tryEmit(null)
    }

    fun toast(
        title: String,
        message: String,
    ) {
-        Amber.instance.applicationIOScope.launch { toasts.emit(StringToastMsg(title, message)) }
+        toasts.tryEmit(StringToastMsg(title, message))
    }

    fun toast(
@@ -49,7 +49,7 @@ object ToastManager {
        message: String,
        onOk: () -> Unit,
    ) {
-        Amber.instance.applicationIOScope.launch { toasts.emit(ConfirmationToastMsg(title, message, onOk)) }
+        toasts.tryEmit(ConfirmationToastMsg(title, message, onOk))
    }

    fun toast(
@@ -58,6 +58,6 @@ object ToastManager {
        onAccept: () -> Unit,
        onReject: () -> Unit,
    ) {
-        Amber.instance.applicationIOScope.launch { toasts.emit(AcceptRejectToastMsg(title, message, onAccept, onReject)) }
+        toasts.tryEmit(AcceptRejectToastMsg(title, message, onAccept, onReject))
    }
 }
@@ -82,7 +82,7 @@ fun BunkerMultiEventHomeScreen(
    val key = bunkerRequests.first().localKey
    var rememberType by remember { mutableStateOf(RememberType.NEVER) }
    var relayAuthScope by remember { mutableStateOf(RelayAuthScope.SPECIFIC) }
-    var appName by remember { mutableStateOf(ApplicationNameCache.names["$localAccount-$key"] ?: key.toShortenHex()) }
+    var appName by remember { mutableStateOf(ApplicationNameCache["$localAccount-$key"] ?: key.toShortenHex()) }

    LaunchedEffect(Unit) {
        MultiEventScreenIntents.checkedStates.clear()
@@ -97,14 +97,14 @@ fun BunkerMultiEventHomeScreen(
                bunkerRequests.first().currentAccount,
            )?.npub?.toShortenHex() ?: ""

-            if (ApplicationNameCache.names["$localAccount-$key"] == null) {
+            if (ApplicationNameCache["$localAccount-$key"] == null) {
                val app = Amber.instance.getDatabase(accountParam.npub).dao().getByKey(key)
                app?.let {
                    appName = it.application.name
-                    ApplicationNameCache.names["$localAccount-$key"] = it.application.name
+                    ApplicationNameCache["$localAccount-$key"] = it.application.name
                }
            } else {
-                ApplicationNameCache.names["$localAccount-$key"]?.let {
+                ApplicationNameCache["$localAccount-$key"]?.let {
                    appName = it
                }
            }