Allow archived tasks to be queried by object PHID and order by id

Summary: Ref T5402.

Test Plan:
  - Queried archived tasks.
  - Grepped for use sites and verified no other callsites are order-sensitive.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5402

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

Author: epriestley

src/applications/daemon/controller/PhabricatorDaemonConsoleController.php

@@ -131,8 +131,11 @@ public function processRequest() {
     $daemon_panel->appendChild($daemon_table);
 
 
-    $tasks = id(new PhabricatorWorkerActiveTask())->loadAllWhere(
-      'leaseOwner IS NOT NULL');
+    $tasks = id(new PhabricatorWorkerLeaseQuery())
+      ->setSkipLease(true)
+      ->withLeasedTasks(true)
+      ->setLimit(100)
+      ->execute();
 
     $tasks_table = id(new PhabricatorDaemonTasksTableView())
       ->setTasks($tasks)

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

@@ -6,6 +6,7 @@ final class PhabricatorWorkerArchiveTaskQuery
   private $ids;
   private $dateModifiedSince;
   private $dateCreatedBefore;
+  private $objectPHIDs;
   private $limit;
 
   public function withIDs(array $ids) {
@@ -23,20 +24,24 @@ public function withDateCreatedBefore($timestamp) {
     return $this;
   }
 
+  public function withObjectPHIDs(array $phids) {
+    $this->objectPHIDs = $phids;
+    return $this;
+  }
+
   public function setLimit($limit) {
     $this->limit = $limit;
     return $this;
   }
 
   public function execute() {
-
     $task_table = new PhabricatorWorkerArchiveTask();
 
     $conn_r = $task_table->establishConnection('r');
 
     $rows = queryfx_all(
       $conn_r,
-      'SELECT * FROM %T %Q %Q',
+      'SELECT * FROM %T %Q ORDER BY id DESC %Q',
       $task_table->getTableName(),
       $this->buildWhereClause($conn_r),
       $this->buildLimitClause($conn_r));
@@ -54,6 +59,13 @@ private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
         $this->ids);
     }
 
+    if ($this->objectPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn_r,
+        'objectPHID IN (%Ls)',
+        $this->objectPHIDs);
+    }
+
     if ($this->dateModifiedSince) {
       $where[] = qsprintf(
         $conn_r,

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

@@ -5,13 +5,15 @@
  */
 final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery {
 
+  const PHASE_LEASED = 'leased';
   const PHASE_UNLEASED = 'unleased';
   const PHASE_EXPIRED  = 'expired';
 
   private $ids;
   private $objectPHIDs;
   private $limit;
   private $skipLease;
+  private $leased = false;
 
   public static function getDefaultWaitBeforeRetry() {
     return phutil_units('5 minutes in seconds');
@@ -45,14 +47,44 @@ public function withObjectPHIDs(array $phids) {
     return $this;
   }
 
+  /**
+   * Select only leased tasks, only unleased tasks, or both types of task.
+   *
+   * By default, queries select only unleased tasks (equivalent to passing
+   * `false` to this method). You can pass `true` to select only leased tasks,
+   * or `null` to ignore the lease status of tasks.
+   *
+   * If your result set potentially includes leased tasks, you must disable
+   * leasing using @{method:setSkipLease}. These options are intended for use
+   * when displaying task status information.
+   *
+   * @param mixed `true` to select only leased tasks, `false` to select only
+   *              unleased tasks (default), or `null` to select both.
+   * @return this
+   */
+  public function withLeasedTasks($leased) {
+    $this->leased = $leased;
+    return $this;
+  }
+
   public function setLimit($limit) {
     $this->limit = $limit;
     return $this;
   }
 
   public function execute() {
     if (!$this->limit) {
-      throw new Exception('You must setLimit() when leasing tasks.');
+      throw new Exception(
+        pht('You must setLimit() when leasing tasks.'));
+    }
+
+    if ($this->leased !== false) {
+      if (!$this->skipLease) {
+        throw new Exception(
+          pht(
+            'If you potentially select leased tasks using withLeasedTasks(), '.
+            'you MUST disable lease acquisition by calling setSkipLease().'));
+      }
     }
 
     $task_table = new PhabricatorWorkerActiveTask();
@@ -65,10 +97,16 @@ public function execute() {
     // find enough tasks, try tasks with expired leases (i.e., tasks which have
     // previously failed).
 
-    $phases = array(
-      self::PHASE_UNLEASED,
-      self::PHASE_EXPIRED,
-    );
+    // If we're selecting leased tasks, look for them first.
+
+    $phases = array();
+    if ($this->leased !== false) {
+      $phases[] = self::PHASE_LEASED;
+    }
+    if ($this->leased !== true) {
+      $phases[] = self::PHASE_UNLEASED;
+      $phases[] = self::PHASE_EXPIRED;
+    }
     $limit = $this->limit;
 
     $leased = 0;
@@ -160,13 +198,22 @@ public function execute() {
       $tasks[$row['id']]->setData($task_data);
     }
 
+    if ($this->skipLease) {
+      // Reorder rows into the original phase order if this is a status query.
+      $tasks = array_select_keys($tasks, $task_ids);
+    }
+
     return $tasks;
   }
 
   private function buildWhereClause(AphrontDatabaseConnection $conn_w, $phase) {
     $where = array();
 
     switch ($phase) {
+      case self::PHASE_LEASED:
+        $where[] = 'leaseOwner IS NOT NULL';
+        $where[] = 'leaseExpires >= UNIX_TIMESTAMP()';
+        break;
       case self::PHASE_UNLEASED:
         $where[] = 'leaseOwner IS NULL';
         break;
@@ -177,7 +224,7 @@ private function buildWhereClause(AphrontDatabaseConnection $conn_w, $phase) {
         throw new Exception("Unknown phase '{$phase}'!");
     }
 
-    if ($this->ids) {
+    if ($this->ids !== null) {
       $where[] = qsprintf($conn_w, 'id IN (%Ld)', $this->ids);
     }
 
@@ -199,6 +246,11 @@ private function buildUpdateWhereClause(
     // `IN (NULL)` doesn't match NULL.
 
     switch ($phase) {
+      case self::PHASE_LEASED:
+        throw new Exception(
+          pht(
+            'Trying to lease tasks selected in the leased phase! This is '.
+            'intended to be imposssible.'));
       case self::PHASE_UNLEASED:
         $where[] = qsprintf($conn_w, 'leaseOwner IS NULL');
         $where[] = qsprintf($conn_w, 'id IN (%Ld)', ipull($rows, 'id'));
@@ -223,6 +275,10 @@ private function buildUpdateWhereClause(
 
   private function buildOrderClause(AphrontDatabaseConnection $conn_w, $phase) {
     switch ($phase) {
+      case self::PHASE_LEASED:
+        // Ideally we'd probably order these by lease acquisition time, but
+        // we don't have that handy and this is a good approximation.
+        return qsprintf($conn_w, 'ORDER BY priority ASC, id ASC');
       case self::PHASE_UNLEASED:
         // When selecting new tasks, we want to consume them in order of
         // increasing priority (and then FIFO).

src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php

@@ -119,6 +119,7 @@ public function archiveTask($result, $duration) {
       ->setFailureCount($this->getFailureCount())
       ->setDataID($this->getDataID())
       ->setPriority($this->getPriority())
+      ->setObjectPHID($this->getObjectPHID())
       ->setResult($result)
       ->setDuration($duration);
 

src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php

@@ -84,6 +84,7 @@ public function unarchiveTask() {
         ->setFailureCount(0)
         ->setDataID($this->getDataID())
         ->setPriority($this->getPriority())
+        ->setObjectPHID($this->getObjectPHID())
         ->insert();
 
       $this->setDataID(null);