diff --git a/.sampo/changesets/cantankerous-duchess-loviatar.md b/.sampo/changesets/cantankerous-duchess-loviatar.md new file mode 100644 index 0000000..6b967d3 --- /dev/null +++ b/.sampo/changesets/cantankerous-duchess-loviatar.md @@ -0,0 +1,5 @@ +--- +hex/posthog: patch +--- + +Allow shared flag definition cache providers to support local evaluation without a secret key. diff --git a/lib/posthog/config_schema.ex b/lib/posthog/config_schema.ex index b602c63..a1c742b 100644 --- a/lib/posthog/config_schema.ex +++ b/lib/posthog/config_schema.ex @@ -44,14 +44,15 @@ defmodule PostHog.Config.Schema do default: nil, doc: """ A privileged project secret (`phs_`) or appropriately scoped personal API key (`phx_`) - used only to load definitions for local feature flag evaluation. Local evaluation is inert - and starts no poller when this value is absent or blank. + required only to fetch local feature flag definitions directly from PostHog. A configured + `flag_definition_cache_provider` can supply definitions without this key. """ ], enable_local_evaluation: [ type: :boolean, default: true, - doc: "Enable local feature flag evaluation when a non-empty `secret_key` is configured." + doc: + "Enable local feature flag evaluation when a non-empty `secret_key` or a `flag_definition_cache_provider` is configured." ], feature_flags_poll_interval_ms: [ type: :pos_integer, diff --git a/lib/posthog/feature_flags.ex b/lib/posthog/feature_flags.ex index 5074de2..90375ff 100644 --- a/lib/posthog/feature_flags.ex +++ b/lib/posthog/feature_flags.ex @@ -108,7 +108,8 @@ defmodule PostHog.FeatureFlags do Evaluates feature flags for a `distinct_id` and returns a snapshot. Returns `{:ok, %PostHog.FeatureFlags.Evaluations{}}` on success. Definitions - are evaluated locally first when privileged local evaluation is configured; + are evaluated locally first when local evaluation is configured with a + privileged key or a shared definition cache provider; unresolved flags are filled by at most one `/flags` call. If that fallback fails after some flags resolved locally, the successful local subset is returned as a partial snapshot. If nothing resolved, the remote error is diff --git a/lib/posthog/feature_flags/definition_loader.ex b/lib/posthog/feature_flags/definition_loader.ex index 08ea830..d400c58 100644 --- a/lib/posthog/feature_flags/definition_loader.ex +++ b/lib/posthog/feature_flags/definition_loader.ex @@ -363,6 +363,14 @@ defmodule PostHog.FeatureFlags.DefinitionLoader do defp refresh_state(%{config: %{flag_definition_cache_provider: nil}} = state), do: fetch_direct(state, false) + # Cache-only readers must not acquire fetch leadership from the provider. + defp refresh_state( + %{ + config: %{secret_key: nil, flag_definition_cache_provider: {module, provider_state}} + } = state + ), + do: read_provider_without_fetch_ownership(state, module, provider_state) + defp refresh_state( %{ config: %{flag_definition_cache_provider: {module, provider_state}} @@ -410,6 +418,14 @@ defmodule PostHog.FeatureFlags.DefinitionLoader do end end + defp fetch_direct(%{config: %{secret_key: nil}} = state, _store?) do + warning( + "definition refresh requires a secret_key to fetch from PostHog; keeping stale definitions" + ) + + state + end + defp fetch_direct(state, store?) do config = state.config diff --git a/lib/posthog/feature_flags/flag_definition_cache_provider.ex b/lib/posthog/feature_flags/flag_definition_cache_provider.ex index e0ee09e..4033f61 100644 --- a/lib/posthog/feature_flags/flag_definition_cache_provider.ex +++ b/lib/posthog/feature_flags/flag_definition_cache_provider.ex @@ -10,6 +10,15 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProvider do the definitions. Only version 2 selects explicit property matching; older cached envelopes omitting the version use released service legacy matching. + With a project `:api_key` and local evaluation enabled, a provider can supply + definitions without a `:secret_key`. These cache-only readers read the provider + on each refresh without invoking the fetch decision or publishing definitions. + Instances with a secret key consult the fetch decision: true fetches and + publishes fresh definitions, while false reads the cache. An empty or failed + cache preserves loaded definitions; a keyed instance without definitions can + recover by fetching directly. Provider-backed instances use the same startup, + polling, and refresh lifecycle. + A minimal provider can coordinate fetching and keep the complete envelope in an application-owned cache: @@ -23,14 +32,14 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProvider do end config :posthog, - secret_key: System.fetch_env!("POSTHOG_SECRET_KEY"), + api_key: System.fetch_env!("POSTHOG_PROJECT_TOKEN"), flag_definition_cache_provider: {MyApp.PostHogDefinitionCache, provider_state} """ @type provider_state :: any() @type definitions :: map() - @doc "Returns whether this SDK instance should fetch fresh definitions from PostHog." + @doc "Returns whether a secret-key-backed SDK instance should fetch fresh definitions from PostHog." @callback should_fetch_flag_definitions(provider_state()) :: boolean() @doc "Reads definitions from the shared cache, or returns nil when it is empty." diff --git a/lib/posthog/supervisor.ex b/lib/posthog/supervisor.ex index 78e43f1..47bc267 100644 --- a/lib/posthog/supervisor.ex +++ b/lib/posthog/supervisor.ex @@ -72,11 +72,12 @@ defmodule PostHog.Supervisor do %{ enabled: true, enable_local_evaluation: true, - secret_key: %PostHog.Config.Secret{value: secret_key} + secret_key: secret_key, + flag_definition_cache_provider: provider } = config, callers ) - when is_binary(secret_key) and secret_key != "" do + when not is_nil(secret_key) or not is_nil(provider) do [ {PostHog.FeatureFlags.DefinitionLoader.NegativeKnowledge, supervisor_name: config.supervisor_name}, diff --git a/test/posthog/feature_flags/flag_definition_cache_provider_test.exs b/test/posthog/feature_flags/flag_definition_cache_provider_test.exs index 6455144..4ca7ad8 100644 --- a/test/posthog/feature_flags/flag_definition_cache_provider_test.exs +++ b/test/posthog/feature_flags/flag_definition_cache_provider_test.exs @@ -26,7 +26,10 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do end defp run(agent, key) do - case Agent.get(agent, &Map.fetch!(&1, key)) do + {owner, result} = Agent.get(agent, &{&1.owner, Map.fetch!(&1, key)}) + send(owner, {:provider_call, key}) + + case result do {:sleep, milliseconds, result} -> Process.sleep(milliseconds) result @@ -34,12 +37,47 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do {:raise, message} -> raise message + {:throw, reason} -> + throw(reason) + value -> value end end end + defmodule LeaseProvider do + @behaviour PostHog.FeatureFlags.FlagDefinitionCacheProvider + + @impl true + def should_fetch_flag_definitions({agent, identity}) do + Agent.get_and_update(agent, fn state -> + send(state.owner, {:lease_decision, identity}) + leader = state.leader || identity + {leader == identity, %{state | leader: leader}} + end) + end + + @impl true + def get_flag_definitions({agent, identity}) do + Agent.get(agent, fn state -> + send(state.owner, {:lease_read, identity}) + state.cached + end) + end + + @impl true + def on_flag_definitions_received({agent, identity}, definitions) do + Agent.update(agent, fn state -> + send(state.owner, {:lease_store, identity}) + %{state | cached: definitions} + end) + end + + @impl true + def shutdown(_state), do: :ok + end + setup :set_mox_from_context setup :verify_on_exit! @@ -71,6 +109,15 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do start_supervised!({PostHog.Supervisor, cfg}) end + defp stub_unexpected_api_requests(owner) do + stub_with(PostHog.API.Mock, PostHog.API.Stub) + + stub(PostHog.API.Mock, :request, fn _client, _method, _path, _opts -> + send(owner, :unexpected_api_fetch) + {:error, :unexpected_api_fetch} + end) + end + test "matching version round trips through providers and version-only hydration replaces results" do owner = self() # No API request stub: only the owned definitions GET below is permitted. @@ -116,7 +163,7 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do Agent.update(provider, &%{&1 | decision: false, read: stored}) name = __MODULE__.VersionReader - start_instance(name, provider) + start_instance(name, provider, secret_key: nil) context = %{ distinct_id: "user", @@ -196,6 +243,9 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do start_instance(__MODULE__.Cached, provider) definitions = DefinitionLoader.definitions(__MODULE__.Cached) + assert_receive {:provider_call, :decision} + assert_receive {:provider_call, :read} + refute_received {:provider_call, :store} assert definitions.flags_by_key["cached"] assert definitions.group_type_mapping == %{"0" => "organization"} assert definitions.minimal_flag_called_events @@ -204,6 +254,265 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do refute_receive :unexpected_api_fetch end + test "provider-only instances evaluate cached definitions on first use and manual refresh" do + owner = self() + stub_unexpected_api_requests(owner) + + cached = + envelope("cached") + |> Map.put("property_matching_version", 2) + |> Map.put("flags", [ + %{ + "key" => "cached", + "active" => true, + "filters" => %{"groups" => [%{"properties" => [], "rollout_percentage" => 100}]} + } + ]) + + for {name, secret_key, decision} <- [ + {__MODULE__.ProviderOnly, nil, false}, + {__MODULE__.BlankSecret, " \t ", true}, + {__MODULE__.RaisingDecision, nil, {:raise, "unavailable"}}, + {__MODULE__.ThrowingDecision, nil, {:throw, :unavailable}}, + {__MODULE__.HangingDecision, nil, {:sleep, 10_000, true}} + ] do + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, decision: decision, read: cached, store: :ok, shutdown: :ok} + end) + + start_instance(name, provider, secret_key: secret_key) + assert {:ok, result} = PostHog.FeatureFlags.evaluate_flags(name, "user") + assert result.flags["cached"].enabled + assert result.flags["cached"].locally_evaluated + refute_received {:provider_call, :decision} + assert_receive {:provider_call, :read} + definitions = DefinitionLoader.definitions(name) + assert definitions.flags == cached["flags"] + assert definitions.group_type_mapping == cached["group_type_mapping"] + assert definitions.cohorts == cached["cohorts"] + assert definitions.property_matching_version == 2 + assert definitions.minimal_flag_called_events + + Agent.update(provider, &%{&1 | read: envelope("refreshed")}) + assert :ok = DefinitionLoader.refresh(name) + assert {:ok, refreshed} = PostHog.FeatureFlags.evaluate_flags(name, "user") + assert Map.keys(refreshed.flags) == ["refreshed"] + assert refreshed.flags["refreshed"].locally_evaluated + assert DefinitionLoader.definitions(name).property_matching_version == 1 + refute_received {:provider_call, :decision} + assert_receive {:provider_call, :read} + refute_received {:provider_call, :store} + assert :ok = stop_supervised(name) + assert_receive {:provider_call, :shutdown} + assert_receive :shutdown + refute_received :unexpected_api_fetch + end + end + + test "provider-only polling hydrates updated shared definitions" do + owner = self() + stub_unexpected_api_requests(owner) + + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, decision: false, read: envelope("initial"), store: :ok, shutdown: :ok} + end) + + name = __MODULE__.ProviderOnlyPolling + start_instance(name, provider, secret_key: nil, feature_flags_poll_interval_ms: 50) + assert {:ok, initial} = PostHog.FeatureFlags.evaluate_flags(name, "user") + assert initial.flags["initial"].locally_evaluated + refute_received {:provider_call, :decision} + assert_receive {:provider_call, :read} + + Agent.update(provider, &%{&1 | read: envelope("polled")}) + assert_receive {:provider_call, :read}, 500 + refute_received {:provider_call, :decision} + # Wait for the in-flight poll to finish without initiating a manual refresh. + GenServer.call(PostHog.Registry.via(name, DefinitionLoader), :definitions) + + assert {:ok, polled} = PostHog.FeatureFlags.evaluate_flags(name, "user") + assert Map.keys(polled.flags) == ["polled"] + assert polled.flags["polled"].locally_evaluated + assert :ok = stop_supervised(name) + assert_receive :shutdown + refute_received {:provider_call, :decision} + refute_received {:provider_call, :store} + refute_received :unexpected_api_fetch + end + + test "provider-only cache misses and read failures warn without fetching or publishing" do + owner = self() + stub_unexpected_api_requests(owner) + + for {suffix, decision, read} <- [ + {"Empty", false, nil}, + {"FailedRead", false, {:raise, "unavailable"}}, + {"TimedOutRead", false, {:sleep, 100, nil}}, + {"MalformedRead", false, %{"flags" => []}}, + {"FetchOwner", true, nil}, + {"FailedDecision", {:raise, "unavailable"}, nil} + ] do + name = Module.concat(__MODULE__, suffix) + + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, decision: decision, read: read, store: :ok, shutdown: :ok} + end) + + log = + capture_log(fn -> + start_instance(name, provider, secret_key: nil) + refute DefinitionLoader.ready?(name) + end) + + assert log =~ "requires a secret_key to fetch from PostHog" + refute_received {:provider_call, :decision} + assert_receive {:provider_call, :read} + + assert :ok = stop_supervised(name) + assert_receive :shutdown + refute_received :unexpected_api_fetch + refute_received {:stored, _definitions} + end + end + + test "provider-only refresh preserves the last snapshot when cache reads fail" do + owner = self() + stub_unexpected_api_requests(owner) + + cached = Map.put(envelope("stale"), "property_matching_version", 2) + + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, decision: false, read: cached, store: :ok, shutdown: :ok} + end) + + name = __MODULE__.ProviderOnlyStale + start_instance(name, provider, secret_key: nil) + initial = DefinitionLoader.definitions(name) + assert initial.property_matching_version == 2 + + for {decision, read} <- [ + {false, nil}, + {false, {:raise, "unavailable"}}, + {false, {:sleep, 100, nil}}, + {false, %{"flags" => []}}, + {true, nil}, + {{:raise, "unavailable"}, nil} + ] do + Agent.update(provider, &%{&1 | decision: decision, read: read}) + capture_log(fn -> assert :ok = DefinitionLoader.refresh(name) end) + assert DefinitionLoader.definitions(name) == initial + assert {:ok, result} = PostHog.FeatureFlags.evaluate_flags(name, "user") + assert result.flags["stale"].locally_evaluated + refute_received {:provider_call, :decision} + refute_received :unexpected_api_fetch + refute_received {:stored, _definitions} + end + + assert :ok = stop_supervised(name) + assert_receive :shutdown + end + + test "cache-only readers cannot claim shared leadership before a keyed publisher refreshes" do + owner = self() + stub_unexpected_api_requests(owner) + + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, leader: nil, cached: envelope("initial")} + end) + + reader = __MODULE__.LeaseReader + publisher = __MODULE__.LeasePublisher + + start_instance(reader, provider, + secret_key: nil, + flag_definition_cache_provider: {LeaseProvider, {provider, :reader}} + ) + + initial = DefinitionLoader.definitions(reader) + assert Agent.get(provider, & &1.leader) == nil + assert initial.flags_by_key["initial"] + assert_receive {:lease_read, :reader} + refute_received {:lease_decision, :reader} + + expect(PostHog.API.Mock, :request, fn :stub_client, :get, "/flags/definitions", _opts -> + {:ok, %{status: 200, body: envelope("published"), headers: %{}}} + end) + + start_instance(publisher, provider, + flag_definition_cache_provider: {LeaseProvider, {provider, :publisher}} + ) + + assert DefinitionLoader.definitions(publisher).flags_by_key["published"] + assert_receive {:lease_decision, :publisher} + assert_receive {:lease_store, :publisher} + assert Agent.get(provider, & &1.leader) == :publisher + + # Expire the lease, then refresh the reader before the publisher can reacquire it. + Agent.update(provider, &%{&1 | leader: nil}) + assert :ok = DefinitionLoader.refresh(reader) + assert DefinitionLoader.definitions(reader).flags_by_key["published"] + assert_receive {:lease_read, :reader} + + loader = GenServer.whereis(PostHog.Registry.via(reader, DefinitionLoader)) + send(loader, {:refresh, :sys.get_state(loader).timer_generation}) + GenServer.call(loader, :definitions) + assert_receive {:lease_read, :reader} + assert Agent.get(provider, & &1.leader) == nil + + expect(PostHog.API.Mock, :request, fn :stub_client, :get, "/flags/definitions", _opts -> + {:ok, %{status: 200, body: envelope("updated"), headers: %{}}} + end) + + assert :ok = DefinitionLoader.refresh(publisher) + assert DefinitionLoader.definitions(publisher).flags_by_key["updated"] + assert_receive {:lease_decision, :publisher} + assert_receive {:lease_store, :publisher} + assert :ok = DefinitionLoader.refresh(reader) + assert DefinitionLoader.definitions(reader).flags_by_key["updated"] + assert_receive {:lease_read, :reader} + refute_received {:lease_decision, :reader} + refute_received {:lease_store, :reader} + refute_received :unexpected_api_fetch + assert :ok = stop_supervised(reader) + assert :ok = stop_supervised(publisher) + end + + test "provider-only instances respect local evaluation and project-token startup controls" do + owner = self() + stub_with(PostHog.API.Mock, PostHog.API.Stub) + + {:ok, provider} = + Agent.start_link(fn -> + %{owner: owner, decision: false, read: envelope("cached"), store: :ok, shutdown: :ok} + end) + + for {name, overrides} <- [ + {__MODULE__.ProviderLocalDisabled, [enable_local_evaluation: false]}, + {__MODULE__.ProviderDisabled, [api_key: ""]} + ] do + capture_log(fn -> start_instance(name, provider, [secret_key: nil] ++ overrides) end) + assert GenServer.whereis(PostHog.Registry.via(name, DefinitionLoader)) == nil + + assert GenServer.whereis(PostHog.Registry.via(name, DefinitionLoader.NegativeKnowledge)) == + nil + + assert {:ok, result} = + PostHog.FeatureFlags.evaluate_flags(name, %{ + distinct_id: "user", + only_evaluate_locally: true + }) + + assert result.flags == %{} + assert :ok = stop_supervised(name) + refute_received {:provider_call, _callback} + end + end + test "positive decision fetches, publishes, and stores the complete envelope" do stub_with(PostHog.API.Mock, PostHog.API.Stub) @@ -221,6 +530,9 @@ defmodule PostHog.FeatureFlags.FlagDefinitionCacheProviderTest do start_instance(__MODULE__.Fresh, provider) assert DefinitionLoader.definitions(__MODULE__.Fresh).flags_by_key["fresh"] + assert_receive {:provider_call, :decision} + refute_received {:provider_call, :read} + assert_receive {:provider_call, :store} assert_receive {:stored, stored} assert stored["flags"] != [] assert stored["group_type_mapping"] == %{"0" => "organization"}