From f049fd935c6dd065fb191209fbe86c86f2b1e3b8 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 8 Oct 2026 07:05:23 +1100 Subject: [PATCH 1/2] Erase group entries a config merge recreated after a linked device erased them When an admin deletes a group, the deleting device destroys the group's info, pushes it, then erases the group from UserGroups. A linked device usually sees the destroyed info first and marks its UserGroups entry destroyed. libsession merges the two diverged configs by replaying each diff, and if the erase applies first the destroyed-flag diff recreates the erased key holding only that field: an entry with the removed status and no name or keys. That entry then syncs back, so the group reappears on the deleting device ("Unknown Group" on iOS, the "deleted by a group admin" preview elsewhere) and stays on the linked device. A real kicked or destroyed entry always keeps its name, so a removed entry with no name, no admin key and no auth data can only be this. Erase it as part of handling the merge and never build a conversation from it; any conversation we still have for it is removed through the normal path. Other admins keep their full entry and still see the deleted state. The same rule is applied on iOS, Android and Desktop. The underlying merge behaviour is tracked separately for libsession. --- .../securesms/dependencies/ConfigFactory.kt | 28 +++++++++++- .../securesms/groups/ErasedGroupStub.kt | 17 ++++++++ .../securesms/groups/ErasedGroupStubTest.kt | 43 +++++++++++++++++++ 3 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/groups/ErasedGroupStubTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt b/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt index 23a45531fa..1c74e310a7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt @@ -34,10 +34,12 @@ import org.session.libsession.utilities.getGroup import org.session.libsession.utilities.withGroupConfigs import org.session.libsession.utilities.withMutableUserConfigs import org.session.libsignal.utilities.AccountId +import org.session.libsignal.utilities.Log import org.thoughtcrime.securesms.auth.LoginStateRepository import org.thoughtcrime.securesms.configs.ConfigToDatabaseSync import org.thoughtcrime.securesms.database.ConfigDatabase import org.thoughtcrime.securesms.database.ConfigVariant +import org.thoughtcrime.securesms.groups.isErasedGroupStub import java.util.EnumSet import java.util.concurrent.locks.ReentrantReadWriteLock import javax.inject.Inject @@ -55,6 +57,8 @@ class ConfigFactory @Inject constructor( @param:ManagerScope private val coroutineScope: CoroutineScope ) : ConfigFactoryProtocol { companion object { + private const val TAG = "ConfigFactory" + // This is a buffer period within which we will process messages which would result in a // config change, any message which would normally result in a config change which was sent // before `lastConfigMessage.timestamp - configChangeBufferPeriod` will not actually have @@ -276,6 +280,8 @@ class ConfigFactory @Inject constructor( return } + var erasedGroupStubs = emptyList() + val result = doWithMutableUserConfigs(fromMerge = true) { configs -> val config = when (userConfigType) { UserConfigType.CONTACTS -> configs.contacts @@ -291,11 +297,29 @@ class ConfigFactory @Inject constructor( .mapNotNull { hash -> messages.firstOrNull { it.hash == hash } } .maxOfOrNull { it.timestamp } + // Finish the erase the merge undid before anything reads the entry back as a group + if (userConfigType == UserConfigType.USER_GROUPS) { + erasedGroupStubs = configs.userGroups.allClosedGroupInfo() + .filter { it.isErasedGroupStub() } + .map { AccountId(it.groupAccountId) } + + erasedGroupStubs.forEach { groupId -> + Log.w(TAG, "Erasing a group the merge recreated after another device erased it") + configs.userGroups.eraseClosedGroup(groupId.hexString) + configs.convoInfoVolatile.eraseClosedGroup(groupId.hexString) + } + } + + val changed = EnumSet.of(userConfigType) + if (erasedGroupStubs.isNotEmpty()) changed.add(UserConfigType.CONVO_INFO_VOLATILE) + maxTimestamp?.let { - (config.dump() to it) to EnumSet.of(userConfigType) - } ?: (null to emptySet()) + (config.dump() to it) to changed + } ?: (null to if (erasedGroupStubs.isEmpty()) emptySet() else changed) } + erasedGroupStubs.forEach(::deleteGroupConfigs) + // Dump now regardless so we can save the timestamp to the database if (result != null) { val (dump, timestamp) = result diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt new file mode 100644 index 0000000000..f8aa709da6 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt @@ -0,0 +1,17 @@ +package org.thoughtcrime.securesms.groups + +import network.loki.messenger.libsession_util.util.GroupInfo + +/** + * Whether this UserGroups entry is what a config merge leaves of a group another of our devices + * erased, rather than a group we are (or were) in. + * + * libsession applies each device's diff in turn, so when one device erases a group while another + * changes one of its fields (marking it destroyed after seeing the group's info, say), the merge + * recreates the erased entry holding only the changed fields: no name and no keys. A real kicked or + * destroyed entry always keeps its name, so a removed entry without one can only be this. + * + * The other clients apply the same rule; keep them in step. + */ +fun GroupInfo.ClosedGroupInfo.isErasedGroupStub(): Boolean = + (destroyed || kicked) && name.isEmpty() && adminKey == null && authData == null diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/ErasedGroupStubTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/ErasedGroupStubTest.kt new file mode 100644 index 0000000000..07e98a8bf2 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/ErasedGroupStubTest.kt @@ -0,0 +1,43 @@ +package org.thoughtcrime.securesms.groups + +import com.google.common.truth.Truth.assertThat +import network.loki.messenger.libsession_util.PRIORITY_VISIBLE +import network.loki.messenger.libsession_util.util.Bytes +import network.loki.messenger.libsession_util.util.GroupInfo +import org.junit.Test + +class ErasedGroupStubTest { + private val stub = GroupInfo.ClosedGroupInfo( + groupAccountId = "03" + "ab".repeat(32), + adminKey = null, + authData = null, + priority = PRIORITY_VISIBLE, + invited = false, + name = "", + destroyed = true, + joinedAtSecs = 0L, + kicked = false, + ) + + @Test + fun `a removed entry with no name and no keys is an erased-group stub`() { + assertThat(stub.isErasedGroupStub()).isTrue() + assertThat(stub.copy(destroyed = false, kicked = true).isErasedGroupStub()).isTrue() + } + + @Test + fun `a destroyed group we were in keeps its name and is not a stub`() { + assertThat(stub.copy(name = "Book club").isErasedGroupStub()).isFalse() + } + + @Test + fun `an entry with an admin key or auth data is not a stub`() { + assertThat(stub.copy(adminKey = Bytes(ByteArray(64) { 1 })).isErasedGroupStub()).isFalse() + assertThat(stub.copy(authData = Bytes(ByteArray(100) { 7 })).isErasedGroupStub()).isFalse() + } + + @Test + fun `a group still in use is never a stub, even without a name`() { + assertThat(stub.copy(destroyed = false).isErasedGroupStub()).isFalse() + } +} From cd8357976441d9dfacb01ee5b52a225e9e2c9282 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 8 Oct 2026 11:18:50 +1100 Subject: [PATCH 2/2] Push and dump the erase of a recreated group stub The erase ran inside the merge's write, which ConfigUploader doesn't push (it drops fromMerge notifications) and which only dumps the merged config type: the UserGroups erase waited for some later local change to be pushed, and the convo-info-volatile erase could be lost on restart. Finish through removeGroup, whose write dumps and pushes like any local change; the erase inside the merge stays so nothing reads the stub back first. Also state the name assumption as the convention it is: libsession's header calls the name invite-only. --- .../thoughtcrime/securesms/dependencies/ConfigFactory.kt | 3 ++- .../org/thoughtcrime/securesms/groups/ErasedGroupStub.kt | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt b/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt index 1c74e310a7..764e071e36 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/dependencies/ConfigFactory.kt @@ -318,7 +318,8 @@ class ConfigFactory @Inject constructor( } ?: (null to if (erasedGroupStubs.isEmpty()) emptySet() else changed) } - erasedGroupStubs.forEach(::deleteGroupConfigs) + // A merge write is neither pushed nor fully dumped; removeGroup's write is both + erasedGroupStubs.forEach(::removeGroup) // Dump now regardless so we can save the timestamp to the database if (result != null) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt index f8aa709da6..12b54c50f4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/ErasedGroupStub.kt @@ -8,8 +8,11 @@ import network.loki.messenger.libsession_util.util.GroupInfo * * libsession applies each device's diff in turn, so when one device erases a group while another * changes one of its fields (marking it destroyed after seeing the group's info, say), the merge - * recreates the erased entry holding only the changed fields: no name and no keys. A real kicked or - * destroyed entry always keeps its name, so a removed entry without one can only be this. + * recreates the erased entry holding only the changed fields: no name and no keys. Every client + * keeps the name on its kicked and destroyed entries (libsession's mark_kicked/mark_destroyed clear + * only the keys), so a removed entry without one can only be this. libsession's header describes + * the name as invite-only, though: if any client starts clearing it after joining, this rule has to + * change. * * The other clients apply the same rule; keep them in step. */