fix: omit failed remote feature flag values

.sampo/changesets/venerable-forgemaster-lempo.md

@@ -0,0 +1,5 @@
+---
+hex/posthog: patch
+---
+
+Treat failed remote feature flag records as no result, omit their values and payloads, and report evaluation_failed on direct reads.

lib/posthog/feature_flags.ex

@@ -411,15 +411,16 @@ defmodule PostHog.FeatureFlags do
         )
 
         merged = Map.merge(remote_results, local.results)
+        failed_keys = for {key, %{"failed" => true}} <- scoped_remote_flags, do: key
 
         if malformed? and map_size(merged) == 0 do
           remote_failure_result(name, distinct_id, local, body, invalid_flags_response())
         else
-          # A returned record, including one with `failed: true`, resolves the flag.
+          # Preserve snapshot unresolved classification without serving failed values.
           metadata =
             Map.merge(response_body, %{
               groups: evaluation_groups(body),
-              unresolved_flags: Map.drop(local.reasons, Map.keys(merged))
+              unresolved_flags: Map.drop(local.reasons, Map.keys(merged) ++ failed_keys)
             })
 
           {:ok, __MODULE__.Evaluations.from_results(name, distinct_id, merged, metadata)}
@@ -450,6 +451,7 @@ defmodule PostHog.FeatureFlags do
       {key, flag_data}, {results, malformed?} ->
         case build_result_safely(key, flag_data, response_body) do
           {:ok, result} -> {Map.put(results, key, result), malformed?}
+          :failed -> {results, malformed?}
           :error -> {results, true}
         end
 
@@ -458,6 +460,9 @@ defmodule PostHog.FeatureFlags do
     end)
   end
 
+  defp build_result_safely(key, %{"failed" => true}, _response_body) when is_binary(key),
+    do: :failed
+
   defp build_result_safely(key, flag_data, response_body)
        when is_binary(key) and is_map(flag_data) do
     {:ok, build_result(key, flag_data, response_body)}
@@ -845,6 +850,15 @@ defmodule PostHog.FeatureFlags do
             {:ok, result, response_body}
 
           {:ok, nil, response_body} ->
+            maybe_log_failed_flag_usage(
+              send_event,
+              name,
+              distinct_id,
+              flag_name,
+              response_body,
+              evaluation_groups(body)
+            )
+
             {:ok, nil, response_body}
 
           {:error, reason} ->
@@ -895,10 +909,36 @@ defmodule PostHog.FeatureFlags do
   defp single_remote_result(flag_name, flag_data, response_body) do
     case build_result_safely(flag_name, flag_data, response_body) do
       {:ok, result} -> {:ok, result, response_body}
+      :failed -> {:ok, nil, response_body}
       :error -> {:ok, nil, response_body}
     end
   end
 
+  defp maybe_log_failed_flag_usage(
+         true,
+         name,
+         distinct_id,
+         key,
+         %{"flags" => flags} = response_body,
+         groups
+       )
+       when is_map(flags) do
+    case Map.get(flags, key) do
+      %{"failed" => true} = flag_data ->
+        metadata = Map.get(flag_data, "metadata")
+        flag_data = if is_map(metadata), do: flag_data, else: Map.delete(flag_data, "metadata")
+        result = build_result(key, flag_data, response_body)
+        diagnostic = %{result | enabled: false, variant: nil, payload: nil}
+        log_feature_flag_usage(name, distinct_id, diagnostic, ["evaluation_failed"], groups)
+
+      _ ->
+        :ok
+    end
+  end
+
+  defp maybe_log_failed_flag_usage(_send_event, _name, _distinct_id, _key, _body, _groups),
+    do: :ok
+
   defp maybe_log_feature_flag_usage(send_event, name, distinct_id, flag_result, groups) do
     if send_event do
       log_feature_flag_usage(name, distinct_id, flag_result, [], groups)
@@ -1050,10 +1090,13 @@ defmodule PostHog.FeatureFlags do
         groups
       )
       when is_list(extra_errors) and is_map(groups) do
