fix: enforce LRU only over device-specific DBs, add one-time deletion of legacy DB on switch (guarded by prefs flag) (meshtastic#3648)

mdecourcy · web-flow · commit bde7c4793121 · 2025-11-11T16:14:12.000Z
diff --git a/core/database/build.gradle.kts b/core/database/build.gradle.kts
@@ -38,5 +38,6 @@ dependencies {
     implementation(libs.kotlinx.serialization.json)
     implementation(libs.timber)
 
+    androidTestImplementation(libs.androidx.test.runner)
     androidTestImplementation(libs.androidx.test.ext.junit)
 }
diff --git a/core/database/src/androidTest/kotlin/org/meshtastic/core/database/DatabaseManagerLegacyCleanupTest.kt b/core/database/src/androidTest/kotlin/org/meshtastic/core/database/DatabaseManagerLegacyCleanupTest.kt
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2025 Meshtastic LLC
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package org.meshtastic.core.database
+
+import android.app.Application
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.runBlocking
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class DatabaseManagerLegacyCleanupTest {
+    @Test
+    fun deletes_legacy_db_on_switch_when_flag_not_set() = runBlocking {
+        val app = ApplicationProvider.getApplicationContext<Application>()
+        val prefs = app.getSharedPreferences("db-manager-prefs", Context.MODE_PRIVATE)
+
+        // Reset the one-time flag
+        prefs.edit().remove(DatabaseConstants.LEGACY_DB_CLEANED_KEY).apply()
+
+        // Ensure legacy DB file exists
+        val legacyName = DatabaseConstants.LEGACY_DB_NAME
+        val legacyFile = app.getDatabasePath(legacyName)
+        // Create or overwrite the legacy DB file by opening it once
+        app.openOrCreateDatabase(legacyName, Context.MODE_PRIVATE, null).close()
+        assertTrue("Precondition: legacy DB should exist before switch", legacyFile.exists())
+
+        val manager = DatabaseManager(app)
+
+        // Switch to a non-null address so active DB != legacy
+        manager.switchActiveDatabase("01:23:45:67:89:AB")
+
+        // Cleanup runs asynchronously; wait briefly for deletion
+        var attempts = 0
+        while (legacyFile.exists() && attempts < 20) {
+            delay(100)
+            attempts++
+        }
+
+        assertFalse("Legacy DB should be deleted after switch", legacyFile.exists())
+    }
+}
diff --git a/core/database/src/main/kotlin/org/meshtastic/core/database/DatabaseManager.kt b/core/database/src/main/kotlin/org/meshtastic/core/database/DatabaseManager.kt
@@ -79,6 +79,9 @@ class DatabaseManager @Inject constructor(private val app: Application) {
     suspend fun switchActiveDatabase(address: String?) = mutex.withLock {
         val dbName = buildDbName(address)
 
+        // Remember the previously active DB name (if any) so we can record its last-used time as well.
+        val previousDbName = _currentDb.value?.openHelper?.databaseName
+
         // Fast path: no-op if already on this address
         if (_currentAddress.value == address && _currentDb.value != null) {
             markLastUsed(dbName)
@@ -93,10 +96,16 @@ class DatabaseManager @Inject constructor(private val app: Application) {
         _currentDb.value = db
         _currentAddress.value = address
         markLastUsed(dbName)
+        // Also mark the previous DB as used "just now" so LRU has an accurate, recent timestamp
+        // even on first run after upgrade where no timestamp might exist yet.
+        previousDbName?.let { markLastUsed(it) }
 
         // Defer LRU eviction so switch is not blocked by filesystem work
         managerScope.launch(Dispatchers.IO) { enforceCacheLimit(activeDbName = dbName) }
 
+        // One-time cleanup: remove legacy DB if present and not active
+        managerScope.launch(Dispatchers.IO) { cleanupLegacyDbIfNeeded(activeDbName = dbName) }
+
         Timber.i("Switched active DB to ${anonymizeDbName(dbName)} for address ${anonymizeAddress(address)}")
     }
 
@@ -126,8 +135,23 @@ class DatabaseManager @Inject constructor(private val app: Application) {
     private suspend fun enforceCacheLimit(activeDbName: String) = mutex.withLock {
         val limit = getCacheLimit()
         val all = listExistingDbNames()
-        if (all.size <= limit) return
-        val victims = all.filter { it != activeDbName }.sortedBy { lastUsed(it) }.take(all.size - limit)
+        // Only enforce the limit over device-specific DBs; exclude legacy and default DBs
+        val deviceDbs =
+            all.filterNot { it == DatabaseConstants.LEGACY_DB_NAME || it == DatabaseConstants.DEFAULT_DB_NAME }
+        Timber.d(
+            "LRU check: limit=%d, active=%s, deviceDbs=%s",
+            limit,
+            anonymizeDbName(activeDbName),
+            deviceDbs.joinToString(", ") { anonymizeDbName(it) },
+        )
+        if (deviceDbs.size <= limit) return
+        val usageSnapshot = deviceDbs.associateWith { lastUsed(it) }
+        Timber.d(
+            "LRU lastUsed(ms): %s",
+            usageSnapshot.entries.joinToString(", ") { (name, ts) -> "${anonymizeDbName(name)}=$ts" },
+        )
+        val victims = selectEvictionVictims(deviceDbs, activeDbName, limit, usageSnapshot)
+        Timber.i("LRU victims: %s", victims.joinToString(", ") { anonymizeDbName(it) })
         victims.forEach { name ->
             runCatching { dbCache.remove(name)?.close() }
                 .onFailure { Timber.w(it, "Failed to close database %s", name) }
@@ -150,6 +174,28 @@ class DatabaseManager @Inject constructor(private val app: Application) {
         val active = _currentDb.value?.openHelper?.databaseName ?: defaultDbName()
         managerScope.launch(Dispatchers.IO) { enforceCacheLimit(activeDbName = active) }
     }
+
+    private suspend fun cleanupLegacyDbIfNeeded(activeDbName: String) = mutex.withLock {
+        if (prefs.getBoolean(DatabaseConstants.LEGACY_DB_CLEANED_KEY, false)) return
+        val legacy = DatabaseConstants.LEGACY_DB_NAME
+        if (legacy == activeDbName) {
+            // Never delete the active DB; mark as cleaned to avoid repeated checks
+            prefs.edit().putBoolean(DatabaseConstants.LEGACY_DB_CLEANED_KEY, true).apply()
+            return
+        }
+        val legacyFile = getDbFile(app, legacy)
+        if (legacyFile != null) {
+            runCatching { dbCache.remove(legacy)?.close() }
+                .onFailure { Timber.w(it, "Failed to close legacy database %s before deletion", legacy) }
+            val deleted = app.deleteDatabase(legacy)
+            if (deleted) {
+                Timber.i("Deleted legacy DB ${anonymizeDbName(legacy)}")
+            } else {
+                Timber.w("Attempted to delete legacy DB %s but deleteDatabase returned false", legacy)
+            }
+        }
+        prefs.edit().putBoolean(DatabaseConstants.LEGACY_DB_CLEANED_KEY, true).apply()
+    }
 }
 
 object DatabaseConstants {
@@ -162,6 +208,8 @@ object DatabaseConstants {
     const val MIN_CACHE_LIMIT: Int = 1
     const val MAX_CACHE_LIMIT: Int = 10
 
+    const val LEGACY_DB_CLEANED_KEY: String = "legacy_db_cleaned"
+
     // Display/truncation and hash sizing for DB names
     const val DB_NAME_HASH_LEN: Int = 10
     const val DB_NAME_SEPARATOR_LEN: Int = 1
@@ -175,7 +223,16 @@ object DatabaseConstants {
 // File-private helpers (kept outside the class to reduce class function count)
 private fun defaultDbName(): String = DatabaseConstants.DEFAULT_DB_NAME
 
-private fun normalizeAddress(addr: String?): String = addr?.uppercase()?.replace(":", "") ?: "DEFAULT"
+private fun normalizeAddress(addr: String?): String {
+    val u = addr?.trim()?.uppercase()
+    val normalized =
+        when {
+            u.isNullOrBlank() -> "DEFAULT"
+            u == "N" || u == "NULL" -> "DEFAULT"
+            else -> u.replace(":", "")
+        }
+    return normalized
+}
 
 private fun shortSha1(s: String): String = MessageDigest.getInstance("SHA-1")
     .digest(s.toByteArray())
@@ -216,3 +273,37 @@ private fun buildRoomDb(app: Application, dbName: String): MeshtasticDatabase =
         .build()
 
 private fun getDbFile(app: Application, dbName: String): File? = app.getDatabasePath(dbName).takeIf { it.exists() }
+
+/**
+ * Compute which DBs to evict using LRU policy.
+ *
+ * Rules:
+ * - Only consider device-specific DBs (exclude legacy and default)
+ * - Never evict the active DB
+ * - If number of device DBs is within the limit, evict none
+ * - Otherwise evict the (size - limit) least-recently-used DBs
+ *
+ * Pass a precomputed [lastUsedMsByDb] snapshot to avoid redundant IO/lookups.
+ */
+internal fun selectEvictionVictims(
+    dbNames: List<String>,
+    activeDbName: String,
+    limit: Int,
+    lastUsedMsByDb: Map<String, Long>,
+): List<String> {
+    val deviceDbNames =
+        dbNames.filterNot { it == DatabaseConstants.LEGACY_DB_NAME || it == DatabaseConstants.DEFAULT_DB_NAME }
+    val victims =
+        if (limit < 1 || deviceDbNames.size <= limit) {
+            emptyList()
+        } else {
+            val candidates = deviceDbNames.filter { it != activeDbName }
+            if (candidates.isEmpty()) {
+                emptyList()
+            } else {
+                val toEvict = deviceDbNames.size - limit
+                candidates.sortedBy { lastUsedMsByDb[it] ?: 0L }.take(toEvict)
+            }
+        }
+    return victims
+}
diff --git a/core/database/src/test/kotlin/org/meshtastic/core/database/DatabaseManagerEvictionTest.kt b/core/database/src/test/kotlin/org/meshtastic/core/database/DatabaseManagerEvictionTest.kt
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2025 Meshtastic LLC
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package org.meshtastic.core.database
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class DatabaseManagerEvictionTest {
+    private val a = "meshtastic_database_a111111111"
+    private val b = "meshtastic_database_b222222222"
+    private val c = "meshtastic_database_c333333333"
+    private val d = "meshtastic_database_d444444444"
+    private val legacy = DatabaseConstants.LEGACY_DB_NAME // "meshtastic_database"
+    private val defaultDb = DatabaseConstants.DEFAULT_DB_NAME // "meshtastic_database_default"
+
+    @Test
+    fun `does not evict when count equals limit`() {
+        val names = listOf(a, b, c)
+        val victims =
+            selectEvictionVictims(names, activeDbName = a, limit = 3, lastUsedMsByDb = names.associateWith { 100L })
+        assertTrue(victims.isEmpty())
+    }
+
+    @Test
+    fun `never evicts active even if oldest`() {
+        val names = listOf(a, b, c, d)
+        val lastUsed = mapOf(a to 1L, b to 2L, c to 3L, d to 4L)
+        val victims = selectEvictionVictims(names, activeDbName = a, limit = 3, lastUsedMsByDb = lastUsed)
+        // Oldest overall is a, but active must not be evicted -> next oldest is b
+        assertEquals(listOf(b), victims)
+    }
+
+    @Test
+    fun `evicts two oldest when over limit by two`() {
+        val names = listOf(a, b, c, d)
+        val lastUsed = mapOf(a to 10L, b to 20L, c to 30L, d to 40L)
+        val victims = selectEvictionVictims(names, activeDbName = d, limit = 2, lastUsedMsByDb = lastUsed)
+        // Need to evict 2; oldest are a, then b
+        assertEquals(listOf(a, b), victims)
+    }
+
+    @Test
+    fun `excludes legacy and default from accounting`() {
+        val names = listOf(a, b, legacy, defaultDb)
+        val lastUsed = mapOf(a to 10L, b to 5L)
+        val victims = selectEvictionVictims(names, activeDbName = a, limit = 1, lastUsedMsByDb = lastUsed)
+        // Only device DBs a & b are counted; with limit 1 and active=a, evict b
+        assertEquals(listOf(b), victims)
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -38,5 +38,6 @@ dependencies {`
`38`	`38`	`implementation(libs.kotlinx.serialization.json)`
`39`	`39`	`implementation(libs.timber)`
`40`	`40`
	`41`	`+ androidTestImplementation(libs.androidx.test.runner)`
`41`	`42`	`androidTestImplementation(libs.androidx.test.ext.junit)`
`42`	`43`	`}`