From 8dc11d4ac89833490a929865398931e1e8483092 Mon Sep 17 00:00:00 2001 From: hawkff <109485367+hawkff@users.noreply.github.com> Date: Sun, 12 Jul 2026 18:04:13 -0400 Subject: [PATCH] fix(database): delete groups and profiles atomically --- .../sagernet/database/GroupManager.kt | 38 ++- .../database/GroupManagerDeleteTest.kt | 249 ++++++++++++++++++ 2 files changed, 275 insertions(+), 12 deletions(-) create mode 100644 app/src/test/java/io/nekohasekai/sagernet/database/GroupManagerDeleteTest.kt diff --git a/app/src/main/java/io/nekohasekai/sagernet/database/GroupManager.kt b/app/src/main/java/io/nekohasekai/sagernet/database/GroupManager.kt index dbf19e9463..1edd8e4d20 100644 --- a/app/src/main/java/io/nekohasekai/sagernet/database/GroupManager.kt +++ b/app/src/main/java/io/nekohasekai/sagernet/database/GroupManager.kt @@ -100,26 +100,40 @@ object GroupManager { } } - suspend fun deleteGroup(groupId: Long) { + suspend fun deleteGroup(groupId: Long) = deleteGroup(groupId) { SubscriptionUpdater.reconfigureUpdater() } + + internal suspend fun deleteGroup(groupId: Long, reconfigureUpdater: suspend () -> Unit) { val selected = DataStore.selectedProxy - if (selected != 0L && SagerDatabase.proxyDao.getById(selected)?.groupId == groupId) { - DataStore.selectedProxy = 0L + val clearSelected = + selected != 0L && SagerDatabase.proxyDao.getById(selected)?.groupId == groupId + SagerDatabase.instance.runInTransaction { + SagerDatabase.proxyDao.deleteByGroup(groupId) + SagerDatabase.groupDao.deleteById(groupId) } - SagerDatabase.groupDao.deleteById(groupId) - SagerDatabase.proxyDao.deleteByGroup(groupId) + clearDeletedSelection(selected, clearSelected) iterator { groupRemoved(groupId) } - SubscriptionUpdater.reconfigureUpdater() + reconfigureUpdater() } - suspend fun deleteGroup(group: List) { + suspend fun deleteGroup(group: List) = deleteGroup(group) { SubscriptionUpdater.reconfigureUpdater() } + + internal suspend fun deleteGroup(group: List, reconfigureUpdater: suspend () -> Unit) { val ids = group.map { it.id }.toSet() val selected = DataStore.selectedProxy - if (selected != 0L && SagerDatabase.proxyDao.getById(selected)?.groupId in ids) { - DataStore.selectedProxy = 0L + val clearSelected = + selected != 0L && SagerDatabase.proxyDao.getById(selected)?.groupId in ids + SagerDatabase.instance.runInTransaction { + SagerDatabase.proxyDao.deleteByGroup(ids.toLongArray()) + SagerDatabase.groupDao.deleteGroup(group) } - SagerDatabase.groupDao.deleteGroup(group) - SagerDatabase.proxyDao.deleteByGroup(group.map { it.id }.toLongArray()) + clearDeletedSelection(selected, clearSelected) for (proxyGroup in group) iterator { groupRemoved(proxyGroup.id) } - SubscriptionUpdater.reconfigureUpdater() + reconfigureUpdater() + } + + internal fun clearDeletedSelection(selectedBeforeDelete: Long, selectedWasDeleted: Boolean) { + if (selectedWasDeleted && DataStore.selectedProxy == selectedBeforeDelete) { + DataStore.selectedProxy = 0L + } } } diff --git a/app/src/test/java/io/nekohasekai/sagernet/database/GroupManagerDeleteTest.kt b/app/src/test/java/io/nekohasekai/sagernet/database/GroupManagerDeleteTest.kt new file mode 100644 index 0000000000..a798b0ea0c --- /dev/null +++ b/app/src/test/java/io/nekohasekai/sagernet/database/GroupManagerDeleteTest.kt @@ -0,0 +1,249 @@ +package io.nekohasekai.sagernet.database + +import android.app.Application +import android.database.sqlite.SQLiteException +import io.nekohasekai.sagernet.fmt.ConfigBuilderTestEnv +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [35], application = Application::class) +class GroupManagerDeleteTest { + + @Before + fun setUp() { + ConfigBuilderTestEnv.reset() + } + + @Test + fun deleteGroup_singleSuccessCommitsBeforeSideEffects() = runTest { + withContext(Dispatchers.IO) { + val (group, profile) = createGroupWithProfile(1L) + DataStore.selectedProxy = profile.id + val listener = RecordingListener() + var reconfigurationCount = 0 + GroupManager.addListener(listener) + try { + GroupManager.deleteGroup(group.id) { reconfigurationCount++ } + + assertNull(SagerDatabase.groupDao.getById(group.id)) + assertEquals(emptyList(), SagerDatabase.proxyDao.getIdsByGroup(group.id)) + assertEquals(0L, DataStore.selectedProxy) + assertEquals(listOf(group.id), listener.removedGroupIds) + assertEquals(1, reconfigurationCount) + } finally { + GroupManager.removeListener(listener) + } + } + } + + @Test + fun deleteGroup_batchSuccessCommitsBeforeSideEffects() = runTest { + withContext(Dispatchers.IO) { + val (firstGroup, firstProfile) = createGroupWithProfile(1L) + val (secondGroup, secondProfile) = createGroupWithProfile(2L) + val (untargetedGroup, untargetedProfile) = createGroupWithProfile(3L) + DataStore.selectedProxy = secondProfile.id + val listener = RecordingListener() + var reconfigurationCount = 0 + GroupManager.addListener(listener) + try { + GroupManager.deleteGroup(listOf(firstGroup, secondGroup)) { + reconfigurationCount++ + } + + assertEquals( + setOf(untargetedGroup.id), + SagerDatabase.groupDao.allGroups().map { it.id }.toSet(), + ) + assertNull(SagerDatabase.proxyDao.getById(firstProfile.id)) + assertNull(SagerDatabase.proxyDao.getById(secondProfile.id)) + assertNotNull(SagerDatabase.proxyDao.getById(untargetedProfile.id)) + assertEquals(0L, DataStore.selectedProxy) + assertEquals( + listOf(firstGroup.id, secondGroup.id), + listener.removedGroupIds, + ) + assertEquals(1, reconfigurationCount) + } finally { + GroupManager.removeListener(listener) + } + } + } + + @Test + fun deleteGroup_doesNotClearASelectionChangedAfterDeletionStarted() = runTest { + withContext(Dispatchers.IO) { + val (_, deletedProfile) = createGroupWithProfile(1L) + val (_, replacementProfile) = createGroupWithProfile(2L) + DataStore.selectedProxy = deletedProfile.id + val selectedBeforeDelete = DataStore.selectedProxy + DataStore.selectedProxy = replacementProfile.id + + GroupManager.clearDeletedSelection(selectedBeforeDelete, selectedWasDeleted = true) + + assertEquals(replacementProfile.id, DataStore.selectedProxy) + } + } + + @Test + fun deleteGroup_singleRollbackRestoresRowsAndSuppressesSideEffects() = runTest { + withContext(Dispatchers.IO) { + val (group, profile) = createGroupWithProfile(1L) + DataStore.selectedProxy = profile.id + val listener = RecordingListener() + var reconfigurationCount = 0 + GroupManager.addListener(listener) + try { + withFailingGroupDeleteTrigger { + assertDeletionFails { + GroupManager.deleteGroup(group.id) { reconfigurationCount++ } + } + } + + assertNotNull(SagerDatabase.groupDao.getById(group.id)) + assertEquals(listOf(profile.id), SagerDatabase.proxyDao.getIdsByGroup(group.id)) + assertEquals(profile.id, DataStore.selectedProxy) + assertEquals(emptyList(), listener.removedGroupIds) + assertEquals(0, reconfigurationCount) + } finally { + GroupManager.removeListener(listener) + } + } + } + + @Test + fun deleteGroup_batchRollbackRestoresRowsAndSuppressesSideEffects() = runTest { + withContext(Dispatchers.IO) { + val (firstGroup, firstProfile) = createGroupWithProfile(1L) + val (secondGroup, secondProfile) = createGroupWithProfile(2L) + DataStore.selectedProxy = secondProfile.id + val listener = RecordingListener() + var reconfigurationCount = 0 + GroupManager.addListener(listener) + try { + withFailingGroupDeleteTrigger { + assertDeletionFails { + GroupManager.deleteGroup(listOf(firstGroup, secondGroup)) { + reconfigurationCount++ + } + } + } + + assertEquals( + setOf(firstGroup.id, secondGroup.id), + SagerDatabase.groupDao.allGroups().map { it.id }.toSet(), + ) + assertEquals( + listOf(firstProfile.id), + SagerDatabase.proxyDao.getIdsByGroup(firstGroup.id), + ) + assertEquals( + listOf(secondProfile.id), + SagerDatabase.proxyDao.getIdsByGroup(secondGroup.id), + ) + assertEquals(secondProfile.id, DataStore.selectedProxy) + assertEquals(emptyList(), listener.removedGroupIds) + assertEquals(0, reconfigurationCount) + } finally { + GroupManager.removeListener(listener) + } + } + } + + @Test + fun deleteGroup_publicWrappersSuppressSideEffectsOnRollback() = runTest { + withContext(Dispatchers.IO) { + val (singleGroup, singleProfile) = createGroupWithProfile(1L) + val (batchGroup, batchProfile) = createGroupWithProfile(2L) + val listener = RecordingListener() + GroupManager.addListener(listener) + try { + withFailingGroupDeleteTrigger { + DataStore.selectedProxy = singleProfile.id + assertDeletionFails { GroupManager.deleteGroup(singleGroup.id) } + assertEquals(singleProfile.id, DataStore.selectedProxy) + + DataStore.selectedProxy = batchProfile.id + assertDeletionFails { GroupManager.deleteGroup(listOf(batchGroup)) } + assertEquals(batchProfile.id, DataStore.selectedProxy) + } + + assertNotNull(SagerDatabase.groupDao.getById(singleGroup.id)) + assertNotNull(SagerDatabase.groupDao.getById(batchGroup.id)) + assertEquals( + listOf(singleProfile.id), + SagerDatabase.proxyDao.getIdsByGroup(singleGroup.id), + ) + assertEquals( + listOf(batchProfile.id), + SagerDatabase.proxyDao.getIdsByGroup(batchGroup.id), + ) + assertEquals(emptyList(), listener.removedGroupIds) + } finally { + GroupManager.removeListener(listener) + } + } + } + + private fun createGroupWithProfile(order: Long): Pair { + val group = ProxyGroup(userOrder = order).apply { + id = SagerDatabase.groupDao.createGroup(this) + } + val profile = ProxyEntity(groupId = group.id, userOrder = 1L).apply { + id = SagerDatabase.proxyDao.addProxy(this) + } + return group to profile + } + + private suspend fun withFailingGroupDeleteTrigger(block: suspend () -> Unit) { + val database = SagerDatabase.instance.openHelper.writableDatabase + database.execSQL( + """ + CREATE TEMP TRIGGER fail_group_delete + BEFORE DELETE ON proxy_groups + BEGIN + SELECT RAISE(ABORT, 'forced group deletion failure'); + END + """.trimIndent(), + ) + try { + block() + } finally { + database.execSQL("DROP TRIGGER IF EXISTS fail_group_delete") + } + } + + private suspend fun assertDeletionFails(block: suspend () -> Unit) { + var failure: SQLiteException? = null + try { + block() + } catch (exception: SQLiteException) { + failure = exception + } + assertNotNull("Expected group deletion to fail", failure) + } + + private class RecordingListener : GroupManager.Listener { + val removedGroupIds = mutableListOf() + + override suspend fun groupAdd(group: ProxyGroup) = Unit + + override suspend fun groupUpdated(group: ProxyGroup) = Unit + + override suspend fun groupRemoved(groupId: Long) { + removedGroupIds += groupId + } + + override suspend fun groupUpdated(groupId: Long) = Unit + } +}