Handle deleting nonexistent messages from the message store. Closes: #167

2 files changed, 46 insertions, 11 deletions

diff --git a/src/mailman/model/messagestore.py b/src/mailman/model/messagestore.py
index d5b6eb1e3..7b03621ff 100644
--- a/src/mailman/model/messagestore.py
+++ b/src/mailman/model/messagestore.py
@@ -125,8 +125,12 @@ class MessageStore:
     @dbconnection
     def delete_message(self, store, message_id):
         row = store.query(Message).filter_by(message_id=message_id).first()
-        if row is None:
-            raise LookupError(message_id)
-        path = os.path.join(config.MESSAGES_DIR, row.path)
-        os.remove(path)
-        store.delete(row)
+        if row is not None:
+            path = os.path.join(config.MESSAGES_DIR, row.path)
+            # It's possible that a race condition caused the file system path
+            # to already be deleted.
+            try:
+                os.remove(path)
+            except FileNotFoundError:
+                pass
+            store.delete(row)
diff --git a/src/mailman/model/tests/test_messagestore.py b/src/mailman/model/tests/test_messagestore.py
index 337822cf7..c12686b31 100644
--- a/src/mailman/model/tests/test_messagestore.py
+++ b/src/mailman/model/tests/test_messagestore.py
@@ -22,9 +22,12 @@ __all__ = [
     ]
 
 
+import os
 import unittest
 
+from mailman.config import config
 from mailman.interfaces.messages import IMessageStore
+from mailman.model.message import Message
 from mailman.testing.helpers import (
     specialized_message_from_string as mfs)
 from mailman.testing.layers import ConfigLayer
@@ -51,15 +54,15 @@ This message is very important.
     def test_get_message_by_hash(self):
         # Messages have an X-Message-ID-Hash header, the value of which can be
         # used to look the message up in the message store.
-        message = mfs("""\
+        msg = mfs("""\
 Subject: An important message
 Message-ID: <ant>
 
 This message is very important.
 """)
-        add_message_hash(message)
-        self._store.add(message)
-        self.assertEqual(message['x-message-id-hash'],
+        add_message_hash(msg)
+        self._store.add(msg)
+        self.assertEqual(msg['x-message-id-hash'],
                          'MS6QLWERIJLGCRF44J7USBFDELMNT2BW')
         found = self._store.get_message_by_hash(
             'MS6QLWERIJLGCRF44J7USBFDELMNT2BW')
@@ -67,8 +70,36 @@ This message is very important.
         self.assertEqual(found['x-message-id-hash'],
                          'MS6QLWERIJLGCRF44J7USBFDELMNT2BW')
 
-    def test_cannot_delete_missing_message(self):
-        self.assertRaises(LookupError, self._store.delete_message, 'missing')
+    def test_can_delete_missing_message(self):
+        # Deleting a message which isn't in the store does not raise an
+        # exception.
+        msg = mfs("""\
+Message-ID: <ant>
+
+""")
+        self._store.add(msg)
+        self.assertEqual(len(list(self._store.messages)), 1)
+        self._store.delete_message('missing')
+        self.assertEqual(len(list(self._store.messages)), 1)
+
+    def test_can_survive_missing_message_path(self):
+        # Deleting a message which is in the store, but which doesn't appear
+        # on the file system does not raise an exception, but still removes
+        # the message from the store.
+        msg = mfs("""\
+Message-ID: <ant>
+
+""")
+        self._store.add(msg)
+        self.assertEqual(len(list(self._store.messages)), 1)
+        # We have to use the SQLAlchemy API because the .get_message_by_*()
+        # methods return email Message objects, not IMessages.  The former
+        # does not give us the path to the object on the file system.
+        row = config.db.store.query(Message).filter_by(
+            message_id='<ant>').first()
+        os.remove(os.path.join(config.MESSAGES_DIR, row.path))
+        self._store.delete_message('<ant>')
+        self.assertEqual(len(list(self._store.messages)), 0)
 
     def test_message_id_hash(self):
         # The new specification calls for a Message-ID-Hash header,