From e88b8baaa0b426a35d7deb392c942f8352bf096a Mon Sep 17 00:00:00 2001 From: branchseer Date: Sun, 15 Feb 2026 13:51:05 +0800 Subject: [PATCH] refactor: replace ExecutionGraph with generic AcyclicGraph and show full cycle path in errors - Rename ExecutionGraph to generic AcyclicGraph with ExecutionGraph as a type alias - Remove precomputed topological order; compute on-the-fly via compute_topological_order() - Use DFS with predecessor tracking to detect cycles and reconstruct the full cycle path - Display all tasks in cycle errors as arrow chain (e.g. pkg#a -> pkg#b -> pkg#a) - Handle graphs with multiple disconnected components correctly - Add 14 unit tests covering DAGs, cycles, self-loops, and multi-component graphs --- crates/vite_task/src/session/execute/mod.rs | 15 +- .../snapshots/cycle dependency error.snap | 2 +- crates/vite_task_plan/src/error.rs | 10 +- crates/vite_task_plan/src/execution_graph.rs | 450 ++++++++++++++++-- crates/vite_task_plan/src/lib.rs | 4 + crates/vite_task_plan/src/plan.rs | 35 +- .../query - cycle dependency error.snap | 2 +- 7 files changed, 438 insertions(+), 80 deletions(-) diff --git a/crates/vite_task/src/session/execute/mod.rs b/crates/vite_task/src/session/execute/mod.rs index 73a37186..469e7f92 100644 --- a/crates/vite_task/src/session/execute/mod.rs +++ b/crates/vite_task/src/session/execute/mod.rs @@ -56,8 +56,8 @@ struct ExecutionContext<'a> { impl ExecutionContext<'_> { /// Execute all tasks in an execution graph in dependency order. /// - /// `ExecutionGraph` guarantees acyclicity at construction time and caches a - /// topological order. We iterate `toposort()` in reverse to get execution order + /// `ExecutionGraph` guarantees acyclicity at construction time. + /// We compute a topological order and iterate in reverse to get execution order /// (dependencies before dependents). /// /// The `path_prefix` tracks our position within nested execution graphs. For the @@ -71,14 +71,15 @@ impl ExecutionContext<'_> { graph: &ExecutionGraph, path_prefix: &LeafExecutionPath, ) { - // `toposort()` returns nodes in topological order: for every edge A→B, - // A appears before B. Since our edges mean "A depends on B", dependencies - // (B) appear after their dependents (A). We iterate in reverse to get - // execution order where dependencies run first. + // `compute_topological_order()` returns nodes in topological order: for every + // edge A→B, A appears before B. Since our edges mean "A depends on B", + // dependencies (B) appear after their dependents (A). We iterate in reverse + // to get execution order where dependencies run first. // Execute tasks in dependency-first order. Each task may have multiple items // (from `&&`-split commands), which are executed sequentially. - for &node_ix in graph.toposort().iter().rev() { + let topo_order = graph.compute_topological_order(); + for &node_ix in topo_order.iter().rev() { let task_execution = &graph[node_ix]; for (item_idx, item) in task_execution.items.iter().enumerate() { diff --git a/crates/vite_task_bin/tests/e2e_snapshots/fixtures/error_cycle_dependency/snapshots/cycle dependency error.snap b/crates/vite_task_bin/tests/e2e_snapshots/fixtures/error_cycle_dependency/snapshots/cycle dependency error.snap index 6dc0a699..b9256d25 100644 --- a/crates/vite_task_bin/tests/e2e_snapshots/fixtures/error_cycle_dependency/snapshots/cycle dependency error.snap +++ b/crates/vite_task_bin/tests/e2e_snapshots/fixtures/error_cycle_dependency/snapshots/cycle dependency error.snap @@ -3,4 +3,4 @@ source: crates/vite_task_bin/tests/e2e_snapshots/main.rs expression: e2e_outputs --- [1]> vp run task-a # task-a -> task-b -> task-a cycle -Error: Cycle dependency detected involving task error-cycle-dependency-test#task-b +Error: Cycle dependency detected: error-cycle-dependency-test#task-a -> error-cycle-dependency-test#task-b -> error-cycle-dependency-test#task-a diff --git a/crates/vite_task_plan/src/error.rs b/crates/vite_task_plan/src/error.rs index 51c07c2f..8bba1aac 100644 --- a/crates/vite_task_plan/src/error.rs +++ b/crates/vite_task_plan/src/error.rs @@ -146,11 +146,11 @@ pub enum Error { /// A cycle was detected in the task dependency graph during planning. /// - /// This is caught by `ExecutionGraph::try_from_graph`, which validates that the - /// graph is a DAG. The contained `TaskDisplay` identifies one of the tasks - /// involved in the cycle. - #[error("Cycle dependency detected involving task {0}")] - CycleDependencyDetected(TaskDisplay), + /// This is caught by `AcyclicGraph::try_from_graph`, which validates that the + /// graph is a DAG. The contained path lists all tasks forming the cycle + /// as a closed loop (the first and last elements are the same). + #[error("Cycle dependency detected: {}", _0.iter().map(std::string::ToString::to_string).collect::>().join(" -> "))] + CycleDependencyDetected(Vec), } impl Error { diff --git a/crates/vite_task_plan/src/execution_graph.rs b/crates/vite_task_plan/src/execution_graph.rs index 214b0e46..f9879f3e 100644 --- a/crates/vite_task_plan/src/execution_graph.rs +++ b/crates/vite_task_plan/src/execution_graph.rs @@ -1,10 +1,14 @@ use std::ops::Deref; -use petgraph::graph::{DefaultIx, DiGraph, EdgeIndex, IndexType, NodeIndex}; +use petgraph::{ + graph::{DefaultIx, DiGraph, EdgeIndex, IndexType, NodeIndex}, + visit::{DfsEvent, depth_first_search}, +}; +use rustc_hash::FxHashMap; use crate::TaskExecution; -/// newtype of `DefaultIx` for indices in task graphs +/// newtype of `DefaultIx` for indices in execution graphs #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ExecutionIx(DefaultIx); // SAFETY: ExecutionIx is a newtype over DefaultIx which already implements IndexType correctly @@ -28,29 +32,28 @@ pub type ExecutionEdgeIndex = EdgeIndex; /// The inner directed graph type used for construction and storage. pub(crate) type InnerExecutionGraph = DiGraph; -/// Error returned by [`ExecutionGraph::try_from_graph`] when a cycle is detected. +/// Error returned by [`AcyclicGraph::try_from_graph`] when a cycle is detected. /// -/// Contains the [`ExecutionNodeIndex`] of one node that participates in the cycle, -/// allowing the caller to look up task details for error reporting. +/// Contains the full cycle path as node indices. The path is a closed loop: +/// `[n0, n1, ..., nk, n0]` representing the cycle `n0 → n1 → ... → nk → n0`. #[derive(Debug)] -pub struct CycleError { - node_id: ExecutionNodeIndex, +pub struct CycleError { + cycle_path: Vec>, } -impl CycleError { - /// Return a node index that participates in the detected cycle. +impl CycleError { + /// Return the full cycle path as node indices. + /// + /// The path is a closed loop: `[n0, n1, ..., nk, n0]` where each consecutive + /// pair `(path[i], path[i+1])` is an edge in the graph. #[must_use] - pub const fn node_id(&self) -> ExecutionNodeIndex { - self.node_id + pub fn cycle_path(&self) -> &[NodeIndex] { + &self.cycle_path } } -/// A directed acyclic execution graph. -/// -/// Wraps a `petgraph::graph::DiGraph` with a compile-time guarantee that it contains -/// no cycles. The acyclicity invariant is validated at construction time by -/// [`try_from_graph`](Self::try_from_graph), which performs a topological sort and -/// caches the resulting order for later use. +/// A directed acyclic graph with a compile-time guarantee (via construction-time +/// validation) that it contains no cycles. /// /// Unlike `petgraph::acyclic::Acyclic`, this type is `Sync` (no internal `RefCell`), /// so it can be safely shared via `Arc` across threads. @@ -58,80 +61,425 @@ impl CycleError { /// The type implements `Deref>`, so all read operations on the /// inner graph (indexing, iteration, node/edge counts) work transparently. #[derive(Debug)] -pub struct ExecutionGraph { - /// The underlying directed graph. - graph: InnerExecutionGraph, - /// Pre-computed topological order of node indices. - /// For every edge A→B in the graph, A appears before B in this vector. - toposort: Vec, +pub struct AcyclicGraph { + /// The underlying directed graph, guaranteed to be acyclic. + graph: DiGraph, } -impl ExecutionGraph { - /// Validate that `graph` is acyclic and wrap it in an `ExecutionGraph`. +impl AcyclicGraph { + /// Validate that `graph` is acyclic and wrap it in an `AcyclicGraph`. /// - /// Performs a topological sort using `petgraph::algo::toposort`. If the graph - /// contains a cycle, returns a [`CycleError`] identifying one node in the cycle. - /// On success, the topological order is cached so that subsequent calls to - /// [`toposort`](Self::toposort) are free. + /// Uses a DFS to detect cycles. If a cycle is found, returns a [`CycleError`] + /// containing the full cycle path. The cycle detection correctly handles graphs + /// with multiple disconnected components. /// /// # Errors /// /// Returns [`CycleError`] if the graph contains a cycle. - pub fn try_from_graph(graph: InnerExecutionGraph) -> Result { - let toposort = petgraph::algo::toposort(&graph, None) - .map_err(|cycle| CycleError { node_id: cycle.node_id() })?; - Ok(Self { graph, toposort }) + pub fn try_from_graph(graph: DiGraph) -> Result> { + if let Some(cycle_path) = find_cycle_path(&graph) { + return Err(CycleError { cycle_path }); + } + Ok(Self { graph }) } /// Return a reference to the underlying `DiGraph`. /// /// Useful when an API requires `&DiGraph` explicitly (e.g. `vite_graph_ser::serialize_by_key`). #[must_use] - pub const fn inner(&self) -> &InnerExecutionGraph { + pub const fn inner(&self) -> &DiGraph { &self.graph } - /// Build an `ExecutionGraph` from an iterator of task executions with no edges. + /// Build an `AcyclicGraph` from an iterator of nodes with no edges. /// - /// Each task execution becomes an independent node in the graph. Since there are + /// Each node becomes an independent node in the graph. Since there are /// no edges, the graph is trivially acyclic. - pub fn from_node_list(nodes: impl IntoIterator) -> Self { - let mut graph = InnerExecutionGraph::default(); - let mut toposort = Vec::new(); + pub fn from_node_list(nodes: impl IntoIterator) -> Self { + let mut graph = DiGraph::default(); for node in nodes { - toposort.push(graph.add_node(node)); + graph.add_node(node); } - Self { graph, toposort } + Self { graph } } - /// Return the pre-computed topological order of node indices. + /// Compute the topological order of node indices. /// - /// For every edge A→B in the graph, A appears before B in the returned slice. + /// For every edge A→B in the graph, A appears before B in the returned vector. /// Since our edges mean "A depends on B", dependents come before their /// dependencies. Callers that need execution order (dependencies first) should /// iterate in reverse. + /// + /// This computes the order on each call rather than caching it. + /// + /// # Panics + /// + /// Panics if the graph contains a cycle, which should be impossible since + /// acyclicity is validated at construction time. #[must_use] - pub fn toposort(&self) -> &[ExecutionNodeIndex] { - &self.toposort + pub fn compute_topological_order(&self) -> Vec> { + petgraph::algo::toposort(&self.graph, None) + .expect("AcyclicGraph: acyclicity validated at construction time") } } -impl Default for ExecutionGraph { - /// Create an empty `ExecutionGraph` (no nodes, no edges). +impl Default for AcyclicGraph { + /// Create an empty `AcyclicGraph` (no nodes, no edges). /// /// An empty graph is trivially acyclic. fn default() -> Self { - Self { graph: InnerExecutionGraph::default(), toposort: Vec::new() } + Self { graph: DiGraph::default() } } } /// Deref to the inner `DiGraph` so that read-only graph operations /// (`node_count()`, `node_weights()`, `node_indices()`, indexing by `NodeIndex`, etc.) -/// work transparently on `ExecutionGraph`. -impl Deref for ExecutionGraph { - type Target = InnerExecutionGraph; +/// work transparently on `AcyclicGraph`. +impl Deref for AcyclicGraph { + type Target = DiGraph; fn deref(&self) -> &Self::Target { &self.graph } } + +/// The execution graph type alias, specialized for task execution. +pub type ExecutionGraph = AcyclicGraph; + +/// Find a cycle in the directed graph, returning the cycle path if one exists. +/// +/// Uses a DFS with predecessor tracking. When a back edge `u → v` is detected +/// (where `v` is an ancestor of `u` in the DFS tree), the predecessor chain from +/// `u` back to `v` is walked to reconstruct the full cycle path. +/// +/// The returned path is a closed loop: `[v, ..., u, v]` representing the cycle +/// `v → ... → u → v`. +/// +/// Handles graphs with multiple disconnected components by starting DFS from +/// all nodes via `graph.node_indices()`. +fn find_cycle_path(graph: &DiGraph) -> Option>> { + let mut predecessor = FxHashMap::, NodeIndex>::default(); + + let result: Result<(), Vec>> = + depth_first_search(graph, graph.node_indices(), |event| match event { + DfsEvent::TreeEdge(u, v) => { + predecessor.insert(v, u); + Ok(()) + } + DfsEvent::BackEdge(u, v) => { + // v is an ancestor of u in the DFS tree. + // Walk the predecessor chain from u back to v to reconstruct the cycle. + let mut path = vec![u]; + let mut current = u; + while current != v { + current = predecessor[¤t]; + path.push(current); + } + path.reverse(); // now [v, ..., u] + path.push(v); // close the cycle: [v, ..., u, v] + Err(path) + } + _ => Ok(()), + }); + + result.err() +} + +#[cfg(test)] +#[expect(clippy::many_single_char_names, reason = "short names are clear for graph test nodes")] +mod tests { + use petgraph::graph::{DefaultIx, DiGraph, NodeIndex}; + + use super::*; + + /// Assert that `cycle_path` is a valid cycle in `graph`: + /// - first == last (closed loop) + /// - length >= 2 + /// - each consecutive pair `(path[i], path[i+1])` is an edge in the graph + fn assert_valid_cycle( + graph: &DiGraph, + cycle_path: &[NodeIndex], + ) { + assert!( + cycle_path.len() >= 2, + "cycle path must have at least 2 elements, got {cycle_path:?}" + ); + assert_eq!( + cycle_path.first(), + cycle_path.last(), + "cycle must be closed (first == last), got {cycle_path:?}" + ); + for window in cycle_path.windows(2) { + assert!( + graph.contains_edge(window[0], window[1]), + "missing edge {:?} -> {:?} in graph; cycle_path = {cycle_path:?}", + window[0], + window[1], + ); + } + } + + #[test] + fn empty_graph() { + let graph = AcyclicGraph::::default(); + assert_eq!(graph.node_count(), 0); + assert!(graph.compute_topological_order().is_empty()); + } + + #[test] + fn single_node() { + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + assert_eq!(graph.node_count(), 1); + assert_eq!(graph.compute_topological_order(), vec![a]); + } + + #[test] + fn linear_chain() { + // A → B → C + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + g.add_edge(a, b, ()); + g.add_edge(b, c, ()); + + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + let order = graph.compute_topological_order(); + let pos_a = order.iter().position(|&n| n == a).unwrap(); + let pos_b = order.iter().position(|&n| n == b).unwrap(); + let pos_c = order.iter().position(|&n| n == c).unwrap(); + assert!(pos_a < pos_b, "a must come before b"); + assert!(pos_b < pos_c, "b must come before c"); + } + + #[test] + fn diamond_dag() { + // A + // / \ + // B C + // \ / + // D + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + let d = g.add_node("d"); + g.add_edge(a, b, ()); + g.add_edge(a, c, ()); + g.add_edge(b, d, ()); + g.add_edge(c, d, ()); + + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + let order = graph.compute_topological_order(); + let pos = |n: NodeIndex| order.iter().position(|&x| x == n).unwrap(); + assert!(pos(a) < pos(b)); + assert!(pos(a) < pos(c)); + assert!(pos(b) < pos(d)); + assert!(pos(c) < pos(d)); + } + + #[test] + fn simple_cycle() { + // A → B → A + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + g.add_edge(a, b, ()); + g.add_edge(b, a, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_valid_cycle(&g, path); + // Both nodes must appear in the cycle + let unique: rustc_hash::FxHashSet<_> = path.iter().copied().collect(); + assert!(unique.contains(&a)); + assert!(unique.contains(&b)); + } + + #[test] + fn self_loop() { + // A → A + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + g.add_edge(a, a, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_eq!(path, &[a, a]); + assert_valid_cycle(&g, path); + } + + #[test] + fn three_node_cycle() { + // A → B → C → A + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + g.add_edge(a, b, ()); + g.add_edge(b, c, ()); + g.add_edge(c, a, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_valid_cycle(&g, path); + let unique: rustc_hash::FxHashSet<_> = path.iter().copied().collect(); + assert!(unique.contains(&a)); + assert!(unique.contains(&b)); + assert!(unique.contains(&c)); + } + + #[test] + fn from_node_list_is_acyclic() { + let graph = AcyclicGraph::<&str>::from_node_list(["a", "b", "c"]); + assert_eq!(graph.node_count(), 3); + assert_eq!(graph.compute_topological_order().len(), 3); + } + + #[test] + fn compute_topological_order_valid() { + // Verify every edge A→B has A before B in the topological order + let mut g = DiGraph::::new(); + let n0 = g.add_node(0); + let n1 = g.add_node(1); + let n2 = g.add_node(2); + let n3 = g.add_node(3); + let n4 = g.add_node(4); + g.add_edge(n0, n1, ()); + g.add_edge(n0, n2, ()); + g.add_edge(n1, n3, ()); + g.add_edge(n2, n3, ()); + g.add_edge(n3, n4, ()); + + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + let order = graph.compute_topological_order(); + let pos = |n: NodeIndex| order.iter().position(|&x| x == n).unwrap(); + + // Check every edge + assert!(pos(n0) < pos(n1)); + assert!(pos(n0) < pos(n2)); + assert!(pos(n1) < pos(n3)); + assert!(pos(n2) < pos(n3)); + assert!(pos(n3) < pos(n4)); + } + + #[test] + fn deref_to_inner() { + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("hello"); + g.add_node("world"); + + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + // Deref allows calling DiGraph methods directly + assert_eq!(graph.node_count(), 2); + assert_eq!(graph.edge_count(), 0); + assert_eq!(graph[a], "hello"); + assert_eq!(graph.node_weights().count(), 2); + } + + #[test] + fn disconnected_acyclic_components() { + // Component 1: A → B + // Component 2: C → D + // No edges between components — acyclic + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + let d = g.add_node("d"); + g.add_edge(a, b, ()); + g.add_edge(c, d, ()); + + let graph = AcyclicGraph::try_from_graph(g).unwrap(); + let order = graph.compute_topological_order(); + assert_eq!(order.len(), 4); + let pos = |n: NodeIndex| order.iter().position(|&x| x == n).unwrap(); + assert!(pos(a) < pos(b)); + assert!(pos(c) < pos(d)); + } + + #[test] + fn disconnected_with_cycle_in_one_component() { + // Component 1: A → B (acyclic) + // Component 2: C → D → C (cycle) + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + let d = g.add_node("d"); + g.add_edge(a, b, ()); + g.add_edge(c, d, ()); + g.add_edge(d, c, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_valid_cycle(&g, path); + + // The cycle path must only contain nodes from the cyclic component (c, d) + let unique: rustc_hash::FxHashSet<_> = path.iter().copied().collect(); + assert!(unique.contains(&c), "cycle path must contain c"); + assert!(unique.contains(&d), "cycle path must contain d"); + assert!(!unique.contains(&a), "cycle path must not contain a"); + assert!(!unique.contains(&b), "cycle path must not contain b"); + } + + #[test] + fn disconnected_with_cycles_in_multiple_components() { + // Component 1: A → B → A (cycle) + // Component 2: C → D → C (cycle) + let mut g = DiGraph::<&str, ()>::new(); + let a = g.add_node("a"); + let b = g.add_node("b"); + let c = g.add_node("c"); + let d = g.add_node("d"); + g.add_edge(a, b, ()); + g.add_edge(b, a, ()); + g.add_edge(c, d, ()); + g.add_edge(d, c, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_valid_cycle(&g, path); + + // DFS detects the first cycle encountered — nodes must be from one component + let unique: rustc_hash::FxHashSet<_> = path.iter().copied().collect(); + let is_component_1 = unique.contains(&a) && unique.contains(&b); + let is_component_2 = unique.contains(&c) && unique.contains(&d); + assert!(is_component_1 || is_component_2, "cycle path must belong to one component"); + assert!(!(is_component_1 && is_component_2), "cycle path must not span components"); + } + + #[test] + fn large_graph_cycle_in_later_component() { + // Component 1: chain of 10 nodes (n0 → n1 → ... → n9), acyclic + // Component 2: X → Y → X (cycle) + let mut g = DiGraph::::new(); + + let mut chain_nodes = Vec::new(); + for i in 0..10 { + chain_nodes.push(g.add_node(i)); + } + for i in 0..9 { + g.add_edge(chain_nodes[i], chain_nodes[i + 1], ()); + } + + let x = g.add_node(100); + let y = g.add_node(101); + g.add_edge(x, y, ()); + g.add_edge(y, x, ()); + + let err = AcyclicGraph::try_from_graph(g.clone()).unwrap_err(); + let path = err.cycle_path(); + assert_valid_cycle(&g, path); + + // The cycle must be in the second component + let unique: rustc_hash::FxHashSet<_> = path.iter().copied().collect(); + assert!(unique.contains(&x), "cycle path must contain x"); + assert!(unique.contains(&y), "cycle path must contain y"); + for &chain_node in &chain_nodes { + assert!(!unique.contains(&chain_node), "cycle path must not contain chain nodes"); + } + } +} diff --git a/crates/vite_task_plan/src/lib.rs b/crates/vite_task_plan/src/lib.rs index c4f1cd25..e80f6764 100644 --- a/crates/vite_task_plan/src/lib.rs +++ b/crates/vite_task_plan/src/lib.rs @@ -151,6 +151,10 @@ fn serialize_execution_graph_by_key( /// An execution item, from a split subcommand in a task's command (`item1 && item2 && ...`). #[derive(Debug, Serialize)] +#[expect( + clippy::large_enum_variant, + reason = "SpawnExecution in Leaf is large but not worth boxing" +)] pub enum ExecutionItemKind { /// Expanded from a known vp subcommand, like `vp run ...` or a synthesized task. Expanded(#[serde(serialize_with = "serialize_execution_graph_by_key")] ExecutionGraph), diff --git a/crates/vite_task_plan/src/plan.rs b/crates/vite_task_plan/src/plan.rs index cb394079..615a89d4 100644 --- a/crates/vite_task_plan/src/plan.rs +++ b/crates/vite_task_plan/src/plan.rs @@ -544,23 +544,28 @@ pub async fn plan_query_request( } // Validate the graph is acyclic. - // `try_from_graph` performs a topological sort; if a cycle is found, - // it returns `CycleError` identifying one node in the cycle. + // `try_from_graph` performs a DFS; if a cycle is found, it returns + // `CycleError` containing the full cycle path as node indices. ExecutionGraph::try_from_graph(inner_graph).map_err(|cycle| { - // Look up the human-readable task display for the node involved in the cycle. - // Every node in `inner_graph` was added via `inner_graph.add_node()` above, - // with a corresponding entry inserted into `execution_node_indices_by_task_index`. - // The cycle error's node_id comes from the same graph, so the lookup always succeeds. - let display = execution_node_indices_by_task_index + // Map each execution node index in the cycle path to its human-readable TaskDisplay. + // Every node in the cycle was added via `inner_graph.add_node()` above, + // with a corresponding entry in `execution_node_indices_by_task_index`. + let displays = cycle + .cycle_path() .iter() - .find_map(|(task_idx, &exec_idx)| { - if exec_idx == cycle.node_id() { - Some(context.indexed_task_graph().display_task(*task_idx)) - } else { - None - } + .map(|&exec_idx| { + execution_node_indices_by_task_index + .iter() + .find_map(|(task_idx, &mapped_exec_idx)| { + if mapped_exec_idx == exec_idx { + Some(context.indexed_task_graph().display_task(*task_idx)) + } else { + None + } + }) + .expect("cycle node must exist in execution_node_indices_by_task_index") }) - .expect("cycle node must exist in execution_node_indices_by_task_index"); - Error::CycleDependencyDetected(display) + .collect(); + Error::CycleDependencyDetected(displays) }) } diff --git a/crates/vite_task_plan/tests/plan_snapshots/fixtures/cycle-dependency/snapshots/query - cycle dependency error.snap b/crates/vite_task_plan/tests/plan_snapshots/fixtures/cycle-dependency/snapshots/query - cycle dependency error.snap index 7d0db585..c967fd8c 100644 --- a/crates/vite_task_plan/tests/plan_snapshots/fixtures/cycle-dependency/snapshots/query - cycle dependency error.snap +++ b/crates/vite_task_plan/tests/plan_snapshots/fixtures/cycle-dependency/snapshots/query - cycle dependency error.snap @@ -3,4 +3,4 @@ source: crates/vite_task_plan/tests/plan_snapshots/main.rs expression: err_str.as_ref() input_file: crates/vite_task_plan/tests/plan_snapshots/fixtures/cycle-dependency --- -Cycle dependency detected involving task cycle-dependency-test#task-b +Cycle dependency detected: cycle-dependency-test#task-a -> cycle-dependency-test#task-b -> cycle-dependency-test#task-a