Skip to content

Fix: leaving a group we hold no encryption keys for retried forever - #2210

Draft
mpretty-cyro wants to merge 3 commits into
devfrom
fix/group-leave-retry-loop
Draft

Fix: leaving a group we hold no encryption keys for retried forever#2210
mpretty-cyro wants to merge 3 commits into
devfrom
fix/group-leave-retry-loop

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

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 "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: GroupLeavingJob never 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

  • MessageSendJob types the keyless-group case as NonRetryableException where 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.
  • GroupLeavingWorker falls through to the local cleanup on that error instead of reporting a failure: the same catch-log-proceed the destroyGroup() 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.
  • The error message is cleared alongside the leaving message when a leave starts, so a conversation carries at most one of them.
  • The status channel the two leave messages report through is now buffered. They report with 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 which receive() waited forever, holding that group's GroupScope semaphore 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

  • The GroupUpdated job serialization fix (separate PR) is independent and can land in either order; the two branches merge cleanly.
  • Test fixtures are synthetic throughout.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant