Repository navigation
Fix: leaving a group we hold no encryption keys for retried forever - #2210
Merged
Merged
Conversation
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 dda6e90. 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.
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.
mpretty-cyro
marked this pull request as ready for review
September 21, 2026 01:31
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.
…r coming" This reverts commit 4a182b1. 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
GroupLeavingWorkerblocked on that send, classified the failure as retryable and returned WorkManager'sResult.retry(), which has backoff but no attempt cap — so the leave was re-attempted for as long as the group existed, appending another permanent "failed to leave" message to the conversation each time. One reported case ran for ten days; the group was never left.iOS has the same decision already:
GroupLeavingJobnever retries, and swallows the "cannot send the leave, but the user still gets to leave" errors so they fall through to the local cleanup. This brings Android to the same behaviour.Changes
MessageSendJobtypes the keyless-group case asNonRetryableExceptionwhere it is detected, so a caller waiting on the send can tell it apart from a transient failure. The job's own retry policy is untouched — keys granted late still let an ordinary message through on a later attempt.GroupLeavingWorkerfalls through to the local cleanup on that error instead of reporting a failure: the same catch-log-proceed thedestroyGroup()branch fifteen lines below has had since dda6e90. Every other failure keeps the retry, now capped, so no error class can reintroduce an unbounded loop.trySend, which delivers nothing unless a receiver is parked at that instant, and they run on their own dispatcher — so either result could land while the worker was still enqueueing the second send, after whichreceive()waited forever, holding that group'sGroupScopesemaphore and stalling every other operation queued for the group. Same shape as 462fe77.Tests
Six new unit tests (
MessageSendJobTest,GroupLeavingWorkerTest) — the first worker tests in the repo; they need no new test dependency. Full suite green: 304/304.Each test was mutation-controlled: reverting the exception type, narrowing the worker's catch, deleting the retry cap and deleting the message dedupe each kill exactly one test. Reverting the channel capacity hangs all four worker tests, which is deliberate — the suite will not run green against an unbuffered channel.
Notes for review
GroupUpdatedjob serialization fix (separate PR) is independent and can land in either order; the two branches merge cleanly.