fix: Fix affected chains in action graph.

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## Unreleased
+
+#### 🐞 Fixes
+
+- Fixed an issue where dependencies/dependents of an affected task would be skipped in the action
+  graph if they were also not affected.
+
+#### ⚙️ Internal
+
+- Improved task dependent resolution in the action graph.
+
 ## 1.30.1
 
 #### 🐞 Fixes

crates/action-graph/src/action_graph_builder.rs

@@ -257,6 +257,17 @@ impl<'app> ActionGraphBuilder<'app> {
         reqs: &RunRequirements,
         config: Option<&TaskDependencyConfig>,
     ) -> miette::Result<Option<NodeIndex>> {
+        // Create a new requirements object as we don't have our dependencies/
+        // dependents to check for affected or run their own dependents!
+        let child_reqs = RunRequirements {
+            ci: reqs.ci,
+            ci_check: reqs.ci_check,
+            dependents: false,
+            interactive: reqs.interactive,
+            skip_affected: true,
+            ..Default::default()
+        };
+
         // Only apply checks when requested. This applies to `moon ci`,
         // but not `moon run`, since the latter should be able to
         // manually run local tasks in CI (deploys, etc).
@@ -280,7 +291,7 @@ impl<'app> ActionGraphBuilder<'app> {
                     );
 
                     if affected.is_task_marked(task) {
-                        self.run_task_dependents(task, reqs)?;
+                        self.run_task_dependents(task, &child_reqs)?;
                     }
                 }
             }
