Merge bounce cleanup branch. This is a major cleansing and refactoring of the

bounce processor.  Note that one thing this does not do is implement any of
the policy around bounce scores.  It "merely" cleans up all the crud in the
BounceRunner, and all the logic around registering bounce events.  This means
the bounce runner can actually be enabled again.  More code needs to be
written to process bounces events occasionally.

Details:

* maybe_forward() moved to app/bounces.py, and it can now discard the
  message, forward it to the list administrators (moderators + owners), or
  site owners.  See UnrecognizedBounceDisposition.
* scan_message() always returns a set of addresses.
* The DSN bounce detector is cleaned up.
* An IBounceProcessor utility is added.
* IBounceEvents are added, with database support.
* bounce_unrecognized_goes_to_list_owner -> forward_unrecognized_bounces_to
* bounce_processing -> process_bounces
* ReopenableFileHandler.filename is exposed as a public attribute.  This aids
  in testing.
* Fix the signature of UserNotification.__init__() to be more PEP 8 compliant.
* Change the OwnerNotification.__init__() signature to take a roster object
  instead of `tomoderators`.  When the roster is None, the site owner is used
  instead.
* Fix the default style setting; s/personalization/personalize/
* BounceMixin is gone, baby gone.
* Add tests for the OutgoingRunner.
* make_testable_runner() can now take a predicate object, which can change how
  often the runner goes through its main loop.  Use this e.g. to go through
  only once when a message does not get dequeued.
* A new LogFileMark helper for testing messages to a log file.

1 files changed, 55 insertions, 41 deletions

diff --git a/src/mailman/queue/outgoing.py b/src/mailman/queue/outgoing.py
index 7ff194219..ed27f014c 100644
--- a/src/mailman/queue/outgoing.py
+++ b/src/mailman/queue/outgoing.py
@@ -22,12 +22,16 @@ import logging
 
 from datetime import datetime
 from lazr.config import as_boolean, as_timedelta
+from zope.component import getUtility
 
 from mailman.config import config
+from mailman.interfaces.bounce import BounceContext, IBounceProcessor
 from mailman.interfaces.mailinglist import Personalization
+from mailman.interfaces.membership import ISubscriptionService
 from mailman.interfaces.mta import SomeRecipientsFailed
+from mailman.interfaces.pending import IPendings
 from mailman.queue import Runner
-from mailman.queue.bounce import BounceMixin
+from mailman.utilities.datetime import now
 from mailman.utilities.modules import find_name
 
 
@@ -36,15 +40,15 @@ from mailman.utilities.modules import find_name
 DEAL_WITH_PERMFAILURES_EVERY = 10
 
 log = logging.getLogger('mailman.error')
+smtp_log = logging.getLogger('mailman.smtp')
 
 
 
-class OutgoingRunner(Runner, BounceMixin):
+class OutgoingRunner(Runner):
     """The outgoing queue runner."""
 
     def __init__(self, slice=None, numslices=1):
-        Runner.__init__(self, slice, numslices)
-        BounceMixin.__init__(self)
+        super(OutgoingRunner, self).__init__(slice, numslices)
         # We look this function up only at startup time.
         self._func = find_name(config.mta.outgoing)
         # This prevents smtp server connection problems from filling up the
@@ -56,7 +60,7 @@ class OutgoingRunner(Runner, BounceMixin):
     def _dispose(self, mlist, msg, msgdata):
         # See if we should retry delivery of this message again.
         deliver_after = msgdata.get('deliver_after', datetime.fromtimestamp(0))
-        if datetime.now() < deliver_after:
+        if now() < deliver_after:
             return True
         # Calculate whether we should VERP this message or not.  The results of
         # this set the 'verp' key in the message metadata.
@@ -69,7 +73,7 @@ class OutgoingRunner(Runner, BounceMixin):
         # Also, if personalization is /not/ enabled, but
         # verp_delivery_interval is set (and we've hit this interval), then
         # again, this message should be VERP'd. Otherwise, no.
-        elif mlist.personalize <> Personalization.none:
+        elif mlist.personalize != Personalization.none:
             if as_boolean(config.mta.verp_personalized_deliveries):
                 msgdata['verp'] = True
         elif interval == 0:
@@ -88,59 +92,69 @@ class OutgoingRunner(Runner, BounceMixin):
             # There was a problem connecting to the SMTP server.  Log this
             # once, but crank up our sleep time so we don't fill the error
             # log.
-            port = int(config.mta.port)
+            port = int(config.mta.smtp_port)
             if port == 0:
-                port = 'smtp'
-            # Log this just once.
+                port = 'smtp'            # Log this just once.
             if not self._logged:
                 log.error('Cannot connect to SMTP server %s on port %s',
-                          config.mta.host, port)
+                          config.mta.smtp_host, port)
                 self._logged = True
             return True
         except SomeRecipientsFailed as error:
-            # Handle local rejects of probe messages differently.
-            if msgdata.get('probe_token') and error.permanent_failures:
-                self._probe_bounce(mlist, msgdata['probe_token'])
+            processor = getUtility(IBounceProcessor)
+            # BAW: msg is the original message that failed delivery, not a
+            # bounce message.  This may be confusing if this is what's sent to
+            # the user in the probe message.  Maybe we should craft a
+            # bounce-like message containing information about the permanent
+            # SMTP failure?
+            if 'probe_token' in msgdata:
+                # This is a failure of our local MTA to deliver to a probe
+                # message recipient.  Register the bounce event for permanent
+                # failures.  Start by grabbing and confirming (i.e. removing)
+                # the pendable record associated with this bounce token,
+                # regardless of what address was actually failing.
+                if len(error.permanent_failures) > 0:
+                    pended = getUtility(IPendings).confirm(
+                        msgdata['probe_token'])
+                    # It's possible the token has been confirmed out of the
+                    # database.  Just ignore that.
+                    if pended is not None:
+                        member = getUtility(ISubscriptionService).get_member(
+                            pended['member_id'])
+                        processor.register(
+                            mlist, member.address.email, msg,
+                            BounceContext.probe)
             else:
                 # Delivery failed at SMTP time for some or all of the
                 # recipients.  Permanent failures are registered as bounces,
                 # but temporary failures are retried for later.
-                #
-                # BAW: msg is going to be the original message that failed
-                # delivery, not a bounce message.  This may be confusing if
-                # this is what's sent to the user in the probe message.  Maybe
-                # we should craft a bounce-like message containing information
-                # about the permanent SMTP failure?
-                if error.permanent_failures:
-                    self._queue_bounces(
-                        mlist.fqdn_listname, error.permanent_failures, msg)
+                for email in error.permanent_failures:
+                    processor.register(mlist, email, msg, BounceContext.normal)
                 # Move temporary failures to the qfiles/retry queue which will
                 # occasionally move them back here for another shot at
                 # delivery.
                 if error.temporary_failures:
-                    now = datetime.now()
-                    recips = error.temporary_failures
+                    current_time = now()
+                    recipients = error.temporary_failures
                     last_recip_count = msgdata.get('last_recip_count', 0)
-                    deliver_until = msgdata.get('deliver_until', now)
-                    if len(recips) == last_recip_count:
-                        # We didn't make any progress, so don't attempt
-                        # delivery any longer.  BAW: is this the best
-                        # disposition?
-                        if now > deliver_until:
+                    deliver_until = msgdata.get('deliver_until', current_time)
+                    if len(recipients) == last_recip_count:
+                        # We didn't make any progress.  If we've exceeded the
+                        # configured retry period, log this failure and
+                        # discard the message.
+                        if current_time > deliver_until:
+                            smtp_log.error('Discarding message with '
+                                           'persistent temporary failures: '
+                                           '{0}'.format(msg['message-id']))
                             return False
                     else:
-                        # Keep trying to delivery this message for a while
-                        deliver_until = now + as_timedelta(
+                        # We made some progress, so keep trying to delivery
+                        # this message for a while longer.
+                        deliver_until = current_time + as_timedelta(
                             config.mta.delivery_retry_period)
-                    msgdata['last_recip_count'] = len(recips)
+                    msgdata['last_recip_count'] = len(recipients)
                     msgdata['deliver_until'] = deliver_until
-                    msgdata['recipients'] = recips
+                    msgdata['recipients'] = recipients
                     self._retryq.enqueue(msg, msgdata)
-        # We've successfully completed handling of this message
+        # We've successfully completed handling of this message.
         return False
-
-    _do_periodic = BounceMixin._do_periodic
-
-    def _clean_up(self):
-        BounceMixin._clean_up(self)
-        Runner._clean_up(self)