-    flag_missing? =
-      Enum.any?(extra_errors, &(&1 in ["flag_missing", "local_evaluation_inconclusive"]))
+    value_unavailable? =
+      Enum.any?(
+        extra_errors,
+        &(&1 in ["flag_missing", "local_evaluation_inconclusive", "evaluation_failed"])
+      )
 
-    value = if flag_missing?, do: nil, else: __MODULE__.Result.value(result)
+    value = if value_unavailable?, do: nil, else: __MODULE__.Result.value(result)
     errors = build_error_codes(result, extra_errors)
 
     properties =
@@ -1078,7 +1121,7 @@ defmodule PostHog.FeatureFlags do
       capture_called_event(name, distinct_id, result, properties)
     end
 
-    if flag_missing? do
+    if value_unavailable? do
       :ok
     else
       PostHog.set_context(name, %{"$feature/#{result.key}" => value})

test/posthog/feature_flags/local_evaluation_integration_test.exs

@@ -790,6 +790,76 @@ defmodule PostHog.FeatureFlags.LocalEvaluationIntegrationTest do
     refute Map.has_key?(PostHog.get_context(name), "$feature/checkout")
   end
 
+  test "failed fallback preserves successful local values and payloads" do
+    variant =
+      flag("variant", [], %{
+        "filters" => %{
+          "groups" => [%{"properties" => [], "rollout_percentage" => 100, "variant" => "blue"}],
+          "multivariate" => %{"variants" => [%{"key" => "blue", "rollout_percentage" => 100}]},
+          "payloads" => %{"blue" => ~s({"copy":"local"})}
+        }
+      })
+
+    disabled = flag("disabled", [], %{"active" => false})
+    checkout = flag("checkout", [], %{"ensure_experience_continuity" => true})
+
+    expect(PostHog.API.Mock, :request, fn :stub_client, :get, "/flags/definitions", _opts ->
+      {:ok,
+       %{
+         status: 200,
+         body: envelope([flag("enabled"), disabled, variant, checkout]),
+         headers: %{}
+       }}
+    end)
+
+    name = __MODULE__.FailedFallback
+    start_instance(name)
+
+    for error_bit <- [nil, false, true],
+        metadata <- [%{"payload" => ~s({"copy":"remote"})}, "bad"] do
+      remote_flags =
+        Map.new(["enabled", "disabled", "variant", "checkout"], fn key ->
+          {key,
+           %{
+             "enabled" => false,
+             "variant" => "placeholder",
+             "failed" => true,
+             "metadata" => metadata
+           }}
+        end)
+
+      body = %{"flags" => remote_flags}
+
+      body =
+        if is_nil(error_bit),
+          do: body,
+          else: Map.put(body, "errorsWhileComputingFlags", error_bit)
+
+      expect(PostHog.API.Mock, :request, 2, fn :stub_client, :post, "/flags", _opts ->
+        {:ok, %{status: 200, body: body}}
+      end)
+
+      assert {:ok, snapshot} = FeatureFlags.evaluate_flags(name, "user")
+      assert Result.value(snapshot.flags["enabled"]) == true
+      assert Result.value(snapshot.flags["disabled"]) == false
+      assert Result.value(snapshot.flags["variant"]) == "blue"
+      assert snapshot.flags["variant"].payload == %{"copy" => "local"}
+      refute Map.has_key?(snapshot.flags, "checkout")
+      refute Map.has_key?(Evaluations.event_properties(snapshot), "$feature/checkout")
+      refute snapshot.flags["enabled"].errors_while_computing
+
+      distinct_id = "user-#{inspect(error_bit)}-#{inspect(metadata)}"
+      assert {:ok, nil} = FeatureFlags.get_feature_flag_result(name, "checkout", distinct_id)
+      event = Enum.find(PostHog.Test.all_captured(name), &(&1.distinct_id == distinct_id))
+      assert event.properties[:"$feature_flag_response"] == nil
+      assert event.properties[:"$feature_flag_error"] =~ "evaluation_failed"
+      refute event.properties[:"$feature_flag_error"] =~ "flag_missing"
+
+      assert event.properties[:"$feature_flag_error"] =~ "errors_while_computing_flags" ==
+               (error_bit == true)
+    end
+  end
+
   test "a flag the remote fallback returns, even as failed, is not unresolved" do
     checkout = flag("checkout", [], %{"ensure_experience_continuity" => true})
 
