From 55a9eb6ffa47f4f8678b3edbb538f15aac38e1a9 Mon Sep 17 00:00:00 2001 From: davidmarne-wf Date: Tue, 14 Apr 2026 08:45:43 -0700 Subject: [PATCH] fix: reachable_paths does not return all reachable paths (#8510) * topdown: add failing tests for graph.reachable_paths with shared ancestors Adds a regression test (shared_ancestor) reproducing the bug reported in #5871: given a graph where node 4 has edges to both 3 and 2, and 3 also has an edge to 2, graph.reachable_paths from 5 drops the path [5,4,2,1]. Also corrects the expected result for the existing cycle_1022_3 test, which was asserting the buggy output ([one,five,six] truncated) rather than the correct complete path [one,five,six,seven,eight,three]. Signed-off-by: David Marne * topdown: fix graph.reachable_paths dropping paths with shared ancestors graph.reachable_paths had two bugs that caused paths to be silently dropped when a node is reachable via multiple routes (diamond-shaped graphs). Bug 1: the `reached` set was mutated and shared across sibling recursive calls. After branch A finished traversal and added nodes to `reached`, branch B would see those nodes as already visited and terminate early, emitting a truncated path instead of continuing. Fix: copy `reached` once per pathBuilder invocation and pass the copy to all recursive calls. Each branch now has its own ancestor-only visited set. Bug 2: ast.NewArray stores the slice it receives directly (elems: a) without copying. When sibling calls appended to a shared backing array, the in-place write by one sibling corrupted the already-committed path term of a previous sibling. Fix: pass append([]*ast.Term(nil), path...) to each recursive call, giving each branch its own independent backing array. Signed-off-by: David Marne --------- Signed-off-by: David Marne --- .../reachable/test-reachable-paths-0422.yaml | 31 +++++++++++ .../reachable/test-reachable-paths-1022.yaml | 5 +- .../reachable/test-reachable-paths-0422.yaml | 22 ++++++++ .../reachable/test-reachable-paths-1022.yaml | 5 +- v1/topdown/reachable.go | 54 ++++++++----------- 5 files changed, 84 insertions(+), 33 deletions(-) diff --git a/v1/test/cases/testdata/v0/reachable/test-reachable-paths-0422.yaml b/v1/test/cases/testdata/v0/reachable/test-reachable-paths-0422.yaml index 063d67870e..9b5917e87b 100644 --- a/v1/test/cases/testdata/v0/reachable/test-reachable-paths-0422.yaml +++ b/v1/test/cases/testdata/v0/reachable/test-reachable-paths-0422.yaml @@ -167,6 +167,37 @@ cases: - - a - c + - data: {} + input_term: '{ + "graph": { + "1": [], + "2": ["1"], + "3": ["2"], + "4": ["3", "2"], + "5": ["4"] + }, + "initial": ["5"] + }' + modules: + - | + package reachable + + p = result { + graph.reachable_paths(input.graph, input.initial, result) + } + note: reachable_paths/shared_ancestor + query: data.reachable.p = x + want_result: + - x: + - - "5" + - "4" + - "2" + - "1" + - - "5" + - "4" + - "3" + - "2" + - "1" - data: {} input_term: '{ "graph": { diff --git a/v1/test/cases/testdata/v0/reachable/test-reachable-paths-1022.yaml b/v1/test/cases/testdata/v0/reachable/test-reachable-paths-1022.yaml index a409cb5892..995c13d017 100644 --- a/v1/test/cases/testdata/v0/reachable/test-reachable-paths-1022.yaml +++ b/v1/test/cases/testdata/v0/reachable/test-reachable-paths-1022.yaml @@ -117,11 +117,14 @@ cases: - - one - five - six + - nine - - one - five - six - - nine + - seven + - eight + - three - - one - two diff --git a/v1/test/cases/testdata/v1/reachable/test-reachable-paths-0422.yaml b/v1/test/cases/testdata/v1/reachable/test-reachable-paths-0422.yaml index 193703bb96..6c281e5b1e 100644 --- a/v1/test/cases/testdata/v1/reachable/test-reachable-paths-0422.yaml +++ b/v1/test/cases/testdata/v1/reachable/test-reachable-paths-0422.yaml @@ -119,6 +119,28 @@ cases: - c - - a - c + - note: reachable_paths/shared_ancestor + query: data.reachable.p = x + modules: + - | + package reachable + + p := result if { + graph.reachable_paths(input.graph, input.initial, result) + } + data: {} + input_term: '{ "graph": { "1": [], "2": ["1"], "3": ["2"], "4": ["3", "2"], "5": ["4"] }, "initial": ["5"] }' + want_result: + - x: + - - "5" + - "4" + - "2" + - "1" + - - "5" + - "4" + - "3" + - "2" + - "1" - note: reachable_paths/invalid_end query: data.reachable.p = x modules: diff --git a/v1/test/cases/testdata/v1/reachable/test-reachable-paths-1022.yaml b/v1/test/cases/testdata/v1/reachable/test-reachable-paths-1022.yaml index 9eb8f359bf..6b0899c4c7 100644 --- a/v1/test/cases/testdata/v1/reachable/test-reachable-paths-1022.yaml +++ b/v1/test/cases/testdata/v1/reachable/test-reachable-paths-1022.yaml @@ -71,10 +71,13 @@ cases: - - one - five - six + - nine - - one - five - six - - nine + - seven + - eight + - three - - one - two - four diff --git a/v1/topdown/reachable.go b/v1/topdown/reachable.go index 1c31019db9..683c31c6c6 100644 --- a/v1/topdown/reachable.go +++ b/v1/topdown/reachable.go @@ -74,39 +74,31 @@ func builtinReachable(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Ter // pathBuilder is called recursively to build a Set of paths that are reachable from the root func pathBuilder(graph ast.Object, root *ast.Term, path []*ast.Term, edgeRslt ast.Set, reached ast.Set) { - paths := []*ast.Term{} - - if edges := graph.Get(root); edges != nil { - path = append(path, root) - - if numberOfEdges(edges) >= 1 { - - foreachVertex(edges, func(neighbor *ast.Term) { - - if reached.Contains(neighbor) { - // If we've already reached this node, return current path (avoid infinite recursion) - paths = append(paths, path...) - edgeRslt.Add(ast.ArrayTerm(paths...)) - } else { - reached.Add(root) - pathBuilder(graph, neighbor, path, edgeRslt, reached) - - } - - }) - - } else { - paths = append(paths, path...) - edgeRslt.Add(ast.ArrayTerm(paths...)) - - } - } else { - // Node is nonexistent (not in graph). Commit the current path (without adding this root) - paths = append(paths, path...) - edgeRslt.Add(ast.ArrayTerm(paths...)) - + edges := graph.Get(root) + if edges == nil { + // Node not in graph — commit path without this node. + edgeRslt.Add(ast.ArrayTerm(path...)) + return } + path = append(path, root) + + if numberOfEdges(edges) == 0 { + edgeRslt.Add(ast.ArrayTerm(path...)) + return + } + + reached = reached.Copy() + reached.Add(root) + + foreachVertex(edges, func(neighbor *ast.Term) { + if reached.Contains(neighbor) { + // Cycle detected — commit current path. + edgeRslt.Add(ast.ArrayTerm(path...)) + } else { + pathBuilder(graph, neighbor, append([]*ast.Term(nil), path...), edgeRslt, reached) + } + }) } func builtinReachablePaths(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {