Skip to content

Commit 59256c1

Browse files
Mat001claude
Mat001
and
claude
committed
[FSSDK-12369] Fix local holdout ordering: check after forced decision, not before
Move local holdout checks inside getVariationFromExperimentRule() and getVariationFromDeliveryRule(), directly after the forced decision block. Previously the checks were in the outer rule loop, before calling those methods, which caused local holdouts to run before forced decisions — violating the mandatory order: forced decision → local holdout → regular. Add enforcement test: forced decision beats a 100%-traffic local holdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent c3fffde commit 59256c1

2 files changed

Lines changed: 71 additions & 26 deletions

File tree

‎core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -398,20 +398,6 @@ DecisionResponse<FeatureDecision> getVariationFromExperiment(@Nonnull ProjectCon
398398
for (String experimentId : featureFlag.getExperimentIds()) {
399399
Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId);
400400

401-
// Check local holdouts targeting this experiment rule before regular evaluation (FSSDK-12369)
402-
if (experiment != null) {
403-
List<Holdout> localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(experiment.getId());
404-
for (Holdout holdout : localHoldouts) {
405-
DecisionResponse<Variation> holdoutDecision = getVariationForHoldout(holdout, user, projectConfig);
406-
reasons.merge(holdoutDecision.getReasons());
407-
if (holdoutDecision.getResult() != null) {
408-
return new DecisionResponse<>(
409-
new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT),
410-
reasons);
411-
}
412-
}
413-
}
414-
415401
DecisionResponse<Variation> decisionVariation =
416402
getVariationFromExperimentRule(projectConfig, featureFlag.getKey(), experiment, user, options, userProfileTracker, decisionPath);
417403
reasons.merge(decisionVariation.getReasons());
@@ -483,18 +469,6 @@ DecisionResponse<FeatureDecision> getVariationForFeatureInRollout(@Nonnull Featu
483469
while (index < rolloutRulesLength) {
484470
Experiment rule = rollout.getExperiments().get(index);
485471

486-
// Check local holdouts targeting this delivery rule before regular evaluation (FSSDK-12369)
487-
List<Holdout> localHoldoutsForRule = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId());
488-
boolean localHoldoutHit = false;
489-
for (Holdout holdout : localHoldoutsForRule) {
490-
DecisionResponse<Variation> holdoutDecision = getVariationForHoldout(holdout, user, projectConfig);
491-
reasons.merge(holdoutDecision.getReasons());
492-
if (holdoutDecision.getResult() != null) {
493-
FeatureDecision holdoutFeatureDecision = new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT);
494-
return new DecisionResponse(holdoutFeatureDecision, reasons);
495-
}
496-
}
497-
498472
DecisionResponse<AbstractMap.SimpleEntry> decisionVariationResponse = getVariationFromDeliveryRule(
499473
projectConfig,
500474
featureFlag.getKey(),
@@ -872,6 +846,16 @@ private DecisionResponse<Variation> getVariationFromExperimentRule(@Nonnull Proj
872846
return new DecisionResponse(variation, reasons);
873847
}
874848

849+
// Check local holdouts targeting this specific experiment rule (FSSDK-12369)
850+
List<Holdout> localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId());
851+
for (Holdout holdout : localHoldouts) {
852+
DecisionResponse<Variation> holdoutDecision = getVariationForHoldout(holdout, user, projectConfig);
853+
reasons.merge(holdoutDecision.getReasons());
854+
if (holdoutDecision.getResult() != null) {
855+
return new DecisionResponse<>(holdoutDecision.getResult(), reasons);
856+
}
857+
}
858+
875859
//regular decision
876860
DecisionResponse<Variation> decisionResponse = getVariation(rule, user, projectConfig, options, userProfileTracker, null, decisionPath);
877861
reasons.merge(decisionResponse.getReasons());
@@ -922,6 +906,17 @@ DecisionResponse<AbstractMap.SimpleEntry> getVariationFromDeliveryRule(@Nonnull
922906
return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons);
923907
}
924908