@@ -804,8 +874,7 @@ defmodule PostHog.FeatureFlags.LocalEvaluationIntegrationTest do
 
     for {response, values, expected_unresolved} <- [
           {%{"flags" => %{"checkout" => %{"enabled" => true}}}, %{"checkout" => true}, %{}},
-          {%{"flags" => %{"checkout" => failed}, "errorsWhileComputingFlags" => true},
-           %{"checkout" => false}, %{}},
+          {%{"flags" => %{"checkout" => failed}, "errorsWhileComputingFlags" => true}, %{}, %{}},
           {%{"flags" => %{}}, %{}, unresolved},
           {:unavailable, %{}, unresolved}
         ] do

test/posthog/feature_flags_test.exs

@@ -14,6 +14,74 @@ defmodule PostHog.FeatureFlagsTest do
   setup :setup_supervisor
   setup :verify_on_exit!
 
+  describe "failed remote records" do
+    for error_bit <- [nil, false, true] do
+      @error_bit error_bit
+      test "failed values and payloads are absent with global errors #{inspect(error_bit)}" do
+        body = %{
+          "flags" => %{
+            "failed" => %{
+              "enabled" => false,
+              "variant" => "placeholder",
+              "failed" => true,
+              "metadata" => %{"id" => 42, "payload" => ~s({"bad":true})},
+              "reason" => %{"code" => "dependency_error"}
+            },
+            "disabled" => %{"enabled" => false, "failed" => false},
+            "healthy" => %{"enabled" => true}
+          },
+          "requestId" => "failed-request"
+        }
+
+        body =
+          if is_nil(@error_bit),
+            do: body,
+            else: Map.put(body, "errorsWhileComputingFlags", @error_bit)
+
+        expect(API.Mock, :request, 5, fn _client, :post, "/flags", _opts ->
+          {:ok, %{status: 200, body: body}}
+        end)
+
+        assert {:ok, nil} = FeatureFlags.get_feature_flag_result("failed", "foo")
+        assert {:ok, nil} = FeatureFlags.get_feature_flag_result("failed", "foo")
+        assert [event] = all_captured()
+        assert event.properties[:"$feature_flag_response"] == nil
+        assert event.properties[:"$feature_flag_id"] == 42
+        assert event.properties[:"$feature_flag_request_id"] == "failed-request"
+        assert event.properties[:"$feature_flag_reason"] == %{"code" => "dependency_error"}
+
+        assert event.properties[:"$feature_flag_error"] ==
+                 if(@error_bit == true,
+                   do: "errors_while_computing_flags,evaluation_failed",
+                   else: "evaluation_failed"
+                 )
+
+        refute Map.has_key?(event.properties, :"$feature_flag_payload")
+        refute Map.has_key?(PostHog.get_context(), "$feature/failed")
+
+        assert {:ok, nil} =
+                 FeatureFlags.get_feature_flag_result("failed", "suppressed", send_event: false)
+
+        assert [^event] = all_captured()
+
+        assert {:ok, %Result{enabled: false}} =
+                 FeatureFlags.get_feature_flag_result("disabled", "foo")
+
+        assert length(all_captured()) == 2
+        healthy = Enum.find(all_captured(), &(&1.properties[:"$feature_flag"] == "disabled"))
+        refute (healthy.properties[:"$feature_flag_error"] || "") =~ "evaluation_failed"
+
+        assert {:ok, snapshot} = FeatureFlags.evaluate_flags("foo")
+        assert Map.keys(snapshot.flags) |> Enum.sort() == ["disabled", "healthy"]
+
+        refute Map.has_key?(
+                 FeatureFlags.Evaluations.event_properties(snapshot),
+                 "$feature/failed"
+               )
+      end
+    end
+  end
+
   describe "flags/2" do
     test "returns body on success" do
       expect(API.Mock, :request, fn _client, method, url, opts ->
@@ -698,6 +766,29 @@ defmodule PostHog.FeatureFlagsTest do
               }} = FeatureFlags.get_feature_flag_result("myflag", "foo")
     end
 
