diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 713d29d..12df622 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,7 +19,7 @@ concurrency: jobs: test: name: Build and test - runs-on: ubuntu-latest + runs-on: ubuntu-24.04 # This is a complicated set of strategies, but Elixir has a complicated set of supported versions. # See https://hexdocs.pm/elixir/compatibility-and-deprecations.html diff --git a/.sampo/changesets/evaluations-agent-normal-exit.md b/.sampo/changesets/evaluations-agent-normal-exit.md new file mode 100644 index 0000000..b5e2051 --- /dev/null +++ b/.sampo/changesets/evaluations-agent-normal-exit.md @@ -0,0 +1,5 @@ +--- +hex/posthog: patch +--- + +Stop the feature flag evaluations access-tracking Agent when the process that created the snapshot exits normally, instead of leaving it running for the life of the node. diff --git a/lib/posthog/feature_flags/evaluations.ex b/lib/posthog/feature_flags/evaluations.ex index 5510e9f..a546377 100644 --- a/lib/posthog/feature_flags/evaluations.ex +++ b/lib/posthog/feature_flags/evaluations.ex @@ -9,7 +9,9 @@ defmodule PostHog.FeatureFlags.Evaluations do Each snapshot owns a small `Agent` linked to the calling process that tracks which flags were accessed via `enabled?/2` and `get_flag/2`. The Agent exits - with the calling process — no manual cleanup is required. + with the calling process, whatever the exit reason — no manual cleanup is + required. A snapshot is therefore only usable while the process that created + it is alive; don't hand it to a process that outlives its creator. ## Querying @@ -274,7 +276,15 @@ defmodule PostHog.FeatureFlags.Evaluations do end defp start_accessed_agent do - {:ok, pid} = Agent.start_link(fn -> MapSet.new() end) + # Trap exits so the Agent also stops when the caller exits with :normal. + # Without it, a :normal exit signal is ignored and the Agent is orphaned. + {:ok, pid} = + Agent.start_link(fn -> + Logger.metadata(posthog_skip_capture: true) + Process.flag(:trap_exit, true) + MapSet.new() + end) + pid end diff --git a/test/posthog/feature_flags/evaluations_test.exs b/test/posthog/feature_flags/evaluations_test.exs index 05473be..20dba36 100644 --- a/test/posthog/feature_flags/evaluations_test.exs +++ b/test/posthog/feature_flags/evaluations_test.exs @@ -52,6 +52,29 @@ defmodule PostHog.FeatureFlags.EvaluationsTest do } end + describe "access tracker lifetime" do + for reason <- [:normal, :shutdown, {:shutdown, :closed}] do + @reason reason + test "stops when the creating process exits with #{inspect(reason)}" do + test_pid = self() + + {creator, creator_ref} = + spawn_monitor(fn -> + send(test_pid, {:snapshot, Evaluations.empty(PostHog)}) + receive do: (:exit -> exit(@reason)) + end) + + assert_receive {:snapshot, %Evaluations{accessed_pid: agent}} + agent_ref = Process.monitor(agent) + + send(creator, :exit) + + assert_receive {:DOWN, ^creator_ref, :process, ^creator, _} + assert_receive {:DOWN, ^agent_ref, :process, ^agent, _} + end + end + end + describe "evaluate_flags/2" do test "returns a snapshot containing every evaluated flag" do expect(API.Mock, :request, fn _client, :post, "/flags", _opts -> diff --git a/test/posthog/handler_test.exs b/test/posthog/handler_test.exs index cb81311..38842d6 100644 --- a/test/posthog/handler_test.exs +++ b/test/posthog/handler_test.exs @@ -307,6 +307,50 @@ defmodule PostHog.HandlerTest do assert [] = all_captured(supervisor_name) end + test "snapshot tracker exits do not duplicate their creator's exception", %{ + handler_id: handler_id, + handler_ref: ref, + config: %{supervisor_name: supervisor_name} + } do + test_pid = self() + + {creator, creator_ref} = + spawn_monitor(fn -> + {:ok, snapshot} = + PostHog.FeatureFlags.evaluate_flags(supervisor_name, %{ + distinct_id: "snapshot-user", + flag_keys: [] + }) + + filtered = PostHog.FeatureFlags.Evaluations.only(snapshot, []) + send(test_pid, {:trackers, [snapshot.accessed_pid, filtered.accessed_pid]}) + receive do: (:raise -> raise "snapshot creator failed") + end) + + assert_receive {:trackers, trackers} + + for pid <- [creator | trackers] do + LoggerHandlerKit.Arrange.allow(self(), pid, handler_id) + PostHog.Test.allow(supervisor_name, self(), pid) + end + + tracker_refs = for pid <- trackers, do: {pid, Process.monitor(pid)} + send(creator, :raise) + + assert_receive {:DOWN, ^creator_ref, :process, ^creator, _} + + for {pid, tracker_ref} <- tracker_refs do + assert_receive {:DOWN, ^tracker_ref, :process, ^pid, _} + end + + for _ <- 1..3, do: LoggerHandlerKit.Assert.assert_logged(ref) + + assert [%{event: "$exception", properties: properties}] = all_captured(supervisor_name) + + assert [%{type: "RuntimeError", value: "snapshot creator failed"} | _] = + properties[:"$exception_list"] + end + @tag config: [capture_level: :warning] test "logs with crash reason always captured", %{ handler_ref: ref,