fix: detects cyclical dependencies and produces a build error (#1764)

fixes #1745

buildGraph does not check for cycles as it walks dependency chains; this
change detects a member of the cycle and reports an error identifying
that member

backend/controller/console.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/reugn/go-quartz/logger"
 	"time"
 
 	"connectrpc.com/connect"
@@ -161,7 +162,10 @@ func (c *ConsoleService) GetModules(ctx context.Context, req *connect.Request[pb
 		})
 	}
 
-	sorted := buildengine.TopologicalSort(graph(sch))
+	sorted, err := buildengine.TopologicalSort(graph(sch))
+	if err != nil {
+		logger.Debugf(err.Error())
+	}
 	topology := &pbconsole.Topology{
 		Levels: make([]*pbconsole.TopologyGroup, len(sorted)),
 	}

buildengine/discover_test.go

@@ -41,6 +41,32 @@ func TestDiscoverModules(t *testing.T) {
 				Watch:     []string{"**/*.go", "go.mod", "go.sum", "../../../../go-runtime/ftl/**/*.go"},
 			},
 		},
+		Module{
+			ModuleConfig: moduleconfig.ModuleConfig{
+				Dir:       "testdata/projects/depcycle1",
+				Language:  "go",
+				Realm:     "home",
+				Module:    "depcycle1",
+				Deploy:    []string{"main"},
+				DeployDir: "_ftl",
+				Schema:    "schema.pb",
+				Errors:    "errors.pb",
+				Watch:     []string{"**/*.go", "go.mod", "go.sum"},
+			},
+		},
+		Module{
+			ModuleConfig: moduleconfig.ModuleConfig{
+				Dir:       "testdata/projects/depcycle2",
+				Language:  "go",
+				Realm:     "home",
+				Module:    "depcycle2",
+				Deploy:    []string{"main"},
+				DeployDir: "_ftl",
+				Schema:    "schema.pb",
+				Errors:    "errors.pb",
+				Watch:     []string{"**/*.go", "go.mod", "go.sum"},
+			},
+		},
 		Module{
 			ModuleConfig: moduleconfig.ModuleConfig{
 				Dir:      "testdata/projects/echokotlin",

buildengine/engine.go

@@ -189,6 +189,10 @@ func (e *Engine) Graph(projects ...ProjectKey) (map[string][]string, error) {
 
 func (e *Engine) buildGraph(key string, out map[string][]string) error {
 	var deps []string
+	// Short-circuit previously explored nodes
+	if _, ok := out[key]; ok {
+		return nil
+	}
 	if meta, ok := e.projectMetas.Load(ProjectKey(key)); ok {
 		deps = meta.project.Config().Dependencies
 	} else if sch, ok := e.controllerSchema.Load(key); ok {
@@ -478,7 +482,10 @@ func (e *Engine) buildWithCallback(ctx context.Context, callback buildCallback,
 		"builtin": schema.Builtins(),
 	}
 
-	topology := TopologicalSort(graph)
+	topology, err := TopologicalSort(graph)
+	if err != nil {
+		return err
+	}
 	errCh := make(chan error, 1024)
 	for _, group := range topology {
 		// Collect schemas to be inserted into "built" map for subsequent groups.

buildengine/engine_test.go

@@ -58,3 +58,18 @@ func TestEngine(t *testing.T) {
 	err = engine.Build(ctx)
 	assert.NoError(t, err)
 }
+
+func TestCycleDetection(t *testing.T) {
+	if testing.Short() {
+		t.SkipNow()
+	}
+	ctx := log.ContextWithNewDefaultLogger(context.Background())
+	engine, err := buildengine.New(ctx, nil, []string{"testdata/projects/depcycle1", "testdata/projects/depcycle2"}, nil)
+	assert.NoError(t, err)
+
+	defer engine.Close()
+
+	err = engine.Build(ctx)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "detected a module dependency cycle that impacts these modules:")
+}

buildengine/testdata/projects/depcycle1/depcycle1.go

@@ -0,0 +1,18 @@
+package depcycle1
+
+import (
+	"context"
+	"fmt"
+	"ftl/depcycle2"
+)
+
+type Request struct{}
+type Response struct {
+	Message string
+}
+
+//ftl:verb export
+func Cycle1(ctx context.Context, req Request) (Response, error) {
+	var resp depcycle2.Response
+	return Response{Message: fmt.Sprintf("cycle1 %s", resp)}, nil
+}

buildengine/testdata/projects/depcycle1/ftl.toml

@@ -0,0 +1,2 @@
+module = "depcycle1"
+language = "go"

buildengine/testdata/projects/depcycle1/go.mod

@@ -0,0 +1,5 @@
+module ftl/depcycle1
+
+go 1.22.2
+
+replace github.com/TBD54566975/ftl => ./../../../../..

buildengine/testdata/projects/depcycle2/depcycle2.go

@@ -0,0 +1,18 @@
+package depcycle2
+
+import (
+	"context"
+	"fmt"
+	"ftl/depcycle1"
+)
+
+type Request struct{}
+type Response struct {
+	Message string
+}
+
+//ftl:verb export
+func Cycle2(ctx context.Context, req Request) (Response, error) {
+	var resp depcycle1.Response
+	return Response{Message: fmt.Sprintf("cycle2 %s", resp)}, nil
+}

buildengine/testdata/projects/depcycle2/ftl.toml

@@ -0,0 +1,2 @@
+module = "depcycle2"
+language = "go"

buildengine/testdata/projects/depcycle2/go.mod

@@ -0,0 +1,5 @@
+module ftl/depcycle2
+
+go 1.22.2
+
+replace github.com/TBD54566975/ftl => ./../../../../..

buildengine/topological.go

@@ -1,21 +1,22 @@
 package buildengine
 
 import (
+	"fmt"
 	"sort"
 
 	"golang.org/x/exp/maps"
 )
 
-// TopologicalSort returns a sequence of groups of modules in topological order
-// that may be built in parallel.
-func TopologicalSort(graph map[string][]string) [][]string {
+// TopologicalSort attempts to order the modules supplied in the graph based on
+// their topologically sorted order. A cycle in the module dependency graph
+// will cause this sort to be incomplete. The sorted modules are returned as a
+// sequence of `groups` of modules that may be built in parallel. The `unsorted`
+// modules impacted by a dependency cycle get reported as an error.
+func TopologicalSort(graph map[string][]string) (groups [][]string, cycleError error) {
 	modulesByName := map[string]bool{}
 	for module := range graph {
 		modulesByName[module] = true
 	}
-	// Order of modules to build.
-	// Each element is a list of modules that can be built in parallel.
-	groups := [][]string{}
 
 	// Modules that have already been "built"
 	built := map[string]bool{"builtin": true}
@@ -34,12 +35,22 @@ func TopologicalSort(graph map[string][]string) [][]string {
 			group[module] = true
 			delete(modulesByName, module)
 		}
+		// A module dependency cycle prevents further sorting
+		if len(group) == 0 {
+			// The remaining modules are either a member of the cyclical
+			// dependency chain or depend (directly or transitively) on
+			// a member of the cyclical dependency chain
+			modules := maps.Keys(modulesByName)
+			sort.Strings(modules)
+			cycleError = fmt.Errorf("detected a module dependency cycle that impacts these modules: %q", modules)
+			break
+		}
 		orderedGroup := maps.Keys(group)
 		sort.Strings(orderedGroup)
 		for _, module := range orderedGroup {
 			built[module] = true
 		}
 		groups = append(groups, orderedGroup)
 	}
-	return groups
+	return groups, cycleError
 }

buildengine/topological_test.go

@@ -14,11 +14,32 @@ func TestTopologicalSort(t *testing.T) {
 		"kappa": {},
 		"delta": {},
 	}
-	topo := TopologicalSort(graph)
+	topo, err := TopologicalSort(graph)
 	expected := [][]string{
 		{"delta", "kappa"},
 		{"beta", "gamma"},
 		{"alpha"},
 	}
 	assert.Equal(t, expected, topo)
+	assert.NoError(t, err)
+}
+
+func TestTopologicalSortCycleDetection(t *testing.T) {
+	graph := map[string][]string{
+		"alpha": {"beta", "base"},
+		"beta":  {"alpha", "base"},
+		"delta": {"alpha", "base"},
+		"kappa": {"base"},
+		"gamma": {"kappa", "base"},
+		"base":  {},
+	}
+	topo, err := TopologicalSort(graph)
+	expected := [][]string{
+		{"base"},
+		{"kappa"},
+		{"gamma"},
+	}
+	assert.Equal(t, expected, topo)
+	assert.Error(t, err)
+	assert.Equal(t, "detected a module dependency cycle that impacts these modules: [\"alpha\" \"beta\" \"delta\"]", err.Error())
 }

go-runtime/ftl/ftltest/testdata/go/pubsub/go.mod

@@ -21,11 +21,11 @@ require (
 	github.com/alecthomas/repr v0.4.0 // indirect
 	github.com/alecthomas/types v0.16.0 // indirect
 	github.com/alessio/shellescape v1.4.2 // indirect
-	github.com/amacneil/dbmate/v2 v2.16.0 // indirect
-	github.com/aws/aws-sdk-go-v2 v1.27.0 // indirect
-	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.7 // indirect
-	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.7 // indirect
-	github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.29.1 // indirect
+	github.com/amacneil/dbmate/v2 v2.17.0 // indirect
+	github.com/aws/aws-sdk-go-v2 v1.27.2 // indirect
+	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.9 // indirect
+	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.9 // indirect
+	github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.29.3 // indirect
 	github.com/aws/smithy-go v1.20.2 // indirect
 	github.com/danieljoos/wincred v1.2.0 // indirect
 	github.com/deckarep/golang-set/v2 v2.6.0 // indirect
@@ -47,18 +47,18 @@ require (
 	github.com/puzpuzpuz/xsync/v3 v3.1.0 // indirect
 	github.com/swaggest/jsonschema-go v0.3.70 // indirect
 	github.com/swaggest/refl v1.3.0 // indirect
-	github.com/zalando/go-keyring v0.2.4 // indirect
+	github.com/zalando/go-keyring v0.2.5 // indirect
 	go.opentelemetry.io/otel v1.27.0 // indirect
 	go.opentelemetry.io/otel/metric v1.27.0 // indirect
 	go.opentelemetry.io/otel/trace v1.27.0 // indirect
 	golang.org/x/crypto v0.24.0 // indirect
-	golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 // indirect
+	golang.org/x/exp v0.0.0-20240604190554-fc45aab8b7f8 // indirect
 	golang.org/x/mod v0.18.0 // indirect
 	golang.org/x/net v0.26.0 // indirect
 	golang.org/x/sync v0.7.0 // indirect
 	golang.org/x/sys v0.21.0 // indirect
 	golang.org/x/text v0.16.0 // indirect
-	google.golang.org/protobuf v1.34.1 // indirect
+	google.golang.org/protobuf v1.34.2 // indirect
 )
 
 replace github.com/TBD54566975/ftl => ./../../../../../..

go-runtime/ftl/ftltest/testdata/go/subscriber/go.mod

@@ -20,11 +20,11 @@ require (
 	github.com/alecthomas/repr v0.4.0 // indirect
 	github.com/alecthomas/types v0.16.0 // indirect
 	github.com/alessio/shellescape v1.4.2 // indirect
-	github.com/amacneil/dbmate/v2 v2.16.0 // indirect
-	github.com/aws/aws-sdk-go-v2 v1.27.0 // indirect
-	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.7 // indirect
-	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.7 // indirect
-	github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.29.1 // indirect
+	github.com/amacneil/dbmate/v2 v2.17.0 // indirect
+	github.com/aws/aws-sdk-go-v2 v1.27.2 // indirect
+	github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.9 // indirect
+	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.9 // indirect
+	github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.29.3 // indirect
 	github.com/aws/smithy-go v1.20.2 // indirect
 	github.com/danieljoos/wincred v1.2.0 // indirect
 	github.com/deckarep/golang-set/v2 v2.6.0 // indirect
@@ -46,18 +46,18 @@ require (
 	github.com/puzpuzpuz/xsync/v3 v3.1.0 // indirect
 	github.com/swaggest/jsonschema-go v0.3.70 // indirect
 	github.com/swaggest/refl v1.3.0 // indirect
-	github.com/zalando/go-keyring v0.2.4 // indirect
+	github.com/zalando/go-keyring v0.2.5 // indirect
 	go.opentelemetry.io/otel v1.27.0 // indirect
 	go.opentelemetry.io/otel/metric v1.27.0 // indirect
 	go.opentelemetry.io/otel/trace v1.27.0 // indirect
 	golang.org/x/crypto v0.24.0 // indirect
-	golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 // indirect
+	golang.org/x/exp v0.0.0-20240604190554-fc45aab8b7f8 // indirect
 	golang.org/x/mod v0.18.0 // indirect
 	golang.org/x/net v0.26.0 // indirect
 	golang.org/x/sync v0.7.0 // indirect
 	golang.org/x/sys v0.21.0 // indirect
 	golang.org/x/text v0.16.0 // indirect
-	google.golang.org/protobuf v1.34.1 // indirect
+	google.golang.org/protobuf v1.34.2 // indirect
 )
 
 replace github.com/TBD54566975/ftl => ./../../../../../..