Fix detecting cycles in parallelVisit

Fix detecting cycles in parallelVisit when the first alphabetical
module is not part of the cycle, but depends on the cycle.  Instead of
starting from the first alphabetical module, check every module in a
determinsitic order and return the first time a cycle is found.

Test: Test_parallelVisit
Change-Id: I03726f838ec42975251088ba75158103940115c2

context.go

@@ -2074,23 +2074,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 		}
 
 		if len(pauseMap) > 0 {
-			// Probably a deadlock due to a newly added dependency cycle.
-			// Start from a semi-random module being paused for and perform a depth-first
-			// search for the module it is paused on, ignoring modules that are marked as
-			// done.  Note this traverses from modules to the modules that would have been
-			// unblocked when that module finished.
-			var start, end *moduleInfo
-			for _, pauseSpecs := range pauseMap {
-				for _, pauseSpec := range pauseSpecs {
-					if start == nil || start.String() > pauseSpec.paused.String() {
-						start = pauseSpec.paused
-						end = pauseSpec.until
-					}
-				}
-			}
+			// Probably a deadlock due to a newly added dependency cycle. Start from each module in
+			// the order of the input modules list and perform a depth-first search for the module
+			// it is paused on, ignoring modules that are marked as done.  Note this traverses from
+			// modules to the modules that would have been unblocked when that module finished, i.e
+			// the reverse of the visitOrderer.
 
-			var check func(group *moduleInfo) []*moduleInfo
-			check = func(module *moduleInfo) []*moduleInfo {
+			var check func(module, end *moduleInfo) []*moduleInfo
+			check = func(module, end *moduleInfo) []*moduleInfo {
 				if module.waitingCount == -1 {
 					// This module was finished, it can't be part of a loop.
 					return nil
@@ -2101,13 +2092,13 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 				}
 
 				for _, dep := range order.propagate(module) {
-					cycle := check(dep)
+					cycle := check(dep, end)
 					if cycle != nil {
 						return append([]*moduleInfo{module}, cycle...)
 					}
 				}
 				for _, depPauseSpec := range pauseMap[module] {
-					cycle := check(depPauseSpec.paused)
+					cycle := check(depPauseSpec.paused, end)
 					if cycle != nil {
 						return append([]*moduleInfo{module}, cycle...)
 					}
@@ -2116,9 +2107,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int,
 				return nil
 			}
 
-			cycle := check(start)
-			if cycle != nil {
-				return cycleError(cycle)
+			// Iterate over the modules list instead of pauseMap to provide deterministic ordering.
+			for _, module := range modules {
+				for _, pauseSpec := range pauseMap[module] {
+					cycle := check(pauseSpec.paused, pauseSpec.until)
+					if len(cycle) > 0 {
+						return cycleError(cycle)
+					}
+				}
 			}
 		}
 

context_test.go

@@ -840,38 +840,31 @@ func Test_findVariant(t *testing.T) {
 }
 
 func Test_parallelVisit(t *testing.T) {
-	moduleA := &moduleInfo{
-		group: &moduleGroup{
-			name: "A",
-		},
-	}
-	moduleB := &moduleInfo{
-		group: &moduleGroup{
-			name: "B",
-		},
-	}
-	moduleC := &moduleInfo{
-		group: &moduleGroup{
-			name: "C",
-		},
-	}
-	moduleD := &moduleInfo{
-		group: &moduleGroup{
-			name: "D",
-		},
-	}
-	moduleA.group.modules = modulesOrAliases{moduleA}
-	moduleB.group.modules = modulesOrAliases{moduleB}
-	moduleC.group.modules = modulesOrAliases{moduleC}
-	moduleD.group.modules = modulesOrAliases{moduleD}
-
 	addDep := func(from, to *moduleInfo) {
 		from.directDeps = append(from.directDeps, depInfo{to, nil})
 		from.forwardDeps = append(from.forwardDeps, to)
 		to.reverseDeps = append(to.reverseDeps, from)
 	}
 
-	// A depends on B, B depends on C.  Nothing depends on D, and D doesn't depend on anything.
+	create := func(name string) *moduleInfo {
+		m := &moduleInfo{
+			group: &moduleGroup{
+				name: name,
+			},
+		}
+		m.group.modules = modulesOrAliases{m}
+		return m
+	}
+	moduleA := create("A")
+	moduleB := create("B")
+	moduleC := create("C")
+	moduleD := create("D")
+	moduleE := create("E")
+	moduleF := create("F")
+	moduleG := create("G")
+
+	// A depends on B, B depends on C.  Nothing depends on D through G, and they don't depend on
+	// anything.
 	addDep(moduleA, moduleB)
 	addDep(moduleB, moduleC)
 
@@ -1037,8 +1030,45 @@ func Test_parallelVisit(t *testing.T) {
 			})
 		want := []string{
 			`encountered dependency cycle`,
-			`"C" depends on "D"`,
 			`"D" depends on "C"`,
+			`"C" depends on "D"`,
+		}
+		for i := range want {
+			if len(errs) <= i {
+				t.Errorf("missing error %s", want[i])
+			} else if !strings.Contains(errs[i].Error(), want[i]) {
+				t.Errorf("expected error %s, got %s", want[i], errs[i])
+			}
+		}
+		if len(errs) > len(want) {
+			for _, err := range errs[len(want):] {
+				t.Errorf("unexpected error %s", err.Error())
+			}
+		}
+	})
+	t.Run("pause cycle with deps", func(t *testing.T) {
+		pauseDeps := map[*moduleInfo]*moduleInfo{
+			// F and G form a pause cycle
+			moduleF: moduleG,
+			moduleG: moduleF,
+			// D depends on E which depends on the pause cycle, making E the first alphabetical
+			// entry in pauseMap, which is not part of the cycle.
+			moduleD: moduleE,
+			moduleE: moduleF,
+		}
+		errs := parallelVisit([]*moduleInfo{moduleD, moduleE, moduleF, moduleG}, bottomUpVisitorImpl{}, 4,
+			func(module *moduleInfo, pause chan<- pauseSpec) bool {
+				if dep, ok := pauseDeps[module]; ok {
+					unpause := make(chan struct{})
+					pause <- pauseSpec{module, dep, unpause}
+					<-unpause
+				}
+				return false
+			})
+		want := []string{
+			`encountered dependency cycle`,
+			`"G" depends on "F"`,
+			`"F" depends on "G"`,
 		}
 		for i := range want {
 			if len(errs) <= i {