Correct the flow of edit authority when sending messages to HarbormasterBuild objects

Summary:
Ref T13072. Currently, Harbormaster builds react to messages by applying a transaction inline (which can race) that has no real effect.

Later, the BuildEngine picks up the mesasge and applies a real effect, but this isn't transactional.

This is backwards, and makes it more difficult to transition to ModularTransaction and EditEngine. The desired workflow is:

  - sending a message //just// writes to the message table (and queues a worker to process the message);
  - the BuildEngine processes the message and applies effects in a transactional way.

Force this into at least roughly the right sequence of behaviors. This paves the way for porting to ModularTransaction, which should allow further cleanup.

Test Plan: Paused, resumed, aborted, and restarted a build. Ran BuildWorkers to process the commands, saw builds update appropriately.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13072

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

src/applications/harbormaster/constants/HarbormasterBuildStatus.php

@@ -53,10 +53,6 @@ final class HarbormasterBuildStatus extends Phobject {
     return $this->getProperty('isComplete');
   }
 
-  public function isPending() {
-    return $this->getProperty('isPending');
-  }
-
   public function isPassed() {
     return ($this->key === self::STATUS_PASSED);
   }
@@ -81,6 +77,10 @@ final class HarbormasterBuildStatus extends Phobject {
     return ($this->key === self::PENDING_PAUSING);
   }
 
+  public function isPending() {
+    return ($this->key === self::STATUS_PENDING);
+  }
+
   public function getIconIcon() {
     return $this->getProperty('icon');
   }
@@ -170,7 +170,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => false,
       ),
       self::STATUS_PENDING => array(
         'name' => pht('Pending'),
@@ -179,7 +178,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => true,
         'isComplete' => false,
-        'isPending' => false,
       ),
       self::STATUS_BUILDING => array(
         'name' => pht('Building'),
@@ -188,7 +186,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => true,
         'isComplete' => false,
-        'isPending' => false,
       ),
       self::STATUS_PASSED => array(
         'name' => pht('Passed'),
@@ -197,7 +194,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'green',
         'isBuilding' => false,
         'isComplete' => true,
-        'isPending' => false,
       ),
       self::STATUS_FAILED => array(
         'name' => pht('Failed'),
@@ -206,7 +202,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
-        'isPending' => false,
       ),
       self::STATUS_ABORTED => array(
         'name' => pht('Aborted'),
@@ -215,7 +210,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
-        'isPending' => false,
       ),
       self::STATUS_ERROR => array(
         'name' => pht('Unexpected Error'),
@@ -224,7 +218,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
-        'isPending' => false,
       ),
       self::STATUS_PAUSED => array(
         'name' => pht('Paused'),
@@ -233,7 +226,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => false,
       ),
       self::STATUS_DEADLOCKED => array(
         'name' => pht('Deadlocked'),
@@ -242,7 +234,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
-        'isPending' => false,
       ),
       self::PENDING_PAUSING => array(
         'name' => pht('Pausing'),
@@ -251,7 +242,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => true,
       ),
       self::PENDING_RESUMING => array(
         'name' => pht('Resuming'),
@@ -260,7 +250,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => true,
       ),
       self::PENDING_RESTARTING => array(
         'name' => pht('Restarting'),
@@ -269,7 +258,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => true,
       ),
       self::PENDING_ABORTING => array(
         'name' => pht('Aborting'),
@@ -278,7 +266,6 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => false,
-        'isPending' => true,
       ),
     );
   }

src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php

@@ -65,48 +65,32 @@ final class HarbormasterBuildTransactionEditor
     HarbormasterBuild $build,
     HarbormasterBuildTransaction $xaction) {
 
-    $command = $xaction->getNewValue();
+    $actor = $this->getActor();
+    $message_type = $xaction->getNewValue();
 
-    switch ($command) {
-      case HarbormasterBuildCommand::COMMAND_RESTART:
-        $issuable = $build->canRestartBuild();
+    // TODO: Restore logic that tests if the command can issue without causing
+    // anything to lapse into an invalid state. This should not be the same
+    // as the logic which powers the web UI: for example, if an "abort" is
+    // queued we want to disable "Abort" in the web UI, but should obviously
+    // process it here.
+
+    switch ($message_type) {
+      case HarbormasterBuildCommand::COMMAND_ABORT:
+        // TODO: This should move to external effects, perhaps.
+        $build->releaseAllArtifacts($actor);
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED);
         break;
-      case HarbormasterBuildCommand::COMMAND_PAUSE:
-        $issuable = $build->canPauseBuild();
+      case HarbormasterBuildCommand::COMMAND_RESTART:
+        $build->restartBuild($actor);
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
         break;
       case HarbormasterBuildCommand::COMMAND_RESUME:
-        $issuable = $build->canResumeBuild();
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
         break;
-      case HarbormasterBuildCommand::COMMAND_ABORT:
-        $issuable = $build->canAbortBuild();
+      case HarbormasterBuildCommand::COMMAND_PAUSE:
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED);
         break;
-      default:
-        throw new Exception(pht('Unknown command %s', $command));
     }
