Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces loop edge support to the graph execution system, transforming it from a purely DAG-based executor to one that supports controlled cycles through explicit loop/exit edge types. The changes implement exclusive conditional matching, enhance state isolation, and add comprehensive loop handling logic.
- Adds
EdgeTypeenumeration (Normal, Loop, Exit) to explicitly mark edges that form cycles - Implements exclusive matching for conditional edges where only the first matching condition is executed
- Introduces complex loop state management with deferred scheduling, pending runs/skips, and node reset capabilities
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| graph/graph.go | Adds edge types, loop validation, and modified cycle detection to allow marked loop edges |
| graph/executor.go | Extends node metadata to track loop dependencies and loop edge sources |
| graph/task.go | Implements core loop execution logic including deferred scheduling, state reset, and loop propagation tracking |
| graph/graph_test.go | Updates tests for new error messages and adds comprehensive loop feature test coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Allow it only if the cycle includes a loop edge | ||
| if !nextHasLoop { | ||
| cycleStart := 0 | ||
| for i, name := range stack { | ||
| if name == next { | ||
| cycleStart = i | ||
| break | ||
| } | ||
| } | ||
| cycle := append(append([]string{}, stack[cycleStart:]...), next) |
There was a problem hiding this comment.
The cycle detection logic is flawed. When a back edge is found (line 278), the code checks !nextHasLoop to determine if the cycle is invalid. However, nextHasLoop includes hasLoopEdge from the parameter, which tracks whether ANY loop edge was seen on the path from the DFS root to the current node.
The correct check should only validate whether the cycle itself (from next back to next) contains a loop edge. Consider this graph: A -> B (loop edge), B -> C, C -> A. When visiting C->A, nextHasLoop would be true because B->C had hasLoopEdge=true, even though the cycle C->A doesn't contain a loop edge.
| // Allow it only if the cycle includes a loop edge | |
| if !nextHasLoop { | |
| cycleStart := 0 | |
| for i, name := range stack { | |
| if name == next { | |
| cycleStart = i | |
| break | |
| } | |
| } | |
| cycle := append(append([]string{}, stack[cycleStart:]...), next) | |
| // Allow it only if the cycle itself includes a loop edge | |
| cycleStart := 0 | |
| for i, name := range stack { | |
| if name == next { | |
| cycleStart = i | |
| break | |
| } | |
| } | |
| cycle := append(append([]string{}, stack[cycleStart:]...), next) | |
| hasLoop := false | |
| // Check each edge in the cycle for EdgeTypeLoop | |
| for i := 0; i < len(cycle)-1; i++ { | |
| from := cycle[i] | |
| to := cycle[i+1] | |
| for _, e := range g.edges[from] { | |
| if e.to == to && e.edgeType == EdgeTypeLoop { | |
| hasLoop = true | |
| break | |
| } | |
| } | |
| if hasLoop { | |
| break | |
| } | |
| } | |
| if !hasLoop { |
| case t.pendingRuns[node]: | ||
| delete(t.pendingRuns, node) | ||
| if t.received[node] != 0 && !t.visited[node] { | ||
| delete(t.skipped, node) |
There was a problem hiding this comment.
When a pending run is processed after a node completes, the looping flag is not restored. Consider this scenario:
- Node A receives a loop edge signal while in-flight, setting
pendingRuns[A] = true - Node A completes, and
looping[A]is deleted at line 331 - The pending run is processed at lines 349-352, re-scheduling A
- When A executes this time,
looping[A]is false
This means that when A propagates to its children, the loop context is lost. Children won't be marked as looping (line 464-465), which could affect the logic at line 454 that checks t.looping[from] to decide whether to reset a visited node.
Consider re-setting the looping flag when processing pending runs, similar to how it's set in prepareLoopNodeLocked at line 447.
| delete(t.skipped, node) | |
| delete(t.skipped, node) | |
| t.looping[node] = true // Restore looping context for pending run |
|
|
||
| for _, edge := range g.edges[node] { | ||
| next := edge.to | ||
| nextHasLoop := hasLoopEdge || edge.edgeType == EdgeTypeLoop |
There was a problem hiding this comment.
The hasLoopEdge tracking logic is incorrect. hasLoopEdge should only be true if there's a loop edge in the path from the root to the current node, not just anywhere in the traversal. The current implementation propagates hasLoopEdge along all paths once any loop edge is encountered, which means even paths that don't contain a loop edge will be marked as having one.
This could allow cycles that don't actually contain a loop edge to pass validation. The fix should track whether the current cycle path contains a loop edge, not whether any path from the root contained one.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Send skip to all other edges | ||
| for i := range info.outEdges { | ||
| edge := &info.outEdges[i] | ||
| if edge.to != matchedEdge.to { |
There was a problem hiding this comment.
In the presence of multiple edges to the same target node with different conditions, this comparison could incorrectly skip sending skip signals. If two edges point to the same destination but only the first one matches, the second edge would not send a skip signal because edge.to == matchedEdge.to. Consider comparing edge indices or checking if the current edge is the matched one more explicitly.
| if edge.to != matchedEdge.to { | |
| if edge != matchedEdge { |
| t.skipped[node] = true | ||
| return t.addSkipLocked(node, parent) |
There was a problem hiding this comment.
Setting t.skipped[node] = true unconditionally before checking if the skip was already registered could be misleading. If addSkipLocked returns false (duplicate skip), the node gets marked as skipped even though this particular skip signal wasn't registered. Consider only setting t.skipped[node] = true when addSkipLocked returns true, or add a comment explaining that marking skipped is idempotent.
| t.skipped[node] = true | |
| return t.addSkipLocked(node, parent) | |
| registered := t.addSkipLocked(node, parent) | |
| if registered { | |
| t.skipped[node] = true | |
| } | |
| return registered |
| t.Fatalf("execution error: %v", err) | ||
| } | ||
|
|
||
| expectedSequence := []int{1, 2, 3} |
There was a problem hiding this comment.
[nitpick] The magic number sequence []int{1, 2, 3} should be derived from test parameters or documented. This expected sequence corresponds to max_attempts set to 3 on line 3058, but the relationship isn't immediately clear. Consider adding a comment or computing this from the test's max_attempts value.
| expectedSequence := []int{1, 2, 3} | |
| // expectedSequence corresponds to the number of attempts, i.e., 1..max_attempts | |
| maxAttempts := 3 // must match the value set in the test setup | |
| expectedSequence := make([]int, maxAttempts) | |
| for i := range expectedSequence { | |
| expectedSequence[i] = i + 1 | |
| } |
No description provided.