fix: Don't send removal message to contact that hasn't been a chat member (#8298)#8310
Conversation
| transaction.execute( | ||
| "UPDATE chats_contacts | ||
| SET remove_timestamp=MAX(add_timestamp+1, ?) | ||
| SET remove_timestamp=MAX(remove_timestamp, add_timestamp+1, ?) |
There was a problem hiding this comment.
timestamp passed to the function may be a remote timestamp, so we need to make sure to not decrease remove_timestamp. The resulting timestamp shall not depend on the order of received messages. The same for add_timestamp in add_to_chat_contacts_table()
There was a problem hiding this comment.
Is this change needed for the fix? Looks unrelated.
It can also be that our clock was wrong in the past, then timestamps in the database may be stuck in the future and never limited back to current timestamp with this change.
There was a problem hiding this comment.
I can move this to a separate commit/PR and add a test.
If we want to protect from wrong clocks, bumping to add_timestamp+1 also looks incorrect: if it's in the future, we should fix up add_timestamp instead, otherwise it never fixes if this function is called and even increments each time for the passed contacts, see DO UPDATE SET add_timestamp=remove_timestamp below.
This change fixes a more realistic scenario when messages arrive reordered, resulting remove_timestamp shouldn't change because of this. We can protect from wrong clocks as well, but then need to do everything more accurately.
EDIT: Decided to move this to a separate PR at least.
EDIT: Made a fix which decreases member timestamps only when necessary, but not going to create a PR because it seems there are no important cases justifying the code complication: iequidoo/dont-decrease-member-timestamps
| let bob = &tcm.bob().await; | ||
| let charlie = &tcm.charlie().await; | ||
|
|
||
| let alice_broadcast_id = create_broadcast(alice, "Channel".to_string()).await?; |
There was a problem hiding this comment.
The same test for groups may be added, but group contact removal shares the common code with broadcasts, so such a test won't test anything in addition
|
https://github.com/chatmail/core/actions/runs/26922572038/job/79426682834?pr=8310 -- the CFFI Python |
iequidoo
force-pushed
the
iequidoo/cant_remove_nonmember
branch
from
b8f2276 to
402e339
Compare
|
|
||
| /// Removes a contact from the chat | ||
| /// without leaving a trace. | ||
| /// Returns whether the contact has ever been a chat member since this function was called the |
There was a problem hiding this comment.
This is difficult to understand without reading the code and is not entirely true, past members are also removed after some time.
I think it's better to say that it returns whether any database row was removed, either for an existing member or a "past member".
There was a problem hiding this comment.
Changed to
/// Returns whether the contact was removed, even if it was a past contact. If so, a removal message
/// should be sent if the removal is issued by this device.
…mber (#8298) I.e. don't fail `remove_contact_from_chat()` for such a contact because there may be a race condition with a remote removal of the contact done without trace, but don't send a removal message and sync message in this case and don't emit a `ChatModified` event. If a contact is already a past member, we still send a removal message to the chat, this is safe and protects from lost removal messages, so there's no need to complicate the code in this case.
iequidoo
force-pushed
the
iequidoo/cant_remove_nonmember
branch
from
402e339 to
729ef80
Compare
2 participants
I.e. don't fail
remove_contact_from_chat()for such a contact because there may be a race condition with a remote removal of the contact done without trace, but don't send a removal message and sync message in this case and don't emit aChatModifiedevent.If a contact is already a past member, we still send a removal message to the chat, this is safe and protects from lost removal messages, so there's no need to complicate the code in this case.
Fix #8298