When updating revisions in response to commits, reuse previously generated diffs

Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.

When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.

Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.

Test Plan:
  - Used `bin/differential attach-commit <commit> <revision>` to hit this code.
  - Simulated a filesystem write failure, saw the diff get reused.
  - Also did a normal update, which worked properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10968

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

resources/sql/autopatches/20170109.diff.01.commit.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_differential.differential_diff
+  ADD commitPHID VARBINARY(64);

src/applications/differential/engine/DifferentialDiffExtractionEngine.php

@@ -26,6 +26,19 @@ final class DifferentialDiffExtractionEngine extends Phobject {
   public function newDiffFromCommit(PhabricatorRepositoryCommit $commit) {
     $viewer = $this->getViewer();
 
+    // If we already have an unattached diff for this commit, just reuse it.
+    // This stops us from repeatedly generating diffs if something goes wrong
+    // later in the process. See T10968 for context.
+    $existing_diffs = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withCommitPHIDs(array($commit->getPHID()))
+      ->withHasRevision(false)
+      ->needChangesets(true)
+      ->execute();
+    if ($existing_diffs) {
+      return head($existing_diffs);
+    }
+
     $repository = $commit->getRepository();
     $identifier = $commit->getCommitIdentifier();
     $monogram = $commit->getMonogram();
@@ -73,6 +86,7 @@ final class DifferentialDiffExtractionEngine extends Phobject {
 
     $diff = DifferentialDiff::newFromRawChanges($viewer, $changes)
       ->setRepositoryPHID($repository->getPHID())
+      ->setCommitPHID($commit->getPHID())
       ->setCreationMethod('commit')
       ->setSourceControlSystem($repository->getVersionControlSystem())
       ->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP)

src/applications/differential/query/DifferentialDiffQuery.php

@@ -6,6 +6,8 @@ final class DifferentialDiffQuery
   private $ids;
   private $phids;
   private $revisionIDs;
+  private $commitPHIDs;
+  private $hasRevision;
 
   private $needChangesets = false;
   private $needProperties;
@@ -25,6 +27,16 @@ final class DifferentialDiffQuery
     return $this;
   }
 
+  public function withCommitPHIDs(array $phids) {
+    $this->commitPHIDs = $phids;
+    return $this;
+  }
+
+  public function withHasRevision($has_revision) {
+    $this->hasRevision = $has_revision;
+    return $this;
+  }
+
   public function needChangesets($bool) {
     $this->needChangesets = $bool;
     return $this;
@@ -108,27 +120,46 @@ final class DifferentialDiffQuery
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
     $where = parent::buildWhereClauseParts($conn);
 
-    if ($this->ids) {
+    if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
         'id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
         'phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->revisionIDs) {
+    if ($this->revisionIDs !== null) {
       $where[] = qsprintf(
         $conn,
         'revisionID IN (%Ld)',
         $this->revisionIDs);
     }
 
+    if ($this->commitPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'commitPHID IN (%Ls)',
+        $this->commitPHIDs);
+    }
+
+    if ($this->hasRevision !== null) {
+      if ($this->hasRevision) {
+        $where[] = qsprintf(
+          $conn,
+          'revisionID IS NOT NULL');
+      } else {
+        $where[] = qsprintf(
+          $conn,
+          'revisionID IS NULL');
+      }
+    }
+
     return $where;
   }
 

src/applications/differential/storage/DifferentialDiff.php

@@ -13,6 +13,7 @@ final class DifferentialDiff
   protected $revisionID;
   protected $authorPHID;
   protected $repositoryPHID;
+  protected $commitPHID;
 
   protected $sourceMachine;
   protected $sourcePath;
@@ -62,6 +63,7 @@ final class DifferentialDiff
         'branch' => 'text255?',
         'bookmark' => 'text255?',
         'repositoryUUID' => 'text64?',
+        'commitPHID' => 'phid?',
 
         // T6203/NULLABILITY
         // These should be non-null; all diffs should have a creation method
@@ -73,6 +75,9 @@ final class DifferentialDiff
         'revisionID' => array(
           'columns' => array('revisionID'),
         ),
+        'key_commit' => array(
+          'columns' => array('commitPHID'),
+        ),
       ),
     ) + parent::getConfiguration();
   }