@@ -323,7 +334,7 @@ impl<'app> ActionGraphBuilder<'app> {
 
         // Compare against touched files if provided
         if let Some(affected) = &mut self.affected {
-            if !affected.is_task_marked(task) {
+            if !reqs.skip_affected && !affected.is_task_marked(task) {
                 return Ok(None);
             }
         }
@@ -348,14 +359,14 @@ impl<'app> ActionGraphBuilder<'app> {
                 "Linking dependencies for task",
             );
 
-            edges.extend(self.run_task_dependencies(task, reqs)?);
+            edges.extend(self.run_task_dependencies(task, &child_reqs)?);
         }
 
         self.link_requirements(index, edges);
 
         // And possibly dependents
         if reqs.dependents {
-            self.run_task_dependents(task, reqs)?;
+            self.run_task_dependents(task, &child_reqs)?;
         }
 
         Ok(Some(index))
@@ -367,14 +378,9 @@ impl<'app> ActionGraphBuilder<'app> {
     pub fn run_task_dependencies(
         &mut self,
         task: &Task,
-        child_reqs: &RunRequirements,
+        reqs: &RunRequirements,
     ) -> miette::Result<Vec<NodeIndex>> {
         let parallel = task.options.run_deps_in_parallel;
-        let reqs = RunRequirements {
-            ci: child_reqs.ci,
-            ..Default::default()
-        };
-
         let mut indices = vec![];
         let mut previous_target_index = None;
 
@@ -405,60 +411,19 @@ impl<'app> ActionGraphBuilder<'app> {
         Ok(indices)
     }
 
-    // This is costly, is there a better way to do this?
     #[instrument(skip_all)]
     pub fn run_task_dependents(
         &mut self,
         task: &Task,
-        parent_reqs: &RunRequirements,
+        reqs: &RunRequirements,
     ) -> miette::Result<Vec<NodeIndex>> {
         let mut indices = vec![];
 
-        // Create a new requirements object as we only want direct
-        // dependents, and shouldn't recursively create.
-        let reqs = RunRequirements {
-            ci: parent_reqs.ci,
-            interactive: parent_reqs.interactive,
-            skip_affected: true,
-            ..Default::default()
-        };
-
-        if let TargetScope::Project(project_locator) = &task.target.scope {
-            let mut projects_to_build = vec![];
-
-            // From self project
-            let self_project = self.workspace_graph.get_project(project_locator)?;
-
-            projects_to_build.push(self_project.clone());
+        for dep_target in self.workspace_graph.tasks.dependents_of(task) {
+            let (_, dep_indices) = self.run_task_by_target(dep_target, reqs)?;
 
-            // From other projects
-            for dependent_id in self.workspace_graph.projects.dependents_of(&self_project) {
-                projects_to_build.push(self.workspace_graph.get_project(&dependent_id)?);
-            }
-
-            for project in projects_to_build {
-                // Don't skip internal tasks, since they are a dependency of the parent
-                // task, and must still run! They just can't be ran manually.
-                for dep_task in self.workspace_graph.get_tasks_from_project(&project.id)? {
-                    // But do skip persistent tasks!
-                    if dep_task.is_persistent() {
-                        continue;
-                    }
-
-                    // Since these are transient tasks, we should always filter out tasks
-                    // that should not run in CI, as we don't know what side-effects it
-                    // will cause. This applies to both `moon ci` and `moon run`.
-                    if parent_reqs.ci && !dep_task.should_run_in_ci() {
-                        self.passthrough_targets.insert(dep_task.target.clone());
-                        continue;
-                    }
-
-                    if dep_task.deps.iter().any(|dep| dep.target == task.target) {
-                        if let Some(index) = self.run_task(&project, &dep_task, &reqs)? {
-                            indices.push(index);
-                        }
-                    }
-                }
+            for dep_index in dep_indices {
+                indices.push(dep_index);
             }
         }
 
@@ -713,3 +678,12 @@ impl<'app> ActionGraphBuilder<'app> {
         index
     }
 }
+
+#[cfg(debug_assertions)]
+impl<'app> ActionGraphBuilder<'app> {
+    pub fn mock_affected(&mut self, mut op: impl FnMut(&mut AffectedTracker<'app>)) {
+        if let Some(affected) = self.affected.as_mut() {
+            op(affected);
+        }
+    }
+}

crates/action-graph/tests/__fixtures__/tasks/deps-affected/moon.yml

@@ -0,0 +1,12 @@
+tasks:
+  a:
+    command: 'a'
+    inputs: ['a.txt']
+    deps: ['b']
+  b:
+    command: 'b'
+    inputs: ['b.txt']
+    deps: ['c']
+  c:
+    command: 'c'
+    inputs: ['c.txt']

crates/action-graph/tests/action_graph_test.rs

@@ -333,68 +333,6 @@ mod action_graph {
             );
         }
 
-        #[tokio::test]
-        async fn doesnt_graph_if_not_affected_by_touched_files() {
-            let sandbox = create_sandbox("projects");
-            let container = ActionGraphContainer::new(sandbox.path()).await;
-            let mut builder = container.create_builder();
-
-            let project = container.workspace_graph.get_project("bar").unwrap();
-
-            let mut task = create_task("build", "bar");
-            task.platform = PlatformType::Node;
-
-            // Empty set works fine, just needs to be some
-            let touched_files = FxHashSet::default();
-            builder.set_touched_files(&touched_files).unwrap();
-
-            builder
-                .run_task(&project, &task, &RunRequirements::default())
-                .unwrap();
-
-            let graph = builder.build();
-
-            assert!(!topo(graph).into_iter().any(|node| {
-                if let ActionNode::RunTask(inner) = &node {
-                    inner.target.as_str() == "bar:build"
-                } else {
-                    false
-                }
-            }));
-        }
-
-        #[tokio::test]
-        async fn graphs_if_affected_by_touched_files() {
-            let sandbox = create_sandbox("projects");
-            let container = ActionGraphContainer::new(sandbox.path()).await;
-            let mut builder = container.create_builder();
-
-            let file = WorkspaceRelativePathBuf::from("bar/file.js");
-
-            let project = container.workspace_graph.get_project("bar").unwrap();
-
-            let mut task = create_task("build", "bar");
-            task.platform = PlatformType::Node;
-            task.input_files.insert(file.clone());
-
-            let touched_files = FxHashSet::from_iter([file]);
-            builder.set_touched_files(&touched_files).unwrap();
-            builder
-                .affected
-                .as_mut()
-                .unwrap()
-                .mark_task_affected(&task, moon_affected::AffectedBy::AlwaysAffected)
-                .unwrap();
-
-            builder
-                .run_task(&project, &task, &RunRequirements::default())
-                .unwrap();
-
-            let graph = builder.build();
-
-            assert!(!topo(graph).is_empty());
-        }
-
         #[tokio::test]
         async fn task_can_have_a_diff_platform_from_project() {
             let sandbox = create_sandbox("projects");
@@ -947,6 +885,137 @@ mod action_graph {
             );
         }
 
+        mod affected {
+            use super::*;
+            use moon_affected::AffectedBy;
+            use moon_test_utils2::pretty_assertions::assert_eq;
+
+            #[tokio::test]
+            async fn doesnt_graph_if_not_affected_by_touched_files() {
+                let sandbox = create_sandbox("projects");
+                let container = ActionGraphContainer::new(sandbox.path()).await;
+                let mut builder = container.create_builder();
+
+                let project = container.workspace_graph.get_project("bar").unwrap();
+
+                let mut task = create_task("build", "bar");
+                task.platform = PlatformType::Node;
+
+                // Empty set works fine, just needs to be some
+                let touched_files = FxHashSet::default();
+                builder.set_touched_files(&touched_files).unwrap();
+
+                builder
+                    .run_task(&project, &task, &RunRequirements::default())
+                    .unwrap();
+
+                let graph = builder.build();
+
+                assert!(!topo(graph).into_iter().any(|node| {
+                    if let ActionNode::RunTask(inner) = &node {
+                        inner.target.as_str() == "bar:build"
+                    } else {
+                        false
+                    }
+                }));
+            }
+
+            #[tokio::test]
+            async fn graphs_if_affected_by_touched_files() {
+                let sandbox = create_sandbox("projects");
+                let container = ActionGraphContainer::new(sandbox.path()).await;
+                let mut builder = container.create_builder();
+
+                let file = WorkspaceRelativePathBuf::from("bar/file.js");
+
+                let project = container.workspace_graph.get_project("bar").unwrap();
+
+                let mut task = create_task("build", "bar");
+                task.platform = PlatformType::Node;
+                task.input_files.insert(file.clone());
+
+                let touched_files = FxHashSet::from_iter([file]);
+                builder.set_touched_files(&touched_files).unwrap();
+                builder.mock_affected(|affected| {
+                    affected
+                        .mark_task_affected(&task, AffectedBy::AlwaysAffected)
+                        .unwrap();
+                });
+
+                builder
+                    .run_task(&project, &task, &RunRequirements::default())
+                    .unwrap();
+
+                let graph = builder.build();
+
+                assert!(!topo(graph).is_empty());
+            }
+
+            #[tokio::test]
+            async fn includes_deps_if_owning_task_is_affected() {
+                let sandbox = create_sandbox("tasks");
+                let container = ActionGraphContainer::new(sandbox.path()).await;
+                let mut builder = container.create_builder();
+
+                let project = container
+                    .workspace_graph
+                    .get_project("deps-affected")
+                    .unwrap();
+                let task = container
+                    .workspace_graph
+                    .get_task_from_project("deps-affected", "b")
+                    .unwrap();
+
+                let touched_files =
+                    FxHashSet::from_iter([WorkspaceRelativePathBuf::from("deps-affected/b.txt")]);
+                builder.set_touched_files(&touched_files).unwrap();
+                builder.mock_affected(|affected| {
+                    affected
+                        .mark_task_affected(&task, AffectedBy::AlwaysAffected)
+                        .unwrap();
+                });
+
+                builder
+                    .run_task(
+                        &project,
+                        &task,
+                        &RunRequirements {
+                            dependents: true,
+                            ..Default::default()
+                        },
+                    )
+                    .unwrap();
+
+                let graph = builder.build();
+
+                assert_eq!(
+                    topo(graph),
+                    vec![
+                        ActionNode::sync_workspace(),
+                        ActionNode::setup_toolchain(SetupToolchainNode {
+                            runtime: Runtime::system()
+                        }),
+                        ActionNode::sync_project(SyncProjectNode {
+                            project: Id::raw("deps-affected"),
+                            runtime: Runtime::system()
+                        }),
+                        ActionNode::run_task(RunTaskNode::new(
+                            Target::parse("deps-affected:c").unwrap(),
+                            Runtime::system()
+                        )),
+                        ActionNode::run_task(RunTaskNode::new(
+                            Target::parse("deps-affected:b").unwrap(),
+                            Runtime::system()
+                        )),
+                        ActionNode::run_task(RunTaskNode::new(
+                            Target::parse("deps-affected:a").unwrap(),
+                            Runtime::system()
+                        )),
+                    ]
+                );
+            }
+        }
+
         mod run_in_ci {
             use super::*;