Avoid Timeline race condition

Summary: While I should fix the transactional stuff, that patch is going to be
tricky and transactions have some performance implications. This is a simple
fix which prevents the race.

Instead of having the data point at the event ID, have the event point at a
data ID. Insert the data first, then insert the event with the right data
pointer. This is super simple and prevents the race issue.

Test Plan:
  - Ran the schema upgrade script, verified that the database was
    correctly upgraded. Was also prompted to stop daemons.
  - Ran 'repository-launch-master', verified that the discovery daemons were
    able to discover new commits and insert events for them. Verified the
    committask daemon was consuming events and converting them into tasks.
  - Verified new tasks looked correct in the database.
  - Browsed web interface.

Reviewers: jungejason

CC: tuomaspelkonen

Differential Revision: 133

resources/sql/patches/029.cursors.sql

@@ -0,0 +1,13 @@
+ALTER TABLE phabricator_timeline.timeline_event
+  ADD dataID int unsigned;
+
+ALTER TABLE phabricator_timeline.timeline_event
+  ADD UNIQUE KEY (dataID);
+
+UPDATE phabricator_timeline.timeline_event e,
+       phabricator_timeline.timeline_eventdata d
+  SET e.dataID = d.id
+  WHERE d.eventID = e.id;
+
+ALTER TABLE phabricator_timeline.timeline_eventdata
+  DROP eventID;

scripts/sql/upgrade_schema.php

@@ -21,6 +21,8 @@ $root = dirname(dirname(dirname(__FILE__)));
 require_once $root.'/scripts/__init_script__.php';
 require_once $root.'/scripts/__init_env__.php';
 
+phutil_require_module('phutil', 'console');
+
 const TABLE_NAME = 'schema_version';
 
 if (isset($argv[1]) && !is_numeric($argv[1])) {
@@ -33,6 +35,15 @@ if (isset($argv[1]) && !is_numeric($argv[1])) {
   exit(0);
 }
 
+echo phutil_console_wrap(
+  "Before running this script, you should take down the Phabricator web ".
+  "interface and stop any running Phabricator daemons.");
+
+if (!phutil_console_confirm('Are you ready to continue?')) {
+  echo "Cancelled.\n";
+  exit(1);
+}
+
 // Use always the version from the commandline if it is defined
 $next_version = isset($argv[1]) ? (int)$argv[1] : null;
 

src/applications/daemon/controller/timelineevent/PhabricatorDaemonTimelineEventController.php

@@ -34,9 +34,10 @@ class PhabricatorDaemonTimelineEventController
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $data = id(new PhabricatorTimelineEventData())->loadOneWhere(
-      'eventID = %d',
-      $event->getID());
+    if ($event->getDataID()) {
+      $data = id(new PhabricatorTimelineEventData())->load(
+        $event->getDataID());
+    }
 
     if ($data) {
       $data = json_encode($data->getEventData());

src/infrastructure/daemon/timeline/cursor/iterator/PhabricatorTimelineIterator.php

@@ -54,7 +54,7 @@ class PhabricatorTimelineIterator implements Iterator {
       $event->establishConnection('r'),
       'SELECT event.*, event_data.eventData eventData
         FROM %T event
-        LEFT JOIN %T event_data ON event_data.eventID = event.id
+        LEFT JOIN %T event_data ON event_data.id = event.dataID
         WHERE event.id > %d AND event.type in (%Ls)
         ORDER BY event.id ASC LIMIT %d',
       $event->getTableName(),

src/infrastructure/daemon/timeline/storage/event/PhabricatorTimelineEvent.php

@@ -19,6 +19,8 @@
 class PhabricatorTimelineEvent extends PhabricatorTimelineDAO {
 
   protected $type;
+  protected $dataID;
+
   private $data;
 
   public function __construct($type, $data = null) {
@@ -43,14 +45,18 @@ class PhabricatorTimelineEvent extends PhabricatorTimelineDAO {
       throw new Exception("Event has already been recorded!");
     }
 
-    $this->save();
-
+    // Save the data first and point to it from the event to avoid a race
+    // condition where we insert the event before the data and a consumer reads
+    // it immediately.
     if ($this->data !== null) {
       $data = new PhabricatorTimelineEventData();
-      $data->setEventID($this->getID());
       $data->setEventData($this->data);
       $data->save();
+
+      $this->setDataID($data->getID());
     }
+
+    $this->save();
   }
 
   public function setData($data) {

src/infrastructure/daemon/timeline/storage/eventdata/PhabricatorTimelineEventData.php

@@ -18,7 +18,6 @@
 
 class PhabricatorTimelineEventData extends PhabricatorTimelineDAO {
 
-  protected $eventID;
   protected $eventData;
 
   public function getConfiguration() {