Carefully avoid the harbormaster/differential race

Summary: Ref T8650. This should stop the problem, but isn't a root cause fix. See discussion on the task.

Test Plan: Made some local diffs, but this is a bit hard to reproduce reliably.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8650

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

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -585,6 +585,8 @@ final class DifferentialTransactionEditor
               'a race?'));
         }
 
+        // TODO: This can race with diff updates, particularly those from
+        // Harbormaster. See discussion in T8650.
         $diff->setRevisionID($object->getID());
         $diff->save();
 
@@ -593,6 +595,8 @@ final class DifferentialTransactionEditor
         // the old (`null`) container.
 
         // TODO: This is a bit iffy, maybe we can find a cleaner approach?
+        // In particular, this could (rarely) be overwritten by Harbormaster
+        // workers.
         $table = new HarbormasterBuildable();
         $conn_w = $table->establishConnection('w');
         queryfx(

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -406,43 +406,59 @@ final class HarbormasterBuildEngine extends Phobject {
     $should_publish = $did_update &&
                       $new_status != HarbormasterBuildable::STATUS_BUILDING &&
                       !$buildable->getIsManualBuildable();
-    if ($should_publish) {
-      $object = id(new PhabricatorObjectQuery())
-        ->setViewer($viewer)
-        ->withPHIDs(array($buildable->getBuildablePHID()))
-        ->executeOne();
 
-      if ($object instanceof PhabricatorApplicationTransactionInterface) {
+    if (!$should_publish) {
-        $template = $object->getApplicationTransactionTemplate();
+      return;
-        if ($template) {
-          $template
-            ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE)
-            ->setMetadataValue(
-              'harbormaster:buildablePHID',
-              $buildable->getPHID())
-            ->setOldValue($old_status)
-            ->setNewValue($new_status);
-
-          $harbormaster_phid = id(new PhabricatorHarbormasterApplication())
-            ->getPHID();
-
-          $daemon_source = PhabricatorContentSource::newForSource(
-            PhabricatorContentSource::SOURCE_DAEMON,
-            array());
-
-          $editor = $object->getApplicationTransactionEditor()
-            ->setActor($viewer)
-            ->setActingAsPHID($harbormaster_phid)
-            ->setContentSource($daemon_source)
-            ->setContinueOnNoEffect(true)
-            ->setContinueOnMissingFields(true);
-
-          $editor->applyTransactions(
-            $object->getApplicationTransactionObject(),
-            array($template));
-        }
-      }
     }
+
+    $object = id(new PhabricatorObjectQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($buildable->getBuildablePHID()))
+      ->executeOne();
+    if (!$object) {
+      return;
+    }
+
+    if (!($object instanceof PhabricatorApplicationTransactionInterface)) {
+      return;
+    }
+
+    // TODO: Publishing these transactions is causing a race. See T8650.
+    // We shouldn't be publishing to diffs anyway.
+    if ($object instanceof DifferentialDiff) {
+      return;
+    }
+
+    $template = $object->getApplicationTransactionTemplate();
+    if (!$template) {
+      return;
+    }
+
+    $template
+      ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE)
+      ->setMetadataValue(
+        'harbormaster:buildablePHID',
+        $buildable->getPHID())
+      ->setOldValue($old_status)
+      ->setNewValue($new_status);
+
+    $harbormaster_phid = id(new PhabricatorHarbormasterApplication())
+      ->getPHID();
+
+    $daemon_source = PhabricatorContentSource::newForSource(
+      PhabricatorContentSource::SOURCE_DAEMON,
+      array());
+
+    $editor = $object->getApplicationTransactionEditor()
+      ->setActor($viewer)
+      ->setActingAsPHID($harbormaster_phid)
+      ->setContentSource($daemon_source)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true);
+
+    $editor->applyTransactions(
+      $object->getApplicationTransactionObject(),
+      array($template));
   }
 
   private function releaseAllArtifacts(HarbormasterBuild $build) {