Module id fail

CHANGELOG.md

@@ -123,6 +123,9 @@ Main (unreleased)
 
 - Fix `loki.source.file` race condition in cleaning up metrics when stopping to tail files. (@thampiotr)
 
+- Fixed issue where adding a module after initial start, that failed to load then subsequently resolving the issue would cause the module to
+  permanently fail to load with `id already exists` error. (@mattdurham)
+
 v0.36.1 (2023-09-06)
 --------------------
 

component/module/file/file.go

@@ -75,9 +75,11 @@ func New(o component.Options, args Arguments) (*Component, error) {
 
 	c.managedLocalFile, err = c.newManagedLocalComponent(o)
 	if err != nil {
+		m.Remove()
 		return nil, err
 	}
 	if err := c.Update(args); err != nil {
+		m.Remove()
 		return nil, err
 	}
 	return c, nil

component/module/git/git.go

@@ -90,6 +90,7 @@ func New(o component.Options, args Arguments) (*Component, error) {
 	}
 
 	if err := c.Update(args); err != nil {
+		m.Remove()
 		return nil, err
 	}
 	return c, nil

component/module/http/http.go

@@ -74,9 +74,11 @@ func New(o component.Options, args Arguments) (*Component, error) {
 
 	c.managedRemoteHTTP, err = c.newManagedLocalComponent(o)
 	if err != nil {
+		m.Remove()
 		return nil, err
 	}
 	if err := c.Update(args); err != nil {
+		m.Remove()
 		return nil, err
 	}
 	return c, nil

component/module/module.go

@@ -70,6 +70,12 @@ func (c *ModuleComponent) LoadFlowContent(args map[string]any, contentValue stri
 	return nil
 }
 
+// Remove removes the module from the registry. This is normally called after Run but in the case
+// where the parent component fails to load and successfully run it must be called manually.
+func (c *ModuleComponent) Remove() {
+	c.mod.Remove()
+}
+
 // RunFlowController runs the flow controller that all module components start.
 func (c *ModuleComponent) RunFlowController(ctx context.Context) {
 	c.mod.Run(ctx)

component/module/string/string.go

@@ -45,6 +45,7 @@ var (
 
 // New creates a new module.string component.
 func New(o component.Options, args Arguments) (*Component, error) {
+	var err error
 	m, err := module.NewModuleComponent(o)
 	if err != nil {
 		return nil, err
@@ -54,6 +55,7 @@ func New(o component.Options, args Arguments) (*Component, error) {
 	}
 
 	if err := c.Update(args); err != nil {
+		m.Remove()
 		return nil, err
 	}
 	return c, nil

component/registry.go

@@ -35,6 +35,10 @@ type ModuleController interface {
 	// If id is non-empty, it must be a valid River identifier, matching the
 	// regex /[A-Za-z_][A-Za-z0-9_]/.
 	NewModule(id string, export ExportFunc) (Module, error)
+
+	// RemoveID is used to remove the id from the cache, this is normally called from Run but if
+	// the Module is created and then fails during loading then it will leak the id.
+	RemoveID(id string)
 }
 
 // Module is a controller for running components within a Module.
@@ -50,6 +54,9 @@ type Module interface {
 	// Run blocks until the provided context is canceled. The ID of a module as defined in
 	// ModuleController.NewModule will not be released until Run returns.
 	Run(context.Context)
+
+	// Remove is used to manually remove the module from the registry.
+	Remove()
 }
 
 // ExportFunc is used for onExport of the Module

pkg/flow/module.go

@@ -64,7 +64,8 @@ func (m *moduleController) NewModule(id string, export component.ExportFunc) (co
 	return mod, nil
 }
 
-func (m *moduleController) removeID(id string) {
+// RemoveID removes a reference from the module registry and name cache.
+func (m *moduleController) RemoveID(id string) {
 	m.mut.Lock()
 	defer m.mut.Unlock()
 
@@ -129,12 +130,17 @@ func (c *module) LoadConfig(config []byte, args map[string]any) error {
 	return c.f.LoadFile(ff, args)
 }
 
+// Remove removed the ID from the registry. Generally only used if fails to start.
+func (c *module) Remove() {
+	c.o.parent.RemoveID(c.o.ID)
+}
+
 // Run starts the Module. No components within the Module
 // will be run until Run is called.
 //
 // Run blocks until the provided context is canceled.
 func (c *module) Run(ctx context.Context) {
-	defer c.o.parent.removeID(c.o.ID)
+	defer c.o.parent.RemoveID(c.o.ID)
 	c.f.Run(ctx)
 }
 

pkg/flow/module_fail_test.go

@@ -0,0 +1,104 @@
+package flow
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/grafana/agent/component"
+	mod "github.com/grafana/agent/component/module"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIDRemovalIfFailedToLoad(t *testing.T) {
+	f := New(testOptions(t))
+
+	fullContent := "test.fail.module \"t1\" { content = \"\" }"
+	fl, err := ReadFile("test", []byte(fullContent))
+	require.NoError(t, err)
+	err = f.LoadFile(fl, nil)
+	require.NoError(t, err)
+	ctx := context.Background()
+	ctx, cnc := context.WithTimeout(ctx, 600*time.Second)
+	defer cnc()
+	go f.Run(ctx)
+	require.Eventually(t, func() bool {
+		t1 := f.loader.Components()[0].Component().(*testFailModule)
+		return t1 != nil
+	}, 10*time.Second, 100*time.Millisecond)
+	t1 := f.loader.Components()[0].Component().(*testFailModule)
+	badContent :=
+		`test.fail.module "int" {
+content=""
+fail=true
+}`
+	err = t1.updateContent(badContent)
+	require.Error(t, err)
+	goodContent :=
+		`test.fail.module "int" { 
+content=""
+fail=false
+}`
+	err = t1.updateContent(goodContent)
+	require.NoError(t, err)
+}
+
+func init() {
+	component.Register(component.Registration{
+		Name:    "test.fail.module",
+		Args:    TestFailArguments{},
+		Exports: mod.Exports{},
+
+		Build: func(opts component.Options, args component.Arguments) (component.Component, error) {
+			m, err := mod.NewModuleComponent(opts)
+			if err != nil {
+				return nil, err
+			}
+			if args.(TestFailArguments).Fail {
+				m.Remove()
+				return nil, fmt.Errorf("module told to fail")
+			}
+			err = m.LoadFlowContent(nil, args.(TestFailArguments).Content)
+			if err != nil {
+				m.Remove()
+				return nil, err
+			}
+			return &testFailModule{
+				mc:      m,
+				content: args.(TestFailArguments).Content,
+				opts:    opts,
+				fail:    args.(TestFailArguments).Fail,
+				ch:      make(chan error),
+			}, nil
+		},
+	})
+}
+
+type TestFailArguments struct {
+	Content string `river:"content,attr"`
+	Fail    bool   `river:"fail,attr,optional"`
+}
+
+type testFailModule struct {
+	content string
+	opts    component.Options
+	ch      chan error
+	mc      *mod.ModuleComponent
+	fail    bool
+}
+
+func (t *testFailModule) Run(ctx context.Context) error {
+	<-ctx.Done()
+	return nil
+}
+
+func (t *testFailModule) updateContent(content string) error {
+	t.content = content
+	err := t.mc.LoadFlowContent(nil, t.content)
+	return err
+}
+
+func (t *testFailModule) Update(_ component.Arguments) error {
+	return nil
+}