From 92fd1a594bbc3a613814788bcfadf1994dc8e7a9 Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Thu, 28 Nov 2024 13:44:41 -0800 Subject: [PATCH] fix: Fix cycle in affected tracker. --- CHANGELOG.md | 1 + crates/affected/src/affected_tracker.rs | 72 +++++++++-- .../__fixtures__/projects/cycle-a/moon.yml | 1 + .../__fixtures__/projects/cycle-b/moon.yml | 1 + .../__fixtures__/projects/cycle-c/moon.yml | 1 + .../tests/__fixtures__/tasks/cycle/moon.yml | 10 ++ .../affected/tests/affected_tracker_test.rs | 119 +++++++++++++++++- 7 files changed, 192 insertions(+), 13 deletions(-) create mode 100644 crates/affected/tests/__fixtures__/projects/cycle-a/moon.yml create mode 100644 crates/affected/tests/__fixtures__/projects/cycle-b/moon.yml create mode 100644 crates/affected/tests/__fixtures__/projects/cycle-c/moon.yml create mode 100644 crates/affected/tests/__fixtures__/tasks/cycle/moon.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index a78138a3d53..7f4533f89e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fixed an issue where dependencies/dependents of an affected task would be skipped in the action graph if they were also not affected. +- Fixed a potential cycle (stack overflow) that may occur in the affected tracker. #### ⚙️ Internal diff --git a/crates/affected/src/affected_tracker.rs b/crates/affected/src/affected_tracker.rs index 769f9d86bf9..30ebdad59c5 100644 --- a/crates/affected/src/affected_tracker.rs +++ b/crates/affected/src/affected_tracker.rs @@ -151,6 +151,10 @@ impl<'app> AffectedTracker<'app> { } } + pub fn is_project_marked(&self, project: &Project) -> bool { + self.projects.contains_key(&project.id) + } + pub fn mark_project_affected( &mut self, project: &Project, @@ -170,13 +174,24 @@ impl<'app> AffectedTracker<'app> { .or_default() .insert(affected); - self.track_project_dependencies(project, 0)?; - self.track_project_dependents(project, 0)?; + self.track_project_dependencies(project, 0, &mut FxHashSet::default())?; + self.track_project_dependents(project, 0, &mut FxHashSet::default())?; Ok(()) } - fn track_project_dependencies(&mut self, project: &Project, depth: u16) -> miette::Result<()> { + fn track_project_dependencies( + &mut self, + project: &Project, + depth: u16, + cycle: &mut FxHashSet, + ) -> miette::Result<()> { + if cycle.contains(&project.id) { + return Ok(()); + } + + cycle.insert(project.id.clone()); + if self.project_upstream == UpstreamScope::None { trace!( project_id = project.id.as_str(), @@ -212,13 +227,24 @@ impl<'app> AffectedTracker<'app> { let dep_project = self.workspace_graph.get_project(&dep_config.id)?; - self.track_project_dependencies(&dep_project, depth + 1)?; + self.track_project_dependencies(&dep_project, depth + 1, cycle)?; } Ok(()) } - fn track_project_dependents(&mut self, project: &Project, depth: u16) -> miette::Result<()> { + fn track_project_dependents( + &mut self, + project: &Project, + depth: u16, + cycle: &mut FxHashSet, + ) -> miette::Result<()> { + if cycle.contains(&project.id) { + return Ok(()); + } + + cycle.insert(project.id.clone()); + if self.project_downstream == DownstreamScope::None { trace!( project_id = project.id.as_str(), @@ -254,7 +280,7 @@ impl<'app> AffectedTracker<'app> { let dep_project = self.workspace_graph.get_project(&dep_id)?; - self.track_project_dependents(&dep_project, depth + 1)?; + self.track_project_dependents(&dep_project, depth + 1, cycle)?; } Ok(()) @@ -339,8 +365,8 @@ impl<'app> AffectedTracker<'app> { .or_default() .insert(affected); - self.track_task_dependencies(task, 0)?; - self.track_task_dependents(task, 0)?; + self.track_task_dependencies(task, 0, &mut FxHashSet::default())?; + self.track_task_dependents(task, 0, &mut FxHashSet::default())?; if let Some(project_id) = task.target.get_project_id() { self.projects @@ -352,7 +378,18 @@ impl<'app> AffectedTracker<'app> { Ok(()) } - fn track_task_dependencies(&mut self, task: &Task, depth: u16) -> miette::Result<()> { + fn track_task_dependencies( + &mut self, + task: &Task, + depth: u16, + cycle: &mut FxHashSet, + ) -> miette::Result<()> { + if cycle.contains(&task.target) { + return Ok(()); + } + + cycle.insert(task.target.clone()); + if self.task_upstream == UpstreamScope::None { trace!( task_target = task.target.as_str(), @@ -388,13 +425,24 @@ impl<'app> AffectedTracker<'app> { let dep_task = self.workspace_graph.get_task(&dep_config.target)?; - self.track_task_dependencies(&dep_task, depth + 1)?; + self.track_task_dependencies(&dep_task, depth + 1, cycle)?; } Ok(()) } - fn track_task_dependents(&mut self, task: &Task, depth: u16) -> miette::Result<()> { + fn track_task_dependents( + &mut self, + task: &Task, + depth: u16, + cycle: &mut FxHashSet, + ) -> miette::Result<()> { + if cycle.contains(&task.target) { + return Ok(()); + } + + cycle.insert(task.target.clone()); + if self.task_downstream == DownstreamScope::None { trace!( task_target = task.target.as_str(), @@ -430,7 +478,7 @@ impl<'app> AffectedTracker<'app> { let dep_task = self.workspace_graph.get_task(&dep_target)?; - self.track_task_dependents(&dep_task, depth + 1)?; + self.track_task_dependents(&dep_task, depth + 1, cycle)?; } Ok(()) diff --git a/crates/affected/tests/__fixtures__/projects/cycle-a/moon.yml b/crates/affected/tests/__fixtures__/projects/cycle-a/moon.yml new file mode 100644 index 00000000000..6728c5eb877 --- /dev/null +++ b/crates/affected/tests/__fixtures__/projects/cycle-a/moon.yml @@ -0,0 +1 @@ +dependsOn: ['cycle-b'] diff --git a/crates/affected/tests/__fixtures__/projects/cycle-b/moon.yml b/crates/affected/tests/__fixtures__/projects/cycle-b/moon.yml new file mode 100644 index 00000000000..5fc74c22c25 --- /dev/null +++ b/crates/affected/tests/__fixtures__/projects/cycle-b/moon.yml @@ -0,0 +1 @@ +dependsOn: ['cycle-c'] diff --git a/crates/affected/tests/__fixtures__/projects/cycle-c/moon.yml b/crates/affected/tests/__fixtures__/projects/cycle-c/moon.yml new file mode 100644 index 00000000000..538042968ec --- /dev/null +++ b/crates/affected/tests/__fixtures__/projects/cycle-c/moon.yml @@ -0,0 +1 @@ +dependsOn: ['cycle-a'] diff --git a/crates/affected/tests/__fixtures__/tasks/cycle/moon.yml b/crates/affected/tests/__fixtures__/tasks/cycle/moon.yml new file mode 100644 index 00000000000..2a2e0f335e6 --- /dev/null +++ b/crates/affected/tests/__fixtures__/tasks/cycle/moon.yml @@ -0,0 +1,10 @@ +tasks: + a: + deps: ['b'] + inputs: ['a.txt'] + b: + deps: ['c'] + inputs: ['b.txt'] + c: + deps: ['a'] + inputs: ['c.txt'] diff --git a/crates/affected/tests/affected_tracker_test.rs b/crates/affected/tests/affected_tracker_test.rs index 2569124aecc..64a1e5ae771 100644 --- a/crates/affected/tests/affected_tracker_test.rs +++ b/crates/affected/tests/affected_tracker_test.rs @@ -198,6 +198,33 @@ mod affected_projects { ]) ); } + + #[tokio::test] + async fn deep_cycle() { + use moon_test_utils2::pretty_assertions::assert_eq; + + let workspace_graph = generate_workspace_graph("projects").await; + let touched_files = FxHashSet::from_iter(["cycle-a/file.txt".into()]); + + let mut tracker = AffectedTracker::new(&workspace_graph, &touched_files); + tracker.with_project_scopes(UpstreamScope::Deep, DownstreamScope::None); + tracker.track_projects().unwrap(); + let affected = tracker.build(); + + assert_eq!( + affected.projects, + FxHashMap::from_iter([ + (Id::raw("cycle-a"), { + let mut state = create_state_from_file("cycle-a/file.txt"); + state.downstream.insert(Id::raw("cycle-c")); + state + }), + (Id::raw("cycle-b"), create_state_from_dependent("cycle-a")), + (Id::raw("cycle-c"), create_state_from_dependent("cycle-b")), + (Id::raw("root"), create_state_from_file("cycle-a/file.txt")), + ]) + ); + } } mod project_downstream { @@ -301,6 +328,30 @@ mod affected_projects { ]) ); } + + #[tokio::test] + async fn deep_cycle() { + let workspace_graph = generate_workspace_graph("projects").await; + let touched_files = FxHashSet::from_iter(["cycle-c/file.txt".into()]); + + let mut tracker = AffectedTracker::new(&workspace_graph, &touched_files); + tracker.with_project_scopes(UpstreamScope::None, DownstreamScope::Deep); + tracker.track_projects().unwrap(); + let affected = tracker.build(); + + assert_eq!( + affected.projects, + FxHashMap::from_iter([ + (Id::raw("cycle-a"), create_state_from_dependency("cycle-b")), + (Id::raw("cycle-b"), create_state_from_dependency("cycle-c")), + ( + Id::raw("cycle-c"), + create_state_from_file("cycle-c/file.txt") + ), + (Id::raw("root"), create_state_from_file("cycle-c/file.txt")), + ]) + ); + } } } @@ -644,11 +695,44 @@ mod affected_tasks { ]) ); } + + #[tokio::test] + async fn deep_cycle() { + let workspace_graph = generate_workspace_graph("tasks").await; + let touched_files = FxHashSet::from_iter(["cycle/c.txt".into()]); + + let mut tracker = AffectedTracker::new(&workspace_graph, &touched_files); + tracker.with_task_scopes(UpstreamScope::Deep, DownstreamScope::None); + tracker.track_tasks().unwrap(); + let affected = tracker.build(); + + assert_eq!( + affected.tasks, + FxHashMap::from_iter([ + ( + Target::parse("cycle:global").unwrap(), + create_state_from_file("cycle/c.txt") + ), + (Target::parse("cycle:c").unwrap(), { + let mut state = create_state_from_file("cycle/c.txt"); + state.downstream.insert(Target::parse("cycle:b").unwrap()); + state + }), + ( + Target::parse("cycle:a").unwrap(), + create_state_from_dependent("cycle:c") + ), + ( + Target::parse("cycle:b").unwrap(), + create_state_from_dependent("cycle:a") + ), + ]) + ); + } } mod task_downstream { use super::*; - use moon_test_utils2::pretty_assertions::assert_eq; #[tokio::test] async fn none() { @@ -786,5 +870,38 @@ mod affected_tasks { ]) ); } + + #[tokio::test] + async fn deep_cycle() { + let workspace_graph = generate_workspace_graph("tasks").await; + let touched_files = FxHashSet::from_iter(["cycle/c.txt".into()]); + + let mut tracker = AffectedTracker::new(&workspace_graph, &touched_files); + tracker.with_task_scopes(UpstreamScope::None, DownstreamScope::Deep); + tracker.track_tasks().unwrap(); + let affected = tracker.build(); + + assert_eq!( + affected.tasks, + FxHashMap::from_iter([ + ( + Target::parse("cycle:global").unwrap(), + create_state_from_file("cycle/c.txt") + ), + ( + Target::parse("cycle:c").unwrap(), + create_state_from_file("cycle/c.txt") + ), + ( + Target::parse("cycle:b").unwrap(), + create_state_from_dependency("cycle:c") + ), + ( + Target::parse("cycle:a").unwrap(), + create_state_from_dependency("cycle:b") + ), + ]) + ); + } } }