+    test "non-map flags envelopes preserve no-result outcomes without tracking" do
+      for flags <- ["bad", nil, false, 7, [], ["bad"]] do
+        body = %{"flags" => flags}
+
+        expect(API.Mock, :request, 3, fn _client, :post, "/flags", _opts ->
+          {:ok, %{status: 200, body: body}}
+        end)
+
+        assert {:ok, nil} =
+                 FeatureFlags.get_feature_flag_result("myflag", "foo", send_event: false)
+
+        assert {:ok, nil} = FeatureFlags.get_feature_flag_result("myflag", "foo")
+
+        assert {:error,
+                %PostHog.UnexpectedResponseError{
+                  response: ^body,
+                  message: "Feature flag myflag was not found in the response"
+                }} = FeatureFlags.check("myflag", "foo")
+
+        assert all_captured() == []
+      end
+    end
+
     test "treats malformed nested flag metadata as a missing result" do
       expect(API.Mock, :request, fn _client, _method, _url, _opts ->
         {:ok,
@@ -714,6 +805,57 @@ defmodule PostHog.FeatureFlagsTest do
       assert {:ok, nil} = FeatureFlags.get_feature_flag_result("myflag", "foo")
     end
 
+    for error_bit <- [nil, false, true] do
+      @malformed_error_bit error_bit
+      test "failed malformed nested metadata returns no result and tracks failure with global errors #{inspect(error_bit)}" do
+        body = %{
+          "flags" => %{
+            "myflag" => %{
+              "enabled" => false,
+              "variant" => "placeholder",
+              "failed" => true,
+              "metadata" => "bad",
+              "reason" => %{"code" => "dependency_error"}
+            }
+          },
+          "requestId" => "malformed-failure"
+        }
+
+        body =
+          if is_nil(@malformed_error_bit),
+            do: body,
+            else: Map.put(body, "errorsWhileComputingFlags", @malformed_error_bit)
+
+        expect(API.Mock, :request, 4, fn _client, :post, "/flags", _opts ->
+          {:ok, %{status: 200, body: body}}
+        end)
+
+        assert {:ok, nil} =
+                 FeatureFlags.get_feature_flag_result("myflag", "foo", send_event: false)
+
+        assert all_captured() == []
+        assert {:ok, nil} = FeatureFlags.get_feature_flag_result("myflag", "foo")
+        assert {:ok, nil} = FeatureFlags.get_feature_flag_result("myflag", "foo")
+        assert {:error, %PostHog.UnexpectedResponseError{}} = FeatureFlags.check("myflag", "foo")
+
+        assert [event] = all_captured()
+        assert event.properties[:"$feature_flag_response"] == nil
+        assert event.properties[:"$feature_flag_reason"] == %{"code" => "dependency_error"}
+        assert event.properties[:"$feature_flag_request_id"] == "malformed-failure"
+
+        assert event.properties[:"$feature_flag_error"] ==
+                 if(@malformed_error_bit == true,
+                   do: "errors_while_computing_flags,evaluation_failed",
+                   else: "evaluation_failed"
+                 )
+
+        refute Map.has_key?(event.properties, :"$feature_flag_payload")
+        refute Map.has_key?(event.properties, :"$feature_flag_id")
+        refute Map.has_key?(event.properties, :"$feature_flag_version")
+        refute Map.has_key?(PostHog.get_context(), "$feature/myflag")
+      end
+    end
+
     test "returns disabled result when flag not enabled" do
       expect(API.Mock, :request, fn _client, _method, _url, _opts ->
         {:ok,