Skip to content

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

Open
iequidoo wants to merge 2 commits into
mainfrom
iequidoo/send-post-msg-first
Open

fix: Send pre-message after successful sending of post-message (#8063)#8222
iequidoo wants to merge 2 commits into
mainfrom
iequidoo/send-post-msg-first

Conversation

@iequidoo
Copy link
Copy Markdown
Collaborator

@iequidoo iequidoo commented May 8, 2026

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, download post messages w/o known pre-message after 30s to protect from lost
messages.

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 iequidoo force-pushed the iequidoo/send-post-msg-first branch 2 times, most recently from 04832ec to fed3a6a Compare May 11, 2026 08:01
Comment thread src/receive_imf.rs
}

async fn handle_post_message(
async fn update_from_post_msg(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What bug was caused by this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/receive_imf.rs

// Maybe set logging xdc and add gossip topics for webxdcs.
for (part, msg_id) in mime_parser.parts.iter().zip(&created_db_entries) {
if mime_parser.pre_message != PreMessageMode::Post
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compat problem: missing iroh topic id

Comment thread src/download.rs
context: &Context,
session: &mut Session,
) -> Result<()> {
const PRE_MSG_WAIT_TIME: i64 = 30;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compat problem: extra traffic for older versions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it's fine, but let's ask @r10s:

This PR sends the pre-message only after successfully sending the post-message. This is to prevent a problem multiple users complained about: The pre-message arrives, but the post-message is delayed or rejected due to its size. On the receiver's side, the message will just say in "Downloading..." indefinitely, which is irritating.

Now, current versions of Delta Chat will download all Post-messages that do not have a pre-message in the assumption that the pre-message got lost. This means that a large message sent from the future version of DC will be downloaded unconditionally by the current version of DC, ignoring the download_limit setting, potentially unexpectedly wasting mobile data.

Do you think that this is fine?

If we do not want to accept this, we need to deploy this change over two releases: One release stops downloading Post-Messages that do not have a Pre-Message (i.e. the receiving part), and the next release sends the pre-message only after successfully sending the post-message (sending part).

Comment thread src/mimefactory.rs
@iequidoo iequidoo force-pushed the iequidoo/send-post-msg-first branch from fed3a6a to 42d09d4 Compare May 18, 2026 10:24
@iequidoo iequidoo marked this pull request as ready for review May 18, 2026 11:26
@iequidoo iequidoo force-pushed the iequidoo/send-post-msg-first branch from 42d09d4 to f6eacd1 Compare May 20, 2026 13:39
@iequidoo iequidoo requested a review from Hocuri May 20, 2026 13:39
@iequidoo iequidoo force-pushed the iequidoo/send-post-msg-first branch from f6eacd1 to 2947c49 Compare May 25, 2026 23:46
@iequidoo iequidoo marked this pull request as draft May 27, 2026 04:20
iequidoo added 2 commits May 27, 2026 01:41
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, download post messages w/o known pre-message after 30s to protect from lost
messages.

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.
Messages are deleted from the inbox loop, so it should be interrupted. The SMTP doesn't need to be
interrupted explicitly however, chat::sync() already does this internally.
@iequidoo iequidoo force-pushed the iequidoo/send-post-msg-first branch from 2947c49 to 29a6f8d Compare May 27, 2026 04:43
@iequidoo iequidoo marked this pull request as ready for review May 27, 2026 05:07
Comment thread src/download.rs
Comment on lines +327 to +342
const PRE_MSG_WAIT_TIME: i64 = 30;
let now = tools::time();
let rfc724_mids = context
.sql
.query_map_vec("SELECT rfc724_mid FROM available_post_msgs", (), |row| {
let rfc724_mid: String = row.get(0)?;
Ok(rfc724_mid)
})
.query_map_vec(
"
SELECT rfc724_mid FROM available_post_msgs
WHERE timestamp<=? OR timestamp>?
ORDER BY timestamp, rowid
",
(now.saturating_sub(PRE_MSG_WAIT_TIME), now),
|row| {
let rfc724_mid: String = row.get(0)?;
Ok(rfc724_mid)
},
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is becoming complex enough that I'm wondering whether it's worth it to guard against lost pre-messages. I mean, the large message getting through but not the small message is a pretty rare case, it creates extra edge cases, and it creates extra possible problems like accidentally wasting the user's mobile data and ignoring the download limit.

Maybe we should just remove this function. Then we also don't need the timestamp on available_post_msgs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the large message getting through but not the small message is a pretty rare case

I'm not sure. If the server loses messages randomly, regardless of their size, it may happen

Comment thread src/imap.rs
download_later.len(),
);
let trans_fn = |t: &mut rusqlite::Transaction| {
let mut stmt = t.prepare("INSERT OR IGNORE INTO available_post_msgs VALUES (?)")?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This old SQL statement will likely fail if the user upgrades (adding the column) and then downgrades again? Because SQL expects two values for inserting into the table.

This would require us to increase DCBACKUP_VERSION, but at this point, it really seems better to just not add the timestamp column.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a new table instead and add available post-messages to the both tables for the case of possible downgrade.

Maybe we need to add a STYLE.md rule that columns should be listed explicitly in INSERT statements, then the code will be more understandable and there will be no such problem for new columns having default values.

Comment thread src/mimefactory.rs
Comment thread src/download.rs
context: &Context,
session: &mut Session,
) -> Result<()> {
const PRE_MSG_WAIT_TIME: i64 = 30;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it's fine, but let's ask @r10s:

This PR sends the pre-message only after successfully sending the post-message. This is to prevent a problem multiple users complained about: The pre-message arrives, but the post-message is delayed or rejected due to its size. On the receiver's side, the message will just say in "Downloading..." indefinitely, which is irritating.

Now, current versions of Delta Chat will download all Post-messages that do not have a pre-message in the assumption that the pre-message got lost. This means that a large message sent from the future version of DC will be downloaded unconditionally by the current version of DC, ignoring the download_limit setting, potentially unexpectedly wasting mobile data.

Do you think that this is fine?

If we do not want to accept this, we need to deploy this change over two releases: One release stops downloading Post-Messages that do not have a Pre-Message (i.e. the receiving part), and the next release sends the pre-message only after successfully sending the post-message (sending part).

Comment thread src/receive_imf.rs
"Message {rfc724_mid} is already in some chat or deleted."
);
if mime_parser.incoming {
return Ok(None);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/receive_imf.rs
}

async fn handle_post_message(
async fn update_from_post_msg(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What bug was caused by this?

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.

Do not send pre-message if post-message sending failed

2 participants