From 30c4783c426f10d90801178fac2d61bff5b4747d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 16 Jun 2015 16:43:24 -0700
Subject: [PATCH] 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
---
 .../replyhandler/PhabricatorMailTarget.php    |  4 +-
 .../ReleephRequestTransactionalEditor.php     | 12 ++----
 ...habricatorApplicationTransactionEditor.php | 40 +++++++++++--------
 3 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/src/applications/metamta/replyhandler/PhabricatorMailTarget.php b/src/applications/metamta/replyhandler/PhabricatorMailTarget.php
index fd2e7a0f7a..e7e79c5ef9 100644
--- a/src/applications/metamta/replyhandler/PhabricatorMailTarget.php
+++ b/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(
diff --git a/src/applications/releeph/editor/ReleephRequestTransactionalEditor.php b/src/applications/releeph/editor/ReleephRequestTransactionalEditor.php
index 8f6805e661..17ae6fcc35 100644
--- a/src/applications/releeph/editor/ReleephRequestTransactionalEditor.php
+++ b/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) {
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 6bf88b4e25..fd547c4049 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/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(