Improve several exception behaviors for Harbormaster workers

Summary:
Ref T2015. Several fixes:

  - `checkForCancellation()` no longer exists, and isn't relevant for resumable stops. Throw it away for now.
  - Fix an issue where a build could pass even if the final step failed.
  - `phlog()` exceptions so they show up in `bin/harbormaster` and the daemon logs.
  - Write an exception log if a step fails.
  - Add a "throw an exception" step to debug this stuff more easily.

Test Plan:
  - Grepped for `checkForCancellation()`.
  - Ran a failing build where the final step caused the failure.
  - Observed `phlog()` in `bin/harbormaster` output.
  - Observed log in web UI:

{F101168}

Reviewers: btrahan, hach-que

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

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

src/__phutil_library_map__.php

@@ -767,6 +767,7 @@ phutil_register_library_map(array(
     'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php',
     'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php',
     'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php',
+    'HarbormasterThrowExceptionBuildStep' => 'applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php',
     'HarbormasterUIEventListener' => 'applications/harbormaster/event/HarbormasterUIEventListener.php',
     'HarbormasterWorker' => 'applications/harbormaster/worker/HarbormasterWorker.php',
     'HeraldAction' => 'applications/herald/storage/HeraldAction.php',
@@ -3260,6 +3261,7 @@ phutil_register_library_map(array(
     'HarbormasterStepDeleteController' => 'HarbormasterController',
     'HarbormasterStepEditController' => 'HarbormasterController',
     'HarbormasterTargetWorker' => 'HarbormasterWorker',
+    'HarbormasterThrowExceptionBuildStep' => 'BuildStepImplementation',
     'HarbormasterUIEventListener' => 'PhabricatorEventListener',
     'HarbormasterWorker' => 'PhabricatorWorker',
     'HeraldAction' => 'HeraldDAO',

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -163,17 +163,17 @@ final class HarbormasterBuildEngine extends Phobject {
       }
     }
 
-    // If every step is complete, we're done with this build. Mark it passed
+    // If any step failed, fail the whole build, then bail.
-    // and bail.
+    if (count($failed)) {
-    if (count($complete) == count($steps)) {
+      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
-      $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED);
       $build->save();
       return;
     }
 
-    // If any step failed, fail the whole build, then bail.
+    // If every step is complete, we're done with this build. Mark it passed
-    if (count($failed)) {
+    // and bail.
-      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
+    if (count($complete) == count($steps)) {
+      $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED);
       $build->save();
       return;
     }

src/applications/harbormaster/step/CommandBuildStepImplementation.php

@@ -53,16 +53,6 @@ final class CommandBuildStepImplementation
       $log_stderr->append($stderr);
       $future->discardBuffers();
 
-      // Check to see if we have moved from a "Building" status.  This
-      // can occur if the user cancels the build, in which case we want
-      // to terminate whatever we're doing and return as quickly as possible.
-      if ($build->checkForCancellation()) {
-        $log_stdout->finalize($start_stdout);
-        $log_stderr->finalize($start_stderr);
-        $future->resolveKill();
-        return;
-      }
-
       // Wait one second before querying for more data.
       sleep(1);
     }
@@ -80,7 +70,7 @@ final class CommandBuildStepImplementation
     $log_stderr->finalize($start_stderr);
 
     if ($err) {
-      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
+      throw new Exception(pht('Command failed with error %d.', $err));
     }
   }
 

src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php

@@ -0,0 +1,25 @@
+<?php
+
+final class HarbormasterThrowExceptionBuildStep
+  extends BuildStepImplementation {
+
+  public function getName() {
+    return pht('Throw Exception');
+  }
+
+  public function getGenericDescription() {
+    return pht('Throw an exception.');
+  }
+
+  public function getDescription() {
+    return pht('Throw an exception.');
+  }
+
+  public function execute(
+    HarbormasterBuild $build,
+    HarbormasterBuildTarget $build_target) {
+
+    throw new Exception(pht('(This is an explicit exception.)'));
+  }
+
+}

src/applications/harbormaster/step/WaitForPreviousBuildStepImplementation.php

@@ -30,10 +30,6 @@ final class WaitForPreviousBuildStepImplementation
       return;
     }
 
-    // We are blocked until all previous builds finish.
-    $build->setBuildStatus(HarbormasterBuild::STATUS_WAITING);
-    $build->save();
-
     // Block until all previous builds of the same build plan have
     // finished.
     $plan = $build->getBuildPlan();
@@ -42,13 +38,6 @@ final class WaitForPreviousBuildStepImplementation
     $log_start = null;
     $blockers = $this->getBlockers($object, $plan, $build);
     while (count($blockers) > 0) {
-      if ($build->checkForCancellation()) {
-        if ($log !== null) {
-          $log->finalize($log_start);
-        }
-        return;
-      }
-
       if ($log === null) {
         $log = $build->createLog($build_target, "waiting", "blockers");
         $log_start = $log->start();
@@ -56,16 +45,14 @@ final class WaitForPreviousBuildStepImplementation
 
       $log->append("Blocked by: ".implode(",", $blockers)."\n");
 
+      // TODO: This should fail temporarily instead after setting the target to
+      // waiting, and thereby push the build into a waiting status.
       sleep(1);
       $blockers = $this->getBlockers($object, $plan, $build);
     }
     if ($log !== null) {
       $log->finalize($log_start);
     }
-
-    // Move back into building status.
-    $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING);
-    $build->save();
   }
 
   private function getBlockers(

src/applications/harbormaster/worker/HarbormasterTargetWorker.php

@@ -46,6 +46,17 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
         $target->save();
       }
     } catch (Exception $ex) {
+      phlog($ex);
+
+      try {
+        $log = $build->createLog($target, 'core', 'exception');
+        $start = $log->start();
+        $log->append((string)$ex);
+        $log->finalize($start);
+      } catch (Exception $log_ex) {
+        phlog($log_ex);
+      }
+
       $target->setTargetStatus(HarbormasterBuildTarget::STATUS_FAILED);
       $target->save();
     }