diff --git a/.sampo/changesets/fix-failed-flag-records.md b/.sampo/changesets/fix-failed-flag-records.md new file mode 100644 index 000000000..f1500a70e --- /dev/null +++ b/.sampo/changesets/fix-failed-flag-records.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Treat failed feature flag records as unevaluated, report evaluation failure diagnostics, and preserve successful local values and payloads when remote fallback fails. diff --git a/posthog/client.py b/posthog/client.py index 35380d979..069787478 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -4646,6 +4646,8 @@ def _get_feature_flag_result( errors.append(FeatureFlagError.ERRORS_WHILE_COMPUTING) if flag_details is None: errors.append(FeatureFlagError.FLAG_MISSING) + elif flag_details._failed: + errors.append("evaluation_failed") if errors: feature_flag_error = ",".join(errors) @@ -4677,10 +4679,11 @@ def _get_feature_flag_result( self.log.debug( f"Successfully computed flag remotely: #{key} -> #{flag_result}" ) - # A payload lookup for a caller's match_value reports that value, not - # the record's. A failure above falls back to a stale cached value. - value_from_details = ( - flag_details is not None and override_match_value is None + # A successful payload override reports the caller's value. A + # failed record still supplies diagnostics, never the override. + value_from_details = flag_details is not None and ( + override_match_value is None + or (flag_details._failed and flag_details._malformed_field is None) ) except QuotaLimitError as e: self.log.warning(f"[FEATURE FLAGS] Quota limit exceeded: {e}") @@ -5321,7 +5324,32 @@ def get_all_flags_and_payloads( flag_keys_to_evaluate=flag_keys_to_evaluate, device_id=device_id, ) - response = to_flags_and_payloads(decide_response) + remote_response = to_flags_and_payloads(decide_response) + # Only explicit failed records preserve same-context local pairs; + # successful remote records retain the existing precedence. + failed_keys = { + key + for key, detail in decide_response.get("flags", {}).items() + if detail._failed and detail._malformed_field is None + } + response = { + "featureFlags": { + **{ + k: v + for k, v in (response["featureFlags"] or {}).items() + if k in failed_keys + }, + **(remote_response["featureFlags"] or {}), + }, + "featureFlagPayloads": { + **{ + k: v + for k, v in (response["featureFlagPayloads"] or {}).items() + if k in failed_keys + }, + **(remote_response["featureFlagPayloads"] or {}), + }, + } except Exception as e: self.log.exception( f"[FEATURE FLAGS] Unable to get feature flags and payloads: {e}" @@ -5535,7 +5563,14 @@ def evaluate_flags( quota_limited=quota_limited, minimal_flag_called_events=minimal_flag_called_events, unresolved_flags={ - key: reason for key, reason in unresolved.items() if key not in records + key: reason + for key, reason in unresolved.items() + if key not in records + or ( + isinstance(record := records[key], _RemoteFlagRecord) + and record.details._failed + and record.details._malformed_field is None + ) }, ) diff --git a/posthog/feature_flag_evaluations.py b/posthog/feature_flag_evaluations.py index 8ddd10f49..d938b43aa 100644 --- a/posthog/feature_flag_evaluations.py +++ b/posthog/feature_flag_evaluations.py @@ -152,7 +152,14 @@ def __init__( """Internal — instances are created by the SDK via ``Client.evaluate_flags()``.""" self._host = host self._distinct_id = distinct_id - self._flags = flags + self._failed_flags = { + key: flag + for key, flag in flags.items() + if isinstance(flag, _RemoteFlagRecord) + and flag.details._failed + and flag.details._malformed_field is None + } + self._flags = {k: v for k, v in flags.items() if k not in self._failed_flags} self._groups: Dict[str, Union[str, int]] = dict(groups or {}) self._disable_geoip = disable_geoip self._request_id = request_id @@ -337,8 +344,9 @@ def _record_access(self, key: str, send_feature_flag_events: bool) -> None: if not self._distinct_id: return - flag = self._flags.get(key) - if flag is None: + failed = key in self._failed_flags + flag = self._flags.get(key) or self._failed_flags.get(key) + if flag is None or failed: response: Optional[FlagValue] = None elif not flag.enabled: response = False @@ -376,7 +384,9 @@ def _record_access(self, key: str, send_feature_flag_events: bool) -> None: errors.append("errors_while_computing_flags") if self._quota_limited: errors.append("quota_limited") - if flag is None: + if failed: + errors.append("evaluation_failed") + elif flag is None: errors.append( "local_evaluation_inconclusive" if key in self._unresolved_flags diff --git a/posthog/test/test_evaluate_flags.py b/posthog/test/test_evaluate_flags.py index dbe33583c..1382e4dd5 100644 --- a/posthog/test/test_evaluate_flags.py +++ b/posthog/test/test_evaluate_flags.py @@ -885,19 +885,60 @@ def test_dependency_resolved_locally_picks_its_condition(self, patch_flags): self.assertEqual(flags.get_flag("target"), "a") patch_flags.assert_not_called() + @parameterized.expand([(True,), (False,)]) @mock.patch("posthog.client.flags") - def test_flag_resolved_by_remote_fallback_is_not_unresolved(self, patch_flags): + def test_flag_resolved_by_remote_fallback_is_not_unresolved( + self, enabled, patch_flags + ): patch_flags.return_value = { - "flags": {"checkout": {"enabled": True, "variant": None}} + "flags": {"checkout": {"enabled": enabled, "variant": None}} } flags = self.client.evaluate_flags("user-123") - self.assertIs(flags.get_flag("checkout"), True) + self.assertIs(flags.get_flag("checkout"), enabled) self.assertEqual(sorted(flags.keys), ["beta-ui", "checkout"]) self.assertEqual(dict(flags.unresolved_flags), {}) patch_flags.assert_called_once() + @parameterized.expand([(None,), (False,), (True,)]) + @mock.patch("posthog.client.flags") + @mock.patch.object(Client, "capture") + def test_failed_remote_fallback_preserves_unresolved_reason( + self, error_bit, patch_capture, patch_flags + ): + response = { + "flags": { + key: { + "enabled": False, + "failed": True, + "metadata": {"payload": '"placeholder"'}, + } + for key in ("checkout", "remote-only") + } + } + if error_bit is not None: + response["errorsWhileComputingFlags"] = error_bit + patch_flags.return_value = response + + flags = self.client.evaluate_flags("user-123") + + self.assertEqual( + dict(flags.unresolved_flags), + {"checkout": UnresolvedFlagReason.EXPERIENCE_CONTINUITY}, + ) + self.assertEqual(flags.keys, ["beta-ui"]) + self.assertIsNone(flags.get_flag("checkout", send_feature_flag_events=False)) + self.assertTrue( + flags.is_enabled("checkout", True, send_feature_flag_events=False) + ) + self.assertIsNone(flags.get_flag_payload("checkout")) + self.assertNotIn("$feature/checkout", flags._get_event_properties()) + self.assertEqual(dict(flags.only(["checkout"]).unresolved_flags), {}) + self.assertEqual(dict(flags.only_accessed().unresolved_flags), {}) + patch_capture.assert_not_called() + patch_flags.assert_called_once() + @parameterized.expand( [ ( diff --git a/posthog/test/test_experiment_exposure.py b/posthog/test/test_experiment_exposure.py index c12152833..db59f5c66 100644 --- a/posthog/test/test_experiment_exposure.py +++ b/posthog/test/test_experiment_exposure.py @@ -517,18 +517,28 @@ def _attribution_response(source: str) -> Dict[str, Any]: "source, expected", ATTRIBUTION_CASES, ids=[c[0] for c in ATTRIBUTION_CASES] ) def test_flag_called_attribution_follows_the_record(source, expected, path): - events = _read_events(path, _attribution_response(source), key="garden-layout") + response = _attribution_response(source) + failed = response["flags"]["garden-layout"].get("failed") is True + events = _read_events(path, response, key="garden-layout") [called] = _named(events, FLAG_CALLED, "garden-layout") assert _attribution(called.properties) == expected - FLAG_CALLED_SCHEMA.validate(_projection(FLAG_CALLED_SCHEMA, called.properties)) + if failed: + # The vendored schema predates the no-result sentinel for failed records. + assert called.properties["$feature_flag_response"] is None + assert set(called.properties["$feature_flag_error"].split(",")) == { + "evaluation_failed", + "errors_while_computing_flags", + } + else: + FLAG_CALLED_SCHEMA.validate(_projection(FLAG_CALLED_SCHEMA, called.properties)) exposures = _named(events, EXPOSURE, "garden-layout") assert len(exposures) == (1 if "$feature_flag_variant_key" in expected else 0) # A record whose reason names no rule, or that failed, reports no rule even when it -# carries one. Neither sends an exposure. The last element says whether the record is -# a valid server outcome, which the contract's event schema then accepts. +# carries one. Neither sends an exposure. The last element says whether a successful +# record is a valid server outcome, which the contract's event schema then accepts. STRAY_RULE_CASES = [ ("no_rule_match", {}, True), ("missing_group_key", {}, True), @@ -569,7 +579,10 @@ def test_rule_fields_are_sent_only_for_reasons_that_name_a_rule( "$feature_flag_reason_code": code, } assert _named(events, EXPOSURE) == [] - if valid_outcome: + if fields.get("failed") is True: + assert called.properties["$feature_flag_response"] is None + assert called.properties["$feature_flag_error"] == "evaluation_failed" + elif valid_outcome: FLAG_CALLED_SCHEMA.validate(_projection(FLAG_CALLED_SCHEMA, called.properties)) diff --git a/posthog/test/test_failed_flag_records.py b/posthog/test/test_failed_flag_records.py new file mode 100644 index 000000000..09a9fe456 --- /dev/null +++ b/posthog/test/test_failed_flag_records.py @@ -0,0 +1,193 @@ +"""Spec 211: server failed records supply diagnostics, not evaluated values.""" + +import copy +from unittest import mock + +import pytest + +from posthog import AsyncPosthog +from posthog.client import Client +from posthog.types import normalize_flags_response + + +@pytest.fixture +def client(): + client = Client("project-key", send=False) + yield client + client.shutdown() + + +def response(shape, error_bit, failed_value="remote"): + records = {} + for key, value, failed in [("failed", failed_value, True), ("off", False, False)]: + record = { + "key": key, + "failed": failed, + "reason": {"code": "dependency_error" if failed else "flag_disabled"}, + "metadata": {"id": 1, "version": 2, "payload": '{"copy":"remote"}'}, + } + if shape == "v4": + record["value"] = value + record["metadata"]["config_version"] = 2 + else: + record.update( + enabled=bool(value), variant=value if isinstance(value, str) else None + ) + records[key] = record + result = { + "flags": records, + "featureFlags": {"failed": "mirror"}, + "featureFlagPayloads": {"failed": '"mirror"'}, + } + if error_bit is not None: + result["errorsWhileComputingFlags"] = error_bit + return result + + +@pytest.mark.parametrize("shape", ["v2", "v4"]) +@pytest.mark.parametrize("error_bit", [None, False, True]) +@pytest.mark.parametrize("failed_value", [False, "remote"]) +@pytest.mark.parametrize("local_inconclusive", [False, True]) +def test_failed_direct_results_and_events( + client, shape, error_bit, failed_value, local_inconclusive +): + if local_inconclusive: + client.feature_flags = [ + { + "key": "failed", + "active": True, + "ensure_experience_continuity": True, + "filters": {"groups": []}, + } + ] + with ( + mock.patch( + "posthog.client.flags", + side_effect=lambda *args, **kwargs: copy.deepcopy( + response(shape, error_bit, failed_value) + ), + ), + mock.patch.object(client, "capture", return_value=(True, {})) as capture, + ): + assert ( + client.get_feature_flag_result( + "failed", "user", send_feature_flag_events=False + ) + is None + ) + capture.assert_not_called() + assert client.get_feature_flag_result("failed", "user") is None + assert client.get_feature_flag("failed", "user") is None + assert client.feature_enabled("failed", "user") is None + assert ( + client.get_feature_flag_payload( + "failed", "user", match_value=True, send_feature_flag_events=True + ) + is None + ) + assert capture.call_count == 1 + props = capture.call_args.kwargs["properties"] + assert props["$feature_flag_response"] is None + assert "evaluation_failed" in props["$feature_flag_error"].split(",") + assert "flag_missing" not in props["$feature_flag_error"] + assert ("errors_while_computing_flags" in props["$feature_flag_error"]) is bool( + error_bit + ) + if shape == "v4": + assert props["$feature_flag_config_version"] == 2 + assert props["$feature_flag_reason_code"] == "dependency_error" + assert "$feature_flag_payload" not in props + assert client.get_feature_flag_result("off", "user").enabled is False + assert "evaluation_failed" not in capture.call_args.kwargs["properties"].get( + "$feature_flag_error", "" + ) + combined = client.get_all_flags_and_payloads("user") + assert combined["featureFlags"] == {"off": False} + assert "failed" not in combined["featureFlagPayloads"] + + +@pytest.mark.parametrize("shape", ["v2", "v4"]) +@pytest.mark.parametrize("error_bit", [None, False, True]) +def test_failed_snapshot_defaults_diagnostics_and_enrichment(client, shape, error_bit): + with ( + mock.patch("posthog.client.flags", return_value=response(shape, error_bit)), + mock.patch.object(client, "capture", return_value=(True, {})) as capture, + ): + snapshot = client.evaluate_flags("user") + assert ( + snapshot.is_enabled("failed", True, send_feature_flag_events=False) is True + ) + capture.assert_not_called() + assert snapshot.get_flag("failed") is None + assert snapshot.get_flag_payload("failed") is None + props = capture.call_args.kwargs["properties"] + assert props["$feature_flag_response"] is None + assert "evaluation_failed" in props["$feature_flag_error"] + assert "flag_missing" not in props["$feature_flag_error"] + assert ("errors_while_computing_flags" in props["$feature_flag_error"]) is bool( + error_bit + ) + assert snapshot.is_enabled("off", True) is False + assert snapshot.keys == ["off"] + assert snapshot._get_event_properties() == {"$feature/off": False} + + +@pytest.mark.parametrize("shape", ["v2", "v4"]) +@pytest.mark.parametrize("error_bit", [None, False, True]) +def test_failed_fallback_preserves_only_successful_local_pairs( + client, shape, error_bit +): + local = { + "featureFlags": {"on": True, "off": False, "variant": "blue", "success": True}, + "featureFlagPayloads": { + k: '{"copy":"local"}' for k in ["on", "off", "variant", "success"] + }, + } + remote = response(shape, error_bit) + for key in ["on", "off", "variant"]: + remote["flags"][key] = copy.deepcopy(remote["flags"]["failed"]) + remote["flags"][key]["key"] = key + remote["flags"]["success"] = { + "key": "success", + "enabled": True, + "variant": "remote", + "metadata": {"payload": '{"copy":"success"}'}, + } + with ( + mock.patch.object( + client, + "_get_all_flags_and_payloads_locally", + return_value=(local, True, {}), + ), + mock.patch.object( + client, "_get_flags_decision", return_value=normalize_flags_response(remote) + ), + ): + result = client.get_all_flags_and_payloads("user") + assert result["featureFlags"] == { + "on": True, + "off": False, + "variant": "blue", + "success": "remote", + } + assert result["featureFlagPayloads"] == { + k: '{"copy":"success"}' if k == "success" else '{"copy":"local"}' + for k in ["on", "off", "variant", "success"] + } + + +async def test_failed_async_snapshot_honors_default(): + client = AsyncPosthog("project-key", send=False) + try: + with mock.patch( + "posthog.async_client._async_flags", + new=mock.AsyncMock(return_value=response("v4", False)), + ): + snapshot = await client.evaluate_flags("user") + assert ( + snapshot.is_enabled("failed", True, send_feature_flag_events=False) is True + ) + assert snapshot.get_flag("failed", send_feature_flag_events=False) is None + assert snapshot.keys == ["off"] + finally: + await client.shutdown() diff --git a/posthog/test/test_flags_released_behaviour.py b/posthog/test/test_flags_released_behaviour.py index 7f5dd12b9..d646e076e 100644 --- a/posthog/test/test_flags_released_behaviour.py +++ b/posthog/test/test_flags_released_behaviour.py @@ -38,9 +38,8 @@ def test_every_legacy_getter_and_event_matches_7_82_1(request, results): assert (sparse["id"], sparse["version"]) == (0, 0) sparse["id"] = sparse["version"] = None - # A failed record reads as disabled, so it has no payload, even when the record - # carries one. 7.82.1 returned that payload from the payload reads and sent it on - # the flag's $feature_flag_called events. + # An explicit failed record has no evaluated value or payload. 7.82.1 treated + # it as disabled, included it in bulk results, and returned its payload. failed = expected["get_flags_decision"]["flags"]["server-failed"]["metadata"] assert failed["payload"] == '{"columns": 2}' failed["payload"] = None @@ -48,7 +47,15 @@ def test_every_legacy_getter_and_event_matches_7_82_1(request, results): assert expected[f"{read}:server-failed"] == {"columns": 2} expected[f"{read}:server-failed"] = None assert expected["get_feature_flag_result:server-failed"][2] == {"columns": 2} - expected["get_feature_flag_result:server-failed"][2] = None + expected["get_feature_flag_result:server-failed"] = None + for read in ["get_feature_flag", "feature_enabled", "snapshot.get_flag"]: + assert expected[f"{read}:server-failed"] is False + expected[f"{read}:server-failed"] = None + for read in ["get_feature_variants", "get_all_flags"]: + assert expected[read].pop("server-failed") is False + for read in ["get_feature_flags_and_payloads", "get_all_flags_and_payloads"]: + assert expected[read]["featureFlags"].pop("server-failed") is False + expected["snapshot.keys"].remove("server-failed") failed_events = [ properties for _, properties in expected["events"] @@ -57,6 +64,13 @@ def test_every_legacy_getter_and_event_matches_7_82_1(request, results): assert failed_events for properties in failed_events: assert properties.pop("$feature_flag_payload") == {"columns": 2} + assert properties["$feature_flag_response"] is False + properties["$feature_flag_response"] = None + properties["$feature/server-failed"] = None + properties["$feature_flag_error"] = "evaluation_failed" + for name, properties in expected["events"]: + if name in ["snapshot-event", "fetched-event"]: + assert properties.pop("$feature/server-failed") is False if request.node.callspec.params["results"] == "v4": # A $feature_flag_called for a config version 2 record carries its config @@ -108,12 +122,16 @@ def test_fallback_cache_histories_match_7_82_1(results, name): # unknown_error, no request details, and the cached value. expected = copy.deepcopy(RELEASED["cache histories"][name]) if name == "good, server failed, timeout": - # A failed record has no payload, even when the record carries one, so - # neither its read nor the cached result read after the timeout returns - # one, and their events leave it out. 7.82.1 returned it from both. - for result in expected["results"][1:]: - assert result[2] == {"theme": "dark"} - result[2] = None - for _, properties in expected["events"][1:]: - assert properties.pop("$feature_flag_payload") == {"theme": "dark"} + # Explicit failure supplies no result and cannot poison the last good + # cache entry. The later timeout still returns that successful pair. + expected["results"][1] = None + expected["results"][2] = copy.deepcopy(expected["results"][0]) + failed = expected["events"][1][1] + assert failed.pop("$feature_flag_payload") == {"theme": "dark"} + failed["$feature_flag_response"] = None + failed["$feature/cached"] = None + failed["$feature_flag_error"] = "evaluation_failed" + timeout = expected["events"][2][1] + timeout["$feature_flag_response"] = "blue" + timeout["$feature/cached"] = "blue" assert results["cache histories"][name] == expected diff --git a/posthog/test/test_flags_v4_response.py b/posthog/test/test_flags_v4_response.py index 352c793aa..c730f54f5 100644 --- a/posthog/test/test_flags_v4_response.py +++ b/posthog/test/test_flags_v4_response.py @@ -211,8 +211,8 @@ def test_a_failed_record_never_renders_its_value(client): response["flags"]["garden-layout"]["value"] = "compact" snapshot = _evaluate(client, response) - assert snapshot.get_flag("garden-layout") is False - assert snapshot.is_enabled("garden-layout", True) is False + assert snapshot.get_flag("garden-layout") is None + assert snapshot.is_enabled("garden-layout", True) is True @pytest.mark.parametrize( @@ -764,7 +764,7 @@ def _failed_with_payload(shape: str) -> Dict[str, Any]: @pytest.mark.parametrize("shape", ["v4", "older"]) def test_a_failed_record_has_no_payload_on_any_read(shape, client): - # A failed flag reads as disabled, so a payload on it would surprise the caller. + # A failed flag has no evaluated result or payload. response = _failed_with_payload(shape) [flag] = normalize_flags_response(copy.deepcopy(response))["flags"].values() assert flag._failed is True @@ -772,7 +772,7 @@ def test_a_failed_record_has_no_payload_on_any_read(shape, client): with mock.patch("posthog.client.flags", return_value=copy.deepcopy(response)): result = client.get_feature_flag_result("garden-layout", DISTINCT_ID) - assert result is not None and result.payload is None + assert result is None assert client.get_feature_flag_payload("garden-layout", DISTINCT_ID) is None assert ( client.get_feature_flag_payload( @@ -951,7 +951,8 @@ def test_v1_flags_render_the_same_legacy_values_from_both_shapes(case): "flags": v4_flags, } expected = { - "featureFlags": v1_maps["featureFlags"], + # Flat legacy maps retain their interpretation, but detailed failures do not. + "featureFlags": {} if outcome["failed"] else v1_maps["featureFlags"], "featureFlagPayloads": { key: json.loads(value) for key, value in v1_maps["featureFlagPayloads"].items() diff --git a/posthog/types.py b/posthog/types.py index 229b25a16..30e788447 100644 --- a/posthog/types.py +++ b/posthog/types.py @@ -639,7 +639,7 @@ def from_flag_details( FeatureFlagResult | None: A new FeatureFlagResult instance, or None if details is None. """ - if details is None: + if details is None or (details._failed and details._malformed_field is None): return None if override_match_value is not None: @@ -720,6 +720,7 @@ def to_values(response: FlagsResponse) -> Optional[dict[str, FlagValue]]: key: value.get_value() for key, value in flags.items() if isinstance(value, FeatureFlag) + and not (value._failed and value._malformed_field is None) } @@ -731,6 +732,7 @@ def to_payloads(response: FlagsResponse) -> Optional[dict[str, str]]: key: value.metadata.payload for key, value in response.get("flags", {}).items() if isinstance(value, FeatureFlag) + and not value._failed and value.enabled and value.metadata.payload is not None }