[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.

Author: alexjoeyyong

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,

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

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