Fix memory leaks and hot-path inefficiencies

Per-account Room handles for AppDatabase, LogDatabase, and HistoryDatabase were lazy-loaded into ConcurrentHashMaps in Amber and never closed on logout, leaking three open connections plus worker threads per removed account. ApplicationNameCache was an unbounded ConcurrentHashMap keyed by app package, IntentRateLimiter's cleanup only triggered above 256 buckets after an hour, and the duplicate-event LRU was capped at 10 (too small under bursty NIP-46 traffic). EventNotificationConsumer's bunker-event path called runBlocking inside an O(accounts × connections) scan that decrypted each account and re-encoded every connection's pubkey on every relay event, starving the IO dispatcher under load. This change: - Adds Amber.closeDatabasesFor(npub) and wires it into LocalPreferences.updatePrefsForLogout so Room connections are released. - Replaces ApplicationNameCache's map with an LruCache(256) and exposes clearForAccount(npub) for use during logout. - Lowers IntentRateLimiter cleanup thresholds to 64 buckets / 5 minutes. - Bumps notificationCache to 512 entries. - Introduces LocalKeyAccountIndex, populated by NotificationSubscription.updateFilter, so EventNotificationConsumer.consume resolves the connection for an incoming bunker event in O(1) without runBlocking. consume() is now suspend and is launched on applicationIOScope by NotificationSubscription. - Hoists the ProcessLifecycleOwner observer out of runMigrations() and guards registration so re-entry can't double-fire callbacks. - Adds dispose() / idempotent-registration to NotificationSubscription, ProfileSubscription, and ZapstoreUpdater. - Replaces ToastManager's per-call applicationIOScope.launch with tryEmit on its DROP_OLDEST SharedFlow. https://claude.ai/code/session_01GiLqaAahYySDdmBa4VksWR
2026-10-05 19:08:23 +00:00 · 2026-05-09 17:46:19 +00:00
parent 70b98c4018
commit 38445c0018
12 changed files with 187 additions and 68 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,26 @@
+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)
+    }
+
+    fun clearForAccount(npub: String) {
+        byLocalPubKey.entries.removeAll { it.value.npub == npub }
+    }
+}
@@ -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,
@@ -43,15 +44,31 @@ class NotificationSubscription(
    private val eventNotificationConsumer = EventNotificationConsumer(appContext)
    private val subIds = mutableMapOf<String, String>()

+    @Volatile
+    private var registered = false
+
    init {
-        // listens until the app crashes.
-        client.addConnectionListener(this)
+        // listens until the app crashes — guard so re-init in tests
+        // doesn't double-register the listener.
+        if (!registered) {
+            client.addConnectionListener(this)
+            registered = true
+        }
+    }
+
+    fun dispose() {
+        if (registered) {
+            client.removeConnectionListener(this)
+            registered = false
+        }
    }

    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 +90,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 +102,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 +154,9 @@ class NotificationSubscription(
                client.unsubscribe(subId)
            }
        }
+
+        // Refresh the localPubKey -> account index used by EventNotificationConsumer.
+        LocalKeyAccountIndex.replaceAll(indexEntries)
    }

    private fun computeSince(): Long {
@@ -54,9 +54,21 @@ class ProfileSubscription(
    private val relaysPerSubId = mutableMapOf<String, MutableSet<NormalizedRelayUrl>>()
    private val timeoutJobs = mutableMapOf<String, Job>()

+    @Volatile
+    private var registered = false
+
    init {
-        // listens until the app crashes.
-        client.addConnectionListener(this)
+        if (!registered) {
+            client.addConnectionListener(this)
+            registered = true
+        }
+    }
+
+    fun dispose() {
+        if (registered) {
+            client.removeConnectionListener(this)
+            registered = false
+        }
    }

    override fun onIncomingMessage(relay: IRelayClient, msgStr: String, msg: Message) {
@@ -66,8 +66,21 @@ class ZapstoreUpdater(
        UPDATE_RELAY_URLS.mapNotNull { RelayUrlNormalizer.normalizeOrNull(it) }
    private var timeoutJob: Job? = null

+    @Volatile
+    private var registered = false
+
    init {
-        client.addConnectionListener(this)
+        if (!registered) {
+            client.addConnectionListener(this)
+            registered = true
+        }
+    }
+
+    fun dispose() {
+        if (registered) {
+            client.removeConnectionListener(this)
+            registered = false
+        }
    }

    override fun onIncomingMessage(relay: IRelayClient, msgStr: String, msg: Message) {
@@ -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
                }
            }