From 582e2363d402d331b01a86fffc477db203e60692 Mon Sep 17 00:00:00 2001 From: Alex Yong Date: Fri, 16 Oct 2026 13:03:27 -0400 Subject: [PATCH 1/2] [AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit --- pkg/client/client.go | 11 +- pkg/client/client_test.go | 145 +++++++++++++++++- pkg/decision/composite_feature_service.go | 12 +- .../composite_feature_service_test.go | 44 +++++- pkg/decision/entities.go | 3 + pkg/decision/feature_experiment_service.go | 28 ++-- .../feature_experiment_service_test.go | 98 +++++++++++- pkg/decision/rollout_service.go | 10 +- pkg/decision/rollout_service_test.go | 106 +++++++++++-- 9 files changed, 417 insertions(+), 40 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 9f4f232c5..76d2af2ff 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -251,6 +251,15 @@ func (o *OptimizelyClient) dispatchDecisionEvents(projectConfig config.ProjectCo if o.sendHoldoutImpression(projectConfig, featureDecision, usrContext, key) { eventSent = true } + for _, hd := range featureDecision.LocalHoldoutDecisions { + if hd.Variation == nil { + continue + } + if hue, ok := event.CreateImpressionUserEvent(projectConfig, hd.Experiment, hd.Variation, usrContext, key, hd.Experiment.Key, decision.Holdout, hd.Variation.FeatureEnabled, nil); ok { + o.EventProcessor.ProcessEvent(hue) + eventSent = true + } + } return eventSent } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c1b2f8738..4f1141b2c 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2020,2022-2025 Optimizely, Inc. and contributors * + * Copyright 2019-2020,2022-2026 Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -3271,3 +3271,146 @@ func TestClientTestSuiteTrackEvent(t *testing.T) { func TestClientTestSuiteTrackNotification(t *testing.T) { suite.Run(t, new(ClientTestSuiteTrackNotification)) } + +type localHoldoutClientFixture struct { + client *OptimizelyClient + feature entities.Feature + eventProcessor *MockEventProcessor +} + +func newLocalHoldoutClientFixture(featureDecision decision.FeatureDecision) localHoldoutClientFixture { + mockConfig := new(MockProjectConfig) + mockConfigManager := new(MockProjectConfigManager) + mockDecisionService := new(MockDecisionService) + mockEventProcessor := new(MockEventProcessor) + + feature := makeTestFeatureWithExperiment("feature_1", makeTestExperiment("exp_1")) + mockConfig.On("GetFeatureByKey", feature.Key).Return(feature, nil) + mockConfigManager.On("GetConfig").Return(mockConfig, nil) + mockDecisionService.On("GetFeatureDecision", mock.Anything, mock.Anything, mock.Anything).Return(featureDecision, decide.NewDecisionReasons(nil), nil) + mockEventProcessor.On("ProcessEvent", mock.AnythingOfType("event.UserEvent")) + + return localHoldoutClientFixture{ + client: &OptimizelyClient{ + ConfigManager: mockConfigManager, + DecisionService: mockDecisionService, + EventProcessor: mockEventProcessor, + defaultDecideOptions: &decide.Options{}, + logger: logging.GetLogger("", ""), + tracer: &MockTracer{}, + }, + feature: feature, + eventProcessor: mockEventProcessor, + } +} + +func makeLocalHoldoutDecision(holdoutKey string) decision.FeatureDecision { + variation := makeTestVariation("holdout_off", false) + return decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations(holdoutKey, []entities.Variation{variation}), + Variation: &variation, + Source: decision.Holdout, + } +} + +func processedImpressions(ep *MockEventProcessor, ruleType string) []event.UserEvent { + var events []event.UserEvent + for _, call := range ep.Calls { + if call.Method != "ProcessEvent" { + continue + } + ue := call.Arguments.Get(0).(event.UserEvent) + if ue.Impression != nil && ue.Impression.Metadata.RuleType == ruleType { + events = append(events, ue) + } + } + return events +} + +func TestDecideLocalHoldoutHitThenLaterRuleServes(t *testing.T) { + servedVariation := makeTestVariation("on", true) + servedExperiment := makeTestExperimentWithVariations("exp_2", []entities.Variation{servedVariation}) + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Experiment: servedExperiment, + Variation: &servedVariation, + Source: decision.FeatureTest, + LocalHoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "on", d.VariationKey) + assert.Equal(t, "exp_2", d.RuleKey) + assert.True(t, d.Enabled) + holdoutEvents := processedImpressions(f.eventProcessor, decision.Holdout) + assert.Len(t, holdoutEvents, 1) + assert.Equal(t, "local_holdout", holdoutEvents[0].Impression.Metadata.RuleKey) + assert.Equal(t, f.feature.Key, holdoutEvents[0].Impression.Metadata.FlagKey) + assert.Len(t, processedImpressions(f.eventProcessor, decision.FeatureTest), 1) +} + +func TestDecideLocalHoldoutHitsOnSeveralRulesSendSeveralEvents(t *testing.T) { + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Source: decision.Rollout, + LocalHoldoutDecisions: []decision.FeatureDecision{ + makeLocalHoldoutDecision("local_holdout"), + makeLocalHoldoutDecision("local_holdout"), + }, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "", d.VariationKey) + assert.False(t, d.Enabled) + // MockProjectConfig has sendFlagDecisions off: holdout events still go out, no rollout event does. + assert.Len(t, processedImpressions(f.eventProcessor, decision.Holdout), 2) + assert.Len(t, f.eventProcessor.Calls, 2) +} + +func TestDecideLocalHoldoutOnDeliveryRuleSendsEventWithSendFlagDecisionsOff(t *testing.T) { + servedVariation := makeTestVariation("on", true) + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations("rollout_2", []entities.Variation{servedVariation}), + Variation: &servedVariation, + Source: decision.Rollout, + LocalHoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("delivery_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "rollout_2", d.RuleKey) + assert.Len(t, processedImpressions(f.eventProcessor, decision.Holdout), 1) + assert.Len(t, f.eventProcessor.Calls, 1) +} + +func TestDecideDisableDecisionEventSuppressesLocalHoldoutEvents(t *testing.T) { + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Source: decision.Rollout, + LocalHoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + f.client.decide(&userContext, f.feature.Key, &decide.Options{DisableDecisionEvent: true}) + + f.eventProcessor.AssertNotCalled(t, "ProcessEvent", mock.Anything) +} + +func TestIsFeatureEnabledSendsNoLocalHoldoutEvent(t *testing.T) { + servedVariation := makeTestVariation("on", true) + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations("exp_2", []entities.Variation{servedVariation}), + Variation: &servedVariation, + Source: decision.FeatureTest, + LocalHoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + }) + + enabled, err := f.client.IsFeatureEnabled(f.feature.Key, entities.UserContext{ID: "test_user_1"}) + + assert.NoError(t, err) + assert.True(t, enabled) + assert.Empty(t, processedImpressions(f.eventProcessor, decision.Holdout)) + assert.Len(t, processedImpressions(f.eventProcessor, decision.FeatureTest), 1) +} diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 073e7ebf4..b0fec7f9f 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -66,6 +66,7 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont } var featureDecision FeatureDecision + var localHoldoutDecisions []FeatureDecision var err error for _, featureDecisionService := range f.featureServices { var decisionReasons decide.DecisionReasons @@ -77,6 +78,8 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont return FeatureDecision{}, reasons, err } + localHoldoutDecisions = append(localHoldoutDecisions, featureDecision.LocalHoldoutDecisions...) + featureDecision.LocalHoldoutDecisions = localHoldoutDecisions if featureDecision.Variation != nil { return featureDecision, reasons, err } @@ -95,6 +98,7 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu // Skip experiment evaluation entirely (A/B/MAB/CMAB are blocked by holdout). // Evaluate rollout service only (targeted deliveries are excluded from holdout blocking). + var localHoldoutDecisions []FeatureDecision if f.rolloutService != nil { rolloutDecision, rolloutReasons, err := f.rolloutService.GetDecision(decisionContext, userContext, options) reasons.Append(rolloutReasons) @@ -108,15 +112,17 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu rolloutDecision.HoldoutVariation = holdoutVar return rolloutDecision, reasons, nil } + localHoldoutDecisions = rolloutDecision.LocalHoldoutDecisions } emptyDecision := FeatureDecision{ // Match the rollout service's no-match behavior (see RolloutService.GetDecision), // so a served impression under sendFlagDecisions carries ruleType "rollout" rather // than a blank value. - Source: Rollout, - HoldoutExperiment: &holdoutExp, - HoldoutVariation: holdoutVar, + Source: Rollout, + HoldoutExperiment: &holdoutExp, + HoldoutVariation: holdoutVar, + LocalHoldoutDecisions: localHoldoutDecisions, } return emptyDecision, reasons, nil } diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 54f38f914..9c50ebcd9 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -350,9 +350,10 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { rolloutVar := entities.Variation{ID: "rollout_var", Key: "rollout_variation"} rolloutDecision := FeatureDecision{ - Variation: &rolloutVar, - Source: Rollout, - Experiment: entities.Experiment{ID: "rollout_1", Key: "rollout_rule"}, + Variation: &rolloutVar, + Source: Rollout, + Experiment: entities.Experiment{ID: "rollout_1", Key: "rollout_rule"}, + LocalHoldoutDecisions: []FeatureDecision{{Source: Holdout, Experiment: entities.Experiment{Key: "local_holdout"}}}, } mockFeatureService := new(MockFeatureDecisionService) @@ -385,6 +386,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) + assert.Equal(t, rolloutDecision.LocalHoldoutDecisions, decision.LocalHoldoutDecisions) mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) @@ -432,7 +434,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) - emptyDecision := FeatureDecision{} + emptyDecision := FeatureDecision{LocalHoldoutDecisions: []FeatureDecision{{Source: Holdout, Experiment: entities.Experiment{Key: "local_holdout"}}}} mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) compositeFeatureService := &CompositeFeatureService{ @@ -452,6 +454,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) + assert.Equal(t, emptyDecision.LocalHoldoutDecisions, decision.LocalHoldoutDecisions) mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) @@ -509,6 +512,37 @@ func TestLocalHoldoutIgnoresExcludeTargetedDeliveries(t *testing.T) { assert.Equal(t, Holdout, decision.Source) } +func (s *CompositeFeatureServiceTestSuite) TestGetDecisionAccumulatesLocalHoldoutDecisions() { + testUserContext := entities.UserContext{ID: "test_user_1"} + expHoldout := FeatureDecision{Source: Holdout, Experiment: entities.Experiment{Key: "exp_holdout"}, Variation: &entities.Variation{Key: "hv"}} + rolloutHoldout := FeatureDecision{Source: Holdout, Experiment: entities.Experiment{Key: "rollout_holdout"}, Variation: &entities.Variation{Key: "hv"}} + + s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{LocalHoldoutDecisions: []FeatureDecision{expHoldout}}, s.reasons, nil) + s.mockFeatureService2.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{Source: Rollout, Variation: &testExp1113Var2223, LocalHoldoutDecisions: []FeatureDecision{rolloutHoldout}}, s.reasons, nil) + + compositeFeatureService := &CompositeFeatureService{ + featureServices: []FeatureService{s.mockFeatureService, s.mockFeatureService2}, + logger: logging.GetLogger("sdkKey", "CompositeFeatureService"), + } + decision, _, err := compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(&testExp1113Var2223, decision.Variation) + s.Equal([]FeatureDecision{expHoldout, rolloutHoldout}, decision.LocalHoldoutDecisions) + + // No rule serves: the final empty decision still carries every hit. + s.mockFeatureService2.ExpectedCalls = nil + s.mockFeatureService2.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{Source: Rollout, LocalHoldoutDecisions: []FeatureDecision{rolloutHoldout}}, s.reasons, nil) + decision, _, err = compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Equal([]FeatureDecision{expHoldout, rolloutHoldout}, decision.LocalHoldoutDecisions) +} + func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() { // Assert that the service is instantiated with the correct child services in the right order compositeExperimentService := NewCompositeExperimentService("") diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index e663854cd..e5495e1c9 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -79,6 +79,9 @@ type FeatureDecision struct { // the client emit a holdout impression alongside the served rollout decision. HoldoutExperiment *entities.Experiment HoldoutVariation *entities.Variation + // LocalHoldoutDecisions holds local holdout hits for rules that were skipped; the decide + // path sends one holdout impression per entry. + LocalHoldoutDecisions []FeatureDecision } // ExperimentDecision contains the decision information about an experiment diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index 3561846f4..3cc6caa37 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -45,6 +45,7 @@ func NewFeatureExperimentService(logger logging.OptimizelyLogProducer, composite func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { feature := decisionContext.Feature reasons := decide.NewDecisionReasons(options) + var localHoldoutDecisions []FeatureDecision // @TODO this can be improved by getting group ID first and determining experiment and then bucketing in experiment for _, featureExperiment := range feature.FeatureExperiments { @@ -54,22 +55,22 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon reasons.Append(_reasons) if err == nil { featureDecision := FeatureDecision{ - Experiment: featureExperiment, - Variation: forcedDecision, - Source: FeatureTest, + Experiment: featureExperiment, + Variation: forcedDecision, + Source: FeatureTest, + LocalHoldoutDecisions: localHoldoutDecisions, } return featureDecision, reasons, nil } } - // [FSSDK-12369] Check local holdouts targeting this experiment rule. // Local holdouts are evaluated after forced decisions but before audience/traffic checks. if f.holdoutService != nil { holdoutDecision, holdoutReasons, _ := f.holdoutService.GetLocalDecisionForRule(featureExperiment.ID, decisionContext.ProjectConfig, userContext, options) reasons.Append(holdoutReasons) if holdoutDecision.Variation != nil { - // User is in a local holdout — return holdout decision, skip rule evaluation - return holdoutDecision, reasons, nil + localHoldoutDecisions = append(localHoldoutDecisions, holdoutDecision) + continue } } @@ -99,16 +100,17 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon // Variation not nil means we got a decision and should return it if experimentDecision.Variation != nil { featureDecision := FeatureDecision{ - Experiment: experiment, - Decision: experimentDecision.Decision, - Variation: experimentDecision.Variation, - Source: FeatureTest, - CmabUUID: experimentDecision.CmabUUID, + Experiment: experiment, + Decision: experimentDecision.Decision, + Variation: experimentDecision.Variation, + Source: FeatureTest, + CmabUUID: experimentDecision.CmabUUID, + LocalHoldoutDecisions: localHoldoutDecisions, } return featureDecision, reasons, err } } - return FeatureDecision{}, reasons, nil + return FeatureDecision{LocalHoldoutDecisions: localHoldoutDecisions}, reasons, nil } diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index 66e5a9b3f..f62cd4183 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -335,6 +335,102 @@ func (s *FeatureExperimentServiceTestSuite) TestForcedDecisionBeats100PercentLoc s.mockConfig.AssertNotCalled(s.T(), "GetHoldoutsForRule") } +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitSkipsRuleAndNextRuleServes() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1114.ID).Return([]entities.Holdout{}) + + expectedVariation := testExp1114.Variations["2225"] + exp1114Context := ExperimentDecisionContext{Experiment: &testExp1114, ProjectConfig: s.mockConfig} + s.mockExperimentService.On("GetDecision", exp1114Context, testUserContext, s.options).Return(ExperimentDecision{Variation: &expectedVariation}, s.reasons, nil) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(&expectedVariation, decision.Variation) + s.Equal(testExp1114.Key, decision.Experiment.Key) + s.Equal(FeatureTest, decision.Source) + s.Require().Len(decision.LocalHoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.LocalHoldoutDecisions[0].Experiment.Key) + s.Equal(Holdout, decision.LocalHoldoutDecisions[0].Source) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision", ExperimentDecisionContext{Experiment: &testExp1113, ProjectConfig: s.mockConfig}, testUserContext, s.options) + s.mockExperimentService.AssertExpectations(s.T()) +} + +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutOnAllRulesNoRuleServes() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1114.ID).Return([]entities.Holdout{localHoldout}) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Len(decision.LocalHoldoutDecisions, 2) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitThenForcedDecisionOnLaterRule() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + forcedVariation := testExp1114.Variations["2225"] + s.mockConfig.On("GetFlagVariationsMap").Return(map[string][]entities.Variation{ + s.testFeatureDecisionContext.Feature.Key: {forcedVariation}, + }) + s.testFeatureDecisionContext.ForcedDecisionService.SetForcedDecision( + OptimizelyDecisionContext{FlagKey: s.testFeatureDecisionContext.Feature.Key, RuleKey: testExp1114.Key}, + OptimizelyForcedDecision{VariationKey: forcedVariation.Key}, + ) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(forcedVariation.Key, decision.Variation.Key) + s.Equal(testExp1114.Key, decision.Experiment.Key) + s.Require().Len(decision.LocalHoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.LocalHoldoutDecisions[0].Experiment.Key) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + func TestFeatureExperimentServiceTestSuite(t *testing.T) { suite.Run(t, new(FeatureExperimentServiceTestSuite)) } diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index c4cfcf4dc..3e1e5f312 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -109,11 +109,11 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } - // [FSSDK-12369] Check local holdouts targeting this rollout rule. // Local holdouts are evaluated after forced decisions but before audience/traffic checks. if holdoutDecision, holdoutReasons := r.getLocalHoldoutDecision(experiment, decisionContext, userContext, options); holdoutDecision != nil { reasons.Append(holdoutReasons) - return *holdoutDecision, reasons, nil + featureDecision.LocalHoldoutDecisions = append(featureDecision.LocalHoldoutDecisions, *holdoutDecision) + continue } experimentDecisionContext := getExperimentDecisionContext(experiment) @@ -146,10 +146,10 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } - // [FSSDK-12369] Check local holdouts for the fallback/everyone else rule if holdoutDecision, holdoutReasons := r.getLocalHoldoutDecision(experiment, decisionContext, userContext, options); holdoutDecision != nil { reasons.Append(holdoutReasons) - return *holdoutDecision, reasons, nil + featureDecision.LocalHoldoutDecisions = append(featureDecision.LocalHoldoutDecisions, *holdoutDecision) + return featureDecision, reasons, nil } experimentDecisionContext := getExperimentDecisionContext(experiment) diff --git a/pkg/decision/rollout_service_test.go b/pkg/decision/rollout_service_test.go index 8ddc7c1f3..daf552c61 100644 --- a/pkg/decision/rollout_service_test.go +++ b/pkg/decision/rollout_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -381,9 +381,8 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFailsTargeting() { s.mockLogger.AssertExpectations(s.T()) } -// TestGetDecisionLocalHoldout_RegularRule_UserBucketed verifies that when a user is bucketed -// into a local holdout targeting a regular rollout rule, the holdout decision is returned -// immediately — audience and traffic checks for the rule are skipped. +// TestGetDecisionLocalHoldout_RegularRule_UserBucketed verifies that a local holdout hit on a +// regular rollout rule skips only that rule; the next rule serves and the hit is recorded. func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBucketed() { holdoutVar := entities.Variation{ID: "ho_var_1", Key: "holdout_variation"} localHoldout := entities.Holdout{ @@ -398,6 +397,14 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBu }, } s.mockConfig.On("GetHoldoutsForRule", testExp1112.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1117.ID).Return([]entities.Holdout{}) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams, mock.Anything).Return(true, true, s.reasons) + expectedVariation := testExp1117Var2223 + exp1117DecisionContext := ExperimentDecisionContext{Experiment: &testExp1117, ProjectConfig: s.mockConfig} + s.mockExperimentService.On("GetDecision", exp1117DecisionContext, s.testUserContext, s.options).Return(ExperimentDecision{ + Variation: &expectedVariation, + Decision: Decision{Reason: reasons.BucketedIntoVariation}, + }, s.reasons, nil) s.mockLogger.On("Debug", mock.Anything).Return() s.mockLogger.On("Info", mock.Anything).Return() @@ -411,9 +418,49 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBu decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) s.NoError(err) - s.NotNil(decision.Variation) - s.Equal(Holdout, decision.Source) - // Audience and experiment bucketer must not be called — holdout returned early + s.Equal(&expectedVariation, decision.Variation) + s.Equal(testExp1117.Key, decision.Experiment.Key) + s.Equal(Rollout, decision.Source) + s.Require().Len(decision.LocalHoldoutDecisions, 1) + s.Equal(Holdout, decision.LocalHoldoutDecisions[0].Source) + s.Equal(holdoutVar.Key, decision.LocalHoldoutDecisions[0].Variation.Key) + // Rule 1112 is skipped entirely: no audience evaluation or bucketing for it + s.mockAudienceTreeEvaluator.AssertNotCalled(s.T(), "Evaluate", testExp1112.AudienceConditionTree, mock.Anything, mock.Anything) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision", s.testExperiment1112DecisionContext, mock.Anything, mock.Anything) + s.mockExperimentService.AssertExpectations(s.T()) +} + +// TestGetDecisionLocalHoldout_AllRulesHeldOut verifies that hits on every rule each get recorded +// and no rule serves. +func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_AllRulesHeldOut() { + localHoldout := entities.Holdout{ + ID: "local_holdout_rule", + Key: "local_holdout_for_rule", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{ + "ho_var_1": {ID: "ho_var_1", Key: "holdout_variation"}, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "ho_var_1", EndOfRange: 10000}, + }, + } + s.mockConfig.On("GetHoldoutsForRule", mock.Anything).Return([]entities.Holdout{localHoldout}) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: s.mockLogger, + } + + decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Equal(Rollout, decision.Source) + s.Len(decision.LocalHoldoutDecisions, 3) s.mockAudienceTreeEvaluator.AssertNotCalled(s.T(), "Evaluate") s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") } @@ -461,8 +508,7 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserMi } // TestGetDecisionLocalHoldout_FallbackRule_UserBucketed verifies that a local holdout -// targeting the everyone-else (fallback) rule is evaluated and returns a holdout decision -// when the user is bucketed into it. +// targeting the everyone-else (fallback) rule skips it and records the hit. func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_FallbackRule_UserBucketed() { holdoutVar := entities.Variation{ID: "ho_var_1", Key: "holdout_variation"} holdoutMiss := entities.Holdout{ @@ -509,8 +555,10 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_FallbackRule_UserB decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) s.NoError(err) - s.NotNil(decision.Variation) - s.Equal(Holdout, decision.Source) + s.Nil(decision.Variation) + s.Equal(Rollout, decision.Source) + s.Require().Len(decision.LocalHoldoutDecisions, 1) + s.Equal(holdoutHit.Key, decision.LocalHoldoutDecisions[0].Experiment.Key) s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") s.mockConfig.AssertExpectations(s.T()) } @@ -545,6 +593,42 @@ func (s *RolloutServiceTestSuite) TestForcedDecisionBeatsLocalHoldout_Rollout() s.mockConfig.AssertNotCalled(s.T(), "GetHoldoutsForRule") } +func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldoutHitThenForcedDecisionOnLaterRule() { + localHoldout := entities.Holdout{ + ID: "local_holdout_rule", + Key: "local_holdout_for_rule", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var_1": {ID: "ho_var_1", Key: "holdout_variation"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var_1", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1112.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetFlagVariationsMap").Return(map[string][]entities.Variation{ + s.testFeatureDecisionContext.Feature.Key: {testExp1117Var2223}, + }) + s.testFeatureDecisionContext.ForcedDecisionService.SetForcedDecision( + OptimizelyDecisionContext{FlagKey: s.testFeatureDecisionContext.Feature.Key, RuleKey: testExp1117.Key}, + OptimizelyForcedDecision{VariationKey: testExp1117Var2223.Key}, + ) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: s.mockLogger, + } + + decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(testExp1117Var2223.Key, decision.Variation.Key) + s.Equal(reasons.ForcedDecisionFound, decision.Reason) + s.Require().Len(decision.LocalHoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.LocalHoldoutDecisions[0].Experiment.Key) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + func TestNewRolloutService(t *testing.T) { rolloutService := NewRolloutService("") assert.IsType(t, &evaluator.MixedTreeEvaluator{}, rolloutService.audienceTreeEvaluator) From 0e1b1bd3bbadb5271690bacde20fa3fc71584761 Mon Sep 17 00:00:00 2001 From: Alex Yong Date: Fri, 16 Oct 2026 17:57:18 -0400 Subject: [PATCH 2/2] [AI-FSSDK] [FSSDK-12958] Test decision error suppresses local holdout events --- pkg/client/client_test.go | 21 +++++++++++ .../feature_experiment_service_test.go | 36 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 4f1141b2c..1fb72bd8a 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -3398,6 +3398,27 @@ func TestDecideDisableDecisionEventSuppressesLocalHoldoutEvents(t *testing.T) { f.eventProcessor.AssertNotCalled(t, "ProcessEvent", mock.Anything) } +func TestDecideDecisionErrorSuppressesLocalHoldoutEvents(t *testing.T) { + holdoutDecision := decision.FeatureDecision{ + Source: decision.FeatureTest, + LocalHoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + } + f := newLocalHoldoutClientFixture(holdoutDecision) + cmabErrorMessage := "Failed to fetch CMAB data for experiment exp_1." + mockDecisionService := f.client.DecisionService.(*MockDecisionService) + mockDecisionService.ExpectedCalls = nil + mockDecisionService.On("GetFeatureDecision", mock.Anything, mock.Anything, mock.Anything).Return(holdoutDecision, decide.NewDecisionReasons(nil), errors.New(cmabErrorMessage)) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "", d.VariationKey) + assert.Equal(t, "", d.RuleKey) + assert.False(t, d.Enabled) + assert.Contains(t, d.Reasons, cmabErrorMessage) + f.eventProcessor.AssertNotCalled(t, "ProcessEvent", mock.Anything) +} + func TestIsFeatureEnabledSendsNoLocalHoldoutEvent(t *testing.T) { servedVariation := makeTestVariation("on", true) f := newLocalHoldoutClientFixture(decision.FeatureDecision{ diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index f62cd4183..2bd2ac587 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -431,6 +431,42 @@ func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitThenForcedDecisio s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") } +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitThenCmabErrorOnLaterRule() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + cmabExperiment := testExp1114 + cmabExperiment.Cmab = &entities.Cmab{AttributeIds: []string{"attr1"}, TrafficAllocation: 10000} + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", cmabExperiment.ID).Return([]entities.Holdout{}) + + cmabContext := ExperimentDecisionContext{Experiment: &cmabExperiment, ProjectConfig: s.mockConfig} + cmabError := errors.New("Failed to fetch CMAB data for experiment " + cmabExperiment.Key + ".") + s.mockExperimentService.On("GetDecision", cmabContext, testUserContext, s.options).Return(ExperimentDecision{}, s.reasons, cmabError) + + feature := testFeat3335 + feature.FeatureExperiments = []entities.Experiment{testExp1113, cmabExperiment} + s.testFeatureDecisionContext.Feature = &feature + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.ErrorIs(err, cmabError) + s.Nil(decision.Variation) + s.Empty(decision.LocalHoldoutDecisions) + s.mockExperimentService.AssertExpectations(s.T()) +} + func TestFeatureExperimentServiceTestSuite(t *testing.T) { suite.Run(t, new(FeatureExperimentServiceTestSuite)) }