graph: check for cycles in visit methods

Avoid silently returning a subset of nodes.

Author: jcristau

src/taskgraph/graph.py

@@ -97,14 +97,18 @@ def _visit(self, reverse):
                 indegree[dependent] -= 1
                 if indegree[dependent] == 0:
                     queue.append(dependent)
+        loopy_nodes = {node for node, degree in indegree.items() if degree > 0}
+        if loopy_nodes:
+            raise Exception(
+                f"Dependency loop detected involving the following nodes: {loopy_nodes}"
+            )
 
     def visit_postorder(self):
         """
         Generate a sequence of nodes in postorder, such that every node is
         visited *after* any nodes it links to.
 
-        Behavior is undefined (read: it will hang) if the graph contains a
-        cycle.
+        Raises an exception if the graph contains a cycle.
         """
         return self._visit(False)
 

test/test_graph.py

@@ -5,6 +5,8 @@
 
 import unittest
 
+import pytest
+
 from taskgraph.graph import Graph
 
 
@@ -63,6 +65,11 @@ class TestGraph(unittest.TestCase):
         },
     )
 
+    loopy = Graph(
+        {"A", "B", "C"},
+        {tuple(x) for x in "ABL BCL CAL".split()},  # codespell:ignore
+    )
+
     def test_transitive_closure_empty(self):
         "transitive closure of an empty set is an empty graph"
         g = Graph({"a", "b", "c"}, {("a", "b", "L"), ("a", "c", "L")})
@@ -123,6 +130,10 @@ def test_transitive_closure_linear(self):
         "transitive closure of a linear graph includes all nodes in the line"
         self.assertEqual(self.linear.transitive_closure({"1"}), self.linear)
 
+    def test_transitive_closure_loopy(self):
+        "transitive closure of a loop is the whole loop"
+        self.assertEqual(self.loopy.transitive_closure({"A"}), self.loopy)
+
     def test_visit_postorder_empty(self):
         "postorder visit of an empty graph is empty"
         self.assertEqual(list(Graph(set(), set()).visit_postorder()), [])
@@ -154,6 +165,11 @@ def test_visit_postorder_disjoint(self):
         "postorder visit of a disjoint graph satisfies invariant"
         self.assert_postorder(self.disjoint.visit_postorder(), self.disjoint.nodes)
 
+    def test_visit_postorder_loopy(self):
+        with pytest.raises(Exception) as excinfo:
+            list(self.loopy.visit_postorder())
+        assert "Dependency loop detected" in str(excinfo.value)
+
     def assert_preorder(self, seq, all_nodes):
         seen = set()
         for e in seq:
@@ -179,6 +195,11 @@ def test_visit_preorder_disjoint(self):
         "preorder visit of a disjoint graph satisfies invariant"
         self.assert_preorder(self.disjoint.visit_preorder(), self.disjoint.nodes)
 
+    def test_visit_preorder_loopy(self):
+        with pytest.raises(Exception) as excinfo:
+            list(self.loopy.visit_preorder())
+        assert "Dependency loop detected" in str(excinfo.value)
+
     def test_links_dict(self):
         "link dict for a graph with multiple edges is correct"
         self.assertEqual(