-
-    if (!$issuable) {
-      return;
-    }
-
-    $actor = $this->getActor();
-    if (!$build->canIssueCommand($actor, $command)) {
-      return;
-    }
-
-    HarbormasterBuildMessage::initializeNewMessage($actor)
-      ->setAuthorPHID($xaction->getAuthorPHID())
-      ->setReceiverPHID($build->getPHID())
-      ->setType($command)
-      ->save();
-
-    PhabricatorWorker::scheduleTask(
-      'HarbormasterBuildWorker',
-      array(
-        'buildID' => $build->getID(),
-      ),
-      array(
-        'objectPHID' => $build->getPHID(),
-      ));
   }
 
   protected function applyCustomExternalTransaction(

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -49,6 +49,7 @@ final class HarbormasterBuildEngine extends Phobject {
   }
 
   public function continueBuild() {
+    $viewer = $this->getViewer();
     $build = $this->getBuild();
 
     $lock_key = 'harbormaster.build:'.$build->getID();
@@ -68,7 +69,7 @@ final class HarbormasterBuildEngine extends Phobject {
 
       $lock->unlock();
 
-      $this->releaseAllArtifacts($build);
+      $build->releaseAllArtifacts($viewer);
 
       throw $ex;
     }
@@ -99,58 +100,60 @@ final class HarbormasterBuildEngine extends Phobject {
 
     // If we are no longer building for any reason, release all artifacts.
     if (!$build->isBuilding()) {
-      $this->releaseAllArtifacts($build);
+      $build->releaseAllArtifacts($viewer);
     }
   }
 
   private function updateBuild(HarbormasterBuild $build) {
-    if ($build->isAborting()) {
-      $this->releaseAllArtifacts($build);
-      $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED);
-      $build->save();
+    $viewer = $this->getViewer();
+
+    $content_source = PhabricatorContentSource::newForSource(
+      PhabricatorDaemonContentSource::SOURCECONST);
+
+    $acting_phid = $viewer->getPHID();
+    if (!$acting_phid) {
+      $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID();
     }
 
-    if (($build->getBuildStatus() == HarbormasterBuildStatus::STATUS_PENDING) ||
-        ($build->isRestarting())) {
-      $this->restartBuild($build);
-      $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
-      $build->save();
+    $editor = $build->getApplicationTransactionEditor()
+      ->setActor($viewer)
+      ->setActingAsPHID($acting_phid)
+      ->setContentSource($content_source)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true);
+
+    $xactions = array();
+
+    $messages = $build->getUnprocessedMessagesForApply();
+    foreach ($messages as $message) {
+      $message_type = $message->getType();
+
+      $xactions[] = $build->getApplicationTransactionTemplate()
+        ->setAuthorPHID($message->getAuthorPHID())
+        ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
+        ->setNewValue($message_type);
     }
 
-    if ($build->isResuming()) {
-      $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
-      $build->save();
+    if (!$xactions) {
+      if ($build->isPending()) {
+        // TODO: This should be a transaction.
+
+        $build->restartBuild($viewer);
+        $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING);
+        $build->save();
+      }
     }
 
-    if ($build->isPausing() && !$build->isComplete()) {
-      $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED);
-      $build->save();
+    if ($xactions) {
+      $editor->applyTransactions($build, $xactions);
+      $build->markUnprocessedMessagesAsProcessed();
     }
 
-    $build->markUnprocessedMessagesAsProcessed();
-
     if ($build->getBuildStatus() == HarbormasterBuildStatus::STATUS_BUILDING) {
       $this->updateBuildSteps($build);
     }
   }
 
-  private function restartBuild(HarbormasterBuild $build) {
-
-    // We're restarting the build, so release all previous artifacts.
-    $this->releaseAllArtifacts($build);
-
-    // Increment the build generation counter on the build.
-    $build->setBuildGeneration($build->getBuildGeneration() + 1);
-
-    // Currently running targets should periodically check their build
-    // generation (which won't have changed) against the build's generation.
-    // If it is different, they will automatically stop what they're doing
-    // and abort.
-
-    // Previously we used to delete targets, logs and artifacts here. Instead,
-    // leave them around so users can view previous generations of this build.
-  }
-
   private function updateBuildSteps(HarbormasterBuild $build) {
     $all_targets = id(new HarbormasterBuildTargetQuery())
       ->setViewer($this->getViewer())
@@ -596,29 +599,6 @@ final class HarbormasterBuildEngine extends Phobject {
       ->publishBuildable($old, $new);
   }
 
-  private function releaseAllArtifacts(HarbormasterBuild $build) {
-    $targets = id(new HarbormasterBuildTargetQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withBuildPHIDs(array($build->getPHID()))
-      ->withBuildGenerations(array($build->getBuildGeneration()))
-      ->execute();
-
-    if (count($targets) === 0) {
-      return;
-    }
-
-    $target_phids = mpull($targets, 'getPHID');
-
-    $artifacts = id(new HarbormasterBuildArtifactQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withBuildTargetPHIDs($target_phids)
-      ->withIsReleased(false)
-      ->execute();
-    foreach ($artifacts as $artifact) {
-      $artifact->releaseArtifact();
-    }
-  }
-
   private function releaseQueuedArtifacts() {
     foreach ($this->artifactReleaseQueue as $key => $artifact) {
       $artifact->releaseArtifact();

src/applications/harbormaster/storage/build/HarbormasterBuild.php

@@ -207,61 +207,17 @@ final class HarbormasterBuild extends HarbormasterDAO
     return $this->getBuildStatusObject()->isFailed();
   }
 
+  public function isPending() {
+    return $this->getBuildstatusObject()->isPending();
+  }
+
   public function getURI() {
     $id = $this->getID();
     return "/harbormaster/build/{$id}/";
   }
 
   public function getBuildPendingStatusObject() {
-
-    // NOTE: If a build has multiple unprocessed messages, we'll ignore
-    // messages that are obsoleted by a later or stronger message.
-    //
-    // For example, if a build has both "pause" and "abort" messages in queue,
-    // we just ignore the "pause" message and perform an "abort", since pausing
-    // first wouldn't affect the final state, so we can just skip it.
-    //
-    // Likewise, if a build has both "restart" and "abort" messages, the most
-    // recent message is controlling: we'll take whichever action a command
-    // was most recently issued for.
-
-    $is_restarting = false;
-    $is_aborting = false;
-    $is_pausing = false;
-    $is_resuming = false;
-
-    foreach ($this->getUnprocessedMessages() as $message_object) {
-      $message_type = $message_object->getType();
-      switch ($message_type) {
-        case HarbormasterBuildCommand::COMMAND_RESTART:
-          $is_restarting = true;
-          $is_aborting = false;
-          break;
-        case HarbormasterBuildCommand::COMMAND_ABORT:
-          $is_aborting = true;
-          $is_restarting = false;
-          break;
-        case HarbormasterBuildCommand::COMMAND_PAUSE:
-          $is_pausing = true;
-          $is_resuming = false;
-          break;
-        case HarbormasterBuildCommand::COMMAND_RESUME:
-          $is_resuming = true;
-          $is_pausing = false;
-          break;
-      }
-    }
-
-    $pending_status = null;
-    if ($is_restarting) {
-      $pending_status = HarbormasterBuildStatus::PENDING_RESTARTING;
-    } else if ($is_aborting) {
-      $pending_status = HarbormasterBuildStatus::PENDING_ABORTING;
-    } else if ($is_pausing) {
-      $pending_status = HarbormasterBuildStatus::PENDING_PAUSING;
-    } else if ($is_resuming) {
-      $pending_status = HarbormasterBuildStatus::PENDING_RESUMING;
-    }
+    list($pending_status) = $this->getUnprocessedMessageState();
 
     if ($pending_status !== null) {
       return HarbormasterBuildStatus::newBuildStatusObject($pending_status);
@@ -287,6 +243,72 @@ final class HarbormasterBuild extends HarbormasterDAO
     return $this->assertAttached($this->unprocessedMessages);
   }
 
+  public function getUnprocessedMessagesForApply() {
+    $unprocessed_state = $this->getUnprocessedMessageState();
+    list($pending_status, $apply_messages) = $unprocessed_state;
+
+    return $apply_messages;
+  }
+
+  private function getUnprocessedMessageState() {
+    // NOTE: If a build has multiple unprocessed messages, we'll ignore
+    // messages that are obsoleted by a later or stronger message.
+    //
+    // For example, if a build has both "pause" and "abort" messages in queue,
+    // we just ignore the "pause" message and perform an "abort", since pausing
+    // first wouldn't affect the final state, so we can just skip it.
+    //
+    // Likewise, if a build has both "restart" and "abort" messages, the most
+    // recent message is controlling: we'll take whichever action a command
+    // was most recently issued for.
+
+    $is_restarting = false;
+    $is_aborting = false;
+    $is_pausing = false;
+    $is_resuming = false;
+
+    $apply_messages = array();
+
+    foreach ($this->getUnprocessedMessages() as $message_object) {
+      $message_type = $message_object->getType();
+      switch ($message_type) {
+        case HarbormasterBuildCommand::COMMAND_RESTART:
+          $is_restarting = true;
+          $is_aborting = false;
+          $apply_messages = array($message_object);
+          break;
+        case HarbormasterBuildCommand::COMMAND_ABORT:
+          $is_aborting = true;
+          $is_restarting = false;
+          $apply_messages = array($message_object);
+          break;
+        case HarbormasterBuildCommand::COMMAND_PAUSE:
+          $is_pausing = true;
+          $is_resuming = false;
+          $apply_messages = array($message_object);
+          break;
+        case HarbormasterBuildCommand::COMMAND_RESUME:
+          $is_resuming = true;
+          $is_pausing = false;
+          $apply_messages = array($message_object);
+          break;
+      }
+    }
+
+    $pending_status = null;
+    if ($is_restarting) {
+      $pending_status = HarbormasterBuildStatus::PENDING_RESTARTING;
+    } else if ($is_aborting) {
+      $pending_status = HarbormasterBuildStatus::PENDING_ABORTING;
+    } else if ($is_pausing) {
+      $pending_status = HarbormasterBuildStatus::PENDING_PAUSING;
+    } else if ($is_resuming) {
+      $pending_status = HarbormasterBuildStatus::PENDING_RESUMING;
+    }
+
+    return array($pending_status, $apply_messages);
+  }
+
   public function attachUnprocessedMessages(array $messages) {
     assert_instances_of($messages, 'HarbormasterBuildMessage');
     $this->unprocessedMessages = $messages;
@@ -475,32 +497,61 @@ final class HarbormasterBuild extends HarbormasterDAO
   }
 
   public function sendMessage(PhabricatorUser $viewer, $message_type) {
-    // TODO: This should not be an editor transaction, but there are plans to
-    // merge BuildCommand into BuildMessage which should moot this. As this
-    // exists today, it can race against BuildEngine.
+    HarbormasterBuildMessage::initializeNewMessage($viewer)
+      ->setReceiverPHID($this->getPHID())
+      ->setType($message_type)
+      ->save();
 
-    // This is a bogus content source, but this whole flow should be obsolete
-    // soon.
-    $content_source = PhabricatorContentSource::newForSource(
-      PhabricatorConsoleContentSource::SOURCECONST);
+    PhabricatorWorker::scheduleTask(
+      'HarbormasterBuildWorker',
+      array(
+        'buildID' => $this->getID(),
+      ),
+      array(
+        'objectPHID' => $this->getPHID(),
+        'containerPHID' => $this->getBuildablePHID(),
+      ));
+  }
 
-    $editor = id(new HarbormasterBuildTransactionEditor())
-      ->setActor($viewer)
-      ->setContentSource($content_source)
-      ->setContinueOnNoEffect(true)
-      ->setContinueOnMissingFields(true);
+  public function releaseAllArtifacts(PhabricatorUser $viewer) {
+    $targets = id(new HarbormasterBuildTargetQuery())
+      ->setViewer($viewer)
+      ->withBuildPHIDs(array($this->getPHID()))
+      ->withBuildGenerations(array($this->getBuildGeneration()))
+      ->execute();
 
-    $viewer_phid = $viewer->getPHID();
-    if (!$viewer_phid) {
-      $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID();
-      $editor->setActingAsPHID($acting_phid);
+    if (!$targets) {
+      return;
     }
 
-    $xaction = id(new HarbormasterBuildTransaction())
-      ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
-      ->setNewValue($message_type);
+    $target_phids = mpull($targets, 'getPHID');
 
-    $editor->applyTransactions($this, array($xaction));
+    $artifacts = id(new HarbormasterBuildArtifactQuery())
+      ->setViewer($viewer)
+      ->withBuildTargetPHIDs($target_phids)
+      ->withIsReleased(false)
+      ->execute();
+    foreach ($artifacts as $artifact) {
+      $artifact->releaseArtifact();
+    }
+  }
+
+  public function restartBuild(PhabricatorUser $viewer) {
+    // TODO: This should become transactional.
+
+    // We're restarting the build, so release all previous artifacts.
+    $this->releaseAllArtifacts($viewer);
+
+    // Increment the build generation counter on the build.
+    $this->setBuildGeneration($this->getBuildGeneration() + 1);
+
+    // Currently running targets should periodically check their build
+    // generation (which won't have changed) against the build's generation.
+    // If it is different, they will automatically stop what they're doing
+    // and abort.
+
+    // Previously we used to delete targets, logs and artifacts here. Instead,
+    // leave them around so users can view previous generations of this build.
   }