Dramatically limit the range of failures which can cause duplicate mail

Summary:
Ref T8574. Currently, failures during mail body construction, feed publishing, or search indexing could cause us to retry the publishing task and potentially send duplicate mail.

Instead, build (but do not send) the mail first, then send all the mail at the very end.

This isn't completley perfect, but should make it enormously harder for duplicate mail to be generated.

Test Plan: Sent some mail, ran the daemons, saw it show up normally in the outbound queue.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8574

Differential Revision: https://secure.phabricator.com/D13320

src/applications/metamta/replyhandler/PhabricatorMailTarget.php

@@ -55,7 +55,7 @@ final class PhabricatorMailTarget extends Phobject {
     return $this->viewer;
   }
 
-  public function sendMail(PhabricatorMetaMTAMail $mail) {
+  public function willSendMail(PhabricatorMetaMTAMail $mail) {
     $viewer = $this->getViewer();
 
     $mail->addPHIDHeaders('X-Phabricator-To', $this->rawToPHIDs);
@@ -92,7 +92,7 @@ final class PhabricatorMailTarget extends Phobject {
       $mail->addCCs($cc);
     }
 
-    return $mail->save();
+    return $mail;
   }
 
   private function getRecipientsSummary(

src/applications/releeph/editor/ReleephRequestTransactionalEditor.php

@@ -166,28 +166,22 @@ final class ReleephRequestTransactionalEditor
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {
-    return true;
-  }
-
-  protected function sendMail(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
 
     // Avoid sending emails that only talk about commit discovery.
     $types = array_unique(mpull($xactions, 'getTransactionType'));
     if ($types === array(ReleephRequestTransaction::TYPE_DISCOVERY)) {
-      return null;
+      return false;
     }
 
     // Don't email people when we discover that something picks or reverts OK.
     if ($types === array(ReleephRequestTransaction::TYPE_PICK_STATUS)) {
       if (!mfilter($xactions, 'isBoringPickStatus', true /* negate */)) {
         // If we effectively call "isInterestingPickStatus" and get nothing...
-        return null;
+        return false;
       }
     }
 
-    return parent::sendMail($object, $xactions);
+    return true;
   }
 
   protected function buildReplyHandler(PhabricatorLiskDAO $object) {

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1040,10 +1040,10 @@ abstract class PhabricatorApplicationTransactionEditor
     // Hook for edges or other properties that may need (re-)loading
     $object = $this->willPublish($object, $xactions);
 
-    $mailed = array();
+    $messages = array();
     if (!$this->getDisableEmail()) {
       if ($this->shouldSendMail($object, $xactions)) {
-        $mailed = $this->sendMail($object, $xactions);
+        $messages = $this->buildMail($object, $xactions);
       }
     }
 
@@ -1055,10 +1055,21 @@ abstract class PhabricatorApplicationTransactionEditor
     }
 
     if ($this->shouldPublishFeedStory($object, $xactions)) {
-      $this->publishFeedStory(
-        $object,
-        $xactions,
-        $mailed);
+      $mailed = array();
+      foreach ($messages as $mail) {
+        foreach ($mail->buildRecipientList() as $phid) {
+          $mailed[$phid] = true;
+        }
+      }
+
+      $this->publishFeedStory($object, $xactions, $mailed);
+    }
+
+    // NOTE: This actually sends the mail. We do this last to reduce the chance
+    // that we send some mail, hit an exception, then send the mail again when
+    // retrying.
+    foreach ($messages as $mail) {
+      $mail->save();
     }
 
     return $xactions;
@@ -2241,7 +2252,7 @@ abstract class PhabricatorApplicationTransactionEditor
   /**
    * @task mail
    */
-  protected function sendMail(
+  private function buildMail(
     PhabricatorLiskDAO $object,
     array $xactions) {
 
@@ -2255,8 +2266,7 @@ abstract class PhabricatorApplicationTransactionEditor
     // Set this explicitly before we start swapping out the effective actor.
     $this->setActingAsPHID($this->getActingAsPHID());
 
-
-    $mailed = array();
+    $messages = array();
     foreach ($targets as $target) {
       $original_actor = $this->getActor();
 
@@ -2270,7 +2280,7 @@ abstract class PhabricatorApplicationTransactionEditor
         // Reload handles for the new viewer.
         $this->loadHandles($xactions);
 
-        $mail = $this->sendMailToTarget($object, $xactions, $target);
+        $mail = $this->buildMailForTarget($object, $xactions, $target);
       } catch (Exception $ex) {
         $caught = $ex;
       }
@@ -2283,16 +2293,14 @@ abstract class PhabricatorApplicationTransactionEditor
       }
 
       if ($mail) {
-        foreach ($mail->buildRecipientList() as $phid) {
-          $mailed[$phid] = true;
-        }
+        $messages[] = $mail;
       }
     }
 
-    return array_keys($mailed);
+    return $messages;
   }
 
-  private function sendMailToTarget(
+  private function buildMailForTarget(
     PhabricatorLiskDAO $object,
     array $xactions,
     PhabricatorMailTarget $target) {
@@ -2354,7 +2362,7 @@ abstract class PhabricatorApplicationTransactionEditor
       $mail->setParentMessageID($this->getParentMessageID());
     }
 
-    return $target->sendMail($mail);
+    return $target->willSendMail($mail);
   }
 
   private function addMailProjectMetadata(