feat: Remove hidden relays automatically by j-g00da · Pull Request #8402 · chatmail/core

j-g00da · 2026-07-07T06:28:49Z

Automatically remove relays that are hidden (is_published=0) and haven't been used to receive new messages for over 90 days.

Closes: #8384

j-g00da · 2026-07-07T06:43:22Z

There's one open question: #8384 (comment)

CI fails due to: #8403

link2xt · 2026-07-07T16:23:33Z

+    context
+        .sql
+        .execute(
+            "DELETE FROM transports \


All newline escapes can be removed, see https://github.com/chatmail/core/blob/main/STYLE.md#sql

The query should be checked with EXPLAIN QUERY PLAN to make sure it is not extremely slow with large number of messages.

Removing the transport without adding it to removed_transports means its removal will not be synced. This cleanup likely should be close to what delete_transport does, removing the transport and adding it to removed_transport at the same time. And generating a sync message probably so removal is synced immediately.

Why should it be synced? If a transport isn't accessible from some device because of e.g. conectivity issues, it shouldn't be removed from other devices. Anyway it will be removed without additional synchronization normally.

Another problem here is how this interacts with DeleteDeviceAfter. Maybe it's better to add some transports.timestamp_rcvd_last column? This way we also don't need to fully scan msgs table, that may take seconds if the table isn't cached in memory, see also #7848 .

Why should it be synced?

Yeah, since housekeep runs on all devices, maybe syncing it is just a redundant complexity?

Only transports table is fully scanned, which is not a problem since it's a small table.

True, there's msgs_index1 ON msgs (rfc724_mid).

But the question about DeleteDeviceAfter remains, it's not even an advanced setting, many users may have it enabled. How can we reason about messages received within 90 days if they are deleted from the device e.g. in 1 day?

Ah, yes, how this interacts with DeleteDeviceAfter needs to be investigated. Maybe it's even possible to write a test for it.

The interaction is complex: delete_expired_messages() sets the sent_timestamp to 0, but the timestamp (which is called sort_timestamp in the Message struct) is kept, so, maybe it would be enough to look at timestamp instead of sent_timestamp in the SQL query. BUT prune_tombstones() removes rows from the msgs table after 2 days. They are not removed if target!='', but in single-device mode we set target to '' (i.e. mark the message for deletion) right after receiving it (see delete_expired_imap_messages()).

So, the conclusion is that probably this interacts with DeleteDeviceAfter in a bad way. I'm not entirely sure whether we can just ignore this problem, but on first sight we probably do need to explicitly remember when the last message was received on each transport.

After talking about it with @link2xt, yes we do need to explicitly remember when the last message was received on each transport, e.g. via a column on the transports table.

Hocuri

Looks good, only some minor things.

Hocuri · 2026-07-08T14:00:19Z

+        })
+        .await?;
+
+    macro_rules! assert_transport_removed {


Can this just be a regular function rather than a macro? Seems easier to read, and only one additional parameter (&TestContext) is required.

Two additional parameters, as this also needs to take transport_id. Since the macro is pretty simple one, I think the convenience justifies it.

Generally, in chatmail/core, for readability reasons and for avoiding surprising effects (like that $expr is evaluated in all the places it's copy-pasted to), we only use macros when really needed. I made a PR to explicitly state this in STYLE.md: #8410

As a general rule #8410 makes sense, but overall i think it's not that important, especially in the test.

Automatically remove relays that are hidden (`is_published=0`) and haven't been used to receive new messages for over 90 days. Closes: #8384 Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>

Hocuri · 2026-07-10T20:35:35Z

+    sql::update_transport_last_rcvd_timestamp(context, rfc724_mid)
+        .await
+        .context("Failed to update transport's last_rcvd_timestamp value.")?;
+


receive_imf is only called after downloading a message from the relay where it arrived first. When the message arrives again on another relay, it's not called again. (although we had multiple bugs in this area, so that it's worth testing again whether this actually works when basing more logic on it).

This means that with the PR as-is, transport_last_rcvd_timestamp will only be updated for the fastest transport. This is not exactly what was specified in #8384, but actually it is fine: If an unpublished relay is slower than the others AND all the messages are also received on the other, faster relays, then it's safe to delete the unpublished relay.

It does mean that we can't use the same timestamp to automatically unpublish a relay that seems not to be working. Then again, it's not entirely clear how we'll implement this logic, anyways.

So, the two options now are as follows:

Document that last_rcvd_timestamp is only updated on the relay via which the message was received first

Move this function call to fetch_new_msg_batch()

Either of them seems equally fine. @link2xt do you have an opinion?

I'd leave this logic in receive_imf and also check if the contact is blocked or a contact request. I don't think it's a good idea to allow everyone knowing our public key to control which transports we can remove. We consider the public key as confidential data, but it's still not secret, so we can't absolutely protect from spammers getting know it, particularly if the user is a big channel operator.

Hocuri · 2026-07-10T20:39:07Z

+            "UPDATE transports
+            SET last_rcvd_timestamp=MAX(msgs.timestamp_rcvd, transports.last_rcvd_timestamp)
+            FROM transports t1
+            INNER JOIN imap ON t1.id=imap.transport_id
+            INNER JOIN msgs ON msgs.rfc724_mid=imap.rfc724_mid
+            WHERE msgs.rfc724_mid=?",


If I'm not mistaken, this function is called right after receiving the message, which means that msgs.timestamp_rcvd is simply set to the current time? I.e. it would be enough to simply use the current time rather than querying the message's received timestamp?

that is true, I mainly did that for consistency, so we won't have some weird shift-by-few-seconds in the db, I can change it to just time() call, but this query is fast, it only runs index searches.

I think that with just time() it will be easier to understand (e.g. it's clear that the logic works even if the message wasn't added to msgs for some reason, e.g. if it's an MDN or another kind of special message) and also we should correct transports.last_rcvd_timestamp in case if the clock were set back

j-g00da requested review from Hocuri and link2xt and removed request for link2xt July 7, 2026 06:43

j-g00da force-pushed the j-g00da/remove-hidden-relays-automatically branch from 921c5df to 1b3f434 Compare July 7, 2026 15:15

link2xt reviewed Jul 7, 2026

View reviewed changes

Hocuri mentioned this pull request Jul 8, 2026

api!: delete_transport() must not be used by UIs anymore. Instead, set_transport_unpublished() must be called when a user clicks on "Remove" #8408

Merged

j-g00da force-pushed the j-g00da/remove-hidden-relays-automatically branch from 1b3f434 to a0c7c59 Compare July 8, 2026 13:46

Hocuri reviewed Jul 8, 2026

View reviewed changes

j-g00da force-pushed the j-g00da/remove-hidden-relays-automatically branch 3 times, most recently from 495c8ef to 3607f29 Compare July 10, 2026 11:58

feat: Remove hidden relays automatically

967d87f

Automatically remove relays that are hidden (`is_published=0`) and haven't been used to receive new messages for over 90 days. Closes: #8384 Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>

j-g00da force-pushed the j-g00da/remove-hidden-relays-automatically branch from 3607f29 to 967d87f Compare July 10, 2026 12:02

j-g00da requested review from Hocuri and link2xt July 10, 2026 12:10

Hocuri reviewed Jul 10, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

j-g00da commented Jul 7, 2026

Uh oh!

j-g00da commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hocuri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants