Merge pull request #171 from christianlouis/copilot/ensure-emails-are-retried

fix: retry failed email forwards; only mark/delete successfully forwarded messages

backend/tests/unit/test_mail_processor_imap.py

@@ -163,7 +163,7 @@ class TestFetchImapEmailsUidCommands:
         # The fetch call should use UID FETCH
         fetch_call = [c for c in mock_imap.uid.call_args_list if c.args[0] == "fetch"]
         assert len(fetch_call) == 1
-        assert fetch_call[0] == call("fetch", "42", "(RFC822)")
+        assert fetch_call[0] == call("fetch", "42", "(BODY.PEEK[])")
 
     async def test_no_per_message_store_for_seen(self, processor, mock_imap):
         """RFC822 implicitly marks \\Seen; no extra STORE per message is needed."""
@@ -260,8 +260,12 @@ class TestFetchImapEmailsUidCommands:
         # RFC 3501 parenthesised flag syntax
         assert store_calls[0].args[3] == "(\\Seen)"
 
-    async def test_delete_after_forward_uses_single_batch_store(self):
-        """delete_after_forward=True must issue one UID STORE \\Deleted command."""
+    async def test_fetch_does_not_delete_after_forward(self):
+        """_fetch_imap_emails must NOT issue \\Deleted STORE even when delete_after_forward=True.
+
+        Deletion is deferred to post_process_imap() so that only successfully
+        forwarded messages are removed from the source mailbox.
+        """
         from app.services.mail_processor import MailProcessor
 
         account = _make_account(delete_after_forward=True)
@@ -295,17 +299,15 @@ class TestFetchImapEmailsUidCommands:
 
         assert new_uids == ["7", "8"]
 
-        store_calls = [c for c in mock_imap.uid.call_args_list if c.args[0] == "store"]
-        # Exactly one STORE for deletion covering both UIDs
-        assert len(store_calls) == 1
-        uid_set_arg = store_calls[0].args[1]
-        parts = set(uid_set_arg.split(","))
-        assert parts == {"7", "8"}
-        assert store_calls[0].args[2] == "+FLAGS"
-        # RFC 3501 parenthesised flag syntax
-        assert store_calls[0].args[3] == "(\\Deleted)"
-        # expunge must also be called
-        mock_imap.expunge.assert_awaited_once()
+        # No STORE for \\Deleted should be issued during fetch
+        delete_store_calls = [
+            c
+            for c in mock_imap.uid.call_args_list
+            if c.args[0] == "store" and "(\\Deleted)" in c.args
+        ]
+        assert delete_store_calls == [], "fetch must not delete messages"
+        # expunge must NOT be called during fetch either
+        mock_imap.expunge.assert_not_awaited()
 
     async def test_individual_fetch_failure_does_not_abort(self, processor, mock_imap):
         """A single message fetch error should be logged but not stop processing."""
@@ -709,3 +711,176 @@ class TestFetchImapEdgeCases:
         assert len(emails) == 1
         assert emails[0] == email_bytes
         assert new_uids == ["99"]
+
+
+# ---------------------------------------------------------------------------
+# post_process_imap tests
+# ---------------------------------------------------------------------------
+
+
+class TestPostProcessImap:
+    """Tests for the post_process_imap() method."""
+
+    def _make_processor(self, delete_after_forward=False, protocol="imap_ssl"):
+        from app.services.mail_processor import MailProcessor
+
+        account = _make_account(
+            delete_after_forward=delete_after_forward, protocol=protocol
+        )
+        return MailProcessor(account=account, decrypted_password="secret")
+
+    def _mock_imap_client(self):
+        client = MagicMock()
+        client.wait_hello_from_server = AsyncMock()
+        client.login = AsyncMock()
+        client.select = AsyncMock()
+        client.uid = AsyncMock(return_value=_make_imap_response())
+        client.expunge = AsyncMock()
+        client.logout = AsyncMock()
+        return client
+
+    async def test_marks_seen_without_delete(self):
+        """post_process_imap marks UIDs \\Seen when delete_after_forward=False."""
+        processor = self._make_processor(delete_after_forward=False)
+        mock_imap = self._mock_imap_client()
+
+        with patch(
+            "app.services.mail_processor.aioimaplib.IMAP4_SSL",
+            return_value=mock_imap,
+        ):
+            await processor.post_process_imap(["10", "11"])
+
+        store_calls = [c for c in mock_imap.uid.call_args_list if c.args[0] == "store"]
+        assert len(store_calls) == 1
+        assert set(store_calls[0].args[1].split(",")) == {"10", "11"}
+        assert store_calls[0].args[3] == "(\\Seen)"
+        mock_imap.expunge.assert_not_awaited()
+
+    async def test_marks_seen_and_deleted_with_delete(self):
+        """post_process_imap marks \\Seen + \\Deleted and expunges when configured."""
+        processor = self._make_processor(delete_after_forward=True)
+        mock_imap = self._mock_imap_client()
+
+        with patch(
+            "app.services.mail_processor.aioimaplib.IMAP4_SSL",
+            return_value=mock_imap,
+        ):
+            await processor.post_process_imap(["5", "6"])
+
+        store_calls = [c for c in mock_imap.uid.call_args_list if c.args[0] == "store"]
+        assert len(store_calls) == 2
+        flags_used = {c.args[3] for c in store_calls}
+        assert "(\\Seen)" in flags_used
+        assert "(\\Deleted)" in flags_used
+        mock_imap.expunge.assert_awaited_once()
+
+    async def test_noop_on_empty_uid_list(self):
+        """post_process_imap does nothing when the UID list is empty."""
+        processor = self._make_processor()
+        mock_imap = self._mock_imap_client()
+
+        with patch(
+            "app.services.mail_processor.aioimaplib.IMAP4_SSL",
+            return_value=mock_imap,
+        ):
+            await processor.post_process_imap([])
+
+        mock_imap.login.assert_not_awaited()
+
+    async def test_exception_is_swallowed_and_logged(self):
+        """post_process_imap catches exceptions and does not propagate them."""
+        processor = self._make_processor()
+        mock_imap = self._mock_imap_client()
+        mock_imap.login = AsyncMock(side_effect=OSError("connection refused"))
+
+        with patch(
+            "app.services.mail_processor.aioimaplib.IMAP4_SSL",
+            return_value=mock_imap,
+        ):
+            await processor.post_process_imap(["1"])  # must not raise
+
+        mock_imap.logout.assert_awaited_once()
+
+    async def test_uses_imap4_for_non_ssl(self):
+        """Plain IMAP accounts must use IMAP4, not IMAP4_SSL."""
+        from app.services.mail_processor import MailProcessor
+
+        account = _make_account(protocol="imap")
+        processor = MailProcessor(account=account, decrypted_password="pw")
+        mock_imap = MagicMock()
+        mock_imap.wait_hello_from_server = AsyncMock()
+        mock_imap.login = AsyncMock()
+        mock_imap.select = AsyncMock()
+        mock_imap.uid = AsyncMock(return_value=_make_imap_response())
+        mock_imap.logout = AsyncMock()
+
+        with (
+            patch(
+                "app.services.mail_processor.aioimaplib.IMAP4",
+                return_value=mock_imap,
+            ) as mock_cls,
+            patch("app.services.mail_processor.aioimaplib.IMAP4_SSL") as mock_ssl_cls,
+        ):
+            await processor.post_process_imap(["1"])
+            mock_cls.assert_called_once()
+            mock_ssl_cls.assert_not_called()
+
+    async def test_logout_called_even_on_error(self):
+        """logout() is called in the finally block even when an exception occurs."""
+        processor = self._make_processor()
+        mock_imap = self._mock_imap_client()
+        mock_imap.uid = AsyncMock(side_effect=Exception("store failed"))
+
+        with patch(
+            "app.services.mail_processor.aioimaplib.IMAP4_SSL",
+            return_value=mock_imap,
+        ):
+            await processor.post_process_imap(["99"])
+
+        mock_imap.logout.assert_awaited_once()
+
+
+# ---------------------------------------------------------------------------
+# post_process_messages routing test
+# ---------------------------------------------------------------------------
+
+
+class TestPostProcessMessages:
+    """post_process_messages() routes to the correct protocol handler."""
+
+    async def test_routes_to_imap_for_imap_ssl(self):
+        from app.services.mail_processor import MailProcessor
+
+        account = _make_account(protocol="imap_ssl")
+        processor = MailProcessor(account=account, decrypted_password="pw")
+        with (
+            patch.object(
+                processor, "post_process_imap", new_callable=AsyncMock
+            ) as mock_imap,
+            patch.object(
+                processor, "post_process_pop3", new_callable=AsyncMock
+            ) as mock_pop3,
+        ):
+            await processor.post_process_messages(["1", "2"])
+            mock_imap.assert_awaited_once_with(["1", "2"])
+            mock_pop3.assert_not_awaited()
+
+    async def test_routes_to_pop3_for_pop3_ssl(self):
+        from app.services.mail_processor import MailProcessor
+
+        account = _make_account(protocol="imap_ssl")
+        account.protocol = __import__(
+            "app.models.database_models", fromlist=["MailProtocol"]
+        ).MailProtocol.POP3_SSL
+        processor = MailProcessor(account=account, decrypted_password="pw")
+        with (
+            patch.object(
+                processor, "post_process_pop3", new_callable=AsyncMock
+            ) as mock_pop3,
+            patch.object(
+                processor, "post_process_imap", new_callable=AsyncMock
+            ) as mock_imap,
+        ):
+            await processor.post_process_messages(["a"])
+            mock_pop3.assert_awaited_once_with(["a"])
+            mock_imap.assert_not_awaited()

backend/tests/unit/test_mail_processor_pop3.py

@@ -438,8 +438,12 @@ class TestFetchPop3Emails:
         assert len(uids) == 2
 
     @patch("app.services.mail_processor.poplib")
-    async def test_delete_after_forward(self, mock_poplib):
-        """delete_after_forward=True issues dele() for fetched messages."""
+    async def test_no_delete_during_fetch(self, mock_poplib):
+        """_fetch_pop3_emails must NOT call dele() regardless of delete_after_forward.
+
+        Deletion is deferred to post_process_pop3() so that only successfully
+        forwarded messages are removed from the source mailbox.
+        """
         mock_conn = self._make_pop3_mock(uid_entries=[(1, "a"), (2, "b")])
         mock_poplib.POP3_SSL.return_value = mock_conn
 
@@ -447,9 +451,7 @@ class TestFetchPop3Emails:
         proc = MailProcessor(account, "secret")
         await proc._fetch_pop3_emails(10, set())
 
-        assert mock_conn.dele.call_count == 2
-        mock_conn.dele.assert_any_call(1)
-        mock_conn.dele.assert_any_call(2)
+        mock_conn.dele.assert_not_called()
 
     @patch("app.services.mail_processor.poplib")
     async def test_no_delete_when_disabled(self, mock_poplib):
@@ -483,17 +485,16 @@ class TestFetchPop3Emails:
         assert "b" not in uids
 
     @patch("app.services.mail_processor.poplib")
-    async def test_delete_error_does_not_abort(self, mock_poplib):
-        """A dele() error is logged but doesn't raise."""
+    async def test_fetch_does_not_call_dele(self, mock_poplib):
+        """_fetch_pop3_emails never calls dele() — deletion is in post_process_pop3."""
         mock_conn = self._make_pop3_mock(uid_entries=[(1, "a")])
-        mock_conn.dele.side_effect = Exception("delete failed")
         mock_poplib.POP3_SSL.return_value = mock_conn
 
         account = _make_account(protocol="pop3_ssl", delete_after_forward=True)
         proc = MailProcessor(account, "secret")
-        # Should not raise
         emails, uids = await proc._fetch_pop3_emails(10, set())
         assert len(emails) == 1
+        mock_conn.dele.assert_not_called()
 
     @patch("app.services.mail_processor.poplib")
     async def test_connection_failure_raises_mail_fetch_error(self, mock_poplib):
@@ -559,10 +560,78 @@ class TestFetchPop3Emails:
 
 
 # ---------------------------------------------------------------------------
-# forward_email
+# post_process_pop3 tests
 # ---------------------------------------------------------------------------
 
 
+class TestPostProcessPop3:
+    """Unit tests for post_process_pop3()."""
+
+    @patch("app.services.mail_processor.poplib")
+    async def test_deletes_only_forwarded_uids(self, mock_poplib):
+        """Only successfully forwarded UIDs are deleted from the source mailbox."""
+        mock_conn = MagicMock()
+        mock_conn.uidl.return_value = (b"+OK", [b"1 uid-a", b"2 uid-b", b"3 uid-c"], 0)
+        mock_poplib.POP3_SSL.return_value = mock_conn
+
+        account = _make_account(protocol="pop3_ssl", delete_after_forward=True)
+        proc = MailProcessor(account, "secret")
+        await proc.post_process_pop3(["uid-a", "uid-c"])
+
+        # Only messages 1 and 3 should be deleted (uid-a and uid-c)
+        assert mock_conn.dele.call_count == 2
+        mock_conn.dele.assert_any_call(1)
+        mock_conn.dele.assert_any_call(3)
+        mock_conn.quit.assert_called_once()
+
+    @patch("app.services.mail_processor.poplib")
+    async def test_noop_when_delete_after_forward_false(self, mock_poplib):
+        """post_process_pop3 does nothing when delete_after_forward=False."""
+        mock_conn = MagicMock()
+        mock_poplib.POP3_SSL.return_value = mock_conn
+
+        account = _make_account(protocol="pop3_ssl", delete_after_forward=False)
+        proc = MailProcessor(account, "secret")
+        await proc.post_process_pop3(["uid-a"])
+
+        mock_poplib.POP3_SSL.assert_not_called()
+
+    @patch("app.services.mail_processor.poplib")
+    async def test_noop_on_empty_uid_list(self, mock_poplib):
+        """post_process_pop3 does nothing when the UID list is empty."""
+        mock_conn = MagicMock()
+        mock_poplib.POP3_SSL.return_value = mock_conn
+
+        account = _make_account(protocol="pop3_ssl", delete_after_forward=True)
+        proc = MailProcessor(account, "secret")
+        await proc.post_process_pop3([])
+
+        mock_poplib.POP3_SSL.assert_not_called()
+
+    @patch("app.services.mail_processor.poplib")
+    async def test_dele_error_does_not_abort(self, mock_poplib):
+        """A dele() error is logged but post_process_pop3 does not raise."""
+        mock_conn = MagicMock()
+        mock_conn.uidl.return_value = (b"+OK", [b"1 uid-a"], 0)
+        mock_conn.dele.side_effect = Exception("delete failed")
+        mock_poplib.POP3_SSL.return_value = mock_conn
+
+        account = _make_account(protocol="pop3_ssl", delete_after_forward=True)
+        proc = MailProcessor(account, "secret")
+        await proc.post_process_pop3(["uid-a"])  # must not raise
+
+        mock_conn.quit.assert_called_once()
+
+    @patch("app.services.mail_processor.poplib")
+    async def test_connection_error_is_swallowed(self, mock_poplib):
+        """A connection failure is logged but does not propagate."""
+        mock_poplib.POP3_SSL.side_effect = OSError("connection refused")
+
+        account = _make_account(protocol="pop3_ssl", delete_after_forward=True)
+        proc = MailProcessor(account, "secret")
+        await proc.post_process_pop3(["uid-a"])  # must not raise
+
+
 class TestForwardEmail:
     """Unit tests for forward_email()."""
 

backend/tests/unit/test_tasks.py

@@ -591,6 +591,7 @@ class TestProcessMailAccount:
         session.refresh = AsyncMock()
 
         mock_processor = AsyncMock()
+        mock_processor.post_process_messages = AsyncMock()
         # Two emails: first succeeds, second fails
         mock_processor.fetch_emails.return_value = (
             [raw_email, raw_email],
@@ -614,6 +615,8 @@ class TestProcessMailAccount:
             await process_mail_account.run(1)
 
         assert session.commit.await_count >= 2
+        # Only the successfully forwarded UID should be post-processed
+        mock_processor.post_process_messages.assert_awaited_once_with(["uid-1"])
 
     @pytest.mark.asyncio
     async def test_gmail_credential_revocation_on_401(self):