fix: Send pre-message after successful sending of post-message (#8063)

[iequidoo]

Also, send the message text in the post-message too, we can't support the workaround for duplicated
message text shown in ancient versions anymore, otherwise newer versions won't show the message text
at all.

On the receiving side, don't download a post-message w/o known pre-message if the post-message
exceeds DownloadLimit to save user's traffic.

This isn't easy to test in Rust though because the smtp sending code requires an SMTP connection,
so it's only tested (by already existing tests) that messages are queued in the right order.

Another problem is that the smtp sending logic doesn't try to send any messages following a
failed-to-send message until the retry limit is reached, so if there's smth wrong with a
post-message, other unrelated messages are delayed, but this problem has already existed.

Fix #8063

[iequidoo]

Decided to rename it to underline that only an already existing message is updated, to avoid more bugs caused by not fully handling post-messages

[Hocuri]

What bug was caused by this?

[iequidoo]

See #8222 (comment), probably the wrong condition was added there because handle_post_message() looks like hadndling post-messages in all cases. Actually it only updates existing messages

[iequidoo]

Compat problem: missing iroh topic id

[Hocuri]

Do you know what is the case where the incompatibility causes an actual problem? But it's likely fine: The combination a) someone uses a realtime webxdc, b) different versions of DC are in use, c) some message is big enough to be split into pre- and post-message seems very unlikely.

Did you test that things work fine if both sides have the new version? Though I'm not completely sure what to test there - something like using webxdc realtime editor in a group (with a document large enough that it's split into pre- and post-message), and while it is open, a third person joins, then the webxdc is resent to the new person, then the new person should also see the cursor positions.

[iequidoo]

There's JSON-RPC Python test_realtime_large_webxdc that tests this logic, i only removed waiting for the MSGS_CHANGED event from it. Anyway, now the logic is simpler than it was as the iroh gossip topic is set unconditionally, and it's still updated from a post-message if for some reason it arrives after the pre-message. As for cross-version compatibility, maybe it's indeed not a big problem here.

[Hocuri]

I'm having problems understanding why the changes here are necessary, can you clarify?

[iequidoo]

If a post-message appears on IMAP, it should be deleted from smtp because it was sent successfully. Now this logic is common for all outgoing messages. One test started failing before i made this change, but i've forgotten which one exactly.

[Hocuri]

What bug was caused by this?

[iequidoo]

I see the main problem with this PR is adding the available_post_msgs.timestamp column which breaks forward compatibility. Possible solutions:

• Increment BACKUP_VERSION (@Hocuri is against this AFAIU)
• Add a new table and add available post-messages to the both tables for the case of possible Core downgrade.
• Don't download post-messages having no pre-messages at all. Though, we can immediately download messages fitting into DownloadLimit, we already do that. Finally i think this may even be good as we save traffic.

Need to take some decision here. For now have chosen the last one.

[iequidoo]

Usually we don't do so, but decided to clean up the table in case of an "upgrade again" scenario. Don't want to add a migration and migrations are executed only once. Can be removed once we add a migration deleting the table.

[Hocuri]

No need to clean up the table, it can just be left as-is

[iequidoo]

Ok, removed this. What i wanted is to not leave traces in the db that we received a particular message. E.g. for tombstones we forget them after two days, but for available post messages we won't do this at all until we remove the table in some future migration.

[Hocuri]

Read the code, but not finished reviewing yet, this is still on my TODO-list:

TODO: Look at how this logic interacts with rfc724_mid_download_tried()
TODO: Test avatars
TODO: Generally, more testing on a real device
TODO: Test compatibility, esp. whether the text goes lost if an old version sends image+text to the new version

[Hocuri]

No need to clean up the table, it can just be left as-is

[Hocuri]

This PR is probably reviewable without understanding this logic behind the AND (recipients LIKE ?2 OR recipients LIKE ('% ' || ?2)), but still, I tried to understand it. Do you know more about it? In any case, it should probably get a comment.

What I know is this:

It sometimes happens that a slow server (usually a classical email server) receives a message via SMTP, but then the connection to the server dies before it sends the OK response. In order to handle this case, we delete the SMTP send jobs if we receive our own message via IMAP.

Now, we split messages with long recipient lists into multiple SMTP jobs in order to work around server-side recipient size limits; therefore, we need to make sure to always send to self last, so that we don't delete the SMTP jobs before all of them are sent out.

