Fix GraphFlow cycle detection to properly clean up recursion state (#7026)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ekzhu <320302+ekzhu@users.noreply.github.com> Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
This commit is contained in:
@@ -165,12 +165,13 @@ class DiGraph(BaseModel):
|
||||
visited.add(node_name)
|
||||
rec_stack.add(node_name)
|
||||
path.append(node_name)
|
||||
cycle = False
|
||||
|
||||
for edge in self.nodes[node_name].edges:
|
||||
target = edge.target
|
||||
if target not in visited:
|
||||
if dfs(target):
|
||||
return True
|
||||
cycle = True
|
||||
elif target in rec_stack:
|
||||
# Found a cycle → extract the cycle
|
||||
cycle_start_index = path.index(target)
|
||||
@@ -182,11 +183,11 @@ class DiGraph(BaseModel):
|
||||
raise ValueError(
|
||||
f"Cycle detected without exit condition: {' -> '.join(cycle_nodes + cycle_nodes[:1])}"
|
||||
)
|
||||
return True # Found cycle, but it has an exit condition
|
||||
cycle = True # Found cycle, but it has an exit condition
|
||||
|
||||
rec_stack.remove(node_name)
|
||||
path.pop()
|
||||
return False
|
||||
return cycle
|
||||
|
||||
has_cycle = False
|
||||
for node in self.nodes:
|
||||
|
||||
@@ -247,6 +247,51 @@ def test_cycle_detection_without_exit_condition() -> None:
|
||||
graph.has_cycles_with_exit()
|
||||
|
||||
|
||||
def test_cycle_detection_cleanup_bug() -> None:
|
||||
"""Test that cycle detection properly cleans up recursion state.
|
||||
|
||||
This test reproduces the bug where the DFS algorithm in has_cycles_with_exit
|
||||
didn't properly clean up rec_stack and path when returning early upon finding
|
||||
a cycle with valid exit conditions. The bug could cause incorrect behavior
|
||||
when processing subsequent unvisited nodes in graphs with multiple components.
|
||||
"""
|
||||
|
||||
# Create a graph that exposes the cleanup bug:
|
||||
# A -> B -> C -> A (cycle with condition)
|
||||
# A -> D (separate branch that could be affected by stale rec_stack/path)
|
||||
# E (disconnected component that could be affected by cleanup issues)
|
||||
graph = DiGraph(
|
||||
nodes={
|
||||
"A": DiGraphNode(name="A", edges=[DiGraphEdge(target="B"), DiGraphEdge(target="D")]),
|
||||
"B": DiGraphNode(name="B", edges=[DiGraphEdge(target="C")]),
|
||||
"C": DiGraphNode(name="C", edges=[DiGraphEdge(target="A", condition="loop")]),
|
||||
"D": DiGraphNode(name="D", edges=[]),
|
||||
"E": DiGraphNode(name="E", edges=[]), # Disconnected component
|
||||
}
|
||||
)
|
||||
|
||||
# This should work correctly with the fix - proper cleanup ensures
|
||||
# that processing node E is not affected by stale recursion state
|
||||
# from processing the A->B->C->A cycle
|
||||
result = graph.has_cycles_with_exit()
|
||||
assert result is True # Has valid cycles
|
||||
|
||||
# Test with multiple cycles to ensure thorough cleanup
|
||||
multi_cycle_graph = DiGraph(
|
||||
nodes={
|
||||
"A": DiGraphNode(name="A", edges=[DiGraphEdge(target="B")]),
|
||||
"B": DiGraphNode(name="B", edges=[DiGraphEdge(target="A", condition="cycle1")]),
|
||||
"C": DiGraphNode(name="C", edges=[DiGraphEdge(target="D")]),
|
||||
"D": DiGraphNode(name="D", edges=[DiGraphEdge(target="C", condition="cycle2")]),
|
||||
"E": DiGraphNode(name="E", edges=[DiGraphEdge(target="F")]),
|
||||
"F": DiGraphNode(name="F", edges=[]),
|
||||
}
|
||||
)
|
||||
|
||||
result = multi_cycle_graph.has_cycles_with_exit()
|
||||
assert result is True # Has valid cycles
|
||||
|
||||
|
||||
def test_different_activation_groups_detection() -> None:
|
||||
"""Test different activation groups."""
|
||||
graph = DiGraph(
|
||||
|
||||
Reference in New Issue
Block a user