Fix dirtying of uncacheable nodes (Cherry-pick of pantsbuild#17079) (p…

…antsbuild#17396)

As reported in pantsbuild#17060, the same `@rule` running within a single "session"/"run" of Pants was not correctly re-running due to changed inputs. The `@rule` is uncacheable (via its output type being `EngineAwareReturnType.cacheable = False`), for the purposes of a logging side-effect.

The `@rule` was failing to re-run due to a bug in the dirtying of uncacheable rules that had likely existed since pantsbuild#13199 split `Node::restartable` out from `Node::cacheable`. When the `restartable` property was introduced, it took over controlling whether a Node could be interrupted while running. But the `Entry::dirty` codepath was still incorrectly preventing `uncacheable` `Node`s from being dirtied.

Fixes pantsbuild#17377.

src/rust/engine/graph/src/entry.rs

@@ -122,10 +122,12 @@ impl<N: Node> EntryResult<N> {
   /// If the value is in a Clean state, mark it Dirty.
   fn dirty(&mut self) {
     match self {
-      EntryResult::Clean(v) | EntryResult::UncacheableDependencies(v, _) => {
+      EntryResult::Clean(v)
+      | EntryResult::UncacheableDependencies(v, _)
+      | EntryResult::Uncacheable(v, _) => {
         *self = EntryResult::Dirty(v.clone());
       }
-      EntryResult::Dirty(_) | EntryResult::Uncacheable(_, _) => {}
+      EntryResult::Dirty(_) => {}
     }
   }
 

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

@@ -280,6 +280,12 @@ impl<N: Node> InnerGraph<N> {
       dirtied: transitive_ids.len(),
     };
 
+    // If there were no roots, then nothing will be invalidated. Return early to avoid scanning all
+    // edges in `retain_edges`.
+    if root_ids.is_empty() {
+      return invalidation_result;
+    }
+
     // Clear roots and remove their outbound edges.
     for id in &root_ids {
       if let Some(entry) = self.pg.node_weight_mut(*id) {

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

@@ -43,13 +43,18 @@ pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static {
   /// If a node's output is cacheable based solely on properties of the node, and not the output,
   /// return true.
   ///
+  /// Nodes which are not cacheable will be recomputed once (at least, in case of dirtying) per
+  /// RunId.
+  ///
   /// This property must remain stable for the entire lifetime of a particular Node, but a Node
   /// may change its cacheability for a particular output value using `cacheable_item`.
   ///
   fn cacheable(&self) -> bool;
 
+  ///
   /// A Node may want to compute cacheability differently based on properties of the Node's item.
   /// The output of this method will be and'd with `cacheable` to compute overall cacheability.
+  ///
   fn cacheable_item(&self, _item: &Self::Item) -> bool {
     self.cacheable()
   }

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

@@ -99,6 +99,53 @@ async fn invalidate_and_rerun() {
   assert_eq!(context.runs(), vec![TNode::new(1), TNode::new(2)]);
 }
 
+#[tokio::test]
+async fn invalidate_uncacheable() {
+  let graph = empty_graph();
+
+  // Create three nodes, with the middle node as uncacheable.
+  let context = {
+    let mut uncacheable = HashSet::new();
+    uncacheable.insert(TNode::new(1));
+    TContext::new(graph.clone()).with_uncacheable(uncacheable)
+  };
+  assert_eq!(
+    graph.create(TNode::new(2), &context).await,
+    Ok(vec![T(0, 0), T(1, 0), T(2, 0)])
+  );
+  assert_eq!(
+    context.runs(),
+    vec![TNode::new(2), TNode::new(1), TNode::new(0)]
+  );
+
+  // Clear the bottom Node, which dirties the middle and upper node.
+  assert_eq!(
+    graph.invalidate_from_roots(true, |n| n.id == 0),
+    InvalidationResult {
+      cleared: 1,
+      dirtied: 2
+    }
+  );
+
+  // Re-request in the same session with new salt, and validate that all three re-run.
+  let context = context.with_salt(1);
+  assert_eq!(
+    graph.create(TNode::new(2), &context).await,
+    Ok(vec![T(0, 1), T(1, 1), T(2, 1)])
+  );
+  assert_eq!(
+    context.runs(),
+    vec![
+      TNode::new(2),
+      TNode::new(1),
+      TNode::new(0),
+      TNode::new(1),
+      TNode::new(0),
+      TNode::new(2)
+    ]
+  );
+}
+
 #[tokio::test]
 async fn invalidate_with_changed_dependencies() {
   let graph = empty_graph();