From b06a7e719758fca6f9a8fb929fd9853d0cf58718 Mon Sep 17 00:00:00 2001
From: iequidoo <dgreshilov@gmail.com>
Date: Thu, 9 Nov 2023 05:30:54 -0300
Subject: [PATCH] fix: Context::execute_sync_items: Ignore all errors (#4817)

An error while executing an item mustn't prevent next items from being executed. There was a comment
that only critical errors like db write failures must be reported upstack, but in fact it's hard to
achieve in the current design, there are no error codes or so, so it's bug-prone. E.g.
`ChatAction::Block` and `Unblock` already reported all errors upstack. So, let's make error handling
the same as everywhere and just ignore any errors in the item execution loop. In the worst case we
just do more unsuccessful db writes f.e.
---
 src/chat.rs        |  3 ---
 src/receive_imf.rs |  4 +--
 src/sync.rs        | 66 ++++++++++++++++++++++++----------------------
 3 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/src/chat.rs b/src/chat.rs
index 0d7f00421..40388fc92 100644
--- a/src/chat.rs
+++ b/src/chat.rs
@@ -4274,9 +4274,6 @@ impl Context {
             }
             ChatAction::SetContacts(addrs) => set_contacts_by_addrs(self, chat_id, addrs).await,
         }
-        .log_err(self)
-        .ok();
-        Ok(())
     }
 }
 
diff --git a/src/receive_imf.rs b/src/receive_imf.rs
index 463a2483b..f009024a5 100644
--- a/src/receive_imf.rs
+++ b/src/receive_imf.rs
@@ -291,9 +291,7 @@ pub(crate) async fn receive_imf_inner(
     if let Some(ref sync_items) = mime_parser.sync_items {
         if from_id == ContactId::SELF {
             if mime_parser.was_encrypted() {
-                if let Err(err) = context.execute_sync_items(sync_items).await {
-                    warn!(context, "receive_imf cannot execute sync items: {err:#}.");
-                }
+                context.execute_sync_items(sync_items).await;
             } else {
                 warn!(context, "Sync items are not encrypted.");
             }
diff --git a/src/sync.rs b/src/sync.rs
index 176fb0850..b715a474a 100644
--- a/src/sync.rs
+++ b/src/sync.rs
@@ -10,6 +10,7 @@ use crate::config::Config;
 use crate::constants::Blocked;
 use crate::contact::ContactId;
 use crate::context::Context;
+use crate::log::LogExt;
 use crate::message::{Message, MsgId, Viewtype};
 use crate::mimeparser::SystemMessage;
 use crate::param::Param;
@@ -252,41 +253,44 @@ impl Context {
     /// as otherwise we would add in a dead-loop between two devices
     /// sending message back and forth.
     ///
-    /// If an error is returned, the caller shall not try over.
-    /// Therefore, errors should only be returned on database errors or so.
-    /// If eg. just an item cannot be deleted,
-    /// that should not hold off the other items to be executed.
-    pub(crate) async fn execute_sync_items(&self, items: &SyncItems) -> Result<()> {
+    /// If an error is returned, the caller shall not try over because some sync items could be
+    /// already executed. Sync items are considered independent and executed in the given order but
+    /// regardless of whether executing of the previous items succeeded.
+    pub(crate) async fn execute_sync_items(&self, items: &SyncItems) {
         info!(self, "executing {} sync item(s)", items.items.len());
         for item in &items.items {
             match &item.data {
-                AddQrToken(token) => {
-                    let chat_id = if let Some(grpid) = &token.grpid {
-                        if let Some((chat_id, _, _)) =
-                            chat::get_chat_id_by_grpid(self, grpid).await?
-                        {
-                            Some(chat_id)
-                        } else {
-                            warn!(
-                                self,
-                                "Ignoring token for nonexistent/deleted group '{}'.", grpid
-                            );
-                            continue;
-                        }
-                    } else {
-                        None
-                    };
-                    token::save(self, Namespace::InviteNumber, chat_id, &token.invitenumber)
-                        .await?;
-                    token::save(self, Namespace::Auth, chat_id, &token.auth).await?;
-                }
-                DeleteQrToken(token) => {
-                    token::delete(self, Namespace::InviteNumber, &token.invitenumber).await?;
-                    token::delete(self, Namespace::Auth, &token.auth).await?;
-                }
-                AlterChat { id, action } => self.sync_alter_chat(id, action).await?,
+                AddQrToken(token) => self.add_qr_token(token).await,
+                DeleteQrToken(token) => self.delete_qr_token(token).await,
+                AlterChat { id, action } => self.sync_alter_chat(id, action).await,
             }
+            .log_err(self)
+            .ok();
         }
+    }
+
+    async fn add_qr_token(&self, token: &QrTokenData) -> Result<()> {
+        let chat_id = if let Some(grpid) = &token.grpid {
+            if let Some((chat_id, _, _)) = chat::get_chat_id_by_grpid(self, grpid).await? {
+                Some(chat_id)
+            } else {
+                warn!(
+                    self,
+                    "Ignoring token for nonexistent/deleted group '{}'.", grpid
+                );
+                return Ok(());
+            }
+        } else {
+            None
+        };
+        token::save(self, Namespace::InviteNumber, chat_id, &token.invitenumber).await?;
+        token::save(self, Namespace::Auth, chat_id, &token.auth).await?;
+        Ok(())
+    }
+
+    async fn delete_qr_token(&self, token: &QrTokenData) -> Result<()> {
+        token::delete(self, Namespace::InviteNumber, &token.invitenumber).await?;
+        token::delete(self, Namespace::Auth, &token.auth).await?;
         Ok(())
     }
 }
@@ -516,7 +520,7 @@ mod tests {
                 .to_string(),
             )
             ?;
-        t.execute_sync_items(&sync_items).await?;
+        t.execute_sync_items(&sync_items).await;
 
         assert!(
             Contact::lookup_id_by_addr(&t, "bob@example.net", Origin::Unknown)