diff --git a/.sampo/changesets/local-eval-holdouts.md b/.sampo/changesets/local-eval-holdouts.md new file mode 100644 index 000000000..32a5e81bd --- /dev/null +++ b/.sampo/changesets/local-eval-holdouts.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Honor `filters.holdout` during local feature flag evaluation. A user in an experiment holdout now receives the `holdout-` variant instead of being bucketed into a regular variant, matching how the server evaluates the same flag. Holdout membership is resolved before release conditions, so a held-out user never reaches the flag's targeting. diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index cb4daebb1..87da025b5 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -113,12 +113,48 @@ class RequiresServerEvaluation(Exception): # Given the same bucketing value and key, it'll always return the same float. These floats are # uniformly distributed between 0 and 1, so if we want to show this feature to 20% of traffic # we can do _hash(key, bucketing_value) < 0.2 -def _hash(key: str, bucketing_value: str, salt: str = "") -> float: - hash_key = f"{key}.{bucketing_value}{salt}" +def _hash( + key: str, bucketing_value: str, salt: str = "", separator: str = "." +) -> float: + hash_key = f"{key}{separator}{bucketing_value}{salt}" hash_val = int(hashlib.sha1(hash_key.encode("utf-8")).hexdigest()[:15], 16) return hash_val / __LONG_SCALE__ +def _holdout_hash(bucketing_value: str) -> float: + """Hash a bucketing value for holdout membership, matching the server. + + The separator is the whole point: the server hashes `holdout-`, while flag + rollout hashing joins with a dot. Taking the default separator here would still look + uniform and deterministic while holding out a different set of people than the server. + """ + return _hash("holdout", bucketing_value, separator="-") + + +def _get_holdout_variant(flag, bucketing_value) -> Optional[str]: + """The `holdout-` variant this value is held out into, or None. + + Mirrors the server's evaluation order: a held-out value never reaches the flag's + release conditions, so callers check this before matching any condition. + """ + holdout = (flag.get("filters") or {}).get("holdout") + if not holdout: + return None + + exclusion_percentage = holdout.get("exclusion_percentage") + holdout_id = holdout.get("id") + if exclusion_percentage is None or holdout_id is None: + return None + + # The server clamps out-of-range percentages rather than rejecting them, and treats + # 100 as "everyone" without hashing, so a 100% holdout can't miss on a hash boundary. + percentage = min(max(float(exclusion_percentage), 0.0), 100.0) + if percentage != 100.0 and _holdout_hash(bucketing_value) > percentage / 100: + return None + + return f"holdout-{holdout_id}" + + def get_matching_variant(flag, bucketing_value): hash_value = _hash(flag["key"], bucketing_value, salt="variant") for variant in variant_lookup_table(flag): @@ -364,6 +400,13 @@ def match_feature_flag_properties( bucketing_value = resolve_bucketing_value(flag, distinct_id, device_id) flag_filters = flag.get("filters") or {} + + # Holdouts are evaluated before release conditions, so a held-out value is excluded + # from the flag's targeting entirely rather than being bucketed into a variant. + holdout_variant = _get_holdout_variant(flag, bucketing_value) + if holdout_variant is not None: + return holdout_variant + flag_conditions = flag_filters.get("groups") or [] flag_aggregation = flag_filters.get("aggregation_group_type_index") early_exit_enabled = flag_filters.get("early_exit") diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 107f9a930..017a655e7 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1,4 +1,5 @@ import datetime +import hashlib import threading import unittest @@ -462,6 +463,101 @@ def test_early_exit_does_not_trigger_on_property_mismatch(self): ) ) + def _holdout_flag(self, exclusion_percentage, rollout_percentage=100): + return [ + { + "id": 1, + "name": "Experiment Flag", + "key": "experiment-flag", + "active": True, + "filters": { + "multivariate": { + "variants": [ + {"key": "control", "rollout_percentage": 50}, + {"key": "test", "rollout_percentage": 50}, + ] + }, + "groups": [ + {"properties": [], "rollout_percentage": rollout_percentage} + ], + "holdout": { + "id": 727, + "exclusion_percentage": exclusion_percentage, + }, + }, + } + ] + + def test_holdout_at_100_percent_excludes_every_distinct_id(self): + self.client.feature_flags = self._holdout_flag(100) + + for distinct_id in ["user_1", "user_2", "user_3", "user_4", "user_5"]: + self.assertEqual( + self.client.get_feature_flag( + "experiment-flag", distinct_id, only_evaluate_locally=True + ), + "holdout-727", + ) + + def test_holdout_at_0_percent_excludes_nobody(self): + self.client.feature_flags = self._holdout_flag(0) + + for distinct_id in ["user_1", "user_2", "user_3", "user_4", "user_5"]: + self.assertIn( + self.client.get_feature_flag( + "experiment-flag", distinct_id, only_evaluate_locally=True + ), + ["control", "test"], + ) + + def test_holdout_is_evaluated_before_release_conditions(self): + # The flag releases to nobody, but a held-out user is excluded before targeting + # is consulted, so they still get the holdout variant rather than False. + self.client.feature_flags = self._holdout_flag(100, rollout_percentage=0) + + self.assertEqual( + self.client.get_feature_flag( + "experiment-flag", "user_1", only_evaluate_locally=True + ), + "holdout-727", + ) + + def test_holdout_membership_matches_server_bucketing(self): + # The server hashes "holdout-". Pinning the exact membership set + # guards the string construction: reusing the flag hash helper, which joins with + # a dot, still looks uniform and deterministic but holds out different people. + distinct_ids = [f"user_{n}" for n in range(1, 21)] + exclusion_percentage = 20 + + def server_hash(prefix, distinct_id): + digest = hashlib.sha1(f"{prefix}{distinct_id}".encode("utf-8")).hexdigest() + return int(digest[:15], 16) / float(0xFFFFFFFFFFFFFFF) + + expected = { + distinct_id + for distinct_id in distinct_ids + if server_hash("holdout-", distinct_id) <= exclusion_percentage / 100 + } + dot_joined = { + distinct_id + for distinct_id in distinct_ids + if server_hash("holdout.", distinct_id) <= exclusion_percentage / 100 + } + # Guard the guard: if these ever coincide the test would pass with the bug present. + self.assertNotEqual(expected, dot_joined) + + self.client.feature_flags = self._holdout_flag(exclusion_percentage) + held_out = { + distinct_id + for distinct_id in distinct_ids + if self.client.get_feature_flag( + "experiment-flag", distinct_id, only_evaluate_locally=True + ) + == "holdout-727" + } + + self.assertEqual(held_out, expected) + def test_early_exit_on_multivariate_flag(self): self.client.feature_flags = [ {