From 0ed281d25e4448b125228cc964625dc618e1d161 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 8 Dec 2013 21:07:13 -0800
Subject: [PATCH] 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
---
 .../__tests__/PhabricatorWorkerTestCase.php   | 19 +++++++++++++-
 .../query/PhabricatorWorkerLeaseQuery.php     | 26 ++++++++++++++++---
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
index e58650371f..7e5c5f9416 100644
--- a/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
+++ b/src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
@@ -49,7 +49,6 @@ final class PhabricatorWorkerTestCase extends PhabricatorTestCase {
     $this->expectNextLease($task1);
   }
 
-
   public function testExecuteTask() {
     $task = $this->scheduleAndExecuteTask();
 
@@ -155,6 +154,24 @@ final class PhabricatorWorkerTestCase extends PhabricatorTestCase {
     $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)
diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
index e7b07ba844..507a8b6c44 100644
--- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
+++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
@@ -60,7 +60,7 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery {
         '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 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery {
       $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 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery {
 
   }
 
-  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) {