Allow edges to be configured to prevent cycles

Summary:
Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races.

Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type:

  - We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined.
  - After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit.
  - When we're done, we release the edge type locks.

This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way.

Test Plan: Ran unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1162

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

src/__phutil_library_map__.php

@@ -621,8 +621,11 @@ phutil_register_library_map(array(
     'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php',
     'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php',
     'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php',
+    'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php',
     'PhabricatorEdgeEditor' => 'infrastructure/edges/editor/PhabricatorEdgeEditor.php',
+    'PhabricatorEdgeGraph' => 'infrastructure/edges/util/PhabricatorEdgeGraph.php',
     'PhabricatorEdgeQuery' => 'infrastructure/edges/query/PhabricatorEdgeQuery.php',
+    'PhabricatorEdgeTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php',
     'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php',
     'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php',
     'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php',
@@ -1643,7 +1646,10 @@ phutil_register_library_map(array(
     'PhabricatorDraft' => 'PhabricatorDraftDAO',
     'PhabricatorDraftDAO' => 'PhabricatorLiskDAO',
     'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants',
+    'PhabricatorEdgeCycleException' => 'Exception',
+    'PhabricatorEdgeGraph' => 'AbstractDirectedGraph',
     'PhabricatorEdgeQuery' => 'PhabricatorQuery',
+    'PhabricatorEdgeTestCase' => 'PhabricatorTestCase',
     'PhabricatorEmailLoginController' => 'PhabricatorAuthController',
     'PhabricatorEmailTokenController' => 'PhabricatorAuthController',
     'PhabricatorEmailVerificationController' => 'PhabricatorPeopleController',

src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php

@@ -0,0 +1,81 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorEdgeTestCase extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testCycleDetection() {
+
+    // The editor should detect that this introduces a cycle and prevent the
+    // edit.
+
+    $user = new PhabricatorUser();
+
+    $obj1 = id(new HarbormasterObject())->save();
+    $obj2 = id(new HarbormasterObject())->save();
+    $phid1 = $obj1->getPHID();
+    $phid2 = $obj2->getPHID();
+
+    $editor = id(new PhabricatorEdgeEditor())
+      ->setUser($user)
+      ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2)
+      ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1);
+
+    $caught = null;
+    try {
+      $editor->save();
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertEqual(
+      true,
+      $caught instanceof Exception);
+
+
+    // The first edit should go through (no cycle), bu the second one should
+    // fail (it introduces a cycle).
+
+    $editor = id(new PhabricatorEdgeEditor())
+      ->setUser($user)
+      ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2)
+      ->save();
+
+    $editor = id(new PhabricatorEdgeEditor())
+      ->setUser($user)
+      ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1);
+
+    $caught = null;
+    try {
+      $editor->save();
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertEqual(
+      true,
+      $caught instanceof Exception);
+  }
+
+
+}

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php

@@ -24,6 +24,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
   const TYPE_TASK_HAS_COMMIT  = 1;
   const TYPE_COMMIT_HAS_TASK  = 2;
 
+  const TYPE_TEST_NO_CYCLE    = 9000;
+
   public static function getInverse($edge_type) {
     static $map = array(
       self::TYPE_TASK_HAS_COMMIT => self::TYPE_COMMIT_HAS_TASK,
@@ -33,6 +35,13 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
     return idx($map, $edge_type);
   }
 
+  public static function shouldPreventCycles($edge_type) {
+    static $map = array(
+      self::TYPE_TEST_NO_CYCLE  => true,
+    );
+    return isset($map[$edge_type]);
+  }
+
   public static function establishConnection($phid_type, $conn_type) {
     static $class_map = array(
       PhabricatorPHIDConstants::PHID_TYPE_TASK  => 'ManiphestTask',
@@ -43,6 +52,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
       PhabricatorPHIDConstants::PHID_TYPE_PROJ  => 'PhabricatorProject',
       PhabricatorPHIDConstants::PHID_TYPE_MLST  =>
         'PhabricatorMetaMTAMailingList',
+      PhabricatorPHIDConstants::PHID_TYPE_TOBJ  => 'HarbormasterObject',
     );
 
     $class = idx($class_map, $phid_type);

src/infrastructure/edges/editor/PhabricatorEdgeEditor.php

@@ -32,6 +32,7 @@
  *      ->save();
  *
  * @task edit     Editing Edges
+ * @task cycles   Cycle Prevention
  * @task internal Internals
  */
 final class PhabricatorEdgeEditor {
@@ -113,11 +114,27 @@ final class PhabricatorEdgeEditor {
    */
   public function save() {
 
-    // NOTE: We write edge data first, before doing any transactions, since
+    $cycle_types = $this->getPreventCyclesEdgeTypes();
-    // it's OK if we just leave it hanging out in space unattached to anything.
 
+    $locks = array();
+    $caught = null;
+    try {
+
+      // NOTE: We write edge data first, before doing any transactions, since
+      // it's OK if we just leave it hanging out in space unattached to
+      // anything.
       $this->writeEdgeData();
 
+      // If we're going to perform cycle detection, lock the edge type before
+      // doing edits.
+      if ($cycle_types) {
+        $src_phids = ipull($this->addEdges, 'src');
+        foreach ($cycle_types as $cycle_type) {
+          $key = 'edge.cycle:'.$cycle_type;
+          $locks[] = PhabricatorGlobalLock::newLock($key)->lock(15);
+        }
+      }
+
       static $id = 0;
       $id++;
 
@@ -129,10 +146,29 @@ final class PhabricatorEdgeEditor {
       $this->executeRemoves();
       $this->executeAdds();
 
+      foreach ($cycle_types as $cycle_type) {
+        $this->detectCycles($src_phids, $cycle_type);
+      }
+
       $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES);
 
-
       $this->saveTransactions();
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+
+
+    if ($caught) {
+      $this->killTransactions();
+    }
+
+    foreach ($locks as $lock) {
+      $lock->unlock();
+    }
+
+    if ($caught) {
+      throw $caught;
+    }
   }
 
 
@@ -327,6 +363,13 @@ final class PhabricatorEdgeEditor {
     }
   }
 
+  private function killTransactions() {
+    foreach ($this->openTransactions as $key => $conn_w) {
+      $conn_w->killTransaction();
+      unset($this->openTransactions[$key]);
+    }
+  }
+
   private function sendEvent($edit_id, $event_type) {
     $event = new PhabricatorEvent(
       $event_type,
@@ -339,4 +382,58 @@ final class PhabricatorEdgeEditor {
     PhutilEventEngine::dispatchEvent($event);
   }
 
+
+/* -(  Cycle Prevention  )--------------------------------------------------- */
+
+
+  /**
+   * Get a list of all edge types which are being added, and which we should
+   * prevent cycles on.
+   *
+   * @return list<const> List of edge types which should have cycles prevented.
+   * @task cycle
+   */
+  private function getPreventCyclesEdgeTypes() {
+    $edge_types = array();
+    foreach ($this->addEdges as $edge) {
+      $edge_types[$edge['type']] = true;
+    }
+    foreach ($edge_types as $type => $ignored) {
+      if (!PhabricatorEdgeConfig::shouldPreventCycles($type)) {
+        unset($edge_types[$type]);
+      }
+    }
+    return array_keys($edge_types);
+  }
+
+
+  /**
+   * Detect graph cycles of a given edge type. If the edit introduces a cycle,
+   * a @{class:PhabricatorEdgeCycleException} is thrown with details.
+   *
+   * @return void
+   * @task cycle
+   */
+  private function detectCycles(array $phids, $edge_type) {
+    // For simplicity, we just seed the graph with the affected nodes rather
+    // than seeding it with their edges. To do this, we just add synthetic
+    // edges from an imaginary '<seed>' node to the known edges.
+
+
+    $graph = id(new PhabricatorEdgeGraph())
+      ->setEdgeType($edge_type)
+      ->addNodes(
+        array(
+          '<seed>' => $phids,
+        ))
+      ->loadGraph();
+
+    foreach ($phids as $phid) {
+      $cycle = $graph->detectCycles($phid);
+      if ($cycle) {
+        throw new PhabricatorEdgeCycleException($edge_type, $cycle);
+      }
+    }
+  }
+
 }

src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php

@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorEdgeCycleException extends Exception {
+
+  private $cycleEdgeType;
+  private $cycle;
+
+  public function __construct($cycle_edge_type, array $cycle) {
+    $this->cycleEdgeType = $cycle_edge_type;
+    $this->cycle = $cycle;
+
+    $cycle_list = implode(', ', $cycle);
+
+    parent::__construct(
+      "Graph cycle detected (type={$cycle_edge_type}, cycle={$cycle_list}).");
+  }
+
+  public function getCycle() {
+    return $this->cycle;
+  }
+
+  public function getCycleEdgeType() {
+    return $this->cycleEdgeType;
+  }
+
+}

src/infrastructure/edges/util/PhabricatorEdgeGraph.php

@@ -0,0 +1,50 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorEdgeGraph extends AbstractDirectedGraph {
+
+  private $edgeType;
+
+  public function setEdgeType($edge_type) {
+    $this->edgeType = $edge_type;
+    return $this;
+  }
+
+  protected function loadEdges(array $nodes) {
+    if (!$this->edgeType) {
+      throw new Exception("Set edge type before loading graph!");
+    }
+
+    $edges = id(new PhabricatorEdgeQuery())
+      ->withSourcePHIDs($nodes)
+      ->withEdgeTypes(array($this->edgeType))
+      ->execute();
+
+    $results = array_fill_keys($nodes, array());
+    foreach ($edges as $src => $types) {
+      foreach ($types as $type => $dsts) {
+        foreach ($dsts as $dst => $edge) {
+          $results[$src][] = $dst;
+        }
+      }
+    }
+
+    return $results;
+  }
+
+}