Skip to content

feat: Remove "Delete Messages from Server" (delete_server_after) config option#8217

Draft
Hocuri wants to merge 18 commits intomainfrom
hoc/remove-delete_server_after
Draft

feat: Remove "Delete Messages from Server" (delete_server_after) config option#8217
Hocuri wants to merge 18 commits intomainfrom
hoc/remove-delete_server_after

Conversation

@Hocuri
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri commented May 5, 2026

Fix #8195

The most interesting change is in imap.rs because there, messages are marked for immediate deletion on single-device chatmail profiles.

WRT tests:

  • test_immediate_autodelete() tests the auto-deletion; test_imap_autodelete_fully_downloaded_msg() tests that even for a message that is split into pre- and post-message, both messages are deleted.
  • A lot of tests test that if bcc_self is on, messages are not auto-deleted. E.g. test_one_account_send_bcc_setting() and test_markseen_message_and_mdn() relies on the fact that messages stay on the server when bcc_self is on.
  • Unfortunately, it is not possible to test that messages are only deleted for chatmail servers, because we don't have any tests that use a non-chatmail server AFAIK?

Open question: Do we want to treat Nauta.cu like a chatmail server, in order to keep the old behavior on Nauta of auto-deleting? If not, I'll tell @adbenitez how he can do it in AC. In order to do this, need to find out in fn fetch_many_msgs() whether the server is Nauta, probably by querying the transports SQL table

@Hocuri Hocuri marked this pull request as draft May 6, 2026 10:21
@Hocuri Hocuri force-pushed the hoc/remove-delete_server_after branch from eb837a7 to 8e3e939 Compare May 6, 2026 10:32
@Hocuri Hocuri force-pushed the hoc/remove-delete_server_after branch from 8e3e939 to 6ad3f80 Compare May 6, 2026 12:19
@Hocuri Hocuri force-pushed the hoc/remove-delete_server_after branch from 52fe48d to c70be03 Compare May 6, 2026 13:27
assert msg_in.text == msg_out.text


def test_verified_group_vs_delete_server_after(acfactory, tmp_path, lp):
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.

This test tested how delete_server_after interacted with verified groups. We removed both features.

Comment thread python/tests/test_1_online.py
@Hocuri Hocuri requested review from iequidoo and link2xt May 6, 2026 15:10
@Hocuri Hocuri changed the title [WIP] Remove delete_server_after Remove delete_server_after May 6, 2026
@Hocuri Hocuri marked this pull request as ready for review May 6, 2026 15:10
@Hocuri Hocuri changed the title Remove delete_server_after feat: Remove delete_server_after May 6, 2026
@Hocuri Hocuri changed the title feat: Remove delete_server_after feat: Remove "Delete Messages from Server" (delete_server_after) config option May 6, 2026
@iequidoo
Copy link
Copy Markdown
Collaborator

iequidoo commented May 7, 2026

Unfortunately, it is not possible to test that messages are only deleted for chatmail servers, because we don't have any tests that use a non-chatmail server AFAIK?

Config::FixIsChatmail was added for this reason after CI switched to chatmail. But it seems that only test_dont_move_sync_msgs uses it to emulate a non-chatmail use case.

Comment thread deltachat-jsonrpc/src/api.rs Outdated
Comment thread src/message.rs Outdated
Comment thread python/tests/test_1_online.py
Comment thread src/ephemeral/ephemeral_tests.rs
Comment thread src/receive_imf/receive_imf_tests.rs
Comment thread src/imex.rs Outdated
Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
Hocuri and others added 2 commits May 7, 2026 10:23
Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
Comment thread src/imap.rs
context
.sql
.execute(
"UPDATE imap SET target='' WHERE 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.

Should this filter by transport_id? Otherwise we are deleting the message from other transports as well just because the copy arrived to chatmail relay.

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.

Indeed, but filtering by transport_id also doesn't look correct because then the message isn't deleted from chatmail relays if it's received via a non-chatmail relay first because of the logic in prefetch_should_download(). I think that && is_chatmail should be removed from the expr above and SQL UPDATE should be done for all chatmail relays, not just for the current transport.

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.

Hm, if the message arrives later on the other relay, then we also want to remove it on that second relay. This needs some new logic, because currently the message there is ignored bc we already know it

@Hocuri Hocuri marked this pull request as draft May 8, 2026 10:27
@iequidoo iequidoo self-requested a review May 8, 2026 12:45
@Hocuri
Copy link
Copy Markdown
Collaborator Author

Hocuri commented May 8, 2026

OK, so, I now came up with a new logic for deletion:

We have a function:

async fn maybe_mark_for_deletion(
    is_chatmail: bool,
    rfc724_mid: &String,
    context: &Context,
) -> Result<()> {
    if !context.get_config_bool(Config::BccSelf).await? && is_chatmail {
        context
            .sql
            .execute(
                "UPDATE imap SET target='' WHERE rfc724_mid=?",
                (rfc724_mid,),
            )
            .await?;
        context.scheduler.interrupt_inbox().await;
    }

    Ok(())
}

After prefetching, we want to call maybe_mark_for_deletion() iff it's not a post-message OR the post-message was already downloaded. After receive_imf(), we want to call maybe_mark_for_deletion() unconditionally.

I.e. if it's a post-message that is Available, Failure, or InProgress, then we do not want to delete it.
WRT InProgress, we do want to delete it later on all relays later, after we downloaded it.

So, probably we should not filter by transport_id.

I'm not sure about checking is_chatmail - maybe it's fine to just remove a message from all transports
if it also arrived via a chatmail transport,
because generally the only reason why this happens is because the chatpartner has DC with multi-relay.

I.e., we want to delete everything for which prefetch_should_download() returns false, EXCEPT for the case where a post-message's download failed or is in progress.

Right now, much of this is instead done by delete_expired_imap_messages(), which just deletes everything after MIN_DELETE_SERVER_AFTER (2 days) in single-device chatmail profiles. Idk, maybe that logic is good enough and we should just bring it back, though it may certainly delete stuff we still need (post-messages), and it keeps a lot of messages longer than necessary.

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.

Remove delete_server_after config

3 participants