fix: Correctly set and read the deleted flag to read and remove messages on the server - #8460
fix: Correctly set and read the deleted flag to read and remove messages on the server#8460Hocuri wants to merge 9 commits into
deleted flag to read and remove messages on the server#8460Conversation
Hocuri
changed the title
Hocuri
changed the title
deleted flag to read and remove messages on the server
815cf2d to
a5300e3
Compare
8344b16 to
77cb235
Compare
77cb235 to
bcc88c9
Compare
| // `delete_expired_messages()` is called before `delete_tombstoned_messages_from_imap()`, | ||
| // because this matches the behavior in production | ||
| // where the ephemeral loop calls `delete_expired_messages()` as soon as a message timer expired. | ||
| delete_expired_messages(&t, time()).await?; |
There was a problem hiding this comment.
I checked that the test fails now without my fix.
| // This uses `AND chat_id={DC_CHAT_ID_TRASH}`, so that the index on `chat_id` can be used. | ||
| // Only messages in the trash chat are ever marked as deleted, which makes this optimization possible. | ||
| // This speeds up this SQL query by a factor of ~3. |
There was a problem hiding this comment.
I'm wondering whether we should add AND chat_id={DC_CHAT_ID_TRASH} to the other two SQL statements just to get the same behavior in case someone marks non-trashed messages as deleted, even though it doesn't improve performance there because sqlite anyways needs to do a full scan.
There was a problem hiding this comment.
I think it makes sense even just for consistency sake.
| let delete = if let Some(message_id) = &message_id { | ||
| message::rfc724_mid_exists_ex(context, message_id, "deleted=1") | ||
| .await? | ||
| .is_some_and(|(_msg_id, deleted)| deleted) | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| // Generate a fake Message-ID to identify the message in the database | ||
| // if the message has no real Message-ID. | ||
| let message_id = message_id.unwrap_or_else(create_message_id); | ||
|
|
||
| if delete { | ||
| info!(context, "Deleting locally deleted message {message_id}."); | ||
| } | ||
|
|
||
| let target = if delete { "" } else { folder }; | ||
|
|
There was a problem hiding this comment.
This logic is not needed anymore because delete_tombstoned_messages_from_imap() now cares about this.
| WHERE transport_id=?1 | ||
| AND rfc724_mid IN ( | ||
| SELECT rfc724_mid FROM msgs | ||
| WHERE id>9 | ||
| AND ((ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2) OR | ||
| ((param GLOB '*\nc=1*' OR param GLOB 'c=1*') AND download_state=?3)) | ||
| WHERE ((param GLOB '*\nc=1*' OR param GLOB 'c=1*') AND download_state=?2) OR | ||
| deleted=1 | ||
| UNION | ||
| SELECT pre_rfc724_mid FROM msgs | ||
| WHERE pre_rfc724_mid!='' | ||
| AND id>9 | ||
| AND ((ephemeral_timestamp!=0 AND ephemeral_timestamp<=?2) OR | ||
| (param GLOB '*\nc=1*' OR param GLOB 'c=1*')) | ||
| AND param GLOB '*\nc=1*' OR param GLOB 'c=1*' OR deleted=1 | ||
| )", | ||
| (transport_id, now, DownloadState::Done), | ||
| (transport_id, DownloadState::Done), |
There was a problem hiding this comment.
FTR, we will probably remove this whole SQL statement once we remove is_chatmail. I fixed it anyways now.
| if folder == target | ||
| && prefetch_should_download(context, &headers, &message_id, fetch_response.flags()) |
There was a problem hiding this comment.
The folder==target condition is not needed because prefetch_should_download() will return false if a trashed message exists.
Although that there is a bug in prefetch_should_download() that it returns true for pre-messages even when they were already downloaded; I will make a PR for that.
There was a problem hiding this comment.
I will make a PR for that.
Let's maybe merge that one first then.
hpk42
left a comment
hpk42
left a comment
There was a problem hiding this comment.
Thinking more about it, i think we shouldn't remove messages on the server because of device_delete_after. It means deleting encrypted messages after maybe just an hour or a day, blocking all other devices from ever seeing those. This is clearly not the intention or the understanding of a user who sets "device-delete-after" in the current UIs.
4 participants
Make
delete_expired_messages()set thedeletedflag in themsgstable, and makedelete_expired_imap_messages()check it. This fixes two bugs:delete_expired_messages()deletes the message locally, thendelete_expired_imap_messages()doesn't find the message anymore, and can't mark it for deletion. I could reproduce this bug.Additionally, this improves the performance of the SQL statements by removing the superfluous
AND id>9. TheAND id>9made SQLite think that it's smart to use theidindex, but since almost all messages have id>9, using this index actually worsened the performance by preventing the use of other, better-suited indexes.For explanation of the logic, as it was before this PR:
msgsalready has adeletedcolumn, but it's only checked if the message later arrives via IMAP (inreceive_imfwithmessage::rfc724_mid_exists_ex(context, message_id, "deleted=1")). And it is only set byChatId::delete_ex()andMsgId::trash(). It is:delete_expired_messages()(which deletes ephemeral messages locally)delete_expired_imap_messages()(which marks ephemeral messages for deletion on IMAP, and if bcc_self is off it marks all fully downloaded messages for deletion on IMAP [non non-chatmail accounts, only encrypted ones])