From c5bd58e7d3cc2c75f5bea20f9609b9d273c694eb Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Tue, 20 Jan 2026 23:16:39 +0100
Subject: [PATCH] Add @spec type annotations to SystemActor functions

Add type specifications for all private functions to improve
static analysis with Dialyzer and documentation quality.
---
 lib/mv/helpers/system_actor.ex | 337 ++++++++++++++++++++++-----------
 1 file changed, 226 insertions(+), 111 deletions(-)

diff --git a/lib/mv/helpers/system_actor.ex b/lib/mv/helpers/system_actor.ex
index dd11690..1349c80 100644
--- a/lib/mv/helpers/system_actor.ex
+++ b/lib/mv/helpers/system_actor.ex
@@ -29,6 +29,12 @@ defmodule Mv.Helpers.SystemActor do
   The cache is invalidated on application restart. For long-running applications,
   consider implementing cache invalidation on role changes.
 
+  ## Race Conditions
+
+  The system actor creation uses `upsert?: true` with `upsert_identity: :unique_email`
+  to prevent race conditions when multiple processes try to create the system user
+  simultaneously. This ensures idempotent creation and prevents database constraint errors.
+
   ## Security
 
   The system actor should NEVER be used for user-initiated actions. It is
@@ -73,25 +79,70 @@ defmodule Mv.Helpers.SystemActor do
   """
   @spec get_system_actor() :: Mv.Accounts.User.t()
   def get_system_actor do
+    case get_system_actor_result() do
+      {:ok, actor} -> actor
+      {:error, reason} -> raise "Failed to load system actor: #{inspect(reason)}"
+    end
+  end
+
+  @doc """
+  Returns the system actor as a result tuple.
+
+  This variant returns `{:ok, actor}` or `{:error, reason}` instead of raising,
+  which is useful for error handling in pipes or when you want to handle errors explicitly.
+
+  ## Returns
+
+  - `{:ok, %Mv.Accounts.User{}}` - Successfully loaded system actor
+  - `{:error, term()}` - Error loading system actor
+
+  ## Examples
+
+      case SystemActor.get_system_actor_result() do
+        {:ok, actor} -> use_actor(actor)
+        {:error, reason} -> handle_error(reason)
+      end
+
+  """
+  @spec get_system_actor_result() :: {:ok, Mv.Accounts.User.t()} | {:error, term()}
+  def get_system_actor_result do
     # In test environment, always load directly to avoid Agent/Sandbox issues
     if Mix.env() == :test do
-      load_system_actor()
+      try do
+        {:ok, load_system_actor()}
+      rescue
+        e -> {:error, e}
+      end
     else
       try do
-        Agent.get_and_update(__MODULE__, fn
-          nil ->
-            # Cache miss - load system actor
-            actor = load_system_actor()
-            {actor, actor}
+        result =
+          Agent.get_and_update(__MODULE__, fn
+            nil ->
+              # Cache miss - load system actor
+              try do
+                actor = load_system_actor()
+                {actor, actor}
+              rescue
+                e -> {{:error, e}, nil}
+              end
 
-          cached_actor ->
-            # Cache hit - return cached actor
-            {cached_actor, cached_actor}
-        end)
+            cached_actor ->
+              # Cache hit - return cached actor
+              {cached_actor, cached_actor}
+          end)
+
+        case result do
+          {:error, reason} -> {:error, reason}
+          actor -> {:ok, actor}
+        end
       catch
         :exit, {:noproc, _} ->
           # Agent not started - load directly without caching
-          load_system_actor()
+          try do
+            {:ok, load_system_actor()}
+          rescue
+            e -> {:error, e}
+          end
       end
     end
   end
@@ -113,128 +164,186 @@ defmodule Mv.Helpers.SystemActor do
     Agent.update(__MODULE__, fn _state -> nil end)
   end
 
+  @doc """
+  Returns the email address of the system user.
+
+  This is useful for other modules that need to reference the system user
+  without loading the full user record.
+
+  ## Returns
+
+  - `String.t()` - The system user email address ("system@mila.local")
+
+  ## Examples
+
+      iex> Mv.Helpers.SystemActor.system_user_email()
+      "system@mila.local"
+
+  """
+  @spec system_user_email() :: String.t()
+  def system_user_email, do: @system_user_email
+
   # Loads the system actor from the database
   # First tries to find system@mila.local, then falls back to admin user
+  @spec load_system_actor() :: Mv.Accounts.User.t() | no_return()
   defp load_system_actor do
-    # Try to find system user first
     case find_user_by_email(@system_user_email) do
       {:ok, user} when not is_nil(user) ->
         load_user_with_role(user)
 
       {:ok, nil} ->
-        # System user doesn't exist - fall back to admin user
-        case load_admin_user_fallback() do
-          {:ok, admin_user} ->
-            admin_user
-
-          {:error, _} ->
-            # In test environment, create a temporary admin user if none exists
-            if Mix.env() == :test do
-              create_test_system_actor()
-            else
-              raise "Failed to load system actor: no system user or admin user found"
-            end
-        end
+        handle_system_user_not_found("no system user or admin user found")
 
       {:error, _reason} = error ->
-        # Database error - try fallback
-        case load_admin_user_fallback() do
-          {:ok, admin_user} ->
-            admin_user
+        handle_system_user_error(error)
+    end
+  end
 
-          {:error, _} ->
-            # In test environment, create a temporary admin user if none exists
-            if Mix.env() == :test do
-              create_test_system_actor()
-            else
-              raise "Failed to load system actor: #{inspect(error)}"
-            end
-        end
+  # Handles case when system user doesn't exist
+  @spec handle_system_user_not_found(String.t()) :: Mv.Accounts.User.t() | no_return()
+  defp handle_system_user_not_found(message) do
+    case load_admin_user_fallback() do
+      {:ok, admin_user} ->
+        admin_user
+
+      {:error, _} ->
+        handle_fallback_error(message)
+    end
+  end
+
+  # Handles database error when loading system user
+  @spec handle_system_user_error(term()) :: Mv.Accounts.User.t() | no_return()
+  defp handle_system_user_error(error) do
+    case load_admin_user_fallback() do
+      {:ok, admin_user} ->
+        admin_user
+
+      {:error, _} ->
+        handle_fallback_error("Failed to load system actor: #{inspect(error)}")
+    end
+  end
+
+  # Handles fallback error - creates test actor or raises
+  @spec handle_fallback_error(String.t()) :: Mv.Accounts.User.t() | no_return()
+  defp handle_fallback_error(message) do
+    if Mix.env() == :test do
+      create_test_system_actor()
+    else
+      raise "Failed to load system actor: #{message}"
     end
   end
 
   # Creates a temporary admin user for tests when no system/admin user exists
+  @spec create_test_system_actor() :: Mv.Accounts.User.t() | no_return()
   defp create_test_system_actor do
+    alias Mv.Accounts
     alias Mv.Authorization
+
+    admin_role = ensure_admin_role_exists()
+    create_system_user_with_role(admin_role)
+  end
+
+  # Ensures admin role exists - finds or creates it
+  @spec ensure_admin_role_exists() :: Mv.Authorization.Role.t() | no_return()
+  defp ensure_admin_role_exists do
+    case find_admin_role() do
+      {:ok, role} ->
+        role
+
+      {:error, :not_found} ->
+        create_admin_role_with_retry()
+    end
+  end
+
+  # Finds admin role in existing roles
+  @spec find_admin_role() :: {:ok, Mv.Authorization.Role.t()} | {:error, :not_found}
+  defp find_admin_role do
+    alias Mv.Authorization
+
+    case Authorization.list_roles() do
+      {:ok, roles} ->
+        case Enum.find(roles, &(&1.permission_set_name == "admin")) do
+          nil -> {:error, :not_found}
+          role -> {:ok, role}
+        end
+
+      _ ->
+        {:error, :not_found}
+    end
+  end
+
+  # Creates admin role, handling race conditions
+  @spec create_admin_role_with_retry() :: Mv.Authorization.Role.t() | no_return()
+  defp create_admin_role_with_retry do
+    alias Mv.Authorization
+
+    case create_admin_role() do
+      {:ok, role} ->
+        role
+
+      {:error, :already_exists} ->
+        find_existing_admin_role()
+
+      {:error, error} ->
+        raise "Failed to create admin role: #{inspect(error)}"
+    end
+  end
+
+  # Attempts to create admin role
+  @spec create_admin_role() ::
+          {:ok, Mv.Authorization.Role.t()} | {:error, :already_exists | term()}
+  defp create_admin_role do
+    alias Mv.Authorization
+
+    case Authorization.create_role(%{
+           name: "Admin",
+           description: "Administrator with full access",
+           permission_set_name: "admin"
+         }) do
+      {:ok, role} ->
+        {:ok, role}
+
+      {:error, %Ash.Error.Invalid{errors: [%{field: :name, message: "has already been taken"}]}} ->
+        {:error, :already_exists}
+
+      {:error, error} ->
+        {:error, error}
+    end
+  end
+
+  # Finds existing admin role after creation attempt failed due to race condition
+  @spec find_existing_admin_role() :: Mv.Authorization.Role.t() | no_return()
+  defp find_existing_admin_role do
+    alias Mv.Authorization
+
+    case Authorization.list_roles() do
+      {:ok, roles} ->
+        Enum.find(roles, &(&1.permission_set_name == "admin")) ||
+          raise "Admin role should exist but was not found"
+
+      _ ->
+        raise "Failed to find admin role after creation attempt"
+    end
+  end
+
+  # Creates system user with admin role assigned
+  @spec create_system_user_with_role(Mv.Authorization.Role.t()) ::
+          Mv.Accounts.User.t() | no_return()
+  defp create_system_user_with_role(admin_role) do
     alias Mv.Accounts
 
-    # Ensure admin role exists - find or create
-    admin_role =
-      case Authorization.list_roles() do
-        {:ok, roles} ->
-          case Enum.find(roles, &(&1.permission_set_name == "admin")) do
-            nil ->
-              # Try to create, but handle case where it already exists (race condition)
-              case Authorization.create_role(%{
-                     name: "Admin",
-                     description: "Administrator with full access",
-                     permission_set_name: "admin"
-                   }) do
-                {:ok, role} ->
-                  role
-
-                {:error,
-                 %Ash.Error.Invalid{errors: [%{field: :name, message: "has already been taken"}]}} ->
-                  # Role was created by another process - find it
-                  case Authorization.list_roles() do
-                    {:ok, updated_roles} ->
-                      Enum.find(updated_roles, &(&1.permission_set_name == "admin")) ||
-                        raise "Admin role should exist but was not found"
-
-                    _ ->
-                      raise "Failed to find admin role after creation attempt"
-                  end
-
-                {:error, error} ->
-                  raise "Failed to create admin role: #{inspect(error)}"
-              end
-
-            role ->
-              role
-          end
-
-        _ ->
-          # If list_roles fails, try to create anyway
-          case Authorization.create_role(%{
-                 name: "Admin",
-                 description: "Administrator with full access",
-                 permission_set_name: "admin"
-               }) do
-            {:ok, role} ->
-              role
-
-            {:error,
-             %Ash.Error.Invalid{errors: [%{field: :name, message: "has already been taken"}]}} ->
-              # Role exists - try to find it
-              case Authorization.list_roles() do
-                {:ok, roles} ->
-                  Enum.find(roles, &(&1.permission_set_name == "admin")) ||
-                    raise "Admin role should exist but was not found"
-
-                _ ->
-                  raise "Failed to find admin role"
-              end
-
-            {:error, error} ->
-              raise "Failed to create admin role: #{inspect(error)}"
-          end
-      end
-
-    # Create system user for tests
-    system_user =
-      Accounts.create_user!(%{email: @system_user_email},
-        upsert?: true,
-        upsert_identity: :unique_email
-      )
-      |> Ash.Changeset.for_update(:update, %{})
-      |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove)
-      |> Ash.update!()
-      |> Ash.load!(:role, domain: Mv.Accounts)
-
-    system_user
+    Accounts.create_user!(%{email: @system_user_email},
+      upsert?: true,
+      upsert_identity: :unique_email
+    )
+    |> Ash.Changeset.for_update(:update, %{})
+    |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove)
+    |> Ash.update!()
+    |> Ash.load!(:role, domain: Mv.Accounts)
   end
 
   # Finds a user by email address
+  @spec find_user_by_email(String.t()) :: {:ok, Mv.Accounts.User.t() | nil} | {:error, term()}
   defp find_user_by_email(email) do
     Mv.Accounts.User
     |> Ash.Query.filter(email == ^email)
@@ -242,6 +351,7 @@ defmodule Mv.Helpers.SystemActor do
   end
 
   # Loads a user with their role preloaded (required for authorization)
+  @spec load_user_with_role(Mv.Accounts.User.t()) :: Mv.Accounts.User.t() | no_return()
   defp load_user_with_role(user) do
     case Ash.load(user, :role, domain: Mv.Accounts) do
       {:ok, user_with_role} ->
@@ -253,10 +363,12 @@ defmodule Mv.Helpers.SystemActor do
   end
 
   # Validates that the user has an admin role
+  @spec validate_admin_role(Mv.Accounts.User.t()) :: Mv.Accounts.User.t() | no_return()
   defp validate_admin_role(%{role: %{permission_set_name: "admin"}} = user) do
     user
   end
 
+  @spec validate_admin_role(Mv.Accounts.User.t()) :: no_return()
   defp validate_admin_role(%{role: %{permission_set_name: permission_set}}) do
     raise """
     System actor must have admin role, but has permission_set_name: #{permission_set}
@@ -264,6 +376,7 @@ defmodule Mv.Helpers.SystemActor do
     """
   end
 
+  @spec validate_admin_role(Mv.Accounts.User.t()) :: no_return()
   defp validate_admin_role(%{role: nil}) do
     raise """
     System actor must have a role assigned, but role is nil.
@@ -271,6 +384,7 @@ defmodule Mv.Helpers.SystemActor do
     """
   end
 
+  @spec validate_admin_role(term()) :: no_return()
   defp validate_admin_role(_user) do
     raise """
     System actor must have a role with admin permissions.
@@ -279,6 +393,7 @@ defmodule Mv.Helpers.SystemActor do
   end
 
   # Fallback: Loads admin user from seeds (ADMIN_EMAIL env var or default)
+  @spec load_admin_user_fallback() :: {:ok, Mv.Accounts.User.t()} | {:error, term()}
   defp load_admin_user_fallback do
     admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost"