clippy

When connecting to SMTP, transports are now tried
from the most recently successfully used transport to the least recently used.
Transports that were never used for sending
are tried in the order of increasing ID because init_transports()
tries to select the fastest transport for the first one.

This solves the problem of having to wait for timeout each time
if the first tried transport is down.

docs/schema.sql

@@ -456,6 +456,23 @@ CREATE TABLE smtp_status_updates (
     descr TEXT NOT NULL -- text to send along with the updates
 );
 
+-- Table to record the successful usage of a transport for sending.
+-- Sorting the table by rowid in descending order
+-- returns most recently successfully used transport first.
+-- It is used to prefer transports that were recently successfully used
+-- and avoid spending time waiting for not working transports to timeout.
+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 (

src/smtp.rs

@@ -54,6 +54,39 @@ pub(crate) struct Smtp {
     pub(crate) last_send_error: Option<String>,
 }
 
+/// Returns transports with their IDs in the order in which they should be tried.
+async fn sorted_transports(context: &Context) -> Result<Vec<(u32, ConfiguredLoginParam)>> {
+    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 {
@@ -99,13 +132,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(
@@ -325,6 +352,17 @@ pub(crate) async fn smtp_send(
         Ok(()) => SendResult::Success,
     };
 
+    debug_assert!(smtp.transport_id.is_some());
+    if matches!(status, SendResult::Success)
+        && 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
     {
@@ -856,3 +894,6 @@ pub(crate) async fn add_self_recipients(
 
     Ok(())
 }
+
+#[cfg(test)]
+mod smtp_tests;

src/smtp/smtp_tests.rs

@@ -0,0 +1,43 @@
+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);
+
+    Ok(())
+}

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?