From d34dd40aa0cd18c08f5053f1f0dec1d3f2ae1aa4 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 20 Jul 2026 15:11:13 +0200 Subject: [PATCH] feat: Automatically remove oldest unpublished relay in order to make space when the user wants to add more; don't allow more than 5 relays overall (#8428) Unpublished relays aren't shown in the list of relays anymore, and can't be deleted manually. Therefore, we want to automatically delete them in order to make space for more relays when the limit (5) is reached and the user wants to add another relay. --- deltachat-jsonrpc/src/api.rs | 5 +- src/configure.rs | 180 +++++++++++++++++++++++++++++++---- src/sql/migrations.rs | 4 +- 3 files changed, 165 insertions(+), 24 deletions(-) diff --git a/deltachat-jsonrpc/src/api.rs b/deltachat-jsonrpc/src/api.rs index 11c5e59d8..788a220f7 100644 --- a/deltachat-jsonrpc/src/api.rs +++ b/deltachat-jsonrpc/src/api.rs @@ -606,7 +606,10 @@ impl CommandApi { /// so that we don't cause extra messages to the corresponding inbox, /// but can still receive messages from contacts who don't know our new transport addresses yet. /// - /// Unpublished transports that are not used to receive any new messages for a time defined by + /// When more transports are added by [`Self::add_or_update_transport()`] or [`Self::add_transport_from_qr`], + /// the least recently needed unpublished transport is automatically removed + /// if this is necessary in order to stay below the maximum number of allowed relays. + /// Also, unpublished transports that are not used to receive any new messages for a time defined by /// [`UNPUBLISHED_TRANSPORT_KEEP_TIME`] are automatically removed. /// /// [`UNPUBLISHED_TRANSPORT_KEEP_TIME`]: deltachat::sql::UNPUBLISHED_TRANSPORT_KEEP_TIME diff --git a/src/configure.rs b/src/configure.rs index 084eefa04..df1a83c0f 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -45,11 +45,10 @@ use crate::transport::{ use crate::{EventType, stock_str}; use crate::{chat, provider}; -/// Maximum number of published relays. +/// Maximum number of relays. /// -/// See -/// and . -pub(crate) const MAX_PUBLISHED_RELAYS: usize = 5; +/// See . +pub(crate) const MAX_RELAYS: usize = 5; /// Hard-coded candidates for default relays. /// In the future, we want to use it during onboarding; @@ -233,7 +232,7 @@ impl Context { } /// Immediately deletes a transport, potentially causing messages not to arrive. - /// This must ONLY be used by the automated tests. + /// This must ONLY be used internally and by the automated tests. /// UI implementations must use [`Self::set_transport_unpublished`] instead. pub async fn delete_transport(&self, addr: &str) -> Result<()> { let now = time(); @@ -294,7 +293,10 @@ impl Context { /// so that we don't cause extra messages to the corresponding inbox, /// but can still receive messages from contacts who don't know our new transport addresses yet. /// - /// Unpublished transports that are not used to receive any new messages for a time defined by + /// When more transports are added by [`Self::add_or_update_transport()`] or [`Self::add_transport_from_qr`], + /// the least recently needed unpublished transport is automatically removed + /// if this is necessary in order to stay below the maximum number of allowed relays. + /// Also, unpublished transports that are not used to receive any new messages for a time defined by /// `UNPUBLISHED_TRANSPORT_KEEP_TIME` are automatically removed. pub async fn set_transport_unpublished(&self, addr: &str, unpublished: bool) -> Result<()> { self.sql @@ -327,22 +329,15 @@ impl Context { async fn inner_configure(&self, param: &EnteredLoginParam) -> Result<()> { info!(self, "Configure ..."); - let old_addr = self.get_config(Config::ConfiguredAddr).await?; - if old_addr.is_some() - && !self - .sql - .exists( - "SELECT COUNT(*) FROM transports WHERE addr=?", - (¶m.addr,), - ) - .await? - && self - .sql - .count("SELECT COUNT(*) FROM transports WHERE is_published", ()) - .await? - >= MAX_PUBLISHED_RELAYS + if !self + .sql + .exists( + "SELECT COUNT(*) FROM transports WHERE addr=?", + (¶m.addr,), + ) + .await? { - bail!("You have reached the maximum number of relays ({MAX_PUBLISHED_RELAYS}).") + self.try_make_space_for_new_relay().await?; } let provider = match configure(self, param).await { @@ -366,6 +361,42 @@ impl Context { on_configure_completed(self, provider).await?; Ok(()) } + + /// This function is called before adding a new relay. + /// If the maximum number of relays ([`MAX_RELAYS`]) is already reached, + /// then it tries to make space by removing an unpublished relay. + /// If there are multiple unpublished relays, + /// the one that hasn't received a message for longest is removed. + /// If there are no unpublished relays, an error is returned. + /// + /// Note that eviction happens before we know that a new relay works, + /// which is a trade-off we made in favor of implementation complexity. + async fn try_make_space_for_new_relay(&self) -> Result<()> { + if self.count_transports().await? >= MAX_RELAYS { + // Try to automatically remove the unpublished transport that wasn't used for the longest time: + if let Some(addr) = self + .sql + .query_get_value::( + "SELECT addr FROM transports WHERE is_published=0 + ORDER BY last_rcvd_timestamp, add_timestamp LIMIT 1", + (), + ) + .await? + { + info!( + self, + "Auto-deleting relay {addr} to make space for new relay." + ); + self.delete_transport(&addr).await?; + } + + if self.count_transports().await? >= MAX_RELAYS { + // Apparently, all the transports are published + bail!("You have reached the maximum number of relays ({MAX_RELAYS})"); + } + }; + Ok(()) + } } async fn on_configure_completed( @@ -812,10 +843,14 @@ pub enum Error { #[cfg(test)] mod tests { + use crate::tools::SystemTime; + use super::*; use crate::config::Config; use crate::login_param::EnteredImapLoginParam; + use crate::sql::update_transport_last_rcvd_timestamp; use crate::test_utils::TestContext; + use crate::transport::add_pseudo_transport; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_no_panic_on_bad_credentials() { @@ -846,4 +881,107 @@ mod tests { assert_eq!(configured_param.smtp_user, ""); Ok(()) } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_try_make_place_for_new_relay() -> Result<()> { + let t = TestContext::new().await; + + // Setting ConfiguredAddr on an unconfigured account creates a pseudo primary transport + t.set_config(Config::ConfiguredAddr, Some("primary@example.org")) + .await?; + + // Test that try_make_place_for_new_relay() doesn't do anything when we're below the limit + assert_eq!(t.count_transports().await?, 1); + t.try_make_space_for_new_relay().await?; + assert_eq!(t.count_transports().await?, 1); + + for i in 0..(MAX_RELAYS - 2) { + add_pseudo_transport(&t, &format!("transport{i}@example.org")).await?; + } + assert_eq!(t.count_transports().await?, MAX_RELAYS - 1); + t.try_make_space_for_new_relay().await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS - 1); + + // Test that try_make_place_for_new_relay() removes the unpublished transport + // when we're at the limit + add_pseudo_transport(&t, "unpublished@example.org").await?; + t.set_transport_unpublished("unpublished@example.org", true) + .await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS); + t.try_make_space_for_new_relay().await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS - 1); + assert_eq!( + t.sql + .exists( + "SELECT COUNT(*) FROM transports WHERE addr=?", + ("unpublished@example.org",), + ) + .await?, + false + ); + + // Test that if there are multiple unpublished relays, + // the one that was used least recently is removed + t.set_transport_unpublished("transport0@example.org", true) + .await?; + add_pseudo_transport(&t, "other_unpublished@example.org").await?; + t.set_transport_unpublished("other_unpublished@example.org", true) + .await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS); + + let transport0_id: u32 = t + .sql + .query_get_value( + "SELECT id FROM transports WHERE addr=?", + ("transport0@example.org",), + ) + .await? + .unwrap(); + let other_unpublished_id: u32 = t + .sql + .query_get_value( + "SELECT id FROM transports WHERE addr=?", + ("other_unpublished@example.org",), + ) + .await? + .unwrap(); + + update_transport_last_rcvd_timestamp(&t, transport0_id).await?; + SystemTime::shift(std::time::Duration::from_secs(10)); + update_transport_last_rcvd_timestamp(&t, other_unpublished_id).await?; + + // Test that try_make_place_for_new_relay() + // removes the relay with the oldest last_rcvd_timestamp + t.try_make_space_for_new_relay().await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS - 1); + assert_eq!( + t.sql + .exists( + "SELECT COUNT(*) FROM transports WHERE addr=?", + ("transport0@example.org",), + ) + .await?, + false + ); + assert_eq!( + t.sql + .exists( + "SELECT COUNT(*) FROM transports WHERE addr=?", + ("other_unpublished@example.org",), + ) + .await?, + true + ); + + // Test that try_make_place_for_new_relay() fails + // if there are MAX_RELAYS published transports + add_pseudo_transport(&t, "published_extra@example.org").await?; + t.set_transport_unpublished("other_unpublished@example.org", false) + .await?; + assert_eq!(t.count_transports().await?, MAX_RELAYS); + assert!(t.try_make_space_for_new_relay().await.is_err()); + assert_eq!(t.count_transports().await?, MAX_RELAYS); + + Ok(()) + } } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 1c55a2f59..003b1e824 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -2459,8 +2459,8 @@ UPDATE msgs SET state=24 WHERE state=18; -- Change OutPreparing to OutFailed. inc_and_check(&mut migration_version, 155)?; if dbversion < migration_version { // last_rcvd_timestamp tracks timestamp of the last received message - // on the transport. This is used to remove hidden transports that - // were not used to receive messages for some predefined time. + // on the transport. This is used to automatically remove hidden transports + // that were not used to receive messages for some time. // // NOTE: ideally we would use `DEFAULT (unixepoch())`, // but sqlite forbids non-constant default in ALTER TABLE.