The logic here makes sure that we only delete the SMTP job where the recipient list ends with the self address, i.e. it only deletes the last chunk in the list. Maybe this is to make sure that the earlier chunks are tried again if they fail at because of a temporary server error but then the last chunk (with the self address) is sent out successfully?

However, thinking about it, I'm wondering why this works at all: prefetch_should_download() returns false for messages that are already in the database, so that I'm wondering why the message makes it all the way to receive_imf() at all.

[Hocuri]

The logic here makes sure that we only delete the SMTP job where the recipient list ends with the self address, i.e. it only deletes the last chunk in the list. Maybe this is to make sure that the earlier chunks are tried again if they fail at because of a temporary server error but then the last chunk (with the self address) is sent out successfully?

After talking to @link2xt, yes this is the idea

However, thinking about it, I'm wondering why this works at all: prefetch_should_download() returns false for messages that are already in the database, so that I'm wondering why the message makes it all the way to receive_imf() at all.

Neither @link2xt nor me understood it, worth digging into at some point, but unrelated to this PR here.

[iequidoo]

I suspect that a27e84a wasn't actually correct. At least it should be modifying src/imap.rs as well.

[Hocuri]

Do you know what is the case where the incompatibility causes an actual problem? But it's likely fine: The combination a) someone uses a realtime webxdc, b) different versions of DC are in use, c) some message is big enough to be split into pre- and post-message seems very unlikely.

Did you test that things work fine if both sides have the new version? Though I'm not completely sure what to test there - something like using webxdc realtime editor in a group (with a document large enough that it's split into pre- and post-message), and while it is open, a third person joins, then the webxdc is resent to the new person, then the new person should also see the cursor positions.

[Hocuri]

FTR, this doesn't fully fix #8063, because it doesn't yet delete the delete the pre-message smtp entry if sending the post-message fails. This means that when Alice tries to send a message that is too large, it still looks to Bob like he received a message even though he did not. But this doesn't have to happen in the PR here.

It's not too hard, can be done like this: (plus a test)

Details

diff --git a/src/smtp.rs b/src/smtp.rs
index 768e63983..55615501d 100644
--- a/src/smtp.rs
+++ b/src/smtp.rs
@@ -6,6 +6,7 @@
 use anyhow::{Context as _, Error, Result, bail, format_err};
 use async_smtp::response::{Category, Code, Detail};
 use async_smtp::{EmailAddress, SmtpTransport};
+use rusqlite::OptionalExtension as _;
 use tokio::task;
 
 use crate::chat::{ChatId, add_info_msg_with_cmd};
@@ -457,7 +458,25 @@ pub(crate) async fn send_msg_to_smtp(
             }
             context
                 .sql
-                .execute("DELETE FROM smtp WHERE id=?", (rowid,))
+                .transaction(|trans| {
+                    let pre_rfc724_mid: Option<String> = trans
+                        .query_row(
+                            "SELECT pre_rfc724_mid FROM msgs WHERE id=(
+                                SELECT msg_id FROM smtp WHERE id=?
+                            )",
+                            (rowid,),
+                            |row| row.get(0),
+                        )
+                        .optional()?;
+                    if let Some(pre_rfc724_mid) = pre_rfc724_mid
+                        && pre_rfc724_mid != ""
+                    {
+                        // Do not send out the pre-message if sending the post-message failed:
+                        trans.execute("DELETE FROM smtp WHERE rfc724_mid=?", (pre_rfc724_mid,))?;
+                    }
+                    trans.execute("DELETE FROM smtp WHERE id=?", (rowid,))?;
+                    Ok(())
+                })
                 .await?;
         }
     };

[iequidoo]

FTR, this doesn't fully fix #8063, because it doesn't yet delete the delete the pre-message smtp entry if sending the post-message fails. This means that when Alice tries to send a message that is too large, it still looks to Bob like he received a message even though he did not.

I just forgot to do this for the SendResult::Failure case. Have fixed now. But now i'm unsure this is good at all, maybe it's still better to send a pre-message, at least this way the text will be received. Particularly, in the retry scenario -- when 6 unsuccessful tries have been done, it may be too late to cancel sending the pre-message because the user maybe has already switched to another app and won't notice the error.

EDIT: I suggest not to cancel sending of the pre-message. So if everything goes well, we send messages in the expected order, and if the post-message sending fails, e.g. because it's big, we at least send the text. Anyway, there's no guarantee that the post-message reaches the receiver, so trying to fix the fail scenario has a little sense, moreover it's not clear what is better. We can display the message as failed to inform the user however.