909+
// Check local holdouts targeting this specific delivery rule (FSSDK-12369)
910+
List<Holdout> localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId());
911+
for (Holdout holdout : localHoldouts) {
912+
DecisionResponse<Variation> holdoutDecision = getVariationForHoldout(holdout, user, projectConfig);
913+
reasons.merge(holdoutDecision.getReasons());
914+
if (holdoutDecision.getResult() != null) {
915+
variationToSkipToEveryoneElsePair = new AbstractMap.SimpleEntry<>(holdoutDecision.getResult(), false);
916+
return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons);
917+
}
918+
}
919+
925920
// Handle a regular decision
926921
String bucketingId = getBucketingId(user.getUserId(), user.getAttributes());
927922
Boolean everyoneElse = (ruleIndex == rules.size() - 1);

‎core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@
8686
import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_2;
8787
import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE;
8888
import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION;
89+
import static com.optimizely.ab.config.ValidProjectConfigV4.EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY;
8990
import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_HOLDOUT_VARIATION_OFF;
91+
import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY;
9092
import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4_holdout;
9193
import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4WithHoldouts;
9294
import com.optimizely.ab.config.Variation;
@@ -1966,6 +1968,54 @@ public void localHoldouts_appliesToDeliveryRules() {
19661968
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
19671969
}
19681970

1971+
/**
1972+
* Forced decision takes priority over a 100%-traffic local holdout.
1973+
* Even when a local holdout would bucket the user, a forced decision for the same rule must win.
1974+
* This is the mandatory ordering enforcement test — if it fails, the per-rule ordering is wrong.
1975+
*/
1976+
@Test
1977+
public void localHoldouts_forcedDecisionTakesPriorityOverLocalHoldout() {
1978+
final String experimentRuleId = "3262035800"; // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID
1979+
final String flagKey = FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(); // "multi_variate_feature"
1980+
final String ruleKey = EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY; // "multivariate_experiment"
1981+
1982+
// 100% traffic local holdout targeting the same experiment rule
1983+
Holdout localHoldout = new Holdout(
1984+
"local_ho_forced_test",
1985+
"local_holdout_forced_decision_test",
1986+
Holdout.HoldoutStatus.RUNNING.toString(),
1987+
Collections.emptyList(),
1988+
null,
1989+
DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF),
1990+
DatafileProjectConfigTestUtils.createListOfObjects(
1991+
new TrafficAllocation("$opt_dummy_variation_id", 10000) // 100% — would always bucket
1992+
),
1993+
Collections.singletonList(experimentRuleId)
1994+
);
1995+
1996+
ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout));
1997+
Bucketer bucketer = new Bucketer();
1998+
DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService);
1999+
2000+
// Set a forced decision for the same rule
2001+
OptimizelyUserContext userCtx = optimizely.createUserContext("anyUser", Collections.emptyMap());
2002+
OptimizelyDecisionContext ctx = new OptimizelyDecisionContext(flagKey, ruleKey);
2003+
OptimizelyForcedDecision forcedDecision = new OptimizelyForcedDecision(VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY);
2004+
userCtx.setForcedDecision(ctx, forcedDecision);
2005+
2006+
FeatureDecision decision = svc.getVariationForFeature(
2007+
FEATURE_FLAG_MULTI_VARIATE_FEATURE,
2008+
userCtx,
2009+
config
2010+
).getResult();
2011+
2012+
// Forced decision must win — NOT the holdout
2013+
assertNotEquals("Holdout must not win when forced decision is set",
2014+
FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
2015+
assertEquals("Forced variation key must be returned",
2016+
VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY, decision.variation.getKey());
2017+
}
2018+
19692019
/**
19702020
* Builds a full DatafileProjectConfig using the same setup as generateValidProjectConfigV4_holdout()
19712021
* but with the given holdout list substituted. This avoids spy-based mocking and ensures that

0 commit comments

Comments
 (0)