From 9541d3030e9e9b01b9f20a7afea091151bb271d8 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 4 Sep 2026 13:19:18 +1000 Subject: [PATCH 1/6] Fix: leaving a group we hold no encryption keys for retried forever Once our access to a closed group is revoked we no longer hold its encryption keys, so the "member left" message can never be sent. GroupLeavingWorker blocked on that send, classified the failure as retryable and returned WorkManager's Result.retry(), which has backoff but no attempt cap -- so the leave was re-attempted for as long as the group existed, appending another permanent error message to the conversation each time. One reported case ran for ten days before the user gave up; the group was never left. The keyless case is typed as NonRetryableException where it is detected, and the worker now falls through to the local cleanup on it rather than reporting a failure -- the same catch-log-proceed the destroyGroup() branch fifteen lines below has had since dda6e90d65. Every other failure keeps the retry, capped so that no error class can reintroduce an unbounded loop. MessageSendJob's own retry policy is deliberately untouched: keys granted late still let an ordinary message through on a later attempt of the same job. The error message is cleared alongside the leaving message when a leave starts, so a conversation carries at most one of them. --- .../messaging/jobs/MessageSendJob.kt | 14 +- .../securesms/groups/GroupLeavingWorker.kt | 26 +++- .../messaging/jobs/MessageSendJobTest.kt | 83 ++++++++++++ .../groups/GroupLeavingWorkerTest.kt | 128 ++++++++++++++++++ 4 files changed, 243 insertions(+), 8 deletions(-) create mode 100644 app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt diff --git a/app/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt b/app/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt index 7d3a5e6d8a..b62bbd7ede 100644 --- a/app/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt +++ b/app/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt @@ -24,6 +24,7 @@ import org.session.libsession.messaging.utilities.Data import org.session.libsession.utilities.ConfigFactoryProtocol import org.session.libsession.utilities.ConfigUpdateNotification import org.session.libsession.utilities.withGroupConfigs +import org.session.libsignal.exceptions.NonRetryableException import org.session.libsignal.utilities.AccountId import org.session.libsignal.utilities.Log import org.thoughtcrime.securesms.api.error.UnhandledStatusCodeException @@ -94,13 +95,18 @@ class MessageSendJob @AssistedInject constructor( val isSync = destination is Destination.Contact && destination.publicKey == storage.getUserPublicKey() try { - // Shouldn't send message to group when the group has no keys available + // A group we hold no encryption keys for can't be sent to, and the keys can only + // arrive by an admin granting them to us, which may never happen. Typed so a caller + // waiting on this send can tell it apart from a transient failure and give up + // deliberately rather than waiting for keys that aren't coming. if (destination is Destination.ClosedGroup) { - requireNotNull(withTimeoutOrNull(20_000L) { + val keysAvailable = withTimeoutOrNull(20_000L) { configFactory .waitForGroupEncryptionKeys(AccountId(destination.publicKey)) - }) { - "Timeout waiting for group keys to become available" + } != null + + if (!keysAvailable) { + throw NonRetryableException("Timeout waiting for group keys to become available") } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt index e0cbc987f7..c153953d11 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt @@ -61,8 +61,10 @@ class GroupLeavingWorker @AssistedInject constructor( return groupScope.launchAndWait(groupId, "GroupLeavingWorker") { val group = configFactory.getGroup(groupId) - // Make sure we only have one group leaving control message + // Make sure we only have one group leaving control message, and that the error + // message from an earlier attempt doesn't sit alongside it storage.deleteGroupInfoMessages(groupId, UpdateMessageData.Kind.GroupLeaving::class.java) + storage.deleteGroupInfoMessages(groupId, UpdateMessageData.Kind.GroupErrorQuit::class.java) storage.insertGroupInfoLeaving(groupId) // Best effort to unsubscribe ourselves from the registration server. @@ -133,8 +135,19 @@ class GroupLeavingWorker @AssistedInject constructor( ) // Wait for both messages to be sent - repeat(2) { - statusChannel.receive().getOrThrow() + try { + repeat(2) { + statusChannel.receive().getOrThrow() + } + } catch (e: CancellationException) { + throw e + } catch (e: NonRetryableException) { + // Our access to the group can be revoked before we get around to + // leaving it, which leaves us without the keys to encrypt the + // departure to the group. Nothing will grant them back, so honour the + // leave locally instead: the alternative is a group the user can never + // leave, however many times they ask. + Log.e(TAG, "Unable to announce leaving group $groupId. Proceeding...", e) } } @@ -170,7 +183,10 @@ class GroupLeavingWorker @AssistedInject constructor( } catch (e: Exception) { storage.insertGroupInfoErrorQuit(groupId) Log.e(TAG, "Failed to leave group $groupId", e) - if (e is NonRetryableException) { + // WorkManager's retry has backoff but no attempt cap, so an error that never + // resolves itself would have us re-attempting the leave — and reporting the + // failure to the conversation — for as long as the group exists + if (e is NonRetryableException || runAttemptCount >= MAX_RETRIES) { Result.failure() } else { Result.retry() @@ -184,6 +200,8 @@ class GroupLeavingWorker @AssistedInject constructor( companion object { private const val TAG = "GroupLeavingWorker" + private const val MAX_RETRIES = 2 + private const val KEY_GROUP_ID = "group_id" private const val KEY_DELETE_GROUP = "delete_group" diff --git a/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt b/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt new file mode 100644 index 0000000000..68e8c6f3d4 --- /dev/null +++ b/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt @@ -0,0 +1,83 @@ +package org.session.libsession.messaging.jobs + +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.test.runTest +import network.loki.messenger.libsession_util.ReadableGroupKeysConfig +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.session.libsession.messaging.messages.Destination +import org.session.libsession.messaging.messages.control.GroupUpdated +import org.session.libsession.utilities.ConfigFactoryProtocol +import org.session.libsession.utilities.ConfigUpdateNotification +import org.session.libsession.utilities.GroupConfigs +import org.session.libsignal.exceptions.NonRetryableException +import org.session.libsignal.utilities.Log +import org.session.protos.SessionProtos +import org.thoughtcrime.securesms.NoOpLogger + +class MessageSendJobTest { + private val groupId = "03${"11".repeat(32)}" + + @Before + fun setUp() { + Log.initialize(NoOpLogger) + } + + @Test + fun `sending to a group we hold no keys for fails non-retryably`() = runTest { + val statusChannel = Channel>(capacity = 1) + + job(statusChannel, groupKeys = emptyList()).execute("test") + + val error = statusChannel.receive().exceptionOrNull() + assertTrue("expected NonRetryableException, got $error", error is NonRetryableException) + } + + @Test + fun `sending to a group we hold keys for is sent`() = runTest { + val statusChannel = Channel>(capacity = 1) + + job(statusChannel, groupKeys = listOf(ByteArray(32))).execute("test") + + assertTrue(statusChannel.receive().isSuccess) + } + + private fun job( + statusChannel: Channel>, + groupKeys: List, + ): MessageSendJob { + val keysConfig = mockk { + every { keys() } returns groupKeys + } + + val configFactory = mockk { + // The real notification flow never completes; an ending flow would fail the + // wait outright instead of exercising the timeout + every { configUpdateNotifications } returns MutableSharedFlow() + every { dangerouslyAccessGroupConfigs(any()) } returns Pair( + mockk { every { this@mockk.groupKeys } returns keysConfig }, + {}, + ) + } + + return MessageSendJob( + message = GroupUpdated( + SessionProtos.GroupUpdateMessage.newBuilder() + .setMemberLeftMessage(SessionProtos.GroupUpdateMemberLeftMessage.getDefaultInstance()) + .build() + ), + destination = Destination.ClosedGroup(groupId), + statusCallback = statusChannel, + attachmentUploadJobFactory = mockk(relaxed = true), + messageDataProvider = mockk(relaxed = true), + storage = mockk(relaxed = true), + configFactory = configFactory, + messageSender = mockk(relaxed = true), + jobQueue = mockk(relaxed = true), + ) + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt new file mode 100644 index 0000000000..88fe8074c0 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt @@ -0,0 +1,128 @@ +package org.thoughtcrime.securesms.groups + +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.WorkerParameters +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.channels.SendChannel +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import network.loki.messenger.libsession_util.ReadableGroupMembersConfig +import network.loki.messenger.libsession_util.ReadableUserGroupsConfig +import network.loki.messenger.libsession_util.util.GroupInfo +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.session.libsession.messaging.groups.GroupScope +import org.session.libsession.messaging.sending_receiving.MessageSender +import org.session.libsession.messaging.utilities.UpdateMessageData +import org.session.libsession.utilities.GroupConfigs +import org.session.libsession.utilities.UserConfigs +import org.session.libsignal.exceptions.NonRetryableException +import org.session.libsignal.utilities.AccountId +import org.session.libsignal.utilities.Log +import org.thoughtcrime.securesms.NoOpLogger +import org.thoughtcrime.securesms.database.Storage +import org.thoughtcrime.securesms.dependencies.ConfigFactory + +class GroupLeavingWorkerTest { + private val groupId = AccountId("03${"11".repeat(32)}") + + private val storage = mockk(relaxed = true) + private val configFactory = mockk(relaxed = true) + private val messageSender = mockk() + + @Before + fun setUp() { + Log.initialize(NoOpLogger) + + val group = mockk { + every { kicked } returns false + every { destroyed } returns false + } + + every { configFactory.dangerouslyAccessUserConfigs() } returns Pair( + mockk { + every { userGroups } returns mockk { + every { getClosedGroup(groupId.hexString) } returns group + } + }, + {}, + ) + + // No admins, so we are not the only admin and the leave takes the announce-and-go path + every { configFactory.dangerouslyAccessGroupConfigs(groupId) } returns Pair( + mockk { + every { groupMembers } returns mockk { + every { all() } returns emptyList() + } + }, + {}, + ) + } + + @Test + fun `group is left locally when the departure cannot be announced`() = runTest { + answerSendWith(Result.failure(NonRetryableException("no keys for this group"))) + + val result = worker(runAttemptCount = 0).doWork() + + assertEquals(ListenableWorker.Result.success(), result) + verify { configFactory.removeGroup(groupId) } + verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } + } + + @Test + fun `retries stop once the attempts are used up`() = runTest { + answerSendWith(Result.failure(RuntimeException("network went away"))) + + assertEquals(ListenableWorker.Result.retry(), worker(runAttemptCount = 0).doWork()) + assertEquals(ListenableWorker.Result.failure(), worker(runAttemptCount = 2).doWork()) + } + + @Test + fun `the error message replaces the one from the previous attempt`() = runTest { + answerSendWith(Result.failure(RuntimeException("network went away"))) + + worker(runAttemptCount = 0).doWork() + + verify(exactly = 1) { storage.insertGroupInfoErrorQuit(groupId) } + verify { + storage.deleteGroupInfoMessages(groupId, UpdateMessageData.Kind.GroupErrorQuit::class.java) + } + } + + /** + * The status channel is a rendezvous one, so a result can only be handed over while the worker + * is waiting for it — and the worker stops waiting after the first failure, leaving the second + * send parked on the channel for the background scope to cancel. + */ + private fun TestScope.answerSendWith(result: Result) { + every { messageSender.send(any(), any(), any()) } answers { + val statusChannel = thirdArg>>() + backgroundScope.launch { statusChannel.send(result) } + } + } + + private fun CoroutineScope.worker(runAttemptCount: Int) = GroupLeavingWorker( + context = mockk(relaxed = true), + params = mockk(relaxed = true) { + every { inputData } returns Data.Builder() + .putString("group_id", groupId.hexString) + .build() + every { this@mockk.runAttemptCount } returns runAttemptCount + }, + storage = storage, + configFactory = configFactory, + groupScope = GroupScope(this), + tokenFetcher = mockk { every { token } returns MutableStateFlow(null) }, + serverApiExecutor = mockk(relaxed = true), + pushUnregisterApiFactory = mockk(relaxed = true), + messageSender = messageSender, + ) +} From 3c26502cbf90088036debc25a08a834d96a8e1c4 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 4 Sep 2026 14:43:12 +1000 Subject: [PATCH 2/6] Buffer the leave's status channel so a result cannot be lost The two leave messages report through one rendezvous channel, and they report with trySend -- which delivers nothing unless a receiver is parked at that instant. The jobs run on their own dispatcher, so either result can land while the worker is still enqueueing the second send, and the worker then waits on receive() forever. It holds the group's scope while it waits, so every other operation queued for that group waits behind it, and the leave itself only ends when the platform stops the worker -- which WorkManager treats as a reason to run it again. Reachable on the success path, so it outlives the retry-loop fix: honouring a leave we cannot announce depends on that failure result arriving at all. The worker tests now hang without this, which is the intended reading -- the suite will not run green against an unbuffered channel. --- .../securesms/groups/GroupLeavingWorker.kt | 6 ++++- .../groups/GroupLeavingWorkerTest.kt | 25 ++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt index c153953d11..829ceab778 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt @@ -109,7 +109,11 @@ class GroupLeavingWorker @AssistedInject constructor( if (group != null && !group.kicked && !weAreTheOnlyAdmin) { val address = Address.fromSerialized(groupId.hexString) - val statusChannel = Channel>() + // The jobs report with trySend, which delivers nothing unless a receiver + // is already parked, and they run on their own dispatcher: either result + // can land before we reach the wait below. Unbuffered, that result is lost + // and the leave waits for it forever, holding this group's scope with it. + val statusChannel = Channel>(capacity = Channel.UNLIMITED) // Always send a "XXX left" message to the group if we can messageSender.send( diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt index 88fe8074c0..8b374cfbac 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt @@ -97,15 +97,28 @@ class GroupLeavingWorkerTest { } } - /** - * The status channel is a rendezvous one, so a result can only be handed over while the worker - * is waiting for it — and the worker stops waiting after the first failure, leaving the second - * send parked on the channel for the background scope to cancel. - */ + @Test + fun `leave completes when both results arrive before the worker waits for them`() = runTest { + answerSendImmediatelyWith(Result.success(Unit)) + + val result = worker(runAttemptCount = 0).doWork() + + assertEquals(ListenableWorker.Result.success(), result) + verify { configFactory.removeGroup(groupId) } + } + + /** Reports the result once the worker is already waiting for it. */ private fun TestScope.answerSendWith(result: Result) { every { messageSender.send(any(), any(), any()) } answers { val statusChannel = thirdArg>>() - backgroundScope.launch { statusChannel.send(result) } + launch { statusChannel.send(result) } + } + } + + /** Reports the result as the send is made, before the worker gets as far as waiting for it. */ + private fun answerSendImmediatelyWith(result: Result) { + every { messageSender.send(any(), any(), any()) } answers { + thirdArg>>().trySend(result) } } From 828389066bec3335a63b727ca5129032f7d67740 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 4 Sep 2026 14:53:54 +1000 Subject: [PATCH 3/6] Ignore the .claude directory --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 90be087c09..d9a4cfa843 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ project.properties bin/ gen/ .idea/ +.claude/ *.iml out build From 4a182b1f2d223c76dc080acd2a4dd56524b2400f Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 13:49:57 +1000 Subject: [PATCH 4/6] Wait out the attempts before deciding the group keys are never coming Twenty seconds without keys was being read as "our access has been revoked", but it is equally what a group whose keys have not been fetched yet looks like -- a just-accepted invite, a restore from seed, a newly linked device, a slow cold start. Leaving on that evidence erases the group locally and syncs that to the user's other devices while no admin ever hears about it, so they stay in everyone's member list with nothing left to retry the announcement. The attempts the worker already has are now spent waiting for the keys to turn up, and only the last one falls through to the local cleanup. That keeps the property the retry-loop fix was for -- the user can always eventually leave -- without concluding from a single 20s window that nothing is coming. Tests cover keys that arrive during the wait, and both sides of the attempt budget. --- .../securesms/groups/GroupLeavingWorker.kt | 18 ++++++--- .../messaging/jobs/MessageSendJobTest.kt | 39 +++++++++++++++++-- .../groups/GroupLeavingWorkerTest.kt | 13 ++++++- 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt index 829ceab778..9c01841fef 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt @@ -146,11 +146,19 @@ class GroupLeavingWorker @AssistedInject constructor( } catch (e: CancellationException) { throw e } catch (e: NonRetryableException) { - // Our access to the group can be revoked before we get around to - // leaving it, which leaves us without the keys to encrypt the - // departure to the group. Nothing will grant them back, so honour the - // leave locally instead: the alternative is a group the user can never - // leave, however many times they ask. + // We have no keys to encrypt the departure with. That is what a revoked + // access looks like, but it is also what a group whose keys have simply + // not been fetched yet looks like — a just-accepted invite, a restore, a + // cold start — so spend the attempts waiting for them to turn up before + // concluding they never will. + if (runAttemptCount < MAX_RETRIES) { + Log.w(TAG, "No keys to announce leaving group $groupId with. Retrying...", e) + return@launchAndWait Result.retry() + } + + // Out of attempts: honour the leave locally anyway, because the + // alternative is a group the user can never leave, however many times + // they ask. Log.e(TAG, "Unable to announce leaving group $groupId. Proceeding...", e) } } diff --git a/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt b/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt index 68e8c6f3d4..3c60c86097 100644 --- a/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt +++ b/app/src/test/java/org/session/libsession/messaging/jobs/MessageSendJobTest.kt @@ -3,7 +3,12 @@ package org.session.libsession.messaging.jobs import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest import network.loki.messenger.libsession_util.ReadableGroupKeysConfig import org.junit.Assert.assertTrue @@ -17,7 +22,9 @@ import org.session.libsession.utilities.GroupConfigs import org.session.libsignal.exceptions.NonRetryableException import org.session.libsignal.utilities.Log import org.session.protos.SessionProtos +import org.session.libsignal.utilities.AccountId import org.thoughtcrime.securesms.NoOpLogger +import kotlin.time.Duration.Companion.seconds class MessageSendJobTest { private val groupId = "03${"11".repeat(32)}" @@ -37,6 +44,24 @@ class MessageSendJobTest { assertTrue("expected NonRetryableException, got $error", error is NonRetryableException) } + @Test + fun `sending waits for keys that arrive while it is waiting`() = runTest { + val statusChannel = Channel>(capacity = 1) + val keys = MutableStateFlow(emptyList()) + val configUpdates = MutableSharedFlow() + + launch { + // Well inside the 20s the send is prepared to wait, and virtual time, so it is free + delay(5.seconds) + keys.value = listOf(ByteArray(32)) + configUpdates.emit(ConfigUpdateNotification.GroupConfigsUpdated(AccountId(groupId), fromMerge = true)) + } + + job(statusChannel, groupKeys = keys, configUpdates = configUpdates).execute("test") + + assertTrue(statusChannel.receive().isSuccess) + } + @Test fun `sending to a group we hold keys for is sent`() = runTest { val statusChannel = Channel>(capacity = 1) @@ -49,15 +74,21 @@ class MessageSendJobTest { private fun job( statusChannel: Channel>, groupKeys: List, + ): MessageSendJob = job(statusChannel, MutableStateFlow(groupKeys)) + + private fun job( + statusChannel: Channel>, + groupKeys: StateFlow>, + // The real notification flow never completes; an ending flow would fail the wait + // outright instead of exercising the timeout + configUpdates: Flow = MutableSharedFlow(), ): MessageSendJob { val keysConfig = mockk { - every { keys() } returns groupKeys + every { keys() } answers { groupKeys.value } } val configFactory = mockk { - // The real notification flow never completes; an ending flow would fail the - // wait outright instead of exercising the timeout - every { configUpdateNotifications } returns MutableSharedFlow() + every { configUpdateNotifications } returns configUpdates every { dangerouslyAccessGroupConfigs(any()) } returns Pair( mockk { every { this@mockk.groupKeys } returns keysConfig }, {}, diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt index 8b374cfbac..0ed3d80144 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt @@ -67,11 +67,22 @@ class GroupLeavingWorkerTest { } @Test - fun `group is left locally when the departure cannot be announced`() = runTest { + fun `an unannounceable departure is retried before it is given up on`() = runTest { answerSendWith(Result.failure(NonRetryableException("no keys for this group"))) val result = worker(runAttemptCount = 0).doWork() + assertEquals(ListenableWorker.Result.retry(), result) + verify(exactly = 0) { configFactory.removeGroup(any()) } + verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } + } + + @Test + fun `group is left locally once the attempts to announce it are used up`() = runTest { + answerSendWith(Result.failure(NonRetryableException("no keys for this group"))) + + val result = worker(runAttemptCount = 2).doWork() + assertEquals(ListenableWorker.Result.success(), result) verify { configFactory.removeGroup(groupId) } verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } From f39e4fc36eedae6c8d5e2e08dbd6b05afe3a147d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 14:46:06 +1000 Subject: [PATCH 5/6] Revert "Wait out the attempts before deciding the group keys are never coming" This reverts commit 4a182b1f2d. Waiting was the wrong trade. The leave worker only ever runs from an explicit "leave" action, so the user's departure is the thing that has to happen; telling the group about it is best-effort. Waiting sides with the announcement. It also mostly waits for nothing. The kicked flag is only set when a revocation message is actually received and processed, and processing it wipes the group configs -- so "no keys, not kicked" is very often a revocation that was never seen, where no amount of waiting produces a key. A device linked after the revocation lands in the same state and would serve the full budget to announce a departure from a group it was already removed from. The MessageSendJob test for keys arriving during the wait is kept: that wait is unchanged for every other sender. --- .../securesms/groups/GroupLeavingWorker.kt | 18 +++++------------- .../securesms/groups/GroupLeavingWorkerTest.kt | 13 +------------ 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt index 9c01841fef..829ceab778 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt @@ -146,19 +146,11 @@ class GroupLeavingWorker @AssistedInject constructor( } catch (e: CancellationException) { throw e } catch (e: NonRetryableException) { - // We have no keys to encrypt the departure with. That is what a revoked - // access looks like, but it is also what a group whose keys have simply - // not been fetched yet looks like — a just-accepted invite, a restore, a - // cold start — so spend the attempts waiting for them to turn up before - // concluding they never will. - if (runAttemptCount < MAX_RETRIES) { - Log.w(TAG, "No keys to announce leaving group $groupId with. Retrying...", e) - return@launchAndWait Result.retry() - } - - // Out of attempts: honour the leave locally anyway, because the - // alternative is a group the user can never leave, however many times - // they ask. + // Our access to the group can be revoked before we get around to + // leaving it, which leaves us without the keys to encrypt the + // departure to the group. Nothing will grant them back, so honour the + // leave locally instead: the alternative is a group the user can never + // leave, however many times they ask. Log.e(TAG, "Unable to announce leaving group $groupId. Proceeding...", e) } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt index 0ed3d80144..8b374cfbac 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt @@ -67,22 +67,11 @@ class GroupLeavingWorkerTest { } @Test - fun `an unannounceable departure is retried before it is given up on`() = runTest { + fun `group is left locally when the departure cannot be announced`() = runTest { answerSendWith(Result.failure(NonRetryableException("no keys for this group"))) val result = worker(runAttemptCount = 0).doWork() - assertEquals(ListenableWorker.Result.retry(), result) - verify(exactly = 0) { configFactory.removeGroup(any()) } - verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } - } - - @Test - fun `group is left locally once the attempts to announce it are used up`() = runTest { - answerSendWith(Result.failure(NonRetryableException("no keys for this group"))) - - val result = worker(runAttemptCount = 2).doWork() - assertEquals(ListenableWorker.Result.success(), result) verify { configFactory.removeGroup(groupId) } verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } From 4b860309f51fd315d788ae2adb9d4cbfcffefc54 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 14:51:27 +1000 Subject: [PATCH 6/6] Don't attempt a departure we hold no keys to encrypt The send waits 20 seconds for keys before giving up, so a user leaving a group whose keys are gone paid 20 seconds per message to be told what the group config already knew. Check for the keys alongside the admin check that is already there and skip straight to the local cleanup, the same way a kicked group does. The catch below stays as the backstop for keys that disappear between the two. --- .../securesms/groups/GroupLeavingWorker.kt | 13 +++++++++--- .../groups/GroupLeavingWorkerTest.kt | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt index 829ceab778..c8ea3f1554 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupLeavingWorker.kt @@ -100,14 +100,21 @@ class GroupLeavingWorker @AssistedInject constructor( try { if (group?.destroyed != true) { // Only send the left/left notification group message when we are not kicked and we are not the only admin (only admin has a special treatment) - val weAreTheOnlyAdmin = configFactory.withGroupConfigs(groupId) { config -> + val (weAreTheOnlyAdmin, weHoldTheGroupKeys) = configFactory.withGroupConfigs(groupId) { config -> val allMembers = config.groupMembers.all() - allMembers.count { it.admin } == 1 && + val onlyAdmin = allMembers.count { it.admin } == 1 && allMembers.first { it.admin } .accountId() == storage.getUserPublicKey() + + onlyAdmin to config.groupKeys.keys().isNotEmpty() } - if (group != null && !group.kicked && !weAreTheOnlyAdmin) { + // Holding no keys means there is nothing to encrypt the departure with, and it + // is not a state that resolves itself: the flag above is only set when a + // revocation message is actually received, and receiving one clears the keys, + // so a revocation we were never told about looks exactly like this. Attempting + // the send would cost the user 20 seconds per message to learn that. + if (group != null && !group.kicked && !weAreTheOnlyAdmin && weHoldTheGroupKeys) { val address = Address.fromSerialized(groupId.hexString) // The jobs report with trySend, which delivers nothing unless a receiver // is already parked, and they run on their own dispatcher: either result diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt index 8b374cfbac..52df6f6d00 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/GroupLeavingWorkerTest.kt @@ -12,6 +12,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest +import network.loki.messenger.libsession_util.ReadableGroupKeysConfig import network.loki.messenger.libsession_util.ReadableGroupMembersConfig import network.loki.messenger.libsession_util.ReadableUserGroupsConfig import network.loki.messenger.libsession_util.util.GroupInfo @@ -56,11 +57,18 @@ class GroupLeavingWorkerTest { ) // No admins, so we are not the only admin and the leave takes the announce-and-go path + groupConfigsHold(groupKeys = listOf(ByteArray(32))) + } + + private fun groupConfigsHold(groupKeys: List) { every { configFactory.dangerouslyAccessGroupConfigs(groupId) } returns Pair( mockk { every { groupMembers } returns mockk { every { all() } returns emptyList() } + every { this@mockk.groupKeys } returns mockk { + every { keys() } returns groupKeys + } }, {}, ) @@ -77,6 +85,18 @@ class GroupLeavingWorkerTest { verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } } + @Test + fun `no departure is attempted when we hold no keys to send it with`() = runTest { + groupConfigsHold(groupKeys = emptyList()) + + val result = worker(runAttemptCount = 0).doWork() + + assertEquals(ListenableWorker.Result.success(), result) + verify(exactly = 0) { messageSender.send(any(), any(), any()) } + verify { configFactory.removeGroup(groupId) } + verify(exactly = 0) { storage.insertGroupInfoErrorQuit(any()) } + } + @Test fun `retries stop once the attempts are used up`() = runTest { answerSendWith(Result.failure(RuntimeException("network went away")))