Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

groups are hard to leave #5366

Closed
adbenitez opened this issue Mar 21, 2024 · 2 comments · Fixed by #5376
Closed

groups are hard to leave #5366

adbenitez opened this issue Mar 21, 2024 · 2 comments · Fixed by #5376
Assignees
Labels
bug Something is not working

Comments

@adbenitez
Copy link
Member

Leaving a group or trying to remove members prove to be a hard task with the new "group consistency" algorithm, there are 2 main situations

1- Active groups: it happens often in active "chatty" groups that:

  1. You try to remove some members
  2. While your member-removal messages are being sent or in delivery process, you receive messages from the other members of the group, this adds back the members you just removed, if you, or someone else in the same situation send a message then the effect propagate, tending to have the members added back

2 - Some members were offline: Another situation is:

  1. Several messages have been sent during hours, including member removals
  2. Some users were offline (ex. sleeping or in a mountain hike) they sent some messages/pictures while offline to share later when they go online or they simply didn't wait for all the messages of the group to be fetched and started to send messages in the group now that they just go online, these messages will add back removed members for the other group members since the offline users didn't wait to receive the messages of the removals and have old group state, propagating it
@iequidoo
Copy link
Collaborator

At least the second problem should be fixed by 33777d8, but also it can help for the first one, although races involving Param::MemberListTimestamp are still possible. Anyway it would be interesting how much it helps

@iequidoo iequidoo self-assigned this Mar 21, 2024
@iequidoo iequidoo added the bug Something is not working label Mar 21, 2024
@iequidoo
Copy link
Collaborator

iequidoo commented Mar 21, 2024

I think to fix the first problem more reliably, we need to add some tolerance to MemberListTimestamp when deciding whether to execute this logic in apply_group_changes():

        if !recreate_member_list {
            // Don't delete any members locally, but instead add absent ones to provide group                                                                                                                        
            // membership consistency for all members:                                                                                                                                                               
            // - Classical MUA users usually don't intend to remove users from an email thread, so                                                                                                                   
            //   if they removed a recipient then it was probably by accident.                                                                                                                                       
            // - DC users could miss new member additions and then better to handle this in the same                                                                                                                 
            //   way as for classical MUA messages. Moreover, if we remove a member implicitly, they                                                                                                                 
            //   will never know that and continue to think they're still here.                                                                                                                                      
            // But it shouldn't be a big problem if somebody missed a member removal, because they                                                                                                                   
            // will likely recreate the member list from the next received message. The problem                                                                                                                      
            // occurs only if that "somebody" managed to reply earlier. Really, it's a problem for                                                                                                                   
            // big groups with high message rate, but let it be for now.                                                                                                                                             
            let mut diff: HashSet<ContactId> =
                new_members.difference(&chat_contacts).copied().collect();
            new_members = chat_contacts.clone();
            new_members.extend(diff.clone());

Maybe 5-10 minutes are sufficient, i don't think the clocks on users devices are often unsynchronised so much.

iequidoo added a commit that referenced this issue Mar 25, 2024
Let's add a 1-minute tolerance to `Params::MemberListTimestamp`.

This adds to the group membership consistency algo the following properties:
- If remote group membership changes were made by two members in parallel, both of them are applied,
  no matter in which order the messages are received.
- If we remove a member locally, only explicit remote member additions/removals made in parallel are
  allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to
  reply earlier than receiving our removal of a member, we added it back which doesn't look good.
iequidoo added a commit that referenced this issue Mar 25, 2024
Let's add a 1-minute tolerance to `Params::MemberListTimestamp`.

This adds to the group membership consistency algo the following properties:
- If remote group membership changes were made by two members in parallel, both of them are applied,
  no matter in which order the messages are received.
- If we remove a member locally, only explicit remote member additions/removals made in parallel are
  allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to
  reply earlier than receiving our removal of a member, we added it back which doesn't look good.
iequidoo added a commit that referenced this issue Mar 26, 2024
Let's add a 1-minute tolerance to `Params::MemberListTimestamp`.

This adds to the group membership consistency algo the following properties:
- If remote group membership changes were made by two members in parallel, both of them are applied,
  no matter in which order the messages are received.
- If we remove a member locally, only explicit remote member additions/removals made in parallel are
  allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to
  reply earlier than receiving our removal of a member, we added it back which doesn't look good.
iequidoo added a commit that referenced this issue Apr 8, 2024
Let's add a 1-minute tolerance to `Params::MemberListTimestamp`.

This adds to the group membership consistency algo the following properties:
- If remote group membership changes were made by two members in parallel, both of them are applied,
  no matter in which order the messages are received.
- If we remove a member locally, only explicit remote member additions/removals made in parallel are
  allowed, but not the synchronisation of the member list from "To". Before, if somebody managed to
  reply earlier than receiving our removal of a member, we added it back which doesn't look good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants