fix(multi-provider): correct provider state tracking and improve robustness (#441)

- fix critical bug where provider state updates used wrong field
  (e.ProviderName instead of e.providerName), causing state tracking
  to fail when multiple providers of the same type were registered
  with different unique names
- fix metadata merge not being assigned in comparison strategy when
  no fallback provider is configured
- add buffering to outbound event channel to prevent blocking
- adjust code comments
- add debug logging when Shutdown() called on uninitialized provider

Signed-off-by: Roman Dmytrenko <[email protected]>

Author: erka

openfeature/multi/comparison_strategy.go

@@ -23,7 +23,7 @@ type Comparator func(values []any) bool
 // there is a comparison failure -- prior to returning a default result. The [Comparator] parameter is optional and nil
 // can be passed as long as ObjectEvaluation is never called with objects that are not comparable. The custom [Comparator]
 // will only be used for [of.FeatureProvider.ObjectEvaluation] if set. If [of.FeatureProvider.ObjectEvaluation] is
-// called without setting a [Comparator], and the returned object(s) are not comparable, then a panic will occur.
+// called without setting a [Comparator], and the returned object(s) are not comparable, then an error will occur.
 func newComparisonStrategy(providers []*NamedProvider, fallbackProvider of.FeatureProvider, comparator Comparator) StrategyFn[FlagTypes] {
 	return evaluateComparison[FlagTypes](providers, fallbackProvider, comparator)
 }
@@ -240,10 +240,9 @@ func evaluateComparison[T FlagTypes](providers []*NamedProvider, fallbackProvide
 		}
 
 		defaultResult := BuildDefaultResult(StrategyComparison, defaultValue, errors.New("no fallback provider configured"))
-		mergeFlagMeta(defaultResult.FlagMetadata, metadata)
+		defaultResult.FlagMetadata = mergeFlagMeta(defaultResult.FlagMetadata, metadata)
 		defaultResult.FlagMetadata[MetadataFallbackUsed] = false
 		defaultResult.FlagMetadata[MetadataIsDefaultValue] = true
-
 		return defaultResult
 	}
 }

openfeature/multi/multiprovider.go

@@ -247,7 +247,7 @@ func NewProvider(providerMap ProviderMap, evaluationStrategy EvaluationStrategy,
 
 	multiProvider := &Provider{
 		providers:      providers,
-		outboundEvents: make(chan of.Event),
+		outboundEvents: make(chan of.Event, len(providers)),
 		logger:         config.logger,
 		metadata:       buildMetadata(providerMap),
 		overallStatus:  of.NotReadyState,
@@ -476,7 +476,7 @@ func (p *Provider) updateProviderStateFromEvent(e namedEvent) bool {
 		p.logger.LogAttrs(context.Background(), slog.LevelDebug, "ProviderConfigChange event", slog.String("event-message", e.Message))
 	}
 	logProviderState(p.logger, e, p.providerStatus[e.providerName])
-	return p.updateProviderState(e.ProviderName, eventTypeToState[e.EventType])
+	return p.updateProviderState(e.providerName, eventTypeToState[e.EventType])
 }
 
 // evaluateState Determines the overall state of the provider using the weights specified in Appendix A of the
@@ -514,6 +514,7 @@ func logProviderState(l *slog.Logger, e namedEvent, previousState of.State) {
 func (p *Provider) Shutdown() {
 	if !p.initialized {
 		// Don't do anything if we were never initialized
+		p.logger.LogAttrs(context.Background(), slog.LevelDebug, "provider not initialized, skipping shutdown")
 		return
 	}
 	// Stop all event listener workers, shutdown events should not affect overall state

openfeature/multi/multiprovider_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"regexp"
 	"testing"
+	"time"
 
 	of "github.com/open-feature/go-sdk/openfeature"
 	imp "github.com/open-feature/go-sdk/openfeature/memprovider"
@@ -344,3 +345,118 @@ func TestMultiProvider_statusEvaluation(t *testing.T) {
 		assert.Equal(t, of.ErrorState, multiProvider.evaluateState())
 	})
 }
+
+func TestMultiProvider_StateUpdateWithSameTypeProviders(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	t.Cleanup(ctrl.Finish)
+
+	// Create two mock providers with EventHandler support
+	primaryProvider := newMockProviderWithEvents(ctrl, "MockProvider")
+	secondaryProvider := newMockProviderWithEvents(ctrl, "MockProvider")
+
+	providers := ProviderMap{
+		"primary":   primaryProvider,
+		"secondary": secondaryProvider,
+	}
+
+	multiProvider, err := NewProvider(providers, StrategyFirstMatch)
+	if err != nil {
+		t.Fatalf("failed to create multi-provider: %v", err)
+	}
+	t.Cleanup(multiProvider.Shutdown)
+
+	// Initialize the provider
+	ctx := of.NewEvaluationContext("test", nil)
+	if err := multiProvider.Init(ctx); err != nil {
+		t.Fatalf("failed to initialize multi-provider: %v", err)
+	}
+
+	primaryProvider.EmitEvent(of.ProviderError, "fail to fetch data")
+	secondaryProvider.EmitEvent(of.ProviderReady, "rev 1")
+
+	time.Sleep(200 * time.Millisecond)
+
+	// Check the state after the error event
+	multiProvider.providerStatusLock.Lock()
+	primaryState := multiProvider.providerStatus["primary"]
+	secondaryState := multiProvider.providerStatus["secondary"]
+	numProviders := len(multiProvider.providerStatus)
+	multiProvider.providerStatusLock.Unlock()
+
+	if primaryState != of.ErrorState {
+		t.Errorf("Expected primary-mock state to be ERROR after emitting error event, got %s", primaryState)
+	}
+
+	if secondaryState != of.ReadyState {
+		t.Errorf("Expected secondary-mock state to be READY, got %s", secondaryState)
+	}
+
+	if numProviders != 2 {
+		t.Errorf("Expected 2 providers in status map, got %d", numProviders)
+	}
+}
+
+var _ of.StateHandler = (*mockProviderWithEvents)(nil)
+
+// mockProviderWithEvents wraps a mock provider to add EventHandler capability
+type mockProviderWithEvents struct {
+	*of.MockFeatureProvider
+	*of.MockStateHandler
+	eventChannel chan of.Event
+	metadata     of.Metadata
+}
+
+func newMockProviderWithEvents(ctrl *gomock.Controller, name string) *mockProviderWithEvents {
+	mockProvider := of.NewMockFeatureProvider(ctrl)
+	mockStateHandler := of.NewMockStateHandler(ctrl)
+	eventChan := make(chan of.Event, 10)
+
+	metadata := of.Metadata{Name: name}
+
+	// Set up expectations
+	mockProvider.EXPECT().Metadata().Return(metadata).AnyTimes()
+	mockProvider.EXPECT().Hooks().Return([]of.Hook{}).AnyTimes()
+	mockStateHandler.EXPECT().Init(gomock.Any()).DoAndReturn(func(ctx of.EvaluationContext) error {
+		// Emit READY event on init
+		eventChan <- of.Event{
+			ProviderName: name,
+			EventType:    of.ProviderReady,
+			ProviderEventDetails: of.ProviderEventDetails{
+				EventMetadata: make(map[string]any),
+			},
+		}
+		return nil
+	}).AnyTimes()
+	mockStateHandler.EXPECT().Shutdown()
+
+	return &mockProviderWithEvents{
+		MockFeatureProvider: mockProvider,
+		MockStateHandler:    mockStateHandler,
+		eventChannel:        eventChan,
+		metadata:            metadata,
+	}
+}
+
+func (m *mockProviderWithEvents) Init(evalCtx of.EvaluationContext) error {
+	return m.MockStateHandler.Init(evalCtx)
+}
+
+func (m *mockProviderWithEvents) Shutdown() {
+	m.MockStateHandler.Shutdown()
+	close(m.eventChannel)
+}
+
+func (m *mockProviderWithEvents) EventChannel() <-chan of.Event {
+	return m.eventChannel
+}
+
+func (m *mockProviderWithEvents) EmitEvent(eventType of.EventType, message string) {
+	m.eventChannel <- of.Event{
+		ProviderName: m.metadata.Name,
+		EventType:    eventType,
+		ProviderEventDetails: of.ProviderEventDetails{
+			Message:       message,
+			EventMetadata: make(map[string]any),
+		},
+	}
+}

openfeature/multi/strategies.go

@@ -15,8 +15,8 @@ const (
 	// This is executed sequentially, and not in parallel. Any returned errors from a provider other than flag not found
 	// will result in a default response with a set error.
 	StrategyFirstMatch EvaluationStrategy = "strategy-first-match"
-	// StrategyFirstSuccess returns the result of the First [of.FeatureProvider] whose response that is not an error.
-	// This is very similar to [StrategyFirstMatch], but does not raise errors. This executed sequentially.
+	// StrategyFirstSuccess returns the result of the first [of.FeatureProvider] whose response that is not an error.
+	// This is very similar to [StrategyFirstMatch], but does not raise errors. This is executed sequentially.
 	StrategyFirstSuccess EvaluationStrategy = "strategy-first-success"
 	// StrategyComparison returns a response of all [of.FeatureProvider] instances in agreement. All providers are
 	// called in parallel and then the results of each non-error result are compared to each other. If all responses