5 files changed, 63 insertions, 13 deletions

diff --git a/src/mailman/app/tests/test_moderation.py b/src/mailman/app/tests/test_moderation.py
index 006348a3d..12dadf6fb 100644
--- a/src/mailman/app/tests/test_moderation.py
+++ b/src/mailman/app/tests/test_moderation.py
@@ -153,6 +153,15 @@ Message-ID: <alpha>
         self.assertEqual(messages[0].msgdata['recipients'],
                          ['zack@example.com'])
 
+    def test_survive_a_deleted_message(self):
+        # When the message that should be deleted is not found in the store,
+        # no error is raised.
+        request_id = hold_message(self._mlist, self._msg)
+        message_store = getUtility(IMessageStore)
+        message_store.delete_message('<alpha>')
+        handle_message(self._mlist, request_id, Action.discard)
+        self.assertEqual(self._request_db.count, 0)
+
 
 
 class TestUnsubscription(unittest.TestCase):
diff --git a/src/mailman/docs/NEWS.rst b/src/mailman/docs/NEWS.rst
index e2f81c4b4..2ea0002cf 100644
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -50,6 +50,8 @@ Bugs
    addresses.  Given by Aurélien Bompard.
  * Allow mailing lists to have localhost names with a suffix matching the
    subcommand extensions.  Given by Aurélien Bompard.  (Closes: #168)
+ * Don't traceback if a nonexistent message-id is deleted from the message
+   store.  Given by Aurélien Bompard, tweaked by Barry Warsaw.  (Closes: #167)
 
 Configuration
 -------------
@@ -71,6 +73,8 @@ Interfaces
    `X-Message-ID-Hash` although the latter is still included for backward
    compatibility.  Also be sure that all places which add the header use the
    same algorithm.  (Closes #118)
+ * ``IMessageStore.delete_message()`` no longer raises a ``LookupError`` when
+   you attempt to delete a nonexistent message from the message store.
 
 Internal API
 ------------
diff --git a/src/mailman/interfaces/messages.py b/src/mailman/interfaces/messages.py
index 9617a879a..2489012a9 100644
--- a/src/mailman/interfaces/messages.py
+++ b/src/mailman/interfaces/messages.py
@@ -89,8 +89,10 @@ class IMessageStore(Interface):
     def delete_message(message_id):
         """Remove the given message from the store.
 
-        :param message: The Message-ID of the mesage to delete from the store.
-        :raises LookupError: if there is no such message.
+        If the referenced message is missing from the message store, the
+        operation is ignored.
+
+        :param message: The Message-ID of the message to delete from the store.
         """
 
     messages = Attribute(
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,