Don't render cancelled nodes in trace (#5252)

Problem
#3695 initially described a solution to a problem experienced in the original "slow-failing" implementation of the engine: if there were multiple paths between a root and a failure, we would render all of them, which was hugely redundant.

But since that ticket was opened, we switched to a fast-failing implementation via the switch to Futures. When one dependency of a Node fails, the rest are effectively cancelled, since Futures are pull based. This (should) have the effect of rendering exactly one failure path per root: the first failure that is encountered while computing that root.

But it was still the case that we were printing redundant/useless information: cancelled dependencies were being rendered, which in the case of things like ./pants list :: resulted in a lot of noise.

Solution
Since a cancelled node is never the source of a failure, don't render them.

Result
The added test no longer renders a <None> entry, like:

Computing Select(<pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =A)
  Computing Task(<class 'pants_test.engine.test_engine.A'>, <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =A)
    Computing Task(<class 'pants_test.engine.test_engine.D'>, <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =D)
      <None>
    Computing Task(<function nested_raise at 0xEEEEEEEEE>, <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =C)
      Throw(An exception for B)
        Traceback (most recent call last):
          File LOCATION-INFO, in extern_invoke_runnable
            val = runnable(*args)
          File LOCATION-INFO, in nested_raise
            fn_raises(x)
          File LOCATION-INFO, in fn_raises
            raise Exception('An exception for {}'.format(type(x).__name__))
        Exception: An exception for B
Fixes #3695.

src/rust/engine/src/graph.rs

@@ -379,11 +379,16 @@ impl InnerGraph {
 
     let is_bottom = |eid: EntryId| -> bool {
       match self.unsafe_entry_for_id(eid).peek::<NodeKey>() {
-        None |
         Some(Err(Failure::Invalidated)) => false,
         Some(Err(Failure::Noop(..))) => true,
         Some(Err(Failure::Throw(..))) => false,
         Some(Ok(_)) => true,
+        None => {
+          // A Node with no state is either still running, or effectively cancelled
+          // because a dependent failed. In either case, it's not useful to render
+          // them, as we don't know whether they would have succeeded or failed.
+          true
+        }
       }
     };
 

tests/python/pants_test/engine/test_engine.py

@@ -56,6 +56,14 @@ class B(object):
   pass
 
 
+class C(object):
+  pass
+
+
+class D(object):
+  pass
+
+
 def fn_raises(x):
   raise Exception('An exception for {}'.format(type(x).__name__))
 
@@ -126,3 +134,34 @@ def test_include_trace_error_raises_error_with_trace(self):
             Exception: An exception for B
       ''').lstrip()+'\n',
       remove_locations_from_traceback(str(cm.exception)))
+
+  def test_trace_does_not_include_cancellations(self):
+    # Tests that when the computation of `Select(C)` fails, the cancellation of `Select(D)`
+    # is not rendered as a failure.
+    rules = [
+      RootRule(B),
+      TaskRule(D, [Select(B)], D),
+      TaskRule(C, [Select(B)], nested_raise),
+      TaskRule(A, [Select(C), Select(D)], A),
+    ]
+
+    scheduler = self.scheduler(rules, include_trace_on_error=True)
+    with self.assertRaises(Exception) as cm:
+      list(scheduler.product_request(A, subjects=[(B())]))
+
+    self.assert_equal_with_printing(dedent('''
+      Received unexpected Throw state(s):
+      Computing Select(<pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =A)
+        Computing Task(<class 'pants_test.engine.test_engine.A'>, <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =A)
+          Computing Task(<function nested_raise at 0xEEEEEEEEE>, <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, =C)
+            Throw(An exception for B)
+              Traceback (most recent call last):
+                File LOCATION-INFO, in extern_invoke_runnable
+                  val = runnable(*args)
+                File LOCATION-INFO, in nested_raise
+                  fn_raises(x)
+                File LOCATION-INFO, in fn_raises
+                  raise Exception('An exception for {}'.format(type(x).__name__))
+              Exception: An exception for B
+      ''').lstrip()+'\n',
+      remove_locations_from_traceback(str(cm.exception)))