Fix native cycle detection error message. (#14381)

As described in #13744, #13370 broke the error message for native cycle detection, by reporting unrelated nodes.

Fix the issue (node ids for the running subgraph were being used with the underlying graph when rendering the path) and expand the test.

Fixes #13744.

[ci skip-build-wheels]

src/rust/engine/graph/src/lib.rs

@@ -144,13 +144,19 @@ impl<N: Node> InnerGraph<N> {
       let (running_candidate, should_terminate) = if let Some(dirty_candidate) = running_scc
         .iter()
         .filter(|&id| self.pg[running_graph[*id]].is_cleaning())
-        .max()
+        .max_by_key(|&id| running_graph[*id])
       {
         // Nodes are being cleaned: clear the highest id entry.
         (dirty_candidate, false)
       } else {
         // There are no nodes being cleaned: terminate the Running node with the highest id.
-        (running_scc.iter().max().unwrap(), true)
+        (
+          running_scc
+            .iter()
+            .max_by_key(|&id| running_graph[*id])
+            .unwrap(),
+          true,
+        )
       };
 
       test_trace_log!(
@@ -168,7 +174,7 @@ impl<N: Node> InnerGraph<N> {
       // predecessor (which as a fellow member of the SCC, must also be reachable).
       let running_predecessor = running_graph
         .neighbors_directed(*running_candidate, Direction::Incoming)
-        .next()
+        .find(|id| running_scc.contains(id))
         .unwrap();
       let running_path: Vec<_> = petgraph::algo::all_simple_paths(
         &running_graph,
@@ -184,11 +190,12 @@ impl<N: Node> InnerGraph<N> {
       let candidate = running_graph[*running_candidate];
       if should_terminate {
         // Render the error, and terminate the Node with it.
-        let path_strs = running_path
+        let path = running_path
           .into_iter()
-          .map(|rni| self.pg[rni].node().to_string())
-          .collect();
-        self.pg[candidate].terminate(N::Error::cyclic(path_strs));
+          .map(|rni| self.pg[running_graph[rni]].node())
+          .collect::<Vec<_>>();
+        let error = N::cyclic_error(&path);
+        self.pg[candidate].terminate(error);
       } else {
         // Else, clear.
         let node = self.pg[candidate].node().clone();

src/rust/engine/graph/src/node.rs

@@ -53,6 +53,12 @@ pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static {
   fn cacheable_item(&self, _item: &Self::Item) -> bool {
     self.cacheable()
   }
+
+  ///
+  /// Creates an error instance that represents that a Node dependency was cyclic along the given
+  /// path.
+  ///
+  fn cyclic_error(path: &[&Self]) -> Self::Error;
 }
 
 pub trait NodeError: Clone + Debug + Eq + Send + Sync {
@@ -61,11 +67,6 @@ pub trait NodeError: Clone + Debug + Eq + Send + Sync {
   /// Graph (generally while running).
   ///
   fn invalidated() -> Self;
-
-  ///
-  /// Creates an instance that represents that a Node dependency was cyclic along the given path.
-  ///
-  fn cyclic(path: Vec<String>) -> Self;
 }
 
 ///

src/rust/engine/graph/src/tests.rs

@@ -624,7 +624,7 @@ async fn cyclic_failure() {
 
   assert_eq!(
     graph.create(TNode::new(2), &context).await,
-    Err(TError::Cyclic)
+    Err(TError::Cyclic(vec![0, 2, 1]))
   );
 }
 
@@ -740,6 +740,10 @@ impl Node for TNode {
   fn cacheable(&self) -> bool {
     self.cacheable
   }
+
+  fn cyclic_error(path: &[&Self]) -> Self::Error {
+    TError::Cyclic(path.iter().map(|n| n.id).collect())
+  }
 }
 
 impl std::fmt::Display for TNode {
@@ -991,15 +995,11 @@ impl Drop for AbortGuard {
 
 #[derive(Clone, Debug, Eq, PartialEq)]
 enum TError {
-  Cyclic,
+  Cyclic(Vec<usize>),
   Invalidated,
 }
 impl NodeError for TError {
   fn invalidated() -> Self {
     TError::Invalidated
   }
-
-  fn cyclic(_path: Vec<String>) -> Self {
-    TError::Cyclic
-  }
 }

src/rust/engine/src/nodes.rs

@@ -1569,6 +1569,32 @@ impl Node for NodeKey {
       _ => true,
     }
   }
+
+  fn cyclic_error(path: &[&NodeKey]) -> Failure {
+    let mut path = path.iter().map(|n| n.to_string()).collect::<Vec<_>>();
+    if !path.is_empty() {
+      path[0] += " <-";
+      path.push(path[0].clone());
+    }
+    let gil = Python::acquire_gil();
+    let url = externs::doc_url(
+      gil.python(),
+      "targets#dependencies-and-dependency-inference",
+    );
+    throw(format!(
+      "The dependency graph contained a cycle:\
+      \n\n  \
+      {}\
+      \n\n\
+      If the dependencies in the above path are for your BUILD targets, you may need to use more \
+      granular targets or replace BUILD target dependencies with file dependencies. If they are \
+      not for your BUILD targets, then please file a Github issue!\
+      \n\n\
+      See {} for more information.",
+      path.join("\n  "),
+      url
+    ))
+  }
 }
 
 impl Display for NodeKey {
@@ -1613,32 +1639,6 @@ impl NodeError for Failure {
   fn invalidated() -> Failure {
     Failure::Invalidated
   }
-
-  fn cyclic(mut path: Vec<String>) -> Failure {
-    let path_len = path.len();
-    if path_len > 1 {
-      path[0] += " <-";
-      path[path_len - 1] += " <-"
-    }
-    let gil = Python::acquire_gil();
-    let url = externs::doc_url(
-      gil.python(),
-      "targets#dependencies-and-dependency-inference",
-    );
-    throw(format!(
-      "The dependency graph contained a cycle:\
-      \n\n  \
-      {}\
-      \n\n\
-      If the dependencies in the above path are for your BUILD targets, you may need to use more \
-      granular targets or replace BUILD target dependencies with file dependencies. If they are \
-      not for your BUILD targets, then please file a Github issue!\
-      \n\n\
-      See {} for more information.",
-      path.join("\n  "),
-      url
-    ))
-  }
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]