wip(tests): restore manual pid allowance via NimbleOwnership

Author: solnic

lib/sentry/application.ex

@@ -7,6 +7,8 @@ defmodule Sentry.Application do
 
   alias Sentry.Config
 
+  @compile {:no_warn_undefined, [NimbleOwnership]}
+
   @impl true
   def start(_type, _opts) do
     config = Config.validate!()
@@ -32,7 +34,14 @@ defmodule Sentry.Application do
 
     maybe_test_registry =
       if Config.test_mode?() do
-        [Sentry.Test.Registry]
+        if Code.ensure_loaded?(NimbleOwnership) do
+          [
+            Sentry.Test.Registry,
+            {NimbleOwnership, name: Sentry.Test.OwnershipServer}
+          ]
+        else
+          [Sentry.Test.Registry]
+        end
       else
         []
       end

lib/sentry/test.ex

@@ -9,11 +9,12 @@ defmodule Sentry.Test do
   Events are captured via the existing `before_send` and `before_send_log` callbacks
   and stored in an isolated ETS table per test, preserving the full struct data.
 
-  > #### Bypass Required {: .info}
+  > #### Bypass and NimbleOwnership Required {: .info}
   >
-  > This module requires `bypass` as a test dependency:
+  > This module requires `bypass` and `nimble_ownership` as test dependencies:
   >
   >     {:bypass, "~> 2.0", only: [:test]}
+  >     {:nimble_ownership, "~> 1.0", only: [:test]}
 
   ## Examples
 
@@ -52,9 +53,10 @@ defmodule Sentry.Test do
 
   @moduledoc since: "10.2.0"
 
-  @compile {:no_warn_undefined, [Bypass, Plug.Conn]}
+  @compile {:no_warn_undefined, [Bypass, Plug.Conn, NimbleOwnership]}
 
-  @registry_table :sentry_test_collectors
+  @ownership_server Sentry.Test.OwnershipServer
+  @collector_key :sentry_test_collector
 
   # Public API
 
@@ -108,6 +110,7 @@ defmodule Sentry.Test do
   @spec setup_sentry(keyword()) :: %{bypass: term(), telemetry_processor: atom()}
   def setup_sentry(extra_config \\ []) do
     ensure_bypass_loaded!()
+    ensure_nimble_ownership_loaded!()
 
     # Open a per-test Bypass and stub the envelope endpoint
     bypass = Bypass.open()
@@ -251,19 +254,95 @@ defmodule Sentry.Test do
   end
 
   @doc """
-  Allows `pid_to_allow` to collect events back to the root process via `owner_pid`.
+  Allows `pid_to_allow` to collect events back to `owner_pid`'s test scope.
+
+  Use this when an unrelated process — one that does not appear in the
+  current test's `$callers` chain — needs to have its captured events
+  routed into this test's collector. Typical examples include Broadway
+  workers, processes started by `phoenix_test_playwright`, or
+  long-lived `GenServer`s that outlive the calling test process.
+
+  `pid_to_allow` may be a pid or a zero-arity function returning a pid;
+  the function form is resolved on call and is convenient when the pid
+  is not known until later.
+
+  This function is idempotent for the same `owner_pid`. It raises
+  `ArgumentError` when `owner_pid` has not yet called `setup_sentry/1`
+  (or `start_collecting_sentry_reports/0`), and raises when a different
+  live test scope already owns `pid_to_allow`.
+
+  Cleanup is automatic: allow entries are removed when the test exits
+  via the same `on_exit` callback registered by `setup_sentry/1`.
+
+  ## Example
+
+      setup do
+        Sentry.Test.setup_sentry()
+      end
+
+      test "events from a Broadway worker are captured" do
+        {:ok, worker_pid} = MyApp.Worker.start_link()
+        :ok = Sentry.Test.allow_sentry_reports(self(), worker_pid)
+
+        send(worker_pid, :do_work_that_reports)
+
+        assert_receive {:done, _}
+        assert [%Sentry.Event{}] = Sentry.Test.pop_sentry_reports()
+      end
 
-  > #### Deprecated {: .warning}
-  >
-  > This function is deprecated and will be removed in v13.0.0.
-  > Child processes are automatically tracked via the `$callers` mechanism.
-  > There is no need to explicitly allow processes.
   """
-  @doc since: "10.2.0"
-  @doc deprecated: "Child processes are now automatically tracked via $callers"
+  @doc since: "13.0.2"
   @spec allow_sentry_reports(pid(), pid() | (-> pid())) :: :ok
-  def allow_sentry_reports(_owner_pid, _pid_to_allow) do
-    :ok
+  def allow_sentry_reports(owner_pid, pid_or_fun) when is_pid(owner_pid) do
+    ensure_nimble_ownership_loaded!()
+    allowed_pid = resolve_allowed_pid(pid_or_fun)
+
+    case NimbleOwnership.allow(@ownership_server, owner_pid, allowed_pid, @collector_key) do
+      :ok ->
+        # Also route per-test config overrides (DSN, before_send hooks,
+        # the internal collector callback, etc.) through the owner's
+        # scope so that Sentry callbacks invoked from `allowed_pid`
+        # resolve to the same configuration the test set up.
+        Sentry.Test.Config.allow(owner_pid, allowed_pid)
+        :ok
+
+      {:error, %{reason: {:already_allowed, ^owner_pid}}} ->
+        # Idempotent re-allow under the same owner.
+        Sentry.Test.Config.allow(owner_pid, allowed_pid)
+        :ok
+
+      {:error, %{reason: {:already_allowed, existing_owner}}} ->
+        raise ArgumentError,
+              "cannot allow #{inspect(allowed_pid)} for #{inspect(owner_pid)}: " <>
+                "already allowed by another live test scope " <>
+                "(owner: #{inspect(existing_owner)})"
+
+      {:error, %{reason: :not_allowed}} ->
+        raise ArgumentError,
+              "owner #{inspect(owner_pid)} is not collecting Sentry reports; " <>
+                "call Sentry.Test.setup_sentry/1 or " <>
+                "Sentry.Test.start_collecting_sentry_reports/0 first"
+
+      {:error, %{reason: :already_an_owner}} ->
+        raise ArgumentError,
+              "cannot allow #{inspect(allowed_pid)} for #{inspect(owner_pid)}: " <>
+                "#{inspect(allowed_pid)} is already collecting Sentry reports " <>
+                "itself (called setup_sentry/1 or start_collecting_sentry_reports/0)"
+    end
+  end
+
+  defp resolve_allowed_pid(pid) when is_pid(pid), do: pid
+
+  defp resolve_allowed_pid(fun) when is_function(fun, 0) do
+    case fun.() do
+      pid when is_pid(pid) ->
+        pid
+
+      other ->
+        raise ArgumentError,
+              "expected the function passed to allow_sentry_reports/2 to return a pid, " <>
+                "got: #{inspect(other)}"
+    end
   end
 
   @doc """
@@ -636,16 +715,39 @@ defmodule Sentry.Test do
     end
   end
 
+  defp ensure_nimble_ownership_loaded! do
+    unless Code.ensure_loaded?(NimbleOwnership) do
+      raise """
+      NimbleOwnership is required for Sentry.Test but is not available.
+
+      Add it to your test dependencies in mix.exs:
+
+          {:nimble_ownership, "~> 1.0", only: [:test]}
+      """
+    end
+  end
+
   # Sets up collection infrastructure (ETS table, before_send wrapping, config)
   # without opening a new Bypass. When no :dsn is provided in extra_config,
   # falls back to the default Bypass DSN from Registry.
   defp setup_collector(extra_config) do
+    ensure_nimble_ownership_loaded!()
+
     uid = System.unique_integer([:positive])
     collector_table = :"sentry_test_collector_#{uid}"
     :ets.new(collector_table, [:ordered_set, :public, :named_table])
 
-    # Register this test's collector
-    :ets.insert(@registry_table, {self(), collector_table})
+    # Register this test as the NimbleOwnership owner of the collector key,
+    # with the collector ETS table as its metadata. NimbleOwnership monitors
+    # the owner pid and auto-cleans the key + every transitive allowance
+    # when the test process exits.
+    {:ok, _} =
+      NimbleOwnership.get_and_update(
+        @ownership_server,
+        self(),
+        @collector_key,
+        fn _prev -> {:ok, collector_table} end
+      )
 
     # Store in process dict for pop_* lookups
     Process.put(:sentry_test_collector, collector_table)
@@ -676,30 +778,19 @@ defmodule Sentry.Test do
     Sentry.Test.Config.put_override(:_internal_before_send_log, collector)
     Sentry.Test.Config.put_override(:_internal_before_send_metric, collector)
 
+    # The TelemetryProcessor's scheduler is not in `$callers` of this test —
+    # allow it explicitly so log/metric events routed through the buffered
+    # pipeline can find this test's collector.
     scheduler_pid = get_scheduler_pid()
 
     if scheduler_pid do
-      :ets.insert_new(@registry_table, {scheduler_pid, collector_table})
+      :ok =
+        NimbleOwnership.allow(@ownership_server, self(), scheduler_pid, @collector_key)
     end
 
-    # Register cleanup
-    test_pid = self()
-
+    # Register cleanup for the collector ETS table only. NimbleOwnership
+    # cleans up the key and allowances automatically when this test exits.
     ExUnit.Callbacks.on_exit(fn ->
-      if :ets.whereis(@registry_table) != :undefined do
-        :ets.delete(@registry_table, test_pid)
-
-        if scheduler_pid do
-          case :ets.lookup(@registry_table, scheduler_pid) do
-            [{^scheduler_pid, ^collector_table}] ->
-              :ets.delete(@registry_table, scheduler_pid)
-
-            _ ->
-              :ok
-          end
-        end
-      end
-
       if :ets.whereis(collector_table) != :undefined do
         :ets.delete(collector_table)
       end
@@ -723,12 +814,15 @@ defmodule Sentry.Test do
   defp find_collector do
     pids = [self() | Process.get(:"$callers", [])]
 
-    Enum.find_value(pids, fn pid ->
-      case :ets.lookup(@registry_table, pid) do
-        [{^pid, table}] -> table
-        [] -> nil
-      end
-    end)
+    case NimbleOwnership.fetch_owner(@ownership_server, pids, @collector_key) do
+      {:ok, owner_pid} ->
+        @ownership_server
+        |> NimbleOwnership.get_owned(owner_pid, %{})
+        |> Map.get(@collector_key)
+
+      :error ->
+        nil
+    end
   end
 
   # Standalone collecting callback. Records the struct in this test's

lib/sentry/test/registry.ex

@@ -8,7 +8,6 @@ defmodule Sentry.Test.Registry do
   # Bypass and Plug.Conn may not be available at compile time (optional deps).
   @compile {:no_warn_undefined, [Bypass, Bypass.Instance, Bypass.Supervisor, Plug.Conn]}
 
-  @table :sentry_test_collectors
   @allows_table :sentry_test_scope_allows
 
   @spec start_link(keyword()) :: GenServer.on_start()
@@ -86,7 +85,6 @@ defmodule Sentry.Test.Registry do
 
   @impl true
   def init(nil) do
-    _table = :ets.new(@table, [:named_table, :public, :set])
     _allows_table = :ets.new(@allows_table, [:named_table, :public, :set])
     maybe_start_default_bypass()
     {:ok, :no_state}

mix.exs

@@ -111,6 +111,7 @@ defmodule Sentry.Mixfile do
       {:plug_cowboy, "~> 2.7", only: [:test]},
       {:bandit, "~> 1.0", only: [:test]},
       {:bypass, "~> 2.0", only: [:test]},
+      {:nimble_ownership, "~> 1.0", only: [:test]},
       {:dialyxir, "~> 1.0", only: [:test, :dev], runtime: false},
       {:ex_doc, "~> 0.29", only: :dev},
       {:excoveralls, "~> 0.17.1", only: [:test]},

mix.lock

@@ -39,6 +39,7 @@
   "mimerl": {:hex, :mimerl, "1.3.0", "d0cd9fc04b9061f82490f6581e0128379830e78535e017f7780f37fea7545726", [:rebar3], [], "hexpm", "a1e15a50d1887217de95f0b9b0793e32853f7c258a5cd227650889b38839fe9d"},
   "mint": {:hex, :mint, "1.7.1", "113fdb2b2f3b59e47c7955971854641c61f378549d73e829e1768de90fc1abf1", [:mix], [{:castore, "~> 0.1.0 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: true]}, {:hpax, "~> 0.1.1 or ~> 0.2.0 or ~> 1.0", [hex: :hpax, repo: "hexpm", optional: false]}], "hexpm", "fceba0a4d0f24301ddee3024ae116df1c3f4bb7a563a731f45fdfeb9d39a231b"},
   "nimble_options": {:hex, :nimble_options, "1.1.1", "e3a492d54d85fc3fd7c5baf411d9d2852922f66e69476317787a7b2bb000a61b", [:mix], [], "hexpm", "821b2470ca9442c4b6984882fe9bb0389371b8ddec4d45a9504f00a66f650b44"},
+  "nimble_ownership": {:hex, :nimble_ownership, "1.0.2", "fa8a6f2d8c592ad4d79b2ca617473c6aefd5869abfa02563a77682038bf916cf", [:mix], [], "hexpm", "098af64e1f6f8609c6672127cfe9e9590a5d3fcdd82bc17a377b8692fd81a879"},
   "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"},
   "nimble_pool": {:hex, :nimble_pool, "1.1.0", "bf9c29fbdcba3564a8b800d1eeb5a3c58f36e1e11d7b7fb2e084a643f645f06b", [:mix], [], "hexpm", "af2e4e6b34197db81f7aad230c1118eac993acc0dae6bc83bac0126d4ae0813a"},
   "oban": {:hex, :oban, "2.18.3", "1608c04f8856c108555c379f2f56bc0759149d35fa9d3b825cb8a6769f8ae926", [:mix], [{:ecto_sql, "~> 3.10", [hex: :ecto_sql, repo: "hexpm", optional: false]}, {:ecto_sqlite3, "~> 0.9", [hex: :ecto_sqlite3, repo: "hexpm", optional: true]}, {:jason, "~> 1.1", [hex: :jason, repo: "hexpm", optional: false]}, {:postgrex, "~> 0.16", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "36ca6ca84ef6518f9c2c759ea88efd438a3c81d667ba23b02b062a0aa785475e"},