fix(connectivity): return false from all_work_done() immediately after connecting

We do not want all_work_done() to return true immediately
after calling start_io(), but only when connection goes idle.

"Connected" state is set immediately after connecting to the server,
but it does not mean there is nothing to do.

This change make all_work_done() return false
from the Connected state and introduces a new Idle
connectivity state that is only set before connection
actually goes idle. For idle state all_work_done() returns true.

From the user point of view both old Connected state
and new Idle state look the same.

Author: link2xt

python/tests/test_1_online.py

@@ -1979,6 +1979,32 @@ def test_connectivity(acfactory, lp):
     ac1._evtracker.wait_for_connectivity(dc.const.DC_CONNECTIVITY_NOT_CONNECTED)
 
 
+def test_all_work_done(acfactory, lp):
+    """
+    Tests that calling start_io() immediately followed by maybe_network()
+    and then waiting for all_work_done() reliably fetches the messages
+    delivered while account was offline.
+    In other words, connectivity should not change to a state
+    where all_work_done() returns true until IMAP connection goes idle.
+    """
+    ac1, ac2 = acfactory.get_online_accounts(2)
+
+    ac1.stop_io()
+    ac1._evtracker.wait_for_connectivity(dc.const.DC_CONNECTIVITY_NOT_CONNECTED)
+
+    ac1.direct_imap.select_config_folder("inbox")
+    with ac1.direct_imap.idle() as idle1:
+        ac2.create_chat(ac1).send_text("Hi")
+        idle1.wait_for_new_message()
+
+    ac1.start_io()
+    ac1.maybe_network()
+    ac1._evtracker.wait_for_all_work_done()
+    msgs = ac1.create_chat(ac2).get_messages()
+    assert len(msgs) == 1
+    assert msgs[0].text == "Hi"
+
+
 def test_fetch_deleted_msg(acfactory, lp):
     """This is a regression test: Messages with \\Deleted flag were downloaded again and again,
     hundreds of times, because uid_next was not updated.

src/scheduler.rs

@@ -590,7 +590,7 @@ async fn fetch_idle(ctx: &Context, connection: &mut Imap, folder_meaning: Folder
         .log_err(ctx)
         .ok();
 
-    connection.connectivity.set_connected(ctx).await;
+    connection.connectivity.set_idle(ctx).await;
 
     ctx.emit_event(EventType::ImapInboxIdle);
     let Some(session) = connection.session.take() else {
@@ -727,7 +727,7 @@ async fn smtp_loop(
             // Fake Idle
             info!(ctx, "smtp fake idle - started");
             match &connection.last_send_error {
-                None => connection.connectivity.set_connected(&ctx).await,
+                None => connection.connectivity.set_idle(&ctx).await,
                 Some(err) => connection.connectivity.set_err(&ctx, err).await,
             }
 

src/scheduler/connectivity.rs

@@ -33,9 +33,18 @@ enum DetailedConnectivity {
     #[default]
     Uninitialized,
     Connecting,
+
+    /// Connection is just established, but there may be work to do.
+    Connected,
+
+    /// There is actual work to do, e.g. there are messages in SMTP queue
+    /// or we detected a message that should be downloaded.
     Working,
+
     InterruptingIdle,
-    Connected,
+
+    /// Connection is established and is idle.
+    Idle,
 
     /// The folder was configured not to be watched or configured_*_folder is not set
     NotConfigured,
@@ -54,6 +63,8 @@ impl DetailedConnectivity {
             // Just don't return a connectivity, probably the folder is configured not to be
             // watched or there is e.g. no "Sent" folder, so we are not interested in it
             DetailedConnectivity::NotConfigured => None,
+
+            DetailedConnectivity::Idle => Some(Connectivity::Connected),
         }
     }
 
@@ -65,7 +76,8 @@ impl DetailedConnectivity {
             DetailedConnectivity::Connecting => "<span class=\"yellow dot\"></span>".to_string(),
             DetailedConnectivity::Working
             | DetailedConnectivity::InterruptingIdle
-            | DetailedConnectivity::Connected => "<span class=\"green dot\"></span>".to_string(),
+            | DetailedConnectivity::Connected
+            | DetailedConnectivity::Idle => "<span class=\"green dot\"></span>".to_string(),
         }
     }
 
@@ -75,9 +87,9 @@ impl DetailedConnectivity {
             DetailedConnectivity::Uninitialized => "Not started".to_string(),
             DetailedConnectivity::Connecting => stock_str::connecting(context).await,
             DetailedConnectivity::Working => stock_str::updating(context).await,
-            DetailedConnectivity::InterruptingIdle | DetailedConnectivity::Connected => {
-                stock_str::connected(context).await
-            }
+            DetailedConnectivity::InterruptingIdle
+            | DetailedConnectivity::Connected
+            | DetailedConnectivity::Idle => stock_str::connected(context).await,
             DetailedConnectivity::NotConfigured => "Not configured".to_string(),
         }
     }
@@ -94,9 +106,9 @@ impl DetailedConnectivity {
             // We don't know any more than that the last message was sent successfully;
             // since sending the last message, connectivity could have changed, which we don't notice
             // until another message is sent
-            DetailedConnectivity::InterruptingIdle | DetailedConnectivity::Connected => {
-                stock_str::last_msg_sent_successfully(context).await
-            }
+            DetailedConnectivity::InterruptingIdle
+            | DetailedConnectivity::Connected
+            | DetailedConnectivity::Idle => stock_str::last_msg_sent_successfully(context).await,
             DetailedConnectivity::NotConfigured => "Not configured".to_string(),
         }
     }
@@ -108,8 +120,9 @@ impl DetailedConnectivity {
             DetailedConnectivity::Connecting => false,
             DetailedConnectivity::Working => false,
             DetailedConnectivity::InterruptingIdle => false,
-            DetailedConnectivity::Connected => true,
+            DetailedConnectivity::Connected => false, // Just connected, there may still be work to do.
             DetailedConnectivity::NotConfigured => true,
+            DetailedConnectivity::Idle => true,
         }
     }
 }
@@ -141,6 +154,9 @@ impl ConnectivityStore {
     pub(crate) async fn set_not_configured(&self, context: &Context) {
         self.set(context, DetailedConnectivity::NotConfigured).await;
     }
+    pub(crate) async fn set_idle(&self, context: &Context) {
+        self.set(context, DetailedConnectivity::Idle).await;
+    }
 
     async fn get_detailed(&self) -> DetailedConnectivity {
         self.0.lock().await.deref().clone()
@@ -164,6 +180,7 @@ pub(crate) async fn idle_interrupted(inbox: ConnectivityStore, oboxes: Vec<Conne
     // return Connected until DC is completely done with fetching folders; this also
     // includes scan_folders() which happens on the inbox thread.
     if *connectivity_lock == DetailedConnectivity::Connected
+        || *connectivity_lock == DetailedConnectivity::Idle
         || *connectivity_lock == DetailedConnectivity::NotConfigured
     {
         *connectivity_lock = DetailedConnectivity::InterruptingIdle;
@@ -172,7 +189,9 @@ pub(crate) async fn idle_interrupted(inbox: ConnectivityStore, oboxes: Vec<Conne
 
     for state in oboxes {
         let mut connectivity_lock = state.0.lock().await;
-        if *connectivity_lock == DetailedConnectivity::Connected {
+        if *connectivity_lock == DetailedConnectivity::Connected
+            || *connectivity_lock == DetailedConnectivity::Idle
+        {
             *connectivity_lock = DetailedConnectivity::InterruptingIdle;
         }
     }