Add an "Abort Older Builds" build step to Harbormaster

Summary:
Ref T13124. See PHI531. When a revision is updated, builds against the older diff tend to stop being relevant. Add an option to abort outstanding older builds automatically.

At least for now, I'm adding this as a build step instead of some kind of special checkbox. An alternate implementation would be some kind of "Edit Options" action on plans with a checkbox like `[X] When this build starts, abort older builds.`

I think adding it as a build step is a bit simpler, and likely to lead to greater consistency and flexibility down the road, make it easier to add options, etc., and since we don't really have any other current use cases for "a bunch of checkboxes". This might change eventually if we add a bunch of checkboxes for some other reason.

The actual step activates //before// the build queues, so it doesn't need to wait in queue before it can actually act. T13088 discusses some plans here if this sticks.

Test Plan:
  - Created a "Sleep for 120 seconds" build plan and triggered it with Herald.
  - Added an "Abort Older Builds" step.
  - Updated a revision several times in a row, saw older builds abort.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13124

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

src/__phutil_library_map__.php

@@ -1263,6 +1263,7 @@ phutil_register_library_map(array(
     'FundInitiativeTransactionType' => 'applications/fund/xaction/FundInitiativeTransactionType.php',
     'FundInitiativeViewController' => 'applications/fund/controller/FundInitiativeViewController.php',
     'FundSchemaSpec' => 'applications/fund/storage/FundSchemaSpec.php',
+    'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php',
     'HarbormasterArcLintBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php',
     'HarbormasterArcUnitBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php',
     'HarbormasterArtifact' => 'applications/harbormaster/artifact/HarbormasterArtifact.php',
@@ -1358,6 +1359,7 @@ phutil_register_library_map(array(
     'HarbormasterCircleCIBuildableInterface' => 'applications/harbormaster/interface/HarbormasterCircleCIBuildableInterface.php',
     'HarbormasterCircleCIHookController' => 'applications/harbormaster/controller/HarbormasterCircleCIHookController.php',
     'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php',
+    'HarbormasterControlBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php',
     'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php',
     'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php',
@@ -6636,6 +6638,7 @@ phutil_register_library_map(array(
     'FundInitiativeTransactionType' => 'PhabricatorModularTransactionType',
     'FundInitiativeViewController' => 'FundController',
     'FundSchemaSpec' => 'PhabricatorConfigSchemaSpec',
+    'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArcLintBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArcUnitBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArtifact' => 'Phobject',
@@ -6771,6 +6774,7 @@ phutil_register_library_map(array(
     'HarbormasterCircleCIBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterCircleCIHookController' => 'HarbormasterController',
     'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod',
+    'HarbormasterControlBuildStepGroup' => 'HarbormasterBuildStepGroup',
     'HarbormasterController' => 'PhabricatorController',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod',
     'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability',

src/applications/harbormaster/constants/HarbormasterBuildStatus.php

@@ -98,6 +98,19 @@ final class HarbormasterBuildStatus extends Phobject {
     );
   }
 
+  public static function getIncompleteStatusConstants() {
+    $map = self::getBuildStatusSpecMap();
+
+    $constants = array();
+    foreach ($map as $constant => $spec) {
+      if (!$spec['isComplete']) {
+        $constants[] = $constant;
+      }
+    }
+
+    return $constants;
+  }
+
   public static function getCompletedStatusConstants() {
     return array(
       self::STATUS_PASSED,

src/applications/harbormaster/controller/HarbormasterBuildActionController.php

@@ -51,18 +51,7 @@ final class HarbormasterBuildActionController
     }
 
     if ($request->isDialogFormPost() && $can_issue) {
-      $editor = id(new HarbormasterBuildTransactionEditor())
-        ->setActor($viewer)
-        ->setContentSourceFromRequest($request)
-        ->setContinueOnNoEffect(true)
-        ->setContinueOnMissingFields(true);
-
-      $xaction = id(new HarbormasterBuildTransaction())
-        ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
-        ->setNewValue($action);
-
-      $editor->applyTransactions($build, array($xaction));
-
+      $build->sendMessage($viewer, $action);
       return id(new AphrontRedirectResponse())->setURI($return_uri);
     }
 

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

@@ -22,48 +22,39 @@ final class HarbormasterBuildStepQuery
     return $this;
   }
 
-  protected function loadPage() {
-    $table = new HarbormasterBuildStep();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+  public function newResultObject() {
+    return new HarbormasterBuildStep();
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
-    $where = array();
+  protected function loadPage() {
+    return $this->loadStandardPage($this->newResultObject());
+  }
 
-    if ($this->ids) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+
+    if ($this->ids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'phid in (%Ls)',
         $this->phids);
     }
 
-    if ($this->buildPlanPHIDs) {
+    if ($this->buildPlanPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'buildPlanPHID in (%Ls)',
         $this->buildPlanPHIDs);
     }
 
-    $where[] = $this->buildPagingClause($conn_r);
-
-    return $this->formatWhereClause($where);
+    return $where;
   }
 
   protected function willFilterPage(array $page) {

src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php

@@ -0,0 +1,135 @@
+<?php
+
+final class HarbormasterAbortOlderBuildsBuildStepImplementation
+  extends HarbormasterBuildStepImplementation {
+
+  public function getName() {
+    return pht('Abort Older Builds');
+  }
+
+  public function getGenericDescription() {
+    return pht(
+      'When building a revision, abort copies of this build plan which are '.
+      'currently running against older diffs.');
+  }
+
+  public function getBuildStepGroupKey() {
+    return HarbormasterControlBuildStepGroup::GROUPKEY;
+  }
+
+  public function getEditInstructions() {
+    return pht(<<<EOTEXT
+When run against a revision, this build step will abort any older copies of
+the same build plan which are currently running against older diffs.
+
+There are some nuances to the behavior:
+
+  - if this build step is triggered manually, it won't abort anything;
+  - this build step won't abort manual builds;
+  - this build step won't abort anything if the diff it is building isn't
+    the active diff when it runs.
+
+Build results on outdated diffs often aren't very important, so this may
+reduce build queue load without any substantial cost.
+EOTEXT
+      );
+  }
+
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan,
+    HarbormasterBuildStep $step) {
+
+    if ($buildable->getIsManualBuildable()) {
+      // Don't abort anything if this is a manual buildable.
+      return;
+    }
+
+    $object_phid = $buildable->getBuildablePHID();
+    if (phid_get_type($object_phid) !== DifferentialDiffPHIDType::TYPECONST) {
+      // If this buildable isn't building a diff, bail out. For example, we
+      // might be building a commit. In this case, this step has no effect.
+      return;
+    }
+
+    $diff = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($object_phid))
+      ->executeOne();
+    if (!$diff) {
+      return;
+    }
+
+    $revision_id = $diff->getRevisionID();
+
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($revision_id))
+      ->executeOne();
+    if (!$revision) {
+      return;
+    }
+
+    $active_phid = $revision->getActiveDiffPHID();
+    if ($active_phid !== $object_phid) {
+      // If we aren't building the active diff, bail out.
+      return;
+    }
+
+    $diffs = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withRevisionIDs(array($revision_id))
+      ->execute();
+    $abort_diff_phids = array();
+    foreach ($diffs as $diff) {
+      if ($diff->getPHID() !== $active_phid) {
+        $abort_diff_phids[] = $diff->getPHID();
+      }
+    }
+
+    if (!$abort_diff_phids) {
+      return;
+    }
+
+    // We're fetching buildables even if they have "passed" or "failed"
+    // because they may still have ongoing builds. At the time of writing
+    // only "failed" buildables may still be ongoing, but it seems likely that
+    // "passed" buildables may be ongoing in the future.
+
+    $abort_buildables = id(new HarbormasterBuildableQuery())
+      ->setViewer($viewer)
+      ->withBuildablePHIDs($abort_diff_phids)
+      ->withManualBuildables(false)
+      ->execute();
+    if (!$abort_buildables) {
+      return;
+    }
+
+    $statuses = HarbormasterBuildStatus::getIncompleteStatusConstants();
+
+    $abort_builds = id(new HarbormasterBuildQuery())
+      ->setViewer($viewer)
+      ->withBuildablePHIDs(mpull($abort_buildables, 'getPHID'))
+      ->withBuildPlanPHIDs(array($plan->getPHID()))
+      ->withBuildStatuses($statuses)
+      ->execute();
+    if (!$abort_builds) {
+      return;
+    }
+
+    foreach ($abort_builds as $abort_build) {
+      $abort_build->sendMessage(
+        $viewer,
+        HarbormasterBuildCommand::COMMAND_ABORT);
+    }
+  }
+
+  public function execute(
+    HarbormasterBuild $build,
+    HarbormasterBuildTarget $build_target) {
+    return;
+  }
+
+}

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php

@@ -308,6 +308,15 @@ abstract class HarbormasterBuildStepImplementation extends Phobject {
           'enabled in configuration.'));
   }
 
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan,
+    HarbormasterBuildStep $step) {
+    return;
+  }
+
 
 /* -(  Automatic Targets  )-------------------------------------------------- */
 

src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php

@@ -0,0 +1,20 @@
+<?php
+
+final class HarbormasterControlBuildStepGroup
+  extends HarbormasterBuildStepGroup {
+
+  const GROUPKEY = 'harbormaster.control';
+
+  public function getGroupName() {
+    return pht('Flow Control');
+  }
+
+  public function getGroupOrder() {
+    return 5000;
+  }
+
+  public function shouldShowIfEmpty() {
+    return false;
+  }
+
+}

src/applications/harbormaster/storage/HarbormasterBuildable.php

@@ -141,6 +141,15 @@ final class HarbormasterBuildable
 
     $build->save();
 
+    $steps = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withBuildPlanPHIDs(array($plan->getPHID()))
+      ->execute();
+
+    foreach ($steps as $step) {
+      $step->willStartBuild($viewer, $this, $build, $plan);
+    }
+
     PhabricatorWorker::scheduleTask(
       'HarbormasterBuildWorker',
       array(

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

@@ -356,6 +356,35 @@ final class HarbormasterBuild extends HarbormasterDAO
     }
   }
 
+  public function sendMessage(PhabricatorUser $viewer, $command) {
+    // 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.
+
+    // This is a bogus content source, but this whole flow should be obsolete
+    // soon.
+    $content_source = PhabricatorContentSource::newForSource(
+      PhabricatorConsoleContentSource::SOURCECONST);
+
+    $editor = id(new HarbormasterBuildTransactionEditor())
+      ->setActor($viewer)
+      ->setContentSource($content_source)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true);
+
+    $viewer_phid = $viewer->getPHID();
+    if (!$viewer_phid) {
+      $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID();
+      $editor->setActingAsPHID($acting_phid);
+    }
+
+    $xaction = id(new HarbormasterBuildTransaction())
+      ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
+      ->setNewValue($command);
+
+    $editor->applyTransactions($this, array($xaction));
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

@@ -100,6 +100,19 @@ final class HarbormasterBuildStep extends HarbormasterDAO
     return ($this->getStepAutoKey() !== null);
   }
 
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan) {
+    return $this->getStepImplementation()->willStartBuild(
+      $viewer,
+      $buildable,
+      $build,
+      $plan,
+      $this);
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */