feat(tests): re-enable manual pid allowance

Author: solnic

lib/sentry/test.ex

@@ -251,19 +251,83 @@ 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
+    allowed_pid = resolve_allowed_pid(pid_or_fun)
+
+    case Sentry.Test.Registry.claim_collector_allow(owner_pid, allowed_pid) 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, :no_collector} ->
+        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, {:taken, existing_owner}} ->
+        raise ArgumentError,
+              "cannot allow #{inspect(allowed_pid)} for #{inspect(owner_pid)}: " <>
+                "already allowed by another live test scope " <>
+                "(owner: #{inspect(existing_owner)})"
+    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 """
@@ -698,6 +762,10 @@ defmodule Sentry.Test do
               :ok
           end
         end
+
+        # Drop any allow_sentry_reports/2 entries pointing at this test's
+        # collector table.
+        :ets.match_delete(@registry_table, {:_, collector_table})
       end
 
       if :ets.whereis(collector_table) != :undefined do

lib/sentry/test/registry.ex

@@ -47,6 +47,30 @@ defmodule Sentry.Test.Registry do
     GenServer.call(__MODULE__, {:claim_allow, owner_pid, allowed_pid, mode})
   end
 
+  @doc """
+  Atomic claim of `allowed_pid` onto the collector table currently
+  registered for `owner_pid`. Used by `Sentry.Test.allow_sentry_reports/2`
+  so that processes which are not `$callers` descendants of the test
+  (Broadway workers, `phoenix_test_playwright` drivers, long-lived
+  GenServers, etc.) can have their captured events routed into the
+  test's collector ETS table.
+
+  Returns:
+
+    * `:ok` — claim succeeded (or was already held by `owner_pid`)
+    * `{:error, :no_collector}` — `owner_pid` has not called
+      `setup_sentry/1` / `start_collecting_sentry_reports/0` yet
+    * `{:error, {:taken, existing_owner}}` — a different live test
+      already owns `allowed_pid`; clears stale entries from dead owners
+      automatically.
+  """
+  @spec claim_collector_allow(pid(), pid()) ::
+          :ok | {:error, :no_collector} | {:error, {:taken, pid()}}
+  def claim_collector_allow(owner_pid, allowed_pid)
+      when is_pid(owner_pid) and is_pid(allowed_pid) do
+    GenServer.call(__MODULE__, {:claim_collector_allow, owner_pid, allowed_pid})
+  end
+
   @doc """
   Removes every allow entry whose owner is `owner_pid`. Atomic batch
   delete via `:ets.match_delete/2` — safe to call from a test's on_exit
@@ -120,6 +144,57 @@ defmodule Sentry.Test.Registry do
     {:reply, reply, state}
   end
 
+  def handle_call({:claim_collector_allow, owner_pid, allowed_pid}, _from, state) do
+    reply =
+      case :ets.lookup(@table, owner_pid) do
+        [{^owner_pid, table}] ->
+          claim_collector_allow(allowed_pid, table, owner_pid)
+
+        [] ->
+          {:error, :no_collector}
+      end
+
+    {:reply, reply, state}
+  end
+
+  defp claim_collector_allow(allowed_pid, table, owner_pid) do
+    case :ets.lookup(@table, allowed_pid) do
+      [] ->
+        true = :ets.insert_new(@table, {allowed_pid, table})
+        :ok
+
+      [{^allowed_pid, ^table}] ->
+        :ok
+
+      [{^allowed_pid, other_table}] ->
+        # Find the live owner (if any) of the conflicting table.
+        case :ets.match(@table, {:"$1", other_table}) do
+          [] ->
+            true = :ets.insert(@table, {allowed_pid, table})
+            :ok
+
+          owners ->
+            live =
+              owners
+              |> List.flatten()
+              |> Enum.reject(&(&1 == allowed_pid))
+              |> Enum.find(&Process.alive?/1)
+
+            cond do
+              is_nil(live) ->
+                true = :ets.insert(@table, {allowed_pid, table})
+                :ok
+
+              live == owner_pid ->
+                :ok
+
+              true ->
+                {:error, {:taken, live}}
+            end
+        end
+    end
+  end
+
   # Starts a global Bypass instance that acts as a silent HTTP sink for all tests.
   # This ensures every test has a valid DSN even without calling setup_sentry/1,
   # preserving backward compatibility where capture_* returns {:ok, ""}.

test/sentry/test_test.exs

@@ -154,10 +154,6 @@ defmodule Sentry.TestTest do
       SentryTest.setup_sentry()
     end
 
-    test "allow_sentry_reports/2 is a no-op" do
-      assert :ok = SentryTest.allow_sentry_reports(self(), self())
-    end
-
     test "start_collecting/1 is a no-op when already collecting" do
       assert :ok = SentryTest.start_collecting()
     end
@@ -167,6 +163,63 @@ defmodule Sentry.TestTest do
     end
   end
 
