From 4357b9a00d5506753d52692a8a4804cb10155ccc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 5 May 2026 18:39:43 -0500 Subject: [PATCH 1/2] fix(graph): use round-based topological sort in sortNodeIDsWithPriority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-queue Kahn's algorithm processed one node at a time, which could interleave nodes from different topological levels. This aligns sortNodeIDsWithPriority with the round-based pattern already used by sortNodeIDs — collecting all zero-in-degree nodes per round, sorting within the round by priority, and emitting the full round before advancing. --- pkg/devcontainer/graph/graph.go | 31 +++-------- pkg/devcontainer/graph/graph_test.go | 83 ++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 23 deletions(-) diff --git a/pkg/devcontainer/graph/graph.go b/pkg/devcontainer/graph/graph.go index 5c4fd496f..2fac49fe8 100644 --- a/pkg/devcontainer/graph/graph.go +++ b/pkg/devcontainer/graph/graph.go @@ -445,17 +445,16 @@ func processNeighbors( func (g *Graph[T]) sortNodeIDsWithPriority(priority map[string]int) ([]string, error) { workingInDegree := copyInDegreeMap(g.inDegree) - ready := initializeQueue(workingInDegree) sortedResult := make([]string, 0, len(g.nodes)) - for len(ready) > 0 { - sortByPriority(ready, priority) - - current := ready[0] - ready = ready[1:] - sortedResult = append(sortedResult, current) - - processNeighborsUnordered(g.edges, current, workingInDegree, &ready) + for { + round := collectZeroInDegree(workingInDegree) + if len(round) == 0 { + break + } + sortByPriority(round, priority) + sortedResult = append(sortedResult, round...) + advanceRound(round, g.edges, workingInDegree) } if len(sortedResult) != len(g.nodes) { @@ -483,20 +482,6 @@ func comparePriority(a, b string, priority map[string]int) bool { return a < b } -func processNeighborsUnordered( - edges map[string][]string, - currentNode string, - inDegree map[string]int, - queue *[]string, -) { - for _, neighbor := range edges[currentNode] { - inDegree[neighbor]-- - if inDegree[neighbor] == 0 { - *queue = append(*queue, neighbor) - } - } -} - func circularDependencyError(inDegree map[string]int) error { remaining := []string{} for id, deg := range inDegree { diff --git a/pkg/devcontainer/graph/graph_test.go b/pkg/devcontainer/graph/graph_test.go index 523384c88..afee6be36 100644 --- a/pkg/devcontainer/graph/graph_test.go +++ b/pkg/devcontainer/graph/graph_test.go @@ -641,3 +641,86 @@ func (suite *GraphTestSuite) TestTopologicalSortAdvanced() { }) } } + +func (suite *GraphTestSuite) TestSortWithPriorityRoundBased() { + testCases := []struct { + name string + setup func() + priority map[string]int + expected []string + }{ + { + name: "round-based emits all zero-in-degree before advancing", + setup: suite.buildParallelChains, + priority: map[string]int{"B": 1, "A": 2}, + expected: []string{"B", "A", "C", "D"}, + }, + { + name: "diamond graph groups siblings in same round", + setup: suite.buildDiamond, + priority: map[string]int{"C": 1, "B": 2}, + expected: []string{"A", "C", "B", "D"}, + }, + { + name: "nodes without priority sort alphabetically after prioritized nodes", + setup: suite.buildIndependentXYZ, + priority: map[string]int{"Z": 1}, + expected: []string{"Z", "X", "Y"}, + }, + { + name: "circular dependency returns error", + setup: suite.buildCycle, + priority: map[string]int{"A": 1}, + expected: nil, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + suite.SetupTest() + tc.setup() + + result, err := suite.graph.sortNodeIDsWithPriority(tc.priority) + if tc.expected == nil { + suite.Error(err) + suite.Contains(err.Error(), "circular dependency") + } else { + suite.NoError(err) + suite.Equal(tc.expected, result) + } + }) + } +} + +func (suite *GraphTestSuite) buildParallelChains() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddNode("D", "dataD")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) + suite.Require().NoError(suite.graph.AddEdge("B", "D")) +} + +func (suite *GraphTestSuite) buildDiamond() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddNode("D", "dataD")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) + suite.Require().NoError(suite.graph.AddEdge("B", "D")) + suite.Require().NoError(suite.graph.AddEdge("C", "D")) +} + +func (suite *GraphTestSuite) buildIndependentXYZ() { + suite.Require().NoError(suite.graph.AddNode("X", "dataX")) + suite.Require().NoError(suite.graph.AddNode("Y", "dataY")) + suite.Require().NoError(suite.graph.AddNode("Z", "dataZ")) +} + +func (suite *GraphTestSuite) buildCycle() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddEdge("A", "B")) + suite.Require().NoError(suite.graph.AddEdge("B", "A")) +} From 8c8018d4c795907ae47f32ba051fcf2ba4e23c1a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 5 May 2026 18:53:04 -0500 Subject: [PATCH 2/2] test(graph): add mid-round promotion test for round-based priority sort Adds a test case where the old single-queue algorithm would produce a different (incorrect) ordering by promoting C ahead of B when A frees C mid-queue. The round-based approach correctly emits [A, B] in round 1 and [C] in round 2. --- pkg/devcontainer/graph/graph_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/devcontainer/graph/graph_test.go b/pkg/devcontainer/graph/graph_test.go index afee6be36..eaedd0e69 100644 --- a/pkg/devcontainer/graph/graph_test.go +++ b/pkg/devcontainer/graph/graph_test.go @@ -667,6 +667,12 @@ func (suite *GraphTestSuite) TestSortWithPriorityRoundBased() { priority: map[string]int{"Z": 1}, expected: []string{"Z", "X", "Y"}, }, + { + name: "mid-round promotion prevented by round boundary", + setup: suite.buildMidRoundPromotion, + priority: map[string]int{"A": 1, "C": 2, "B": 3}, + expected: []string{"A", "B", "C"}, + }, { name: "circular dependency returns error", setup: suite.buildCycle, @@ -718,6 +724,13 @@ func (suite *GraphTestSuite) buildIndependentXYZ() { suite.Require().NoError(suite.graph.AddNode("Z", "dataZ")) } +func (suite *GraphTestSuite) buildMidRoundPromotion() { + suite.Require().NoError(suite.graph.AddNode("A", "dataA")) + suite.Require().NoError(suite.graph.AddNode("B", "dataB")) + suite.Require().NoError(suite.graph.AddNode("C", "dataC")) + suite.Require().NoError(suite.graph.AddEdge("A", "C")) +} + func (suite *GraphTestSuite) buildCycle() { suite.Require().NoError(suite.graph.AddNode("A", "dataA")) suite.Require().NoError(suite.graph.AddNode("B", "dataB"))