From ff49b617bb96c3e7f6c9f710a42342a652828cdb Mon Sep 17 00:00:00 2001 From: Alex Yong Date: Thu, 1 Oct 2026 15:03:44 -0400 Subject: [PATCH] [FSSDK-12958] Continue rule evaluation after local holdout hit A local holdout hit now excludes the user from that rule only. The hit is recorded, the rule is skipped, and evaluation continues with the next rule (experiment and delivery rules alike). The decide path sends one holdout impression per hit in the existing format, regardless of sendFlagDecisions. The final decision and its events are unchanged. --- optimizely/decision_service.py | 94 +++++++-------- optimizely/optimizely.py | 42 ++++--- tests/test_decision_service.py | 2 +- tests/test_decision_service_holdout.py | 153 ++++++++++++++++++++----- 4 files changed, 194 insertions(+), 97 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index b12352c2..6ebc089d 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -65,7 +65,7 @@ class VariationResult(TypedDict): class _DecisionResultOptional(TypedDict, total=False): - holdout_decision: Decision + holdout_decisions: List[Decision] class DecisionResult(_DecisionResultOptional): @@ -552,7 +552,8 @@ def get_variation( } def get_variation_for_rollout( - self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext + self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext, + holdout_decisions: Optional[list[Decision]] = None ) -> tuple[Decision, list[str]]: """ Determine which experiment/variation the user is in for a given rollout. Returns the variation of the first experiment the user qualifies for. @@ -563,6 +564,7 @@ def get_variation_for_rollout( rollout: Rollout for which we are getting the variation. user: ID and attributes for user. options: Decide options. + holdout_decisions: If given, local holdout hits are appended to it. Returns: Decision namedtuple consisting of experiment and variation for the user and @@ -606,22 +608,14 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons - local_holdouts = project_config.get_holdouts_for_rule(rule.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - decide_reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for delivery rule '{rule.key}'." - ) - self.logger.info(message) - decide_reasons.append(message) - return local_decision, decide_reasons + local_holdout_hit = self._find_local_holdout_hit( + project_config, rule, 'delivery', user_context, decide_reasons + ) + if local_holdout_hit is not None: + if holdout_decisions is not None: + holdout_decisions.append(local_holdout_hit) + index += 1 + continue bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons @@ -748,6 +742,7 @@ def get_decision_for_flag( global_holdout_result: DecisionResult | None = None global_holdout_key: str | None = None + holdout_decisions: list[Decision] = [] # Check global holdouts (flag level — before any rules are evaluated) global_holdouts = project_config.get_global_holdouts() @@ -781,6 +776,7 @@ def get_decision_for_flag( reasons.append(message) global_holdout_result = holdout_decision global_holdout_key = holdout.key + holdout_decisions.append(holdout_decision['decision']) break # Check experiments then rollouts @@ -807,30 +803,16 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions return result - local_holdouts = project_config.get_holdouts_for_rule(experiment.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for experiment rule '{experiment.key}'." - ) - self.logger.info(message) - reasons.append(message) - return { - 'decision': local_holdout_decision['decision'], - 'error': False, - 'reasons': reasons - } + local_holdout_hit = self._find_local_holdout_hit( + project_config, experiment, 'experiment', user_context, reasons + ) + if local_holdout_hit is not None: + holdout_decisions.append(local_holdout_hit) + continue # Get variation for experiment variation_result = self.get_variation( @@ -856,8 +838,8 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions return result if global_holdout_result is not None: @@ -870,7 +852,7 @@ def get_decision_for_flag( # If no experiment decision, check rollouts rollout_decision, rollout_reasons = self.get_variation_for_rollout( - project_config, feature_flag, user_context + project_config, feature_flag, user_context, holdout_decisions ) if rollout_reasons: reasons.extend(rollout_reasons) @@ -893,10 +875,32 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - final_result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + final_result['holdout_decisions'] = holdout_decisions return final_result + def _find_local_holdout_hit( + self, + project_config: ProjectConfig, + rule: entities.Experiment, + rule_type: str, + user_context: OptimizelyUserContext, + reasons: list[str] + ) -> Optional[Decision]: + """Returns the decision of the first local holdout on the rule that buckets the user, else None.""" + for holdout in project_config.get_holdouts_for_rule(rule.id): + holdout_result = self.get_variation_for_holdout(holdout, user_context, project_config) + reasons.extend(holdout_result['reasons']) + if holdout_result['decision'].variation is not None: + message = ( + f"The user '{user_context.user_id}' is bucketed into local holdout '{holdout.key}' " + f"for {rule_type} rule '{rule.key}'. Skipping to the next rule." + ) + self.logger.info(message) + reasons.append(message) + return holdout_result['decision'] + return None + def get_variation_for_holdout( self, holdout: entities.Holdout, diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 0f7c1da5..992a4f39 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1246,7 +1246,7 @@ def _create_optimizely_decision( decision_reasons: Optional[list[str]], decide_options: list[str], project_config: ProjectConfig, - holdout_decision: Optional[Decision] = None + holdout_decisions: Optional[list[Decision]] = None ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False @@ -1265,24 +1265,22 @@ def _create_optimizely_decision( feature_flag = project_config.feature_key_map.get(flag_key) - # Send holdout impression when user was bucketed into a holdout bypassed due to exclude_targeted_deliveries - if (holdout_decision is not None - and decision_source != DecisionSources.HOLDOUT - and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options): - holdout_enabled = self._get_feature_enabled(holdout_decision.variation) - holdout_rule_key = holdout_decision.experiment.key if holdout_decision.experiment else '' - self._send_impression_event( - project_config, - holdout_decision.experiment, - holdout_decision.variation, - flag_key, - holdout_rule_key, - str(DecisionSources.HOLDOUT), - holdout_enabled, - user_id, - attributes, - holdout_decision.cmab_uuid - ) + # Holdouts the user was bucketed into without ending evaluation: local holdout hits and + # global holdouts bypassed due to exclude_targeted_deliveries. Sent regardless of send_flag_decisions. + if holdout_decisions and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: + for holdout_decision in holdout_decisions: + self._send_impression_event( + project_config, + holdout_decision.experiment, + holdout_decision.variation, + flag_key, + holdout_decision.experiment.key if holdout_decision.experiment else '', + str(DecisionSources.HOLDOUT), + self._get_feature_enabled(holdout_decision.variation), + user_id, + attributes, + holdout_decision.cmab_uuid + ) decision_event_dispatched = True # Send impression event if Decision came from a feature @@ -1469,13 +1467,13 @@ def _decide_for_keys( user_context, merged_decide_options ) - holdout_decisions: dict[str, Optional[Decision]] = {} + holdout_decisions: dict[str, list[Decision]] = {} for i in range(0, len(flags_without_forced_decision)): decision = decision_list[i]['decision'] reasons = decision_list[i]['reasons'] error = decision_list[i]['error'] flag_key = flags_without_forced_decision[i].key - holdout_decisions[flag_key] = decision_list[i].get('holdout_decision') + holdout_decisions[flag_key] = decision_list[i].get('holdout_decisions', []) # store error decision against key and remove key from valid keys if error: optimizely_decision = OptimizelyDecision.new_error_decision(flags_without_forced_decision[i].key, @@ -1496,7 +1494,7 @@ def _decide_for_keys( decision_reasons, merged_decide_options, project_config, - holdout_decision=holdout_decisions.get(key) + holdout_decisions=holdout_decisions.get(key) ) enabled_flags_only_missing = OptimizelyDecideOption.ENABLED_FLAGS_ONLY not in merged_decide_options is_enabled = optimizely_decision.enabled diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index b38a03b2..9d25a32d 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1510,7 +1510,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel ) mock_get_variation_for_rollout.assert_called_once_with( - self.project_config, feature, user + self.project_config, feature, user, [] ) # Assert no log messages were generated diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index da85ae4e..96894647 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy import json from unittest import mock @@ -1467,8 +1468,8 @@ def test_global_holdout_miss_falls_through_to_experiment(self): # Branch 2: Local holdout hit — user bucketed into local holdout for rule X # ------------------------------------------------------------------ - def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): - """User bucketed into local holdout for experiment rule X returns holdout decision. + def test_local_holdout_hit_skips_experiment_rule(self): + """User bucketed into local holdout for experiment rule X is excluded from rule X only. The experiment rule ID is '111127' (test_experiment linked to test_feature_in_experiment). The local holdout targets only this rule with full traffic. @@ -1488,14 +1489,13 @@ def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): user_ctx = opt.create_user_context('user_in_local_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - # User must be caught by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) - # Regular experiment evaluation (get_variation) must not have run + # Rule X is skipped and the hit is recorded; the decision does not come from the holdout + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_exp']) mock_get_var.assert_not_called() - def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): - """User bucketed into local holdout for delivery/rollout rule returns holdout decision. + def test_local_holdout_hit_skips_delivery_rule(self): + """User bucketed into local holdout for the last delivery rule ends with no rule serving. Rule '211147' is the everyone-else rollout rule for 'test_feature_in_rollout'. The local holdout targets this delivery rule with full traffic. @@ -1512,9 +1512,8 @@ def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNone(result['decision'].variation) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_delivery']) # ------------------------------------------------------------------ # Branch 3: Local holdout miss — user falls through to regular rule evaluation @@ -1607,7 +1606,8 @@ def test_local_holdout_applies_to_experiment_rule(self): user_ctx = opt.create_user_context('user_exp_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) def test_local_holdout_applies_to_rollout_delivery_rule(self): """Local holdout check applies to rollout/delivery rules in get_decision_for_flag.""" @@ -1624,9 +1624,8 @@ def test_local_holdout_applies_to_rollout_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Precedence: Global → Forced → Local → Regular rule @@ -1858,7 +1857,7 @@ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) mock_get_var.assert_not_called() - self.assertIsNotNone(result.get('holdout_decision')) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches @@ -1880,7 +1879,7 @@ def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self) decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(result.get('holdout_decision')) + self.assertEqual(len(result['holdout_decisions']), 1) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, " @@ -1910,8 +1909,8 @@ def test_local_holdout_delivery_rule_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('user_delivery_exclude', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True @@ -1936,8 +1935,8 @@ def test_local_holdout_td_experiment_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('testUserId', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True @@ -1964,9 +1963,9 @@ def test_local_holdout_ab_experiment_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('user_ab_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - # A/B experiment should still be blocked by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + # A/B experiment is still skipped by the local holdout + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) mock_get_var.assert_not_called() # ------------------------------------------------------------------ @@ -2017,8 +2016,8 @@ def test_global_holdout_exclude_td_true_td_match_has_holdout_decision(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(result.get('holdout_decision')) - self.assertEqual(result['holdout_decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) + self.assertEqual(result['holdout_decisions'][0].source, enums.DecisionSources.HOLDOUT) # ------------------------------------------------------------------ # Test 10: No TD match returns non-holdout with holdout_decision attached @@ -2040,9 +2039,7 @@ def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - holdout_dec = result.get('holdout_decision') - self.assertIsNotNone(holdout_dec) - self.assertEqual(holdout_dec.source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.source for d in result['holdout_decisions']], [enums.DecisionSources.HOLDOUT]) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, " @@ -2090,3 +2087,101 @@ def capture_notification(notification_type: str, user_id: str, captured_notifications[0].get('decision_event_dispatched'), 'decision_event_dispatched should be True when holdout impression is sent' ) + + +class LocalHoldoutContinueEvaluationTest(base.BaseTest): + """A local holdout hit excludes the user from that rule only; evaluation continues.""" + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def _make_opt(self, local_holdouts, send_flag_decisions=True, experiment_flag_has_rollout=False): + cfg = copy.deepcopy(self.config_dict_with_features) + cfg['holdouts'] = [] + cfg['localHoldouts'] = local_holdouts + cfg['sendFlagDecisions'] = send_flag_decisions + # Everyone Else rule '211147' takes all traffic so a later rule always serves. + for rollout in cfg['rollouts']: + for rule in rollout['experiments']: + if rule['id'] == '211147': + rule['trafficAllocation'] = [{'entityId': '211149', 'endOfRange': 10000}] + if experiment_flag_has_rollout: + for flag in cfg['featureFlags']: + if flag['key'] == 'test_feature_in_experiment': + flag['rolloutId'] = '211111' + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + def _decide(self, opt, flag_key): + with mock.patch.object(opt, '_send_impression_event') as mock_send: + decision = opt.create_user_context('test_user', {}).decide(flag_key) + # (rule_key, rule_type) per impression + return decision, [(c.args[4], c.args[5]) for c in mock_send.call_args_list] + + def test_holdout_hit_then_later_rule_serves(self): + opt = self._make_opt( + [_holdout('lh1', 'local_exp', included_rules=['111127'])], + experiment_flag_has_rollout=True, + ) + decision, impressions = self._decide(opt, 'test_feature_in_experiment') + + self.assertEqual(decision.rule_key, '211147') + self.assertEqual(impressions, [ + ('local_exp', str(enums.DecisionSources.HOLDOUT)), + ('211147', str(enums.DecisionSources.ROLLOUT)), + ]) + + def test_holdout_hits_on_several_rules_send_several_holdout_events(self): + opt = self._make_opt( + [ + _holdout('lh1', 'local_rule_1', included_rules=['211127']), + _holdout('lh2', 'local_rule_2', included_rules=['211137']), + ], + send_flag_decisions=False, + ) + decision, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(decision.rule_key, '211147') + self.assertEqual(impressions, [ + ('local_rule_1', str(enums.DecisionSources.HOLDOUT)), + ('local_rule_2', str(enums.DecisionSources.HOLDOUT)), + ]) + + def test_first_matching_holdout_per_rule_wins(self): + opt = self._make_opt( + [ + _holdout('lh1', 'first', included_rules=['211127']), + _holdout('lh2', 'second', included_rules=['211127']), + ], + send_flag_decisions=False, + ) + _, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(impressions, [('first', str(enums.DecisionSources.HOLDOUT))]) + + def test_holdout_hit_then_no_rule_serves(self): + opt = self._make_opt([_holdout('lh1', 'local_exp', included_rules=['111127'])], send_flag_decisions=False) + decision, impressions = self._decide(opt, 'test_feature_in_experiment') + + self.assertIsNone(decision.variation_key) + self.assertFalse(decision.enabled) + self.assertEqual(impressions, [('local_exp', str(enums.DecisionSources.HOLDOUT))]) + + def test_delivery_holdout_hit_sends_event_with_send_flag_decisions_off(self): + opt = self._make_opt( + [_holdout('lh1', 'local_delivery', included_rules=['211147'])], + send_flag_decisions=False, + ) + _, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(impressions, [('local_delivery', str(enums.DecisionSources.HOLDOUT))]) + + def test_disable_decision_event_suppresses_holdout_events(self): + opt = self._make_opt([_holdout('lh1', 'local_delivery', included_rules=['211147'])]) + with mock.patch.object(opt, '_send_impression_event') as mock_send: + opt.create_user_context('test_user', {}).decide( + 'test_feature_in_rollout', [OptimizelyDecideOption.DISABLE_DECISION_EVENT] + ) + + mock_send.assert_not_called()