diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index e867a571c..52fcb355d 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -6474,9 +6474,10 @@ void dc_event_unref(dc_event_t* event); * Transport relay added/deleted or default has changed. * UI should update the list. * - * The event is emitted when the transports are modified on another device - * using the JSON-RPC calls `add_or_update_transport`, `add_transport_from_qr`, `delete_transport`, - * `set_transport_unpublished` or `set_config(configured_addr)`. + * The event is emitted on the device modifying the transports + * as well as on other devices applying the synced change, + * for the JSON-RPC calls `add_or_update_transport`, `add_transport_from_qr`, + * `delete_transport` or `set_config(configured_addr)`. */ #define DC_EVENT_TRANSPORTS_MODIFIED 2600 @@ -7282,11 +7283,7 @@ void dc_event_unref(dc_event_t* event); /// "Message pinned by %1$s." #define DC_STR_MESSAGE_PINNED_BY_OTHER 244 -/// "Phasing out" -/// -/// Used in connectivity view to flag unpublished relays. -/// This should match the wording used for relay deletion confirmation, -/// saying "Before deletion, it will be gradually phased out so your contacts can switch over smoothly" +/// @deprecated 2026-08-31 #define DC_STR_PHASING_OUT 245 /** diff --git a/deltachat-jsonrpc/src/api.rs b/deltachat-jsonrpc/src/api.rs index a82f15d3d..f2432c1be 100644 --- a/deltachat-jsonrpc/src/api.rs +++ b/deltachat-jsonrpc/src/api.rs @@ -65,7 +65,6 @@ use self::types::{ }; use crate::api::types::appversions::JsonrpcAppSource; use crate::api::types::chat_list::{ChatListItemFetchResult, get_chat_list_item_by_id}; -use crate::api::types::login_param::TransportListEntry; use crate::api::types::qr::{QrObject, SecurejoinSource, SecurejoinUiPath}; #[derive(Debug)] @@ -502,8 +501,7 @@ impl CommandApi { /// - [Self::add_transport_from_qr()] to add a transport /// from a server encoded in a QR code. /// - [Self::list_transports()] to get a list of all configured transports. - /// - [Self::set_transport_unpublished()] to remove a transport. - /// - [Self::set_transport_unpublished()] to set whether contacts see this transport. + /// - [Self::delete_transport()] to remove a transport. async fn add_or_update_transport( &self, account_id: u32, @@ -528,32 +526,8 @@ impl CommandApi { /// Returns the list of all email accounts that are used as a transport in the current profile. /// Use [Self::add_or_update_transport()] to add or change a transport - /// and [Self::set_transport_unpublished()] to remove a transport. + /// and [Self::delete_transport()] to remove a transport. async fn list_transports(&self, account_id: u32) -> Result> { - let ctx = self.get_context(account_id).await?; - let res = ctx - .list_transports() - .await? - .into_iter() - .filter(|t| !t.is_unpublished) - .map(|t| t.param.into()) - .collect(); - Ok(res) - } - - /// Deprecated 2026-06: This is not needed by UI implementations anymore, - /// because unpublished relays now count as removed from the user point of view, - /// and must not be shown in the list of relays. - /// This means that UIs should use `list_transports()` instead of this function. - /// - /// Returns the list of all email accounts that are used as a transport in the current profile. - /// - /// As opposed to `list_transports()`, this function also returns unpublished transports, - /// and for each returned transport it returns the information whether or not is `unpublished`. - /// - /// Use [Self::add_or_update_transport()] to add or change a transport - /// and [Self::set_transport_unpublished()] to change whether a transport is 'published'. - async fn list_transports_ex(&self, account_id: u32) -> Result> { let ctx = self.get_context(account_id).await?; let res = ctx .list_transports() @@ -564,41 +538,17 @@ impl CommandApi { Ok(res) } - /// Immediately deletes a transport, potentially causing messages not to arrive. - /// This must ONLY be used by the automated tests. - /// UI implementations must use [`Self::set_transport_unpublished`] instead. + /// Removes a transport. + /// UIs should call this function when the user removes a relay. + /// + /// The last transport cannot be removed. + /// If the removed transport was the one used for sending, + /// another one is chosen automatically. async fn delete_transport(&self, account_id: u32, addr: String) -> Result<()> { let ctx = self.get_context(account_id).await?; ctx.delete_transport(&addr).await } - /// Change whether the transport is unpublished. - /// UIs should call this function when the user clicks on "Remove". - /// Core will keep listening on this transport for some time, - /// and automatically remove it once it is no longer needed. - /// - /// Unpublished transports are not advertised to contacts, - /// and self-sent messages are not sent there, - /// 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. - /// - /// 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 - async fn set_transport_unpublished( - &self, - account_id: u32, - addr: String, - unpublished: bool, - ) -> Result<()> { - let ctx = self.get_context(account_id).await?; - ctx.set_transport_unpublished(&addr, unpublished).await - } - /// Signal an ongoing process to stop. async fn stop_ongoing_process(&self, account_id: u32) -> Result<()> { let ctx = self.get_context(account_id).await?; diff --git a/deltachat-jsonrpc/src/api/types/events.rs b/deltachat-jsonrpc/src/api/types/events.rs index 64ab1a3a4..de193db2b 100644 --- a/deltachat-jsonrpc/src/api/types/events.rs +++ b/deltachat-jsonrpc/src/api/types/events.rs @@ -478,9 +478,9 @@ pub enum EventType { /// /// UI should update the list. /// - /// This event is emitted when transport - /// synchronization messages arrives, - /// but not when the UI modifies the transport list by itself. + /// The event is emitted on the device modifying + /// the transports as well as on other devices + /// applying the synced change. TransportsModified, } diff --git a/deltachat-jsonrpc/src/api/types/login_param.rs b/deltachat-jsonrpc/src/api/types/login_param.rs index 36d6ed9ff..e9832dc6e 100644 --- a/deltachat-jsonrpc/src/api/types/login_param.rs +++ b/deltachat-jsonrpc/src/api/types/login_param.rs @@ -4,16 +4,6 @@ use serde::Deserialize; use serde::Serialize; use yerpc::TypeDef; -#[derive(Serialize, TypeDef, schemars::JsonSchema)] -#[serde(rename_all = "camelCase")] -pub struct TransportListEntry { - /// The login data entered by the user. - pub param: EnteredLoginParam, - /// Whether this transport is set to 'unpublished'. - /// See `set_transport_unpublished` / `setTransportUnpublished` for details. - pub is_unpublished: bool, -} - /// Login parameters entered by the user. /// /// Usually it will be enough to only set `addr` and `password`, @@ -68,15 +58,6 @@ pub struct EnteredLoginParam { pub certificate_checks: Option, } -impl From for TransportListEntry { - fn from(transport: dc::TransportListEntry) -> Self { - TransportListEntry { - param: transport.param.into(), - is_unpublished: transport.is_unpublished, - } - } -} - impl From for EnteredLoginParam { fn from(param: dc::EnteredLoginParam) -> Self { let imap_security: Socket = param.imap.security.into(); diff --git a/deltachat-rpc-client/src/deltachat_rpc_client/account.py b/deltachat-rpc-client/src/deltachat_rpc_client/account.py index 9e2a7b06c..9a91bafe2 100644 --- a/deltachat-rpc-client/src/deltachat_rpc_client/account.py +++ b/deltachat-rpc-client/src/deltachat_rpc_client/account.py @@ -143,10 +143,6 @@ class Account: """Delete a transport.""" self._rpc.delete_transport(self.id, addr) - def set_transport_unpublished(self, addr: str, unpublished: bool = True): - """Unpublish the transport.""" - self._rpc.set_transport_unpublished(self.id, addr, unpublished) - @futuremethod def list_transports(self): """Return the list of all email accounts that are used as a transport in the current profile.""" diff --git a/deltachat-rpc-client/tests/test_cross_core.py b/deltachat-rpc-client/tests/test_cross_core.py index e0ae35668..367119318 100644 --- a/deltachat-rpc-client/tests/test_cross_core.py +++ b/deltachat-rpc-client/tests/test_cross_core.py @@ -89,8 +89,9 @@ def test_second_device(acf, alice_and_remote_bob) -> None: assert new_account.get_config("addr") == remote_eval("bob.get_config('addr')") -def test_keyupdate_against_core_2_48_march_2026(acf, alice_and_remote_bob): - """Test 2.48 Bob learns a new relay of Alice from a keyupdate, and is shown nothing.""" +@pytest.mark.parametrize("replace_relay", [False, True], ids=["add", "replace"]) +def test_keyupdate_against_core_2_48_march_2026(acf, alice_and_remote_bob, replace_relay): + """Test 2.48 Bob learns a relay change of Alice from a keyupdate, and is shown nothing.""" alice, alice_contact_bob, remote_eval = alice_and_remote_bob("2.48.0") def bob_sees(): @@ -115,8 +116,10 @@ def test_keyupdate_against_core_2_48_march_2026(acf, alice_and_remote_bob): # without waiting, the re-signed key can tie with the copy Bob holds, keeping his. time.sleep(2) alice.add_transport_from_qr(acf.get_account_qr()) - alice.bring_online() (new_addr,) = [t["addr"] for t in alice.list_transports() if t["addr"] != old_addr] + if replace_relay: + alice.delete_transport(old_addr) + alice.bring_online() # The 2.48 core has no encryption enforcement, but the keyupdate MDN without # referenced message keeps it invisible; merging happens before the trashing. @@ -130,3 +133,7 @@ def test_keyupdate_against_core_2_48_march_2026(acf, alice_and_remote_bob): # It also leaves no trace: no chat with Alice, no message anywhere, # and no address-contact for the address it was sent from. assert bob_sees() == before + + if replace_relay: + remote_eval("bob_contact_alice.create_chat().send_text('hello after replacement')") + assert alice.wait_for_incoming_msg().get_snapshot().text == "hello after replacement" diff --git a/deltachat-rpc-client/tests/test_multitransport.py b/deltachat-rpc-client/tests/test_multitransport.py index 975608690..2f6fd35c2 100644 --- a/deltachat-rpc-client/tests/test_multitransport.py +++ b/deltachat-rpc-client/tests/test_multitransport.py @@ -20,13 +20,18 @@ def test_add_second_address(acf) -> None: first_addr = account.list_transports()[0]["addr"] second_addr = account.list_transports()[1]["addr"] + third_addr = account.list_transports()[2]["addr"] - # Cannot delete the first address. - with pytest.raises(JsonRpcError): - account.delete_transport(first_addr) + assert account.get_config("configured_addr") == first_addr + account.delete_transport(first_addr) + assert len(account.list_transports()) == 2 + assert account.get_config("configured_addr") != first_addr account.delete_transport(second_addr) - assert len(account.list_transports()) == 2 + assert len(account.list_transports()) == 1 + + with pytest.raises(JsonRpcError): + account.delete_transport(third_addr) def test_change_address(acf) -> None: @@ -63,8 +68,6 @@ def test_change_address(acf) -> None: alice_vcard = alice.self_contact.make_vcard() assert old_alice_addr not in alice_vcard assert new_alice_addr in alice_vcard - with pytest.raises(JsonRpcError): - alice.delete_transport(new_alice_addr) alice.start_io() alice_chat_bob.send_text("Hello again!") @@ -122,6 +125,10 @@ def test_transport_synchronization(acf, log) -> None: if "scheduler is running" in ev.msg: return + def wait_transports(ac, n): + while len(ac.list_transports()) != n: + ac.wait_for_event(EventType.TRANSPORTS_MODIFIED) + ac1, ac2 = acf.get_online_accounts(2) ac1_clone = ac1.clone() ac1_clone.bring_online() @@ -129,15 +136,13 @@ def test_transport_synchronization(acf, log) -> None: qr = acf.get_account_qr() ac1.add_transport_from_qr(qr) - ac1_clone.wait_for_event(EventType.TRANSPORTS_MODIFIED) + wait_transports(ac1_clone, 2) wait_for_io_started(ac1_clone) assert len(ac1.list_transports()) == 2 - assert len(ac1_clone.list_transports()) == 2 ac1_clone.add_transport_from_qr(qr) - ac1.wait_for_event(EventType.TRANSPORTS_MODIFIED) + wait_transports(ac1, 3) wait_for_io_started(ac1) - assert len(ac1.list_transports()) == 3 assert len(ac1_clone.list_transports()) == 3 log.section("ac1 clone removes second transport") @@ -145,21 +150,17 @@ def test_transport_synchronization(acf, log) -> None: addr3 = transport3["addr"] ac1_clone.delete_transport(transport2["addr"]) - ac1.wait_for_event(EventType.TRANSPORTS_MODIFIED) + wait_transports(ac1, 2) wait_for_io_started(ac1) [transport1, transport3] = ac1.list_transports() - log.section("ac1 changes the primary transport") + log.section("ac1 changes the sending transport") ac1.set_config("configured_addr", transport3["addr"]) - ac1_clone.wait_for_event(EventType.TRANSPORTS_MODIFIED) - [transport1, transport3] = ac1_clone.list_transports() - assert ac1_clone.get_config("configured_addr") == transport1["addr"] - log.section("ac1 removes the first transport") ac1.delete_transport(transport1["addr"]) - ac1_clone.wait_for_event(EventType.TRANSPORTS_MODIFIED) + wait_transports(ac1_clone, 1) wait_for_io_started(ac1_clone) [transport3] = ac1_clone.list_transports() assert transport3["addr"] == addr3 @@ -181,6 +182,7 @@ def test_transport_sync_new_as_primary(acf, log) -> None: qr = acf.get_account_qr() ac1.add_transport_from_qr(qr) + ac1.wait_for_event(EventType.TRANSPORTS_MODIFIED) ac1_transports = ac1.list_transports() assert len(ac1_transports) == 2 [transport1, transport2] = ac1_transports @@ -190,6 +192,7 @@ def test_transport_sync_new_as_primary(acf, log) -> None: log.section("ac1 changes the primary transport") ac1.set_config("configured_addr", transport2["addr"]) + ac1.wait_for_event(EventType.TRANSPORTS_MODIFIED) ac1_clone.wait_for_event(EventType.TRANSPORTS_MODIFIED) assert ac1_clone.get_config("configured_addr") == transport1["addr"] @@ -236,18 +239,8 @@ def test_transport_limit(acf) -> None: account.add_transport_from_qr(qr) second_addr = account.list_transports()[1]["addr"] - third_addr = account.list_transports()[2]["addr"] - # test that adding a transport after unpublishing one works again - account.set_transport_unpublished(second_addr) - account.add_transport_from_qr(qr) - with pytest.raises(JsonRpcError): - account.add_transport_from_qr(qr) - - # UIs are not expected to delete transports directly, - # but we still test that adding a transport - # after deleting one instead of unpublishing works. - account.delete_transport(third_addr) + account.delete_transport(second_addr) account.add_transport_from_qr(qr) with pytest.raises(JsonRpcError): account.add_transport_from_qr(qr) @@ -305,7 +298,6 @@ def test_remove_primary_transport(acf, log) -> None: log.section("Alice sets up second transport") [transport1, transport2] = alice.list_transports() - alice.set_config("configured_addr", transport2["addr"]) bob_chat.send_text("Hello!") msg1 = alice.wait_for_incoming_msg().get_snapshot() @@ -313,6 +305,7 @@ def test_remove_primary_transport(acf, log) -> None: log.section("Alice removes the primary relay") alice.delete_transport(transport1["addr"]) + assert alice.get_config("configured_addr") == transport2["addr"] alice.stop_io() alice.start_io() diff --git a/docs/schema.sql b/docs/schema.sql index dbe487ade..a9a743d1f 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -626,13 +626,10 @@ CREATE TABLE transports ( -- over this table and `removed_transports`, and contacts keep the newest one. add_timestamp INTEGER NOT NULL DEFAULT 0, - -- True if the transport address is published - -- by sending it in the public key signature. + -- Unused since migration 165, which removed unpublished transports. is_published INTEGER DEFAULT 1 NOT NULL, -- Time when the transport was last used to receive a message. - -- Used to remove the least recently used transport - -- when a new transport is added and there are too many relays already. last_rcvd_timestamp INTEGER NOT NULL DEFAULT 0, UNIQUE(addr) ); diff --git a/src/autorelay/autorelay_tests.rs b/src/autorelay/autorelay_tests.rs index a74bd3f6f..9f249f7ad 100644 --- a/src/autorelay/autorelay_tests.rs +++ b/src/autorelay/autorelay_tests.rs @@ -139,8 +139,7 @@ async fn test_maybe_add_additional_relays_does_nothing_after_finishing_once() -> assert!(relay_added); let transports = t.list_transports().await?; - t.delete_transport(&transports.last().unwrap().param.addr) - .await?; + t.delete_transport(&transports.last().unwrap().addr).await?; SystemTime::shift(Duration::from_secs( AUTOMATIC_ADDITION_DEBOUNCE_SECONDS as u64 + 1, diff --git a/src/config.rs b/src/config.rs index 3dce2bfdd..dc0b99fe4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -19,7 +19,7 @@ use crate::log::LogExt; use crate::mimefactory::RECOMMENDED_FILE_SIZE; use crate::sync::{self, Sync::*, SyncData}; use crate::tools::{get_abs_path, time}; -use crate::transport::{add_pseudo_transport, send_sync_transports}; +use crate::transport::{add_pseudo_transport, send_sync_transports, transport_addrs}; use crate::{constants, stats}; /// The available configuration keys. @@ -794,10 +794,6 @@ impl Context { (addr,), )?; - // `is_published=1`: an unpublished primary would be missing - // from the relay list in the public key, so contacts would - // never send to it. - // // The timestamp must strictly increase because // other devices ignore the row update otherwise, // and contacts only adopt the re-signed key @@ -805,7 +801,7 @@ impl Context { transaction .execute( "UPDATE transports - SET add_timestamp=MAX(?, add_timestamp+1), is_published=1 + SET add_timestamp=MAX(?, add_timestamp+1) WHERE addr=?", (time(), addr), ) @@ -915,7 +911,7 @@ impl Context { return Ok(true); } Ok(self - .get_all_self_addrs() + .get_self_addrs() .await? .iter() .any(|a| addr_cmp(addr, a))) @@ -937,49 +933,10 @@ impl Context { } /// Returns all self addresses, newest first. - pub(crate) async fn get_all_self_addrs(&self) -> Result> { + pub(crate) async fn get_self_addrs(&self) -> Result> { + let query_only = true; self.sql - .query_map_vec( - "SELECT addr FROM transports ORDER BY add_timestamp DESC, id DESC", - (), - |row| { - let addr: String = row.get(0)?; - Ok(addr) - }, - ) - .await - } - - /// Returns all published self addresses, newest first. - /// See `[Context::set_transport_unpublished]` - pub(crate) async fn get_published_self_addrs(&self) -> Result> { - self.sql - .query_map_vec( - "SELECT addr FROM transports WHERE is_published=1 ORDER BY add_timestamp DESC, id DESC", - (), - |row| { - let addr: String = row.get(0)?; - Ok(addr) - }, - ) - .await - } - - /// Returns all published secondary self addresses. - /// See `[Context::set_transport_unpublished]` - pub(crate) async fn get_published_secondary_self_addrs(&self) -> Result> { - self.sql - .query_map_vec( - "SELECT addr FROM transports - WHERE is_published - AND addr NOT IN (SELECT value FROM config WHERE keyname='configured_addr') - ORDER BY add_timestamp DESC, id DESC", - (), - |row| { - let addr: String = row.get(0)?; - Ok(addr) - }, - ) + .transaction_ext(query_only, |transaction| transport_addrs(transaction)) .await } diff --git a/src/configure.rs b/src/configure.rs index adb8adcb1..57670059b 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -18,6 +18,7 @@ use deltachat_contact_tools::{EmailAddress, addr_normalize}; use futures::FutureExt; use futures_lite::FutureExt as _; use percent_encoding::utf8_percent_encode; +use rusqlite::OptionalExtension; use server_params::{ServerParams, expand_param_vector}; use tokio::task; @@ -26,8 +27,8 @@ use crate::constants::NON_ALPHANUMERIC_WITHOUT_DOT; use crate::context::Context; use crate::imap::Imap; use crate::log::warn; +use crate::login_param::EnteredCertificateChecks; pub use crate::login_param::EnteredLoginParam; -use crate::login_param::{EnteredCertificateChecks, TransportListEntry}; use crate::net::proxy::ProxyConfig; use crate::provider::{self, Protocol, Socket}; use crate::qr::{login_param_from_account_qr, login_param_from_login_qr}; @@ -36,7 +37,8 @@ use crate::sync::Sync::Nosync; use crate::tools::time; use crate::transport::{ ConfiguredCertificateChecks, ConfiguredLoginParam, ConfiguredServerLoginParam, - ConnectionCandidate, send_sync_transports, + ConnectionCandidate, delete_transport_row, maybe_update_sending_transport, + purge_transport_caches, send_sync_transports, transport_addrs, }; use crate::{EventType, stock_str}; @@ -106,7 +108,6 @@ impl Context { /// from a server encoded in a QR code. /// - [Self::list_transports()] to get a list of all configured transports. /// - [Self::delete_transport()] to remove a transport. - /// - [Self::set_transport_unpublished()] to set whether contacts see this transport. pub async fn add_or_update_transport(&self, param: &mut EnteredLoginParam) -> Result<()> { self.stop_io().await; let result = self.add_transport_inner(param).await; @@ -192,25 +193,13 @@ impl Context { /// Returns the list of all email accounts that are used as a transport in the current profile. /// Use [Self::add_or_update_transport()] to add or change a transport /// and [Self::delete_transport()] to delete a transport. - pub async fn list_transports(&self) -> Result> { - let transports = self - .sql - .query_map_vec( - "SELECT entered_param, is_published FROM transports", - (), - |row| { - let param: String = row.get(0)?; - let param: EnteredLoginParam = serde_json::from_str(¶m)?; - let is_published: bool = row.get(1)?; - Ok(TransportListEntry { - param, - is_unpublished: !is_published, - }) - }, - ) - .await?; - - Ok(transports) + pub async fn list_transports(&self) -> Result> { + self.sql + .query_map_vec("SELECT entered_param FROM transports", (), |row| { + let param: String = row.get(0)?; + Ok(serde_json::from_str(¶m)?) + }) + .await } /// Returns the number of configured transports. @@ -218,101 +207,51 @@ impl Context { self.sql.count("SELECT COUNT(*) FROM transports", ()).await } - /// Immediately deletes a transport, potentially causing messages not to arrive. - /// This must ONLY be used internally and by the automated tests. - /// UI implementations must use [`Self::set_transport_unpublished`] instead. + /// Removes a transport. + /// UIs should call this function when the user removes a relay. + /// + /// The last transport cannot be removed. + /// If the removed transport was the one used for sending, + /// another one is chosen automatically. pub async fn delete_transport(&self, addr: &str) -> Result<()> { let now = time(); - let removed_transport_id = self + let (removed_transport_id, reelected) = self .sql .transaction(|transaction| { - let primary_addr = transaction.query_row( - "SELECT value FROM config WHERE keyname='configured_addr'", - (), - |row| { - let addr: String = row.get(0)?; - Ok(addr) - }, - )?; - - if primary_addr == addr { - bail!("Cannot delete primary transport"); + if transport_addrs(transaction)?.len() <= 1 { + bail!("Cannot remove the last transport"); } - let (transport_id, add_timestamp) = transaction.query_row( - "DELETE FROM transports WHERE addr=? RETURNING id, add_timestamp", - (addr,), - |row| { - let id: u32 = row.get(0)?; - let add_timestamp: i64 = row.get(1)?; - Ok((id, add_timestamp)) - }, - )?; - + let add_timestamp: i64 = transaction + .query_row( + "SELECT add_timestamp FROM transports WHERE addr=?", + (addr,), + |row| row.get(0), + ) + .optional()? + .context("Transport does not exist")?; // Removal timestamp should not be lower than addition timestamp // to be accepted by other devices when synced. let remove_timestamp = std::cmp::max(now, add_timestamp); - - transaction.execute( - "INSERT INTO removed_transports (addr, remove_timestamp) - VALUES (?, ?) - ON CONFLICT (addr) - DO UPDATE SET remove_timestamp = excluded.remove_timestamp", - (addr, remove_timestamp), - )?; - - Ok(transport_id) + let transport_id = delete_transport_row(transaction, addr, remove_timestamp)? + .context("Transport disappeared")?; + let reelected = maybe_update_sending_transport(transaction)?; + Ok((transport_id, reelected)) }) .await?; + if let Some(new_addr) = reelected { + info!(self, "Using transport {new_addr:?} for sending now."); + self.sql.uncache_raw_config("configured_addr").await; + } send_sync_transports(self).await?; - self.quota.write().await.remove(&removed_transport_id); + purge_transport_caches(self, removed_transport_id).await; + // Restarting all IO also stops the removed transport's IMAP loop. + // Scheduler reconciliation would stop only that one loop, + // see https://github.com/chatmail/core/issues/8513 self.restart_io_if_running().await; Ok(()) } - /// Change whether the transport is unpublished. - /// UIs should call this function when the user clicks on "Remove". - /// Core will keep listening on this transport for some time, - /// and automatically remove it once it is no longer needed. - /// - /// Unpublished transports are not advertised to contacts, - /// and self-sent messages are not sent there, - /// 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. - /// - /// 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 - .transaction(|trans| { - let primary_addr: String = trans - .query_row( - "SELECT value FROM config WHERE keyname='configured_addr'", - (), - |row| row.get(0), - ) - .context("Select primary address")?; - if primary_addr == addr && unpublished { - bail!("Can't set primary relay as unpublished"); - } - // We need to update the timestamp so that the key's timestamp changes - // and is recognized as newer by our peers - trans - .execute( - "UPDATE transports SET is_published=?, add_timestamp=? WHERE addr=? AND is_published!=?1", - (!unpublished, time(), addr), - ) - .context("Update transports")?; - Ok(()) - }) - .await?; - send_sync_transports(self).await?; - Ok(()) - } - async fn inner_configure(&self, param: &EnteredLoginParam) -> Result<()> { info!(self, "Configure ..."); @@ -324,7 +263,7 @@ impl Context { ) .await? { - self.try_make_space_for_new_relay().await?; + self.check_relay_limit().await?; } let skip_network = false; @@ -350,39 +289,11 @@ impl Context { 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})"); - } - }; + async fn check_relay_limit(&self) -> Result<()> { + ensure!( + self.count_transports().await? < MAX_RELAYS, + "You have reached the maximum number of relays ({MAX_RELAYS})" + ); Ok(()) } } @@ -746,13 +657,10 @@ pub enum Error { #[cfg(test)] mod tests { - use crate::tools::SystemTime; - use super::*; use crate::autorelay::login_param_from_host; use crate::config::Config; use crate::login_param::EnteredImapLoginParam; - use crate::sql::update_transport_last_rcvd_timestamp; use crate::test_utils::{TestContext, TestContextManager}; use crate::transport::add_pseudo_transport; @@ -814,104 +722,27 @@ mod tests { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_try_make_place_for_new_relay() -> Result<()> { - let t = TestContext::new().await; + async fn test_relay_limit() -> Result<()> { + let mut tcm = TestContextManager::new(); + let t = &tcm.unconfigured().await; - // Setting ConfiguredAddr on an unconfigured account creates a pseudo primary transport + // Setting ConfiguredAddr on an unconfigured account creates a pseudo 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); + t.check_relay_limit().await?; - for i in 0..(MAX_RELAYS - 2) { - add_pseudo_transport(&t, &format!("transport{i}@example.org")).await?; + for i in 0..(MAX_RELAYS - 1) { + 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 + t.check_relay_limit().await.unwrap_err().to_string(), + format!("You have reached the maximum number of relays ({MAX_RELAYS})") ); - // 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); + t.delete_transport("transport0@example.org").await?; + t.check_relay_limit().await?; Ok(()) } diff --git a/src/contact.rs b/src/contact.rs index 0f5d5f343..e96470376 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1174,7 +1174,7 @@ VALUES (?, ?, ?, ?, ?, ?) query: Option<&str>, ) -> Result> { let self_addrs = context - .get_all_self_addrs() + .get_self_addrs() .await? .into_iter() .collect::>(); diff --git a/src/contact/contact_tests.rs b/src/contact/contact_tests.rs index 63c603ed0..ecf781fdd 100644 --- a/src/contact/contact_tests.rs +++ b/src/contact/contact_tests.rs @@ -153,12 +153,8 @@ async fn test_get_contacts() -> Result<()> { let contacts = Contact::get_all(&context, 0, Some("δ")).await?; assert_eq!(contacts.len(), 1); - // Searching for a secondary self address finds "Me", - // even if the transport is unpublished. + // Searching for another self address finds "Me". crate::transport::add_pseudo_transport(&context, "bob@second.example").await?; - context - .set_transport_unpublished("bob@second.example", true) - .await?; let contacts = Contact::get_all( &context, constants::DC_GCL_ADD_SELF, diff --git a/src/context.rs b/src/context.rs index 04034afa8..418d81f91 100644 --- a/src/context.rs +++ b/src/context.rs @@ -327,9 +327,9 @@ pub struct InnerContext { /// Mutex is also held while generating the key to avoid generating the key twice. pub(crate) self_public_key: Mutex>, - /// `Connectivity` values for published relays, unordered. Used to compute the aggregate connectivity, + /// `Connectivity` values for the relays, unordered. Used to compute the aggregate connectivity, /// see [`Context::get_connectivity()`]. - pub(crate) published_connectivities: parking_lot::Mutex>, + pub(crate) connectivities: parking_lot::Mutex>, /// Timestamp after which the SMTP loop checks for a keyupdate to send, or 0 if none is due. pub(crate) next_keyupdate_check: AtomicI64, @@ -508,7 +508,7 @@ impl Context { iroh: Arc::new(RwLock::new(None)), self_fingerprint: OnceLock::new(), self_public_key: Mutex::new(None), - published_connectivities: parking_lot::Mutex::new(Vec::new()), + connectivities: parking_lot::Mutex::new(Vec::new()), next_keyupdate_check: AtomicI64::new(0), }; @@ -841,7 +841,7 @@ impl Context { let all_transports: Vec = ConfiguredLoginParam::load_all(self) .await? .into_iter() - .map(|(transport_id, param, _)| format!("{transport_id}: {param}")) + .map(|(transport_id, param)| format!("{transport_id}: {param}")) .collect(); let all_transports = if all_transports.is_empty() { "Not configured".to_string() diff --git a/src/events/payload.rs b/src/events/payload.rs index 0d500236e..4e07d6709 100644 --- a/src/events/payload.rs +++ b/src/events/payload.rs @@ -433,9 +433,9 @@ pub enum EventType { /// /// UI should update the list. /// - /// This event is emitted when a transport - /// synchronization message modifies transports, - /// but not when the UI modifies the transport list by itself. + /// The event is emitted on the device modifying + /// the transports as well as on other devices + /// applying the synced change. TransportsModified, /// Event for using in tests, e.g. as a fence between normally generated events. diff --git a/src/key.rs b/src/key.rs index 1f9febb9e..997fb9e6d 100644 --- a/src/key.rs +++ b/src/key.rs @@ -302,7 +302,7 @@ pub(crate) async fn load_self_public_key_opt(context: &Context) -> Result