Make taskmaster consumption of failed tasks more FIFO-ey

Summary:
Ref T1049. See discussion in D7745. We have some specific interest in this for D7745, but generally we want to consume tasks with expired leases in roughly FIFO order, just like we consume new tasks in roughly FIFO order. Currently, when we select an expired task we order them by `id`, but this is the original insert order, not lease expiration order. Instead, order by `leaseExpires`.

This query is actually much better than the old one was, since the WHERE part is `leaseExpries < VALUE`.

Test Plan: Ran `EXPLAIN` on the query. Ran a taskmaster in debug mode and saw it lease new and expired tasks successfully.

Reviewers: hach-que, btrahan

Reviewed By: hach-que

CC: aran

Maniphest Tasks: T1049

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

Author: epriestley

src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php

@@ -49,7 +49,6 @@ public function testNewBeforeLeased() {
     $this->expectNextLease($task1);
   }
 
-
   public function testExecuteTask() {
     $task = $this->scheduleAndExecuteTask();
 
@@ -155,6 +154,24 @@ public function testRequiredLeaseTime() {
     $this->assertEqual(true, ($task->getLeaseExpires() - time()) > 1000);
   }
 
+  public function testLeasedIsOldestFirst() {
+    // Tasks which expired earlier should lease first, all else being equal.
+
+    $task1 = $this->scheduleTask();
+    $task2 = $this->scheduleTask();
+
+    $task1->setLeaseOwner('test');
+    $task1->setLeaseExpires(time() - 100000);
+    $task1->forceSaveWithoutLease();
+
+    $task2->setLeaseOwner('test');
+    $task2->setLeaseExpires(time() - 200000);
+    $task2->forceSaveWithoutLease();
+
+    $this->expectNextLease($task2);
+    $this->expectNextLease($task1);
+  }
+
   private function expectNextLease($task) {
     $leased = id(new PhabricatorWorkerLeaseQuery())
       ->setLimit(1)

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

@@ -60,7 +60,7 @@ public function execute() {
         'SELECT id, leaseOwner FROM %T %Q %Q %Q',
         $task_table->getTableName(),
         $this->buildWhereClause($conn_w, $phase),
-        $this->buildOrderClause($conn_w),
+        $this->buildOrderClause($conn_w, $phase),
         $this->buildLimitClause($conn_w, $limit - $leased));
 
       // NOTE: Sometimes, we'll race with another worker and they'll grab
@@ -102,7 +102,7 @@ public function execute() {
       $task_table->getTableName(),
       $taskdata_table->getTableName(),
       $lease_ownership_name,
-      $this->buildOrderClause($conn_w),
+      $this->buildOrderClause($conn_w, $phase),
       $this->buildLimitClause($conn_w, $limit));
 
     $tasks = $task_table->loadAllFromArray($data);
@@ -188,8 +188,26 @@ private function buildUpdateWhereClause(
 
   }
 
-  private function buildOrderClause(AphrontDatabaseConnection $conn_w) {
-    return qsprintf($conn_w, 'ORDER BY id ASC');
+  private function buildOrderClause(AphrontDatabaseConnection $conn_w, $phase) {
+    switch ($phase) {
+      case self::PHASE_UNLEASED:
+        // When selecting new tasks, we want to consume them in roughly
+        // FIFO order, so we order by the task ID.
+        return qsprintf($conn_w, 'ORDER BY id ASC');
+      case self::PHASE_EXPIRED:
+        // When selecting failed tasks, we want to consume them in roughly
+        // FIFO order of their failures, which is not necessarily their original
+        // queue order.
+
+        // Particularly, this is important for tasks which use soft failures to
+        // indicate that they are waiting on other tasks to complete: we need to
+        // push them to the end of the queue after they fail, at least on
+        // average, so we don't deadlock retrying the same blocked task over
+        // and over again.
+        return qsprintf($conn_w, 'ORDER BY leaseExpires ASC');
+      default:
+        throw new Exception(pht('Unknown phase "%s"!', $phase));
+    }
   }
 
   private function buildLimitClause(AphrontDatabaseConnection $conn_w, $limit) {