From fd8414abe3c7ab30db9cd1daa9ebbb8804b12e73 Mon Sep 17 00:00:00 2001 From: simulationstation <32910678+simulationstation@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:36:57 -1000 Subject: [PATCH] fix(database): delete nodes and metadata atomically (#6623) --- .../datasource/NodeInfoWriteDataSource.kt | 6 +- .../SwitchingNodeInfoWriteDataSource.kt | 12 +- .../data/repository/NodeRepositoryImpl.kt | 12 +- .../NodeDeletionDatabaseSwitchTest.kt | 156 ++++++++++++++++++ .../dao/NodeInfoDaoAtomicDeleteTest.kt | 25 +++ .../core/database/dao/NodeInfoDao.kt | 22 +++ .../dao/CommonNodeInfoDaoAtomicDeleteTest.kt | 112 +++++++++++++ .../dao/NodeInfoDaoAtomicDeleteTest.kt | 19 +++ 8 files changed, 344 insertions(+), 20 deletions(-) create mode 100644 core/data/src/commonTest/kotlin/org/meshtastic/core/data/datasource/NodeDeletionDatabaseSwitchTest.kt create mode 100644 core/database/src/androidHostTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt create mode 100644 core/database/src/commonTest/kotlin/org/meshtastic/core/database/dao/CommonNodeInfoDaoAtomicDeleteTest.kt create mode 100644 core/database/src/jvmTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/NodeInfoWriteDataSource.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/NodeInfoWriteDataSource.kt index 2506d38fa6..cef300d641 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/NodeInfoWriteDataSource.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/NodeInfoWriteDataSource.kt @@ -30,11 +30,9 @@ interface NodeInfoWriteDataSource { suspend fun clearMyNodeInfo() - suspend fun deleteNode(num: Int) + suspend fun deleteNodeAndMetadata(num: Int) - suspend fun deleteNodes(nodeNums: List) - - suspend fun deleteMetadata(num: Int) + suspend fun deleteNodesAndMetadata(nodeNums: List) suspend fun upsert(metadata: MetadataEntity) diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/SwitchingNodeInfoWriteDataSource.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/SwitchingNodeInfoWriteDataSource.kt index 4a85eb70af..239b041489 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/SwitchingNodeInfoWriteDataSource.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/SwitchingNodeInfoWriteDataSource.kt @@ -51,16 +51,12 @@ class SwitchingNodeInfoWriteDataSource( withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().clearMyNodeInfo() } } } - override suspend fun deleteNode(num: Int) { - withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().deleteNode(num) } } + override suspend fun deleteNodeAndMetadata(num: Int) { + withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().deleteNodeAndMetadata(num) } } } - override suspend fun deleteNodes(nodeNums: List) { - withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().deleteNodes(nodeNums) } } - } - - override suspend fun deleteMetadata(num: Int) { - withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().deleteMetadata(num) } } + override suspend fun deleteNodesAndMetadata(nodeNums: List) { + withContext(dispatchers.io) { dbManager.withDb { it.nodeInfoDao().deleteNodesAndMetadata(nodeNums) } } } override suspend fun upsert(metadata: MetadataEntity) { diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/NodeRepositoryImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/NodeRepositoryImpl.kt index 698aa8e209..eea4a2f7d7 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/NodeRepositoryImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/NodeRepositoryImpl.kt @@ -211,16 +211,12 @@ class NodeRepositoryImpl( override suspend fun clearMyNodeInfo() = withContext(dispatchers.io) { nodeInfoWriteDataSource.clearMyNodeInfo() } /** Deletes a node and its metadata by [num]. */ - override suspend fun deleteNode(num: Int) = withContext(dispatchers.io) { - nodeInfoWriteDataSource.deleteNode(num) - nodeInfoWriteDataSource.deleteMetadata(num) - } + override suspend fun deleteNode(num: Int) = + withContext(dispatchers.io) { nodeInfoWriteDataSource.deleteNodeAndMetadata(num) } /** Deletes multiple nodes and their metadata. */ - override suspend fun deleteNodes(nodeNums: List) = withContext(dispatchers.io) { - nodeInfoWriteDataSource.deleteNodes(nodeNums) - nodeNums.forEach { nodeInfoWriteDataSource.deleteMetadata(it) } - } + override suspend fun deleteNodes(nodeNums: List) = + withContext(dispatchers.io) { nodeInfoWriteDataSource.deleteNodesAndMetadata(nodeNums) } override suspend fun getNodesOlderThan(lastHeard: Int): List = withContext(dispatchers.io) { nodeInfoReadDataSource.getNodesOlderThan(lastHeard).map { it.toModel() } } diff --git a/core/data/src/commonTest/kotlin/org/meshtastic/core/data/datasource/NodeDeletionDatabaseSwitchTest.kt b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/datasource/NodeDeletionDatabaseSwitchTest.kt new file mode 100644 index 0000000000..92bcf3bed4 --- /dev/null +++ b/core/data/src/commonTest/kotlin/org/meshtastic/core/data/datasource/NodeDeletionDatabaseSwitchTest.kt @@ -0,0 +1,156 @@ +/* + * Copyright (c) 2026 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 . + */ +package org.meshtastic.core.data.datasource + +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.flatMapLatest +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.meshtastic.core.data.repository.NodeRepositoryImpl +import org.meshtastic.core.database.DatabaseProvider +import org.meshtastic.core.database.MeshtasticDatabase +import org.meshtastic.core.database.entity.MetadataEntity +import org.meshtastic.core.database.entity.NodeEntity +import org.meshtastic.core.database.getInMemoryDatabaseBuilder +import org.meshtastic.core.di.CoroutineDispatchers +import org.meshtastic.core.testing.FakeLocalStatsDataSource +import org.meshtastic.proto.DeviceMetadata +import org.meshtastic.proto.User +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +@OptIn(ExperimentalCoroutinesApi::class) +class NodeDeletionDatabaseSwitchTest { + private val testDispatcher = UnconfinedTestDispatcher() + private val dispatchers = CoroutineDispatchers(main = testDispatcher, io = testDispatcher, default = testDispatcher) + private lateinit var dbA: MeshtasticDatabase + private lateinit var dbB: MeshtasticDatabase + private lateinit var provider: SwitchAfterFirstWriteProvider + private lateinit var lifecycleOwner: TestLifecycleOwner + private lateinit var repository: NodeRepositoryImpl + + @BeforeTest + fun setUp() { + Dispatchers.setMain(testDispatcher) + dbA = getInMemoryDatabaseBuilder().build() + dbB = getInMemoryDatabaseBuilder().build() + provider = SwitchAfterFirstWriteProvider(dbA, dbB) + lifecycleOwner = TestLifecycleOwner() + lifecycleOwner.moveTo(Lifecycle.Event.ON_CREATE) + val switchingWriteDataSource = SwitchingNodeInfoWriteDataSource(provider, dispatchers) + val writeDataSourceWithoutStartupBackfill = + object : NodeInfoWriteDataSource by switchingWriteDataSource { + override suspend fun backfillDenormalizedNames() = Unit + } + repository = + NodeRepositoryImpl( + processLifecycle = lifecycleOwner.lifecycle, + nodeInfoReadDataSource = SwitchingNodeInfoReadDataSource(provider), + nodeInfoWriteDataSource = writeDataSourceWithoutStartupBackfill, + dispatchers = dispatchers, + localStatsDataSource = FakeLocalStatsDataSource(), + ) + provider.arm() + } + + @AfterTest + fun tearDown() { + lifecycleOwner.moveTo(Lifecycle.Event.ON_DESTROY) + dbA.close() + dbB.close() + Dispatchers.resetMain() + } + + @Test + fun deleteNodeUsesOneDatabaseCaptureForNodeAndMetadata() = runTest(testDispatcher) { + seedNodeAndMetadata(dbA, "A") + seedNodeAndMetadata(dbB, "B") + + repository.deleteNode(NODE_NUM) + + assertEquals(1, provider.withDbCalls, "one logical deletion must capture the active database exactly once") + assertNull(dbA.nodeInfoDao().getNodeByNum(NODE_NUM), "database A node must be deleted") + assertFalse(dbA.hasMetadata(NODE_NUM), "database A metadata must not be orphaned") + assertNotNull(dbB.nodeInfoDao().getNodeByNum(NODE_NUM), "database B node must be untouched") + assertTrue(dbB.hasMetadata(NODE_NUM), "database B metadata must be untouched") + } + + private suspend fun seedNodeAndMetadata(database: MeshtasticDatabase, label: String) { + database.nodeInfoDao().upsert(NodeEntity(num = NODE_NUM, user = User(id = "!node-$label"))) + database.nodeInfoDao().upsert(MetadataEntity(num = NODE_NUM, proto = DeviceMetadata(firmware_version = label))) + } + + private suspend fun MeshtasticDatabase.hasMetadata(num: Int): Boolean = + nodeInfoDao().getAllMetadataSnapshot().any { it.num == num } + + private class TestLifecycleOwner : LifecycleOwner { + override val lifecycle = LifecycleRegistry.createUnsafe(this) + + fun moveTo(event: Lifecycle.Event) { + lifecycle.handleLifecycleEvent(event) + } + } + + /** Switches A -> B after the first armed callback, reproducing a device change between two logical writes. */ + private class SwitchAfterFirstWriteProvider(dbA: MeshtasticDatabase, private val dbB: MeshtasticDatabase) : + DatabaseProvider { + private val mutableCurrentDb = MutableStateFlow(dbA) + override val currentDb: StateFlow = mutableCurrentDb + private var armed = false + var withDbCalls: Int = 0 + private set + + override fun observeCurrentDb(query: (MeshtasticDatabase) -> Flow): Flow = + currentDb.flatMapLatest(query) + + override suspend fun withReadDb(block: suspend (MeshtasticDatabase) -> T): T = block(currentDb.value) + + override suspend fun withDb(block: suspend (MeshtasticDatabase) -> T): T? { + val capturedDb = currentDb.value + val result = block(capturedDb) + if (armed) { + withDbCalls += 1 + if (withDbCalls == 1) mutableCurrentDb.value = dbB + } + return result + } + + fun arm() { + armed = true + withDbCalls = 0 + } + } + + private companion object { + const val NODE_NUM = 0x1234 + } +} diff --git a/core/database/src/androidHostTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt b/core/database/src/androidHostTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt new file mode 100644 index 0000000000..fc75622ce9 --- /dev/null +++ b/core/database/src/androidHostTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2026 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 . + */ +package org.meshtastic.core.database.dao + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.runner.RunWith +import org.robolectric.annotation.Config + +@RunWith(AndroidJUnit4::class) +@Config(sdk = [34]) +class NodeInfoDaoAtomicDeleteTest : CommonNodeInfoDaoAtomicDeleteTest() diff --git a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt index 3d29453e07..0e2a3b0def 100644 --- a/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt +++ b/core/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt @@ -366,6 +366,28 @@ interface NodeInfoDao { @Query("DELETE FROM metadata WHERE num=:num") suspend fun deleteMetadata(num: Int) + @Query("DELETE FROM metadata WHERE num IN (:nodeNums)") + suspend fun deleteMetadataForNodes(nodeNums: List) + + /** Atomically deletes one node and its separately stored device metadata. */ + @Transaction + suspend fun deleteNodeAndMetadata(num: Int) { + deleteNode(num) + deleteMetadata(num) + } + + /** + * Atomically deletes nodes and their metadata, chunking both `IN` queries below SQLite's bind-parameter limit. + * Every chunk participates in the same transaction, so cancellation or a query failure rolls back the whole batch. + */ + @Transaction + suspend fun deleteNodesAndMetadata(nodeNums: List) { + for (chunk in nodeNums.chunked(MAX_BIND_PARAMS)) { + deleteNodes(chunk) + deleteMetadataForNodes(chunk) + } + } + /** Snapshot used by DatabaseMerger to carry per-node DeviceMetadata across transports (newest timestamp wins). */ @Query("SELECT * FROM metadata") suspend fun getAllMetadataSnapshot(): List diff --git a/core/database/src/commonTest/kotlin/org/meshtastic/core/database/dao/CommonNodeInfoDaoAtomicDeleteTest.kt b/core/database/src/commonTest/kotlin/org/meshtastic/core/database/dao/CommonNodeInfoDaoAtomicDeleteTest.kt new file mode 100644 index 0000000000..1c1367ce13 --- /dev/null +++ b/core/database/src/commonTest/kotlin/org/meshtastic/core/database/dao/CommonNodeInfoDaoAtomicDeleteTest.kt @@ -0,0 +1,112 @@ +/* + * Copyright (c) 2026 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 . + */ +package org.meshtastic.core.database.dao + +import androidx.room3.executeSQL +import androidx.room3.useWriterConnection +import kotlinx.coroutines.test.runTest +import org.meshtastic.core.database.MeshtasticDatabase +import org.meshtastic.core.database.entity.MetadataEntity +import org.meshtastic.core.database.entity.NodeEntity +import org.meshtastic.core.database.getInMemoryDatabaseBuilder +import org.meshtastic.core.testing.setupTestContext +import org.meshtastic.proto.DeviceMetadata +import org.meshtastic.proto.User +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertFails +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +abstract class CommonNodeInfoDaoAtomicDeleteTest { + private lateinit var database: MeshtasticDatabase + private lateinit var dao: NodeInfoDao + + @BeforeTest + fun setUp() { + setupTestContext() + database = getInMemoryDatabaseBuilder().build() + dao = database.nodeInfoDao() + } + + @AfterTest + fun tearDown() { + database.close() + } + + @Test + fun deleteNodeAndMetadataRemovesBothRows() = runTest { + seedNodeAndMetadata(NODE_NUM, "test") + + dao.deleteNodeAndMetadata(NODE_NUM) + + assertNull(dao.getNodeByNum(NODE_NUM)) + assertTrue(dao.getAllMetadataSnapshot().none { it.num == NODE_NUM }) + } + + @Test + fun deleteNodesAndMetadataChunksPastBindLimit() = runTest { + val secondNum = NODE_NUM + 1 + seedNodeAndMetadata(NODE_NUM, "first") + seedNodeAndMetadata(secondNum, "second") + val nodeNums = buildList { + add(NODE_NUM) + repeat(NodeInfoDao.MAX_BIND_PARAMS - 1) { add(-(it + 1)) } + add(secondNum) + } + + dao.deleteNodesAndMetadata(nodeNums) + + assertNull(dao.getNodeByNum(NODE_NUM)) + assertNull(dao.getNodeByNum(secondNum)) + assertTrue(dao.getAllMetadataSnapshot().none { it.num == NODE_NUM || it.num == secondNum }) + } + + @Test + fun deleteNodeAndMetadataRollsBackWhenMetadataDeleteFails() = runTest { + seedNodeAndMetadata(NODE_NUM, "test") + database.useWriterConnection { + it.executeSQL( + """ + CREATE TRIGGER fail_metadata_delete + BEFORE DELETE ON metadata + WHEN OLD.num = $NODE_NUM + BEGIN + SELECT RAISE(ABORT, 'forced metadata delete failure'); + END + """ + .trimIndent(), + ) + } + + assertFails { dao.deleteNodeAndMetadata(NODE_NUM) } + + assertNotNull(dao.getNodeByNum(NODE_NUM), "node deletion must roll back with metadata deletion") + assertTrue(dao.getAllMetadataSnapshot().any { it.num == NODE_NUM }, "metadata must remain after rollback") + } + + private suspend fun seedNodeAndMetadata(nodeNum: Int, label: String) { + dao.upsert(NodeEntity(num = nodeNum, user = User(id = "!node-$label"))) + dao.upsert(MetadataEntity(num = nodeNum, proto = DeviceMetadata(firmware_version = label))) + } + + private companion object { + const val NODE_NUM = 0x1234 + } +} diff --git a/core/database/src/jvmTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt b/core/database/src/jvmTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt new file mode 100644 index 0000000000..b59018b7ae --- /dev/null +++ b/core/database/src/jvmTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoAtomicDeleteTest.kt @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2026 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 . + */ +package org.meshtastic.core.database.dao + +class NodeInfoDaoAtomicDeleteTest : CommonNodeInfoDaoAtomicDeleteTest()