BCF-2368: ensure unique name per LOOPP (#9762)

End to end work uncovered a problem where a plugin would not start
because the prom server couldn't get a port. Ultimately this was caused
by duplicate names in the LoopRegistry.

This change makes names unique and, to ensure better error handling, it
changes the Register function to error if called more than once. The latter
change will make the problem much more clear should it ever happen again.

core/cmd/shell.go

@@ -168,7 +168,7 @@ func (n ChainlinkAppFactory) NewApplication(ctx context.Context, cfg chainlink.G
 		MailMon:          mailMon,
 	}
 
-	loopRegistry := plugins.NewLoopRegistry()
+	loopRegistry := plugins.NewLoopRegistry(appLggr.Named("LoopRegistry"))
 
 	var chains chainlink.Chains
 	chains.EVM, err = evm.NewTOMLChainSet(ctx, ccOpts)

core/cmd/shell_test.go

@@ -333,10 +333,11 @@ func TestNewUserCache(t *testing.T) {
 }
 
 func TestSetupSolanaRelayer(t *testing.T) {
-	reg := plugins.NewLoopRegistry()
+	lggr := logger.TestLogger(t)
+	reg := plugins.NewLoopRegistry(lggr)
 	ks := mocks.NewSolana(t)
 	rf := cmd.RelayerFactory{
-		Logger:        logger.TestLogger(t),
+		Logger:        lggr,
 		DB:            pgtest.NewSqlxDB(t),
 		GeneralConfig: configtest.NewGeneralConfig(t, nil),
 		LoopRegistry:  reg,
@@ -364,10 +365,11 @@ func TestSetupSolanaRelayer(t *testing.T) {
 }
 
 func TestSetupStarkNetRelayer(t *testing.T) {
-	reg := plugins.NewLoopRegistry()
+	lggr := logger.TestLogger(t)
+	reg := plugins.NewLoopRegistry(lggr)
 	ks := mocks.NewStarkNet(t)
 	rf := cmd.RelayerFactory{
-		Logger:        logger.TestLogger(t),
+		Logger:        lggr,
 		DB:            pgtest.NewSqlxDB(t),
 		GeneralConfig: configtest.NewGeneralConfig(t, nil),
 		LoopRegistry:  reg,

core/internal/cltest/cltest.go

@@ -507,7 +507,7 @@ func NewApplicationWithConfig(t testing.TB, cfg chainlink.GeneralConfig, flagsAn
 		RestrictedHTTPClient:     c,
 		UnrestrictedHTTPClient:   c,
 		SecretGenerator:          MockSecretGenerator{},
-		LoopRegistry:             plugins.NewLoopRegistry(),
+		LoopRegistry:             plugins.NewLoopRegistry(lggr),
 	})
 	require.NoError(t, err)
 	app := appInstance.(*chainlink.ChainlinkApplication)

core/services/chainlink/application.go

@@ -213,7 +213,7 @@ func NewApplication(opts ApplicationOpts) (Application, error) {
 	// we need to initialize in case we serve OCR2 LOOPs
 	loopRegistry := opts.LoopRegistry
 	if loopRegistry == nil {
-		loopRegistry = plugins.NewLoopRegistry()
+		loopRegistry = plugins.NewLoopRegistry(globalLogger.Named("LoopRegistry"))
 	}
 
 	// If the audit logger is enabled

core/services/ocr2/plugins/median/services.go

@@ -104,8 +104,9 @@ func NewMedianServices(ctx context.Context,
 	}, lggr)
 
 	if cmdName := env.MedianPluginCmd.Get(); cmdName != "" {
-		medianLggr := lggr.Named("Median")
-		// use logger name to ensure unique naming
+
+		// use unique logger names so we can use it to register a loop
+		medianLggr := lggr.Named("Median").Named(spec.ContractID).Named(spec.GetID())
 		cmdFn, telem, err2 := cfg.RegisterLOOP(medianLggr.Name(), cmdName)
 		if err2 != nil {
 			err = fmt.Errorf("failed to register loop: %w", err2)

core/web/loop_registry_internal_test.go

@@ -6,10 +6,11 @@ import (
 	"testing"
 
 	"github.com/pkg/errors"
-	"github.com/smartcontractkit/chainlink/v2/core/logger"
-	"github.com/smartcontractkit/chainlink/v2/plugins"
 	"github.com/stretchr/testify/assert"
 	"go.uber.org/zap"
+
+	"github.com/smartcontractkit/chainlink/v2/core/logger"
+	"github.com/smartcontractkit/chainlink/v2/plugins"
 )
 
 type responseWriter struct {
@@ -37,7 +38,7 @@ func TestLoopRegistryServer_CantWriteToResponse(t *testing.T) {
 	l, o := logger.TestLoggerObserved(t, zap.ErrorLevel)
 	s := &LoopRegistryServer{
 		exposedPromPort: 1,
-		registry:        plugins.NewLoopRegistry(),
+		registry:        plugins.NewLoopRegistry(l),
 		logger:          l.(logger.SugaredLogger),
 		jsonMarshalFn:   json.Marshal,
 	}
@@ -52,7 +53,7 @@ func TestLoopRegistryServer_CantMarshal(t *testing.T) {
 	l, o := logger.TestLoggerObserved(t, zap.ErrorLevel)
 	s := &LoopRegistryServer{
 		exposedPromPort: 1,
-		registry:        plugins.NewLoopRegistry(),
+		registry:        plugins.NewLoopRegistry(l),
 		logger:          l.(logger.SugaredLogger),
 		jsonMarshalFn: func(any) ([]byte, error) {
 			return []byte(""), errors.New("can't unmarshal")

plugins/loop_registry.go

@@ -4,6 +4,8 @@ import (
 	"errors"
 	"sort"
 	"sync"
+
+	"github.com/smartcontractkit/chainlink-relay/pkg/logger"
 )
 
 const (
@@ -22,26 +24,35 @@ type RegisteredLoop struct {
 type LoopRegistry struct {
 	mu       sync.Mutex
 	registry map[string]*RegisteredLoop
+
+	lggr logger.Logger
 }
 
-func NewLoopRegistry() *LoopRegistry {
+func NewLoopRegistry(lggr logger.Logger) *LoopRegistry {
 	return &LoopRegistry{
 		registry: map[string]*RegisteredLoop{},
+		lggr:     lggr,
 	}
 }
 
-// Register creates a port of the plugin. It is idempotent. Duplicate calls to Register will return the same port
+// Register creates a port of the plugin. It is not idempotent. Duplicate calls to Register will return [ErrExists]
+// Safe for concurrent use.
 func (m *LoopRegistry) Register(id string) (*RegisteredLoop, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	p, ok := m.get(id)
-	if !ok {
-		return m.create(id)
+	if _, exists := m.registry[id]; exists {
+		return nil, ErrExists
 	}
-	return p, nil
+	nextPort := pluginDefaultPort + len(m.registry)
+	envCfg := NewEnvConfig(nextPort)
+
+	m.registry[id] = &RegisteredLoop{Name: id, EnvCfg: envCfg}
+	m.lggr.Debug("Registered loopp %q with config %v, port %d", id, envCfg, envCfg.PrometheusPort())
+	return m.registry[id], nil
 }
 
+// Return slice sorted by plugin name. Safe for concurrent use.
 func (m *LoopRegistry) List() []*RegisteredLoop {
 	var registeredLoops []*RegisteredLoop
 	m.mu.Lock()
@@ -56,29 +67,11 @@ func (m *LoopRegistry) List() []*RegisteredLoop {
 	return registeredLoops
 }
 
+// Get plugin by id. Safe for concurrent use.
 func (m *LoopRegistry) Get(id string) (*RegisteredLoop, bool) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	return m.get(id)
-}
-
-// create returns a port number for the given plugin to use for prometheus handler.
-// NOT safe for concurrent use.
-func (m *LoopRegistry) create(pluginName string) (*RegisteredLoop, error) {
-	if _, exists := m.registry[pluginName]; exists {
-		return nil, ErrExists
-	}
-	nextPort := pluginDefaultPort + len(m.registry)
-	envCfg := NewEnvConfig(nextPort)
-
-	m.registry[pluginName] = &RegisteredLoop{Name: pluginName, EnvCfg: envCfg}
-	return m.registry[pluginName], nil
-}
-
-// get is a helper to return the port assigned to the plugin, if any
-// NOT safe for concurrent use.
-func (m *LoopRegistry) get(pluginName string) (*RegisteredLoop, bool) {
-	p, exists := m.registry[pluginName]
+	p, exists := m.registry[id]
 	return p, exists
 }

plugins/loop_registry_test.go

@@ -5,20 +5,21 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/smartcontractkit/chainlink/v2/core/logger"
 	"github.com/smartcontractkit/chainlink/v2/plugins"
 )
 
 func TestPluginPortManager(t *testing.T) {
 	// register one
-	m := plugins.NewLoopRegistry()
+	m := plugins.NewLoopRegistry(logger.TestLogger(t))
 	pFoo, err := m.Register("foo")
 	require.NoError(t, err)
 	require.Equal(t, "foo", pFoo.Name)
 	require.Greater(t, pFoo.EnvCfg.PrometheusPort(), 0)
-	// test idempotent
-	pSame, err := m.Register("foo")
-	require.NoError(t, err)
-	require.Equal(t, pFoo, pSame)
+	// test duplicate
+	pNil, err := m.Register("foo")
+	require.ErrorIs(t, err, plugins.ErrExists)
+	require.Nil(t, pNil)
 	// ensure increasing port assignment
 	pBar, err := m.Register("bar")
 	require.NoError(t, err)

plugins/prom.go

@@ -60,6 +60,7 @@ func NewPromServer(port int, lggr logger.Logger, opts ...PromServerOpt) *PromSer
 
 // Start start HTTP server on specified port to handle metrics requests
 func (p *PromServer) Start() error {
+	p.lggr.Debugf("Starting prom server on port %d", p.port)
 	err := p.setupListener()
 	if err != nil {
 		return err