From ca7a0de1cf77b552e0984783cada9ed208eb9002 Mon Sep 17 00:00:00 2001
From: epriestley <epriestley@phabricator.com>
Date: Thu, 14 Apr 2011 12:07:57 -0700
Subject: [PATCH] Prevent a race in Phabricator workers

Summary: See D133. Workers can also be subject to the same race, invert the
row relationship in the same way.

Test Plan: Launched repository master daemons and some taskmasters and used
the Daemon console to veify that they were able to process tasks. Manually
checked the database to make sure data got linked correctly and that new data
was inserted correctly.

Reviewers: jungejason

CC: tuomaspelkonen

Differential Revision: 135
---
 resources/sql/patches/031.workerrace.sql            | 13 +++++++++++++
 .../PhabricatorWorkerTaskDetailController.php       |  4 ++--
 .../workers/storage/task/PhabricatorWorkerTask.php  |  8 ++++----
 .../storage/taskdata/PhabricatorWorkerTaskData.php  |  1 -
 .../taskmaster/PhabricatorTaskmasterDaemon.php      |  6 +++---
 5 files changed, 22 insertions(+), 10 deletions(-)
 create mode 100644 resources/sql/patches/031.workerrace.sql

diff --git a/resources/sql/patches/031.workerrace.sql b/resources/sql/patches/031.workerrace.sql
new file mode 100644
index 0000000000..e643068f23
--- /dev/null
+++ b/resources/sql/patches/031.workerrace.sql
@@ -0,0 +1,13 @@
+ALTER TABLE phabricator_worker.worker_task
+  ADD dataID int unsigned;
+
+ALTER TABLE phabricator_worker.worker_task
+  ADD UNIQUE KEY (dataID);
+
+UPDATE phabricator_worker.worker_task t,
+       phabricator_worker.worker_taskdata d
+  SET t.dataID = d.id
+  WHERE d.taskID = t.id;
+
+ALTER TABLE phabricator_worker.worker_taskdata
+  DROP taskID;
diff --git a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php
index 80fadc6047..03cbfdcadc 100644
--- a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php
+++ b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php
@@ -44,8 +44,8 @@ class PhabricatorWorkerTaskDetailController
     }
 
     $data = id(new PhabricatorWorkerTaskData())->loadOneWhere(
-      'taskID = %d',
-      $task->getID());
+      'id = %d',
+      $task->getDataID());
     if ($data) {
       $data = json_encode($data->getData());
     }
diff --git a/src/infrastructure/daemon/workers/storage/task/PhabricatorWorkerTask.php b/src/infrastructure/daemon/workers/storage/task/PhabricatorWorkerTask.php
index 9d980095db..8eaa414674 100644
--- a/src/infrastructure/daemon/workers/storage/task/PhabricatorWorkerTask.php
+++ b/src/infrastructure/daemon/workers/storage/task/PhabricatorWorkerTask.php
@@ -22,6 +22,7 @@ class PhabricatorWorkerTask extends PhabricatorWorkerDAO {
   protected $leaseOwner;
   protected $leaseExpires;
   protected $failureCount;
+  protected $dataID;
 
   private $serverTime;
   private $localTime;
@@ -58,16 +59,15 @@ class PhabricatorWorkerTask extends PhabricatorWorkerDAO {
       $this->failureCount = 0;
     }
 
-    $ret = parent::save();
-
     if ($is_new && $this->data) {
       $data = new PhabricatorWorkerTaskData();
-      $data->setTaskID($this->getID());
       $data->setData($this->data);
       $data->save();
+
+      $this->setDataID($data->getID());
     }
 
-    return $ret;
+    return parent::save();
   }
 
   public function setData($data) {
diff --git a/src/infrastructure/daemon/workers/storage/taskdata/PhabricatorWorkerTaskData.php b/src/infrastructure/daemon/workers/storage/taskdata/PhabricatorWorkerTaskData.php
index 8407e47aa7..007aea0892 100644
--- a/src/infrastructure/daemon/workers/storage/taskdata/PhabricatorWorkerTaskData.php
+++ b/src/infrastructure/daemon/workers/storage/taskdata/PhabricatorWorkerTaskData.php
@@ -18,7 +18,6 @@
 
 class PhabricatorWorkerTaskData extends PhabricatorWorkerDAO {
 
-  protected $taskID;
   protected $data;
 
   public function getConfiguration() {
diff --git a/src/infrastructure/daemon/workers/taskmaster/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/taskmaster/PhabricatorTaskmasterDaemon.php
index 4a5d202312..a8cc469b8a 100644
--- a/src/infrastructure/daemon/workers/taskmaster/PhabricatorTaskmasterDaemon.php
+++ b/src/infrastructure/daemon/workers/taskmaster/PhabricatorTaskmasterDaemon.php
@@ -50,7 +50,7 @@ class PhabricatorTaskmasterDaemon extends PhabricatorDaemon {
           $conn_w,
           'SELECT task.*, taskdata.data _taskData, UNIX_TIMESTAMP() _serverTime
             FROM %T task LEFT JOIN %T taskdata
-              ON taskdata.taskID = task.id
+              ON taskdata.id = task.dataID
             WHERE leaseOwner = %s AND leaseExpires > UNIX_TIMESTAMP()
             LIMIT 1',
           $task_table->getTableName(),
@@ -97,9 +97,9 @@ class PhabricatorTaskmasterDaemon extends PhabricatorDaemon {
             if ($data !== null) {
               queryfx(
                 $conn_w,
-                'DELETE FROM %T WHERE taskID = %d',
+                'DELETE FROM %T WHERE id = %d',
                 $taskdata_table->getTableName(),
-                $task->getID());
+                $task->getDataID());
             }
           } catch (Exception $ex) {
             $task->setFailureCount($task->getFailureCount() + 1);