+  # Reproduction for https://github.com/getsentry/sentry-elixir/issues/1052
+  #
+  # When events are captured from a process that is not a `$callers`
+  # descendant of the test (e.g. Broadway workers, processes started by
+  # `phoenix_test_playwright`, long-lived GenServers, etc.), there is
+  # currently no way to have those events collected by the test.
+  #
+  # `Sentry.Test.allow_sentry_reports/2` used to provide this manual
+  # allowance but was turned into a no-op. These tests document the
+  # missing behavior.
+  describe "allow_sentry_reports/2 (issue #1052)" do
+    setup do
+      SentryTest.setup_sentry()
+    end
+
+    test "events from a process without $callers are not collected" do
+      test_pid = self()
+
+      pid =
+        spawn(fn ->
+          assert [] == Process.get(:"$callers", [])
+          assert {:ok, _} = Sentry.capture_message("from unrelated process", result: :sync)
+          send(test_pid, :done)
+        end)
+
+      ref = Process.monitor(pid)
+      assert_receive :done, 5000
+      assert_receive {:DOWN, ^ref, :process, ^pid, _}, 5000
+
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "allow_sentry_reports/2 should let an unrelated process report into the test" do
+      test_pid = self()
+
+      pid =
+        spawn(fn ->
+          receive do
+            :go ->
+              assert {:ok, _} = Sentry.capture_message("from allowed process", result: :sync)
+              send(test_pid, :done)
+          end
+        end)
+
+      ref = Process.monitor(pid)
+
+      assert :ok = SentryTest.allow_sentry_reports(self(), pid)
+
+      send(pid, :go)
+      assert_receive :done, 5000
+      assert_receive {:DOWN, ^ref, :process, ^pid, _}, 5000
+
+      assert [%Sentry.Event{} = event] = SentryTest.pop_sentry_reports()
+      assert event.message.formatted == "from allowed process"
+    end
+  end
+
   describe "before_send wrapping" do
     test "wraps existing before_send callback" do
       test_pid = self()

test_integrations/phoenix_app/test/phoenix_app/oban_test.exs

@@ -4,7 +4,7 @@ defmodule Sentry.Integrations.Phoenix.ObanTest do
 
   import ExUnit.CaptureLog
   import Sentry.TestHelpers
-  
+
   require OpenTelemetry.Tracer
 
   alias Sentry.Integrations.Oban.ErrorReporter
@@ -438,4 +438,105 @@ defmodule Sentry.Integrations.Phoenix.ObanTest do
       assert event["tags"]["oban_worker"] == "NonExistent.Worker.Module"
     end
   end
+
+  describe "allow_sentry_reports/2 with a real Oban worker process" do
+    setup do
+      Sentry.Test.setup_sentry()
+      Sentry.Integrations.Oban.ErrorReporter.attach()
+      on_exit(fn -> :telemetry.detach(Sentry.Integrations.Oban.ErrorReporter) end)
+    end
+
+    test "events from the worker process are dropped without an explicit allow" do
+      run_failing_worker_in_detached_process(before_perform: fn -> :ok end)
+
+      assert [] == Sentry.Test.pop_sentry_reports()
+    end
+
+    test "events from the worker process are captured when allowed via a telemetry hook" do
+      test_pid = self()
+      handler_id = {:sentry_allow_test, System.unique_integer([:positive])}
+
+      try do
+        :telemetry.attach(
+          handler_id,
+          [:oban, :job, :start],
+          fn _event, _measurements, _metadata, _config ->
+            Sentry.Test.allow_sentry_reports(test_pid, self())
+          end,
+          nil
+        )
+
+        run_failing_worker_in_detached_process(before_perform: fn -> :ok end)
+      after
+        :telemetry.detach(handler_id)
+      end
+
+      assert [%Sentry.Event{} = event] = Sentry.Test.pop_sentry_reports()
+
+      assert [exception] = event.exception
+      assert exception.type == "RuntimeError"
+      assert exception.value == "intentional failure for testing"
+
+      assert event.tags[:oban_worker] ==
+               "Sentry.Integrations.Phoenix.ObanTest.FailingWorker"
+    end
+  end
+
+  defp run_failing_worker_in_detached_process(opts) do
+    parent = self()
+    ref = make_ref()
+
+    spawn(fn ->
+      Keyword.fetch!(opts, :before_perform).()
+
+      job = %Oban.Job{
+        id: System.unique_integer([:positive]),
+        args: %{"should_fail" => true},
+        worker: "Sentry.Integrations.Phoenix.ObanTest.FailingWorker",
+        queue: "background",
+        attempt: 1,
+        max_attempts: 1,
+        meta: %{},
+        inserted_at: DateTime.utc_now(),
+        scheduled_at: DateTime.utc_now(),
+        attempted_at: DateTime.utc_now()
+      }
+
+      start_metadata = %{job: job, conf: %{name: Oban}}
+      :telemetry.execute([:oban, :job, :start], %{system_time: System.system_time()}, start_metadata)
+
+      {kind, reason, stacktrace} =
+        try do
+          FailingWorker.perform(job)
+          {:ok, nil, []}
+        catch
+          kind, reason -> {kind, reason, __STACKTRACE__}
+        end
+
+      exception_metadata =
+        Map.merge(start_metadata, %{
+          kind: kind,
+          reason: reason,
+          error: reason,
+          stacktrace: stacktrace,
+          state: :failure
+        })
+
+      :telemetry.execute(
+        [:oban, :job, :exception],
+        %{duration: 0},
+        exception_metadata
+      )
+
+      send(parent, {ref, :done})
+    end)
+
+    receive do
+      {^ref, :done} -> :ok
+    after
+      5_000 -> flunk("worker process did not finish in time")
+    end
+
+    Process.sleep(50)
+  end
 end