diff --git a/docs/schema.sql b/docs/schema.sql index 18c0d58958..af57e8c6e2 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -456,6 +456,21 @@ CREATE TABLE smtp_status_updates ( descr TEXT NOT NULL -- text to send along with the updates ); +-- Table to record the successful usage transports for sending. +-- Sorting the table by rowid in descending order +-- returns most recently successfully used transport first. +CREATE TABLE smtp_success ( + -- Sequentially increasing ID of the success. + -- Transport with the highest ID is to be used first. + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + + -- ID of the transport that was used to send a message. + transport_id INTEGER UNIQUE NOT NULL, + + -- Delete `smtp_success` rows when the transport is deleted. + FOREIGN KEY(transport_id) REFERENCES transports(id) ON DELETE CASCADE +) STRICT; + -- Table of "sync items" to be grouped into sync messages -- and sent to own devices. CREATE TABLE multi_device_sync ( diff --git a/src/smtp.rs b/src/smtp.rs index 6707dca59d..7b166231cf 100644 --- a/src/smtp.rs +++ b/src/smtp.rs @@ -54,6 +54,39 @@ pub(crate) struct Smtp { pub(crate) last_send_error: Option, } +/// Returns transports with their IDs in the order in which they should be tried. +async fn sorted_transports(context: &Context) -> Result> { + context + .sql + .query_map_vec( + "SELECT transports.id, configured_param FROM transports + LEFT JOIN smtp_success ON smtp_success.transport_id=transports.id + ORDER BY IFNULL(smtp_success.id, 0) DESC, transports.id ASC", + (), + |row| { + let id: u32 = row.get(0)?; + let json: String = row.get(1)?; + let param = ConfiguredLoginParam::from_json(&json)?; + Ok((id, param)) + }, + ) + .await +} + +/// Records successful use of SMTP transport so it is tried first next time we connect to SMTP. +async fn record_success(context: &Context, transport_id: u32) -> Result<()> { + // INSERT OR REPLACE essentially replaces rowid of the row + // if the row exists already, so it becomes the highest rowid in the table. + context + .sql + .execute( + "INSERT OR REPLACE INTO smtp_success (transport_id) VALUES (?)", + (transport_id,), + ) + .await?; + Ok(()) +} + impl Smtp { /// Create a new Smtp instances. pub fn new() -> Self { @@ -101,13 +134,7 @@ impl Smtp { self.connectivity.set_connecting(context); let proxy_config = ProxyConfig::load(context).await?; - let transports = ConfiguredLoginParam::load_all(context).await?; - - // Try to connect to the newest transport first. If sending is unreliable, - // user can configure a new transport and it will be the one used. - // Conversely, if user just added a new transport and sending got less reliable, - // user can restore old state by removing the just added transport. - for (transport_id, lp) in transports.into_iter().rev() { + for (transport_id, lp) in sorted_transports(context).await? { info!(context, "Trying to connect to transport {transport_id}."); match self .connect( @@ -327,6 +354,18 @@ pub(crate) async fn smtp_send( Ok(()) => SendResult::Success, }; + if matches!(status, SendResult::Success) { + debug_assert!(smtp.transport_id.is_some()); + if let Some(transport_id) = smtp.transport_id + && let Err(err) = record_success(context, transport_id).await + { + warn!( + context, + "Failed to record successful use of transport {transport_id} in smtp_success table: {err:#}." + ); + } + } + if let SendResult::Failure(err) = &status && let Some(msg_id) = msg_id { @@ -858,3 +897,6 @@ pub(crate) async fn add_self_recipients( Ok(()) } + +#[cfg(test)] +mod smtp_tests; diff --git a/src/smtp/smtp_tests.rs b/src/smtp/smtp_tests.rs new file mode 100644 index 0000000000..81f3a86eb8 --- /dev/null +++ b/src/smtp/smtp_tests.rs @@ -0,0 +1,49 @@ +use anyhow::Result; + +use crate::test_utils::TestContextManager; +use crate::transport; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_smtp_candidates() -> Result<()> { + let mut tcm = TestContextManager::new(); + let t = &tcm.unconfigured().await; + + transport::add_pseudo_transport(t, "foo@example.net").await?; + transport::add_pseudo_transport(t, "bar@example.net").await?; + transport::add_pseudo_transport(t, "baz@example.net").await?; + + let transports = super::sorted_transports(t).await?; + let [ + (transport_id1, ref transport1), + (transport_id2, ref transport2), + (transport_id3, ref transport3), + ] = transports[..] + else { + panic!("Unexpected number of transports"); + }; + + // By default first added transport is used first. + assert_eq!(transport1.addr, "foo@example.net"); + assert_eq!(transport2.addr, "bar@example.net"); + assert_eq!(transport3.addr, "baz@example.net"); + + super::record_success(t, transport_id3).await?; + let transports2 = super::sorted_transports(t).await?; + assert_eq!(transports2[0].0, transport_id3); + assert_eq!(transports2[1].0, transport_id1); + assert_eq!(transports2[2].0, transport_id2); + + super::record_success(t, transport_id2).await?; + let transports3 = super::sorted_transports(t).await?; + assert_eq!(transports3[0].0, transport_id2); + assert_eq!(transports3[1].0, transport_id3); + assert_eq!(transports3[2].0, transport_id1); + + super::record_success(t, transport_id3).await?; + let transports4 = super::sorted_transports(t).await?; + assert_eq!(transports4[0].0, transport_id3); + assert_eq!(transports4[1].0, transport_id2); + assert_eq!(transports4[2].0, transport_id1); + + Ok(()) +} diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 205f474e94..4229b4967b 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -2672,6 +2672,21 @@ CREATE TABLE smtp2 ( .await?; } + inc_and_check(&mut migration_version, 168)?; + if dbversion < migration_version { + sql.execute_migration( + " +CREATE TABLE smtp_success ( + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + transport_id INTEGER UNIQUE NOT NULL, + FOREIGN KEY(transport_id) REFERENCES transports(id) ON DELETE CASCADE +) STRICT; +", + migration_version, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await?