Add more selectors to existing "bin/worker" commands

Summary:
Ref T13591. Add more selector flags to let "bin/worker" commands operate on tasks by container PHID, object PHID, priority, etc.

This anticipates adding "bin/worker reprioritize" and "bin/worker delay" workflows, to provide more tools for handling repository imports.

Test Plan:
  - Ran `bin/worker execute`, `cancel`, `retry`, and `free` with various sets of selector flags.
  - Used `--min-priority`, `--max-priority`, `--object`, `--container`, `--archived`, `--max-failure-count` to select tasks.
  - Specified invalid, duplicate, aliased objects with "--object".
  - Specified invalid range priority selectors.

Maniphest Tasks: T13591

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

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementCancelWorkflow.php

@@ -6,7 +6,7 @@ final class PhabricatorWorkerManagementCancelWorkflow
   protected function didConstruct() {
     $this
       ->setName('cancel')
-      ->setExamples('**cancel** --id __id__')
+      ->setExamples('**cancel** __selectors__')
       ->setSynopsis(
         pht(
           'Cancel selected tasks. The work these tasks represent will never '.
@@ -15,14 +15,21 @@ final class PhabricatorWorkerManagementCancelWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
     $tasks = $this->loadTasks($args);
 
+    if (!$tasks) {
+      $this->logWarn(
+        pht('NO TASKS'),
+        pht('No tasks selected to cancel.'));
+
+      return 0;
+    }
+
+    $cancel_count = 0;
     foreach ($tasks as $task) {
       $can_cancel = !$task->isArchived();
       if (!$can_cancel) {
-        $console->writeOut(
+        $this->logWarn(
-          "**<bg:yellow> %s </bg>** %s\n",
           pht('ARCHIVED'),
           pht(
             '%s is already archived, and can not be cancelled.',
@@ -32,20 +39,25 @@ final class PhabricatorWorkerManagementCancelWorkflow
 
       // Forcibly break the lease if one exists, so we can archive the
       // task.
-      $task->setLeaseOwner(null);
+      $task
-      $task->setLeaseExpires(PhabricatorTime::getNow());
+        ->setLeaseOwner(null)
-      $task->archiveTask(
+        ->setLeaseExpires(PhabricatorTime::getNow());
-        PhabricatorWorkerArchiveTask::RESULT_CANCELLED,
-        0);
 
-      $console->writeOut(
+      $task->archiveTask(PhabricatorWorkerArchiveTask::RESULT_CANCELLED, 0);
-        "**<bg:green> %s </bg>** %s\n",
+
+      $this->logInfo(
         pht('CANCELLED'),
         pht(
           '%s was cancelled.',
           $this->describeTask($task)));
+
+      $cancel_count++;
     }
 
+    $this->logOkay(
+      pht('DONE'),
+      pht('Cancelled %s task(s).', new PhutilNumber($cancel_count)));
+
     return 0;
   }
 

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php

@@ -6,7 +6,7 @@ final class PhabricatorWorkerManagementExecuteWorkflow
   protected function didConstruct() {
     $this
       ->setName('execute')
-      ->setExamples('**execute** --id __id__')
+      ->setExamples('**execute** __selectors__')
       ->setSynopsis(
         pht(
           'Execute a task explicitly. This command ignores leases, is '.
@@ -27,18 +27,24 @@ final class PhabricatorWorkerManagementExecuteWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
-    $tasks = $this->loadTasks($args);
-
     $is_retry = $args->getArg('retry');
     $is_repeat = $args->getArg('repeat');
 
+    $tasks = $this->loadTasks($args);
+    if (!$tasks) {
+      $this->logWarn(
+        pht('NO TASKS'),
+        pht('No tasks selected to execute.'));
+
+      return 0;
+    }
+
+    $execute_count = 0;
     foreach ($tasks as $task) {
       $can_execute = !$task->isArchived();
       if (!$can_execute) {
         if (!$is_retry) {
-          $console->writeOut(
+          $this->logWarn(
-            "**<bg:yellow> %s </bg>** %s\n",
             pht('ARCHIVED'),
             pht(
               '%s is already archived, and will not be executed. '.
@@ -50,8 +56,7 @@ final class PhabricatorWorkerManagementExecuteWorkflow
         $result_success = PhabricatorWorkerArchiveTask::RESULT_SUCCESS;
         if ($task->getResult() == $result_success) {
           if (!$is_repeat) {
-            $console->writeOut(
+            $this->logWarn(
-              "**<bg:yellow> %s </bg>** %s\n",
               pht('SUCCEEDED'),
               pht(
                 '%s has already succeeded, and will not be retried. '.
@@ -61,9 +66,8 @@ final class PhabricatorWorkerManagementExecuteWorkflow
           }
         }
 
-        echo tsprintf(
+        $this->logInfo(
-          "**<bg:yellow> %s </bg>** %s\n",
+          pht('UNARCHIVING'),
-          pht('ARCHIVED'),
           pht(
             'Unarchiving %s.',
             $this->describeTask($task)));
@@ -74,30 +78,36 @@ final class PhabricatorWorkerManagementExecuteWorkflow
       // NOTE: This ignores leases, maybe it should respect them without
       // a parameter like --force?
 
-      $task->setLeaseOwner(null);
+      $task
-      $task->setLeaseExpires(PhabricatorTime::getNow());
+        ->setLeaseOwner(null)
-      $task->save();
+        ->setLeaseExpires(PhabricatorTime::getNow())
+        ->save();
 
       $task_data = id(new PhabricatorWorkerTaskData())->loadOneWhere(
         'id = %d',
         $task->getDataID());
       $task->setData($task_data->getData());
 
-      echo tsprintf(
+      $this->logInfo(
-        "%s\n",
+        pht('EXECUTE'),
         pht(
-          'Executing task %d (%s)...',
+          'Executing %s...',
-          $task->getID(),
+          $this->describeTask($task)));
-          $task->getTaskClass()));
 
       $task = $task->executeTask();
-      $ex = $task->getExecutionException();
 
+      $ex = $task->getExecutionException();
       if ($ex) {
         throw $ex;
       }
+
+      $execute_count++;
     }
 
+    $this->logOkay(
+      pht('DONE'),
+      pht('Executed %s task(s).', new PhutilNumber($execute_count)));
+
     return 0;
   }
 

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementFreeWorkflow.php

@@ -6,7 +6,7 @@ final class PhabricatorWorkerManagementFreeWorkflow
   protected function didConstruct() {
     $this
       ->setName('free')
-      ->setExamples('**free** --id __id__')
+      ->setExamples('**free** __selectors__')
       ->setSynopsis(
         pht(
           'Free leases on selected tasks. If the daemon holding the lease is '.
@@ -16,13 +16,20 @@ final class PhabricatorWorkerManagementFreeWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
     $tasks = $this->loadTasks($args);
 
+    if (!$tasks) {
+      $this->logWarn(
+        pht('NO TASKS'),
+        pht('No tasks selected to free leases on.'));
+
+      return 0;
+    }
+
+    $free_count = 0;
     foreach ($tasks as $task) {
       if ($task->isArchived()) {
-        $console->writeOut(
+        $this->logWarn(
-          "**<bg:yellow> %s </bg>** %s\n",
           pht('ARCHIVED'),
           pht(
             '%s is archived; archived tasks do not have leases.',
@@ -31,8 +38,7 @@ final class PhabricatorWorkerManagementFreeWorkflow
       }
 
       if ($task->getLeaseOwner() === null) {
-        $console->writeOut(
+        $this->logWarn(
-          "**<bg:yellow> %s </bg>** %s\n",
           pht('FREE'),
           pht(
             '%s has no active lease.',
@@ -40,18 +46,24 @@ final class PhabricatorWorkerManagementFreeWorkflow
         continue;
       }
 
-      $task->setLeaseOwner(null);
+      $task
-      $task->setLeaseExpires(PhabricatorTime::getNow());
+        ->setLeaseOwner(null)
-      $task->save();
+        ->setLeaseExpires(PhabricatorTime::getNow())
+        ->save();
 
-      $console->writeOut(
+      $this->logInfo(
-        "**<bg:green> %s </bg>** %s\n",
         pht('LEASE FREED'),
         pht(
           '%s was freed from its lease.',
           $this->describeTask($task)));
+
+      $free_count++;
     }
 
+    $this->logOkay(
+      pht('DONE'),
+      pht('Freed %s task lease(s).', new PhutilNumber($free_count)));
+
     return 0;
   }
 

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementRetryWorkflow.php

@@ -6,7 +6,7 @@ final class PhabricatorWorkerManagementRetryWorkflow
   protected function didConstruct() {
     $this
       ->setName('retry')
-      ->setExamples('**retry** --id __id__')
+      ->setExamples('**retry** __selectors__')
       ->setSynopsis(
         pht(
           'Retry selected tasks which previously failed permanently or '.
@@ -24,14 +24,21 @@ final class PhabricatorWorkerManagementRetryWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
-    $tasks = $this->loadTasks($args);
-
     $is_repeat = $args->getArg('repeat');
+
+    $tasks = $this->loadTasks($args);
+    if (!$tasks) {
+      $this->logWarn(
+        pht('NO TASKS'),
+        pht('No tasks selected to retry.'));
+
+      return 0;
+    }
+
+    $retry_count = 0;
     foreach ($tasks as $task) {
       if (!$task->isArchived()) {
-        $console->writeOut(
+        $this->logWarn(
-          "**<bg:yellow> %s </bg>** %s\n",
           pht('ACTIVE'),
           pht(
             '%s is already in the active task queue.',
@@ -42,8 +49,7 @@ final class PhabricatorWorkerManagementRetryWorkflow
       $result_success = PhabricatorWorkerArchiveTask::RESULT_SUCCESS;
       if ($task->getResult() == $result_success) {
         if (!$is_repeat) {
-          $console->writeOut(
+          $this->logWarn(
-            "**<bg:yellow> %s </bg>** %s\n",
             pht('SUCCEEDED'),
             pht(
               '%s has already succeeded, and will not be repeated. '.
@@ -55,14 +61,19 @@ final class PhabricatorWorkerManagementRetryWorkflow
 
       $task->unarchiveTask();
 
-      $console->writeOut(
+      $this->logInfo(
-        "**<bg:green> %s </bg>** %s\n",
         pht('QUEUED'),
         pht(
           '%s was queued for retry.',
           $this->describeTask($task)));
+
+      $retry_count++;
     }
 
+    $this->logOkay(
+      pht('DONE'),
+      pht('Queued %s task(s) for retry.', new PhutilNumber($retry_count)));
+
     return 0;
   }
 

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php

@@ -14,16 +14,54 @@ abstract class PhabricatorWorkerManagementWorkflow
       array(
         'name' => 'class',
         'param' => 'name',
-        'help' => pht('Select all tasks of a given class.'),
+        'help' => pht('Select tasks of a given class.'),
       ),
       array(
         'name' => 'min-failure-count',
         'param' => 'int',
-        'help' => pht('Limit to tasks with at least this many failures.'),
+        'help' => pht('Select tasks with a minimum failure count.'),
+      ),
+      array(
+        'name' => 'max-failure-count',
+        'param' => 'int',
+        'help' => pht('Select tasks with a maximum failure count.'),
       ),
       array(
         'name' => 'active',
-        'help' => pht('Select all active tasks.'),
+        'help' => pht('Select active tasks.'),
+      ),
+      array(
+        'name' => 'archived',
+        'help' => pht('Select archived tasks.'),
+      ),
+      array(
+        'name' => 'container',
+        'param' => 'name',
+        'help' => pht(
+          'Select tasks with the given container or containers.'),
+        'repeat' => true,
+      ),
+      array(
+        'name' => 'object',
+        'param' => 'name',
+        'repeat' => true,
+        'help' => pht(
+          'Select tasks affecting the given object or objects.'),
+      ),
+      array(
+        'name' => 'min-priority',
+        'param' => 'int',
+        'help' => pht('Select tasks with a minimum priority.'),
+      ),
+      array(
+        'name' => 'max-priority',
+        'param' => 'int',
+        'help' => pht('Select tasks with a maximum priority.'),
+      ),
+      array(
+        'name' => 'limit',
+        'param' => 'int',
+        'help' => pht('Limit selection to a maximum number of tasks.'),
       ),
     );
   }
@@ -31,14 +69,92 @@ abstract class PhabricatorWorkerManagementWorkflow
   protected function loadTasks(PhutilArgumentParser $args) {
     $ids = $args->getArg('id');
     $class = $args->getArg('class');
-    $min_failures = $args->getArg('min-failure-count');
     $active = $args->getArg('active');
+    $archived = $args->getArg('archived');
 
-    if (!$ids && !$class && !$min_failures && !$active) {
+    $container_names = $args->getArg('container');
+    $object_names = $args->getArg('object');
+
+    $min_failures = $args->getArg('min-failure-count');
+    $max_failures = $args->getArg('max-failure-count');
+
+    $min_priority = $args->getArg('min-priority');
+    $max_priority = $args->getArg('max-priority');
+
+    $limit = $args->getArg('limit');
+
+    $any_constraints = false;
+    if ($ids) {
+      $any_constraints = true;
+    }
+
+    if ($class) {
+      $any_constraints = true;
+    }
+
+    if ($active || $archived) {
+      $any_constraints = true;
+      if ($active && $archived) {
         throw new PhutilArgumentUsageException(
           pht(
-          'Use "--id", "--class", "--active", and/or "--min-failure-count" '.
+            'You can not specify both "--active" and "--archived" tasks: '.
-          'to select tasks.'));
+            'no tasks can match both constraints.'));
+      }
+    }
+
+    if ($container_names) {
+      $any_constraints = true;
+      $container_phids = $this->loadObjectPHIDsFromArguments($container_names);
+    } else {
+      $container_phids = array();
+    }
+
+    if ($object_names) {
+      $any_constraints = true;
+      $object_phids = $this->loadObjectPHIDsFromArguments($object_names);
+    } else {
+      $object_phids = array();
+    }
+
+    if (($min_failures !== null) || ($max_failures !== null)) {
+      $any_constraints = true;
+      if (($min_failures !== null) && ($max_failures !== null)) {
+        if ($min_failures > $max_failures) {
+          throw new PhutilArgumentUsageException(
+            pht(
+              'Specified "--min-failures" must not be larger than '.
+              'specified "--max-failures".'));
+        }
+      }
+    }
+
+    if (($min_priority !== null) || ($max_priority !== null)) {
+      $any_constraints = true;
+      if (($min_priority !== null) && ($max_priority !== null)) {
+        if ($min_priority > $max_priority) {
+          throw new PhutilArgumentUsageException(
+            pht(
+              'Specified "--min-priority" may not be larger than '.
+              'specified "--max-priority".'));
+        }
+      }
+    }
+
+    if (!$any_constraints) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Use constraint flags (like "--id" or "--class") to select which '.
+          'tasks to affect. Use "--help" for a list of supported constraint '.
+          'flags.'));
+    }
+
+    if ($limit !== null) {
+      $limit = (int)$limit;
+      if ($limit <= 0) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Specified "--limit" must be a positive integer.'));
+      }
     }
 
     $active_query = new PhabricatorWorkerActiveTaskQuery();
@@ -56,13 +172,35 @@ abstract class PhabricatorWorkerManagementWorkflow
     }
 
     if ($min_failures) {
-      $active_query = $active_query->withFailureCountBetween(
+      $active_query->withFailureCountBetween($min_failures, $max_failures);
-        $min_failures, null);
+      $archive_query->withFailureCountBetween($min_failures, $max_failures);
-      $archive_query = $archive_query->withFailureCountBetween(
-        $min_failures, null);
     }
 
+    if ($container_phids) {
+      $active_query->withContainerPHIDs($container_phids);
+      $archive_query->withContainerPHIDs($container_phids);
+    }
+
+    if ($object_phids) {
+      $active_query->withObjectPHIDs($object_phids);
+      $archive_query->withObjectPHIDs($object_phids);
+    }
+
+    if ($min_priority || $max_priority) {
+      $active_query->withPriorityBetween($min_priority, $max_priority);
+      $archive_query->withPriorityBetween($min_priority, $max_priority);
+    }
+
+    if ($limit) {
+      $active_query->setLimit($limit);
+      $archive_query->setLimit($limit);
+    }
+
+    if ($archived) {
+      $active_tasks = array();
+    } else {
       $active_tasks = $active_query->execute();
+    }
 
     if ($active) {
       $archive_tasks = array();
@@ -74,32 +212,22 @@ abstract class PhabricatorWorkerManagementWorkflow
       mpull($active_tasks, null, 'getID') +
       mpull($archive_tasks, null, 'getID');
 
+    if ($limit) {
+      $tasks = array_slice($tasks, 0, $limit, $preserve_keys = true);
+    }
+
+
     if ($ids) {
       foreach ($ids as $id) {
         if (empty($tasks[$id])) {
           throw new PhutilArgumentUsageException(
-            pht('No task exists with id "%s"!', $id));
+            pht('No task with ID "%s" matches the constraints!', $id));
         }
       }
     }
-    if ($class && $min_failures) {
+
-      if (!$tasks) {
+    // We check that IDs are valid, but for all other constraints it is
-        throw new PhutilArgumentUsageException(
+    // acceptable to select no tasks to act upon.
-          pht('No task exists with class "%s" and at least %d failures!',
-            $class,
-            $min_failures));
-      }
-    } else if ($class) {
-      if (!$tasks) {
-        throw new PhutilArgumentUsageException(
-          pht('No task exists with class "%s"!', $class));
-      }
-    } else if ($min_failures) {
-      if (!$tasks) {
-        throw new PhutilArgumentUsageException(
-          pht('No tasks exist with at least %d failures!', $min_failures));
-      }
-    }
 
     // When we lock tasks properly, this gets populated as a side effect. Just
     // fake it when doing manual CLI stuff. This makes sure CLI yields have
@@ -110,6 +238,20 @@ abstract class PhabricatorWorkerManagementWorkflow
       }
     }
 
+    // If the user specified one or more "--id" flags, process the tasks in
+    // the given order. Otherwise, process them in FIFO order so the sequence
+    // is somewhat consistent with natural execution order.
+
+    // NOTE: When "--limit" is used, we end up selecting the newest tasks
+    // first. At time of writing, there's no way to order the queries
+    // correctly, so just accept it as reasonable behavior.
+
+    if ($ids) {
+      $tasks = array_select_keys($tasks, $ids);
+    } else {
+      $tasks = msort($tasks, 'getID');
+    }
+
     return $tasks;
   }
 
@@ -117,4 +259,52 @@ abstract class PhabricatorWorkerManagementWorkflow
     return pht('Task %d (%s)', $task->getID(), $task->getTaskClass());
   }
 
+  private function loadObjectPHIDsFromArguments(array $names) {
+    $viewer = $this->getViewer();
+
+    $seen_names = array();
+    foreach ($names as $name) {
+      if (isset($seen_names[$name])) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Object "%s" is specified more than once. Specify only unique '.
+            'objects.',
+            $name));
+      }
+      $seen_names[$name] = true;
+    }
+
+    $object_query = id(new PhabricatorObjectQuery())
+      ->setViewer($viewer)
+      ->withNames($names);
+
+    $object_query->execute();
+
+    $name_map = $object_query->getNamedResults();
+    $phid_map = array();
+    foreach ($names as $name) {
+      if (!isset($name_map[$name])) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'No object with name "%s" could be loaded.',
+            $name));
+      }
+
+      $phid = $name_map[$name]->getPHID();
+
+      if (isset($phid_map[$phid])) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Names "%s" and "%s" identify the same object. Specify only '.
+            'unique objects.',
+            $name,
+            $phid_map[$phid]));
+      }
+
+      $phid_map[$phid] = $name;
+    }
+
+    return array_keys($phid_map);
+  }
+
 }

src/infrastructure/daemon/workers/query/PhabricatorWorkerTaskQuery.php

@@ -7,10 +7,13 @@ abstract class PhabricatorWorkerTaskQuery
   private $dateModifiedSince;
   private $dateCreatedBefore;
   private $objectPHIDs;
+  private $containerPHIDs;
   private $classNames;
   private $limit;
   private $minFailureCount;
   private $maxFailureCount;
+  private $minPriority;
+  private $maxPriority;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -32,6 +35,11 @@ abstract class PhabricatorWorkerTaskQuery
     return $this;
   }
 
+  public function withContainerPHIDs(array $phids) {
+    $this->containerPHIDs = $phids;
+    return $this;
+  }
+
   public function withClassNames(array $names) {
     $this->classNames = $names;
     return $this;
@@ -43,6 +51,12 @@ abstract class PhabricatorWorkerTaskQuery
     return $this;
   }
 
+  public function withPriorityBetween($min, $max) {
+    $this->minPriority = $min;
+    $this->maxPriority = $max;
+    return $this;
+  }
+
   public function setLimit($limit) {
     $this->limit = $limit;
     return $this;
@@ -65,6 +79,13 @@ abstract class PhabricatorWorkerTaskQuery
         $this->objectPHIDs);
     }
 
+    if ($this->containerPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'containerPHID IN (%Ls)',
+        $this->containerPHIDs);
+    }
+
     if ($this->dateModifiedSince !== null) {
       $where[] = qsprintf(
         $conn,
@@ -100,6 +121,20 @@ abstract class PhabricatorWorkerTaskQuery
         $this->maxFailureCount);
     }
 
+    if ($this->minPriority !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'priority >= %d',
+        $this->minPriority);
+    }
+
+    if ($this->maxPriority !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'priority <= %d',
+        $this->maxPriority);
+    }
+
     return $this->formatWhereClause($conn, $where);
   }