pgp-contacts migration: tokio-runtime-worker panics because email contact in encrypted chat

[missytake]

• Operating System (Linux/Mac/Windows/iOS/Android): Xubuntu 24
• Core Version: 3abcc3a from PGP contacts #6796 (no idea if it was force-pushed since then)
• Client Version: 0cbc21563 from Pgp contacts deltachat/deltachat-desktop#5090

Expected behavior

I tried to send a message to an encrypted, non-verified chat (a team-bot relay group with another contact being in it, created before the migration). I expect the message to be sent correctly and encrypted to all participants, as I have keys for all involved addresses.

Actual behavior

Instead, the desktop client freezes when I press enter to send the message.

So I looked into the database, I verified that I have an email contact and a verified pgp-contact for this address; before the migration, I had only one contact for the address of course. but during the migration, when the contact was split up into email contact and pgp contact, the email contact was added to the chat instead of the pgp-contact. This means I can't encrypt to all contacts in the chat, even though I could in the past; the client crashes.

After restarting the client, there is a new message in the chat with the text I just failed to send; but it is always in the "sending" state and never gets successfully sent. Also, I still have the text in the composer as a draft.

Steps to reproduce the problem

So far I couldn't reproduce the situation with fresh accounts; let me know whether it is needed.

Screenshots

The client after I tried to send, frozen:

After restarting the client:

Logs

in the command line output I see the following error message:

5.3s [E]DC-RPC: stderr: 
thread 'tokio-runtime-worker' panicked at /home/user/code/chatmail-core/src/mimefactory.rs:377:25:
assertion failed: member_fingerprints.is_empty() || member_fingerprints.len() >= to.len()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 
Socket.<anonymous> (/home/user/code/deltachat-desktop/packages/target-electron/src/deltachat/stdio_server.ts:82:11),
Socket.emit (node:events:518:28),
addChunk (node:internal/streams/readable:561:12),
readableAddChunkPushByteMode (node:internal/streams/readable:512:3),
Readable.push (node:internal/streams/readable:392:5),
Pipe.onStreamRead (node:internal/stream_base_commons:191:23)
5.3s [w]core/event: 23 src/mimefactory.rs:330: Missing key for XXXXXXXXXXXXXX@systemli.org
5.4s [w]DC-RPC: child process exited with signal SIGABRT 
ChildProcess.<anonymous> (/home/user/code/deltachat-desktop/packages/target-electron/src/deltachat/stdio_server.ts:108:13),
ChildProcess.emit (node:events:518:28),
ChildProcess._handle.onexit (node:internal/child_process:293:12)
5.4s [i]DC-RPC: child process close all stdio with signal SIGABRT

Contact before the migration:

sqlite> select * from contacts where addr="XXXXXXXXXXXXXX@systemli.org"; 
10||XXXXXXXXXXXXXX@systemli.org|67108864|0|1749476091|J=1749476059
i=$BLOBDIR/8f082cba12d97e2893a2d7a12e4ac22.jpg
j=1749469081|Phant|0||0

Contacts after the migration:

sqlite> select * from contacts where addr="XXXXXXXXXXXXXX@systemli.org"; 
10||XXXXXXXXXXXXXX@systemli.org|16384|0|1749123428|J=1749073761
i=$BLOBDIR/avatar-cgjohdz.jpg
j=1749123428|Phant|0||0||0
3720||XXXXXXXXXXXXXX@systemli.org|67108864|0|1749476868|J=1749476746
i=$BLOBDIR/8f082cba12d97e2893a2d7a12e4ac22.jpg
j=1749476868|Phant|1749475723||0|0EB8F3310DE2E38F8F3F2CF5B83C0717245BF3B6|1

Chat before the migration:

sqlite> select * from chats_contacts where chat_id=3754;
3754|1|0|0
3754|595|0|0
3754|10|0|0

Chat after the migration (yes, the chat_id changed for the chat):

sqlite> select * from chats_contacts where chat_id=3751;
3751|1|0|0
3751|10|0|0
3751|595|0|0

[missytake]

Meanwhile fixed by removing the profile, upgrading to the current state of the branch, and reimporting a backup from 1.85.2 :) the chat has the pgp contacts now instead of the email contact.

[iequidoo]

Btw, for all debug assertions, we should make sure to have the corresponding errors in the release conf, if a failed check can lead to unexpected behavior. Do we maintain this? Moreover, from the code i see that member_timestamps depend on the db contents, but it's an external state, so the failed assertion doesn't just check the correctness of the Mimefactory::from_msg() code. CC @link2xt @Hocuri

[Hocuri]

Btw, for all debug assertions, we should make sure to have the corresponding errors in the release conf, if a failed check can lead to unexpected behavior. Do we maintain this?

No, but sounds sensible. We could create our own debug_assert!(context, condition) macro that logs a warning in #[cfg(not(debug_assertions))] instead of panicking.

Moreover, from the code i see that member_timestamps depend on the db contents, but it's an external state, so the failed assertion doesn't just check the correctness of the Mimefactory::from_msg() code.

Sounds fine to me to have debug assertions that assert non-local correctness

[iequidoo]

No, but sounds sensible. We could create our own debug_assert!(context, condition) macro that logs a warning in #[cfg(not(debug_assertions))] instead of panicking.

I think logging isn't sufficient, i'd suggest to call anyhow::ensure! in the release conf, even if that changes the function return type to Result.

Sounds fine to me to have debug assertions that assert non-local correctness

Agree, but non-local checks are more likely to fail, so they need to be checked in the release conf even more.

EDIT: I'll try to add such a macro, we don't have many debug assertions atm, so it shouldn't be a huge work.