From d766641ce27eb5ff52baa72bd5f87d0baf378a18 Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 7 Oct 2026 01:49:16 +0000 Subject: [PATCH] feat: switch to another SMTP transport when current one returns an error Classic email servers sometimes start rejecting all messages and ask the user to solve web CAPTCHA, but still allow to connect. In this case we want to switch to another SMTP server by moving the transport to the end of the list returned by smtp::sorted_transports. We disconnect after any message sending failure, so next message or resent same message will be sent over another other transport. Permanent SMTP error does not necessarily mean that switching to another server will help. For example if the user tries to send a large message, any server will reject it, and it is not a failure of the server. We do not try to distinguish between error messages, so it is expected that we may sometimes switch to another server when it is unnecessary. --- docs/schema.sql | 13 +++++++ src/smtp.rs | 54 +++++++++++++++++++++++++---- src/smtp/smtp_tests.rs | 79 ++++++++++++++++++++++++++---------------- src/sql/migrations.rs | 15 ++++++++ 4 files changed, 126 insertions(+), 35 deletions(-) diff --git a/docs/schema.sql b/docs/schema.sql index af57e8c6e..6b7b7a69c 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -471,6 +471,19 @@ CREATE TABLE smtp_success ( FOREIGN KEY(transport_id) REFERENCES transports(id) ON DELETE CASCADE ) STRICT; +-- Similar to smtp_success, but records failures. +CREATE TABLE smtp_failure ( + -- Sequentially increasing ID of the failure + -- Transport with the highest ID is to be used last. + id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, + + -- ID of the transport that was used to attempt sending a message. + transport_id INTEGER UNIQUE NOT NULL, + + -- Delete `smtp_failure` 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 84a2489a4..03093e80d 100644 --- a/src/smtp.rs +++ b/src/smtp.rs @@ -66,7 +66,10 @@ async fn sorted_transports(context: &Context) -> Result Result<()> { // 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,), - ) + .transaction(|t| { + t.execute( + "INSERT OR REPLACE INTO smtp_success (transport_id) VALUES (?)", + (transport_id,), + )?; + t.execute( + "DELETE FROM smtp_failure WHERE transport_id = ?", + (transport_id,), + )?; + Ok(()) + }) + .await?; + Ok(()) +} + +/// Records a failure and removes last remembered success for +/// SMTP transport so another transport is tried next time. +async fn record_failure(context: &Context, transport_id: u32) -> Result<()> { + context + .sql + .transaction(|t| { + t.execute( + "INSERT OR REPLACE INTO smtp_failure (transport_id) VALUES (?)", + (transport_id,), + )?; + t.execute( + "DELETE FROM smtp_success WHERE transport_id = ?", + (transport_id,), + )?; + Ok(()) + }) .await?; Ok(()) } @@ -256,6 +286,7 @@ async fn smtp_send( ); return SendResult::Retry; }; + let transport_id = connection.transport_id; let send_result = connection .send(context, recipients, message.as_bytes()) @@ -267,6 +298,18 @@ async fn smtp_send( // Remote error, retry later. info!(context, "SMTP failed to send: {:?}.", &err); + // Record failure on any error returned by the server, but not network errors. + if matches!( + err, + async_smtp::error::Error::Permanent(_) | async_smtp::error::Error::Transient(_) + ) && let Err(err) = record_failure(context, transport_id).await + { + warn!( + context, + "Failed to remove success for {transport_id}: {err:#}." + ) + } + let res = match err { async_smtp::error::Error::Permanent(ref response) => { // Workaround for incorrectly configured servers returning permanent errors @@ -352,7 +395,6 @@ async fn smtp_send( SendResult::Failure(err) } Ok(()) => { - let transport_id = connection.transport_id; record_success(context, transport_id) .await .with_context(|| { diff --git a/src/smtp/smtp_tests.rs b/src/smtp/smtp_tests.rs index 81f3a86eb..f9814ed2a 100644 --- a/src/smtp/smtp_tests.rs +++ b/src/smtp/smtp_tests.rs @@ -1,8 +1,25 @@ use anyhow::Result; -use crate::test_utils::TestContextManager; +use crate::test_utils::{TestContext, TestContextManager}; use crate::transport; +async fn sorted_transport_addrs(t: &TestContext) -> Result { + let transports = super::sorted_transports(t).await?; + let addrs: Vec = transports + .into_iter() + .map(|(_transport_id, transport)| transport.addr) + .collect(); + Ok(addrs.join(" ")) +} + +async fn sorted_transport_ids(t: &TestContext) -> Result> { + let transports = super::sorted_transports(t).await?; + Ok(transports + .into_iter() + .map(|(transport_id, _transport)| transport_id) + .collect()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_smtp_candidates() -> Result<()> { let mut tcm = TestContextManager::new(); @@ -12,38 +29,42 @@ async fn test_smtp_candidates() -> Result<()> { 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"); + assert_eq!( + sorted_transport_addrs(t).await?, + "foo@example.net bar@example.net baz@example.net" + ); + assert_eq!(sorted_transport_ids(t).await?, [1, 2, 3]); - 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, 3).await?; + assert_eq!(sorted_transport_ids(t).await?, [3, 1, 2]); - 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, 2).await?; + assert_eq!(sorted_transport_ids(t).await?, [2, 3, 1]); - 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); + super::record_success(t, 3).await?; + assert_eq!(sorted_transport_ids(t).await?, [3, 2, 1]); + + super::record_failure(t, 3).await?; + assert_eq!(sorted_transport_ids(t).await?, [2, 1, 3]); + + // New transport is added before all failing transports. + transport::add_pseudo_transport(t, "qux@example.net").await?; + assert_eq!( + sorted_transport_addrs(t).await?, + "bar@example.net foo@example.net qux@example.net baz@example.net" + ); + assert_eq!(sorted_transport_ids(t).await?, [2, 1, 4, 3]); + + super::record_success(t, 4).await?; + assert_eq!(sorted_transport_ids(t).await?, [4, 2, 1, 3]); + + super::record_failure(t, 4).await?; + assert_eq!(sorted_transport_ids(t).await?, [2, 1, 3, 4]); + super::record_failure(t, 3).await?; + assert_eq!(sorted_transport_ids(t).await?, [2, 1, 4, 3]); + super::record_failure(t, 2).await?; + assert_eq!(sorted_transport_ids(t).await?, [1, 4, 3, 2]); Ok(()) } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 4229b4967..c341a65d7 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -2687,6 +2687,21 @@ CREATE TABLE smtp_success ( .await?; } + inc_and_check(&mut migration_version, 169)?; + if dbversion < migration_version { + sql.execute_migration( + " +CREATE TABLE smtp_failure ( + 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?