From 609189c49b320e35b271f00605d96c41fdaae1fe Mon Sep 17 00:00:00 2001 From: timujeen Date: Mon, 24 Nov 2025 17:23:12 +0000 Subject: [PATCH 1/2] Fix UeberAuth configuration access and remove unused cache functions Problems fixed: 1. PhoenixKit.Config.UeberAuth was incorrectly accessing Ueberauth configuration through PhoenixKit.Config methods, which only look in the :phoenix_kit application environment 2. Cache module had unused warming functions from old implementation Changes: - Fixed get_all() to use Application.get_env(:ueberauth, Ueberauth, []) instead of Config.get_list(:ueberauth, []) - Fixed set_all() to use Application.put_env(:ueberauth, Ueberauth, config) instead of Config.set(:ueberauth, config) - Removed unused handle_cache_warming/4 and handle_sync_warming/3 functions from PhoenixKit.Cache This ensures UeberAuth configuration is correctly read from and written to the :ueberauth application environment where it belongs, and removes dead code from the Cache module. --- lib/phoenix_kit/cache/cache.ex | 76 ++++++++++- lib/phoenix_kit/config/ueber_auth.ex | 4 +- lib/phoenix_kit/settings/settings.ex | 70 +++++++++- lib/phoenix_kit/supervisor.ex | 12 +- .../workers/oauth_config_loader.ex | 120 +++++------------- 5 files changed, 181 insertions(+), 101 deletions(-) diff --git a/lib/phoenix_kit/cache/cache.ex b/lib/phoenix_kit/cache/cache.ex index 9dccdea1f..d3fae7692 100644 --- a/lib/phoenix_kit/cache/cache.ex +++ b/lib/phoenix_kit/cache/cache.ex @@ -323,12 +323,24 @@ defmodule PhoenixKit.Cache do def init(opts) do name = Keyword.fetch!(opts, :name) warmer = Keyword.get(opts, :warmer) + critical_warmer = Keyword.get(opts, :critical_warmer) + sync_init = Keyword.get(opts, :sync_init, false) ttl = Keyword.get(opts, :ttl) max_size = Keyword.get(opts, :max_size) table = :ets.new(:"cache_#{name}", [:set, :protected, :named_table, {:read_concurrency, true}]) + # Support legacy cache_settings table for backwards compatibility + if name == :settings do + try do + :ets.new(:cache_settings, [:set, :protected, :named_table, {:read_concurrency, true}]) + rescue + # Table already exists + ArgumentError -> :ok + end + end + state = %__MODULE__{ name: name, table: table, @@ -337,13 +349,19 @@ defmodule PhoenixKit.Cache do max_size: max_size } - # Warm cache if warmer function is provided - if warmer do - send(self(), :warm_cache) + Logger.info( + "Started cache #{name} with table #{table}#{if sync_init, do: " (sync_init enabled)", else: ""}" + ) + + # Use handle_continue to warm cache after init returns + # This prevents blocking supervisor initialization + if sync_init and critical_warmer do + {:ok, state, {:continue, {:warm_critical, critical_warmer, warmer}}} + else + # Standard async warming + if warmer, do: send(self(), :warm_cache) + {:ok, state} end - - Logger.info("Started cache #{name} with table #{table}") - {:ok, state} end @impl GenServer @@ -481,17 +499,63 @@ defmodule PhoenixKit.Cache do {:noreply, state} end + @impl GenServer + def handle_continue({:warm_critical, critical_warmer, warmer}, %{name: name} = state) do + # Load critical data synchronously in handle_continue + # This runs after init returns, so supervisor can continue starting other processes + case safe_warm(critical_warmer) do + {:ok, data} when is_map(data) -> + warm_critical_data(state, data) + Logger.info("Synchronously warmed cache #{name} with #{map_size(data)} critical entries") + + # Schedule loading of remaining data if main warmer exists + if warmer do + send(self(), :warm_remaining_cache) + end + + {:error, error} -> + Logger.error("Failed to synchronously warm critical cache #{name}: #{inspect(error)}") + # Continue with async warming as fallback + if warmer, do: send(self(), :warm_cache) + + _ -> + Logger.warning("Critical warmer for cache #{name} returned invalid data") + # Continue with async warming as fallback + if warmer, do: send(self(), :warm_cache) + end + + {:noreply, state} + end + @impl GenServer def handle_info(:warm_cache, state) do handle_cast(:warm, state) end + @impl GenServer + def handle_info(:warm_remaining_cache, state) do + # Load all remaining data after critical data has been loaded + handle_cast(:warm, state) + end + # Private Functions defp via_tuple(name) do PhoenixKit.Cache.Registry.via_tuple(name) end + defp warm_critical_data(%{name: name, table: table, ttl: ttl}, data) do + Enum.each(data, fn {key, value} -> + expires_at = if ttl, do: System.monotonic_time(:millisecond) + ttl, else: nil + :ets.insert(table, {key, value, expires_at}) + + # Also write to legacy cache_settings table if this is settings cache + if name == :settings do + :ets.insert(:cache_settings, {key, value, expires_at}) + end + end) + end + defp safe_warm(warmer) when is_function(warmer, 0) do {:ok, warmer.()} rescue diff --git a/lib/phoenix_kit/config/ueber_auth.ex b/lib/phoenix_kit/config/ueber_auth.ex index b4bd286e4..a91d7ef75 100644 --- a/lib/phoenix_kit/config/ueber_auth.ex +++ b/lib/phoenix_kit/config/ueber_auth.ex @@ -52,7 +52,7 @@ defmodule PhoenixKit.Config.UeberAuth do """ @spec get_all() :: Keyword.t() def get_all do - Config.get_list(:ueberauth, []) + Application.get_env(:ueberauth, Ueberauth, []) end @doc """ @@ -156,7 +156,7 @@ defmodule PhoenixKit.Config.UeberAuth do def set_all(options) when is_list(options) do current_config = get_all() new_config = Keyword.merge(current_config, options) - Config.set(:ueberauth, new_config) + Application.put_env(:ueberauth, Ueberauth, new_config) :ok end diff --git a/lib/phoenix_kit/settings/settings.ex b/lib/phoenix_kit/settings/settings.ex index 25483d92d..29fe3909b 100644 --- a/lib/phoenix_kit/settings/settings.ex +++ b/lib/phoenix_kit/settings/settings.ex @@ -1328,6 +1328,67 @@ defmodule PhoenixKit.Settings do %{} end + @doc """ + Warm cache with critical OAuth settings only. + + Returns map of critical OAuth settings for synchronous cache warming. + This is used during startup to ensure OAuth configuration is available + immediately, preventing race conditions with OAuthConfigLoader. + + Only loads OAuth-related settings that are required for provider configuration. + """ + def warm_critical_cache do + # Critical OAuth keys that must be loaded synchronously at startup + critical_keys = [ + # Google OAuth + "oauth_google_client_id", + "oauth_google_client_secret", + # GitHub OAuth + "oauth_github_client_id", + "oauth_github_client_secret", + # Apple OAuth + "oauth_apple_client_id", + "oauth_apple_team_id", + "oauth_apple_key_id", + "oauth_apple_private_key_path", + # Facebook OAuth + "oauth_facebook_app_id", + "oauth_facebook_app_secret", + # OAuth general settings + "oauth_enabled" + ] + + # Check if repository is available + if repo_available?() do + settings = + Setting + |> where([s], s.key in ^critical_keys) + |> repo().all() + + settings + |> Enum.map(fn setting -> + # Prioritize JSON value over string value for cache storage + value = + if setting.value_json do + setting.value_json + else + setting.value + end + + {setting.key, value} + end) + |> Map.new() + else + # Repo not available - return empty map + # This should rarely happen as critical cache is loaded at startup + %{} + end + rescue + error -> + Logger.error("Failed to warm critical cache: #{inspect(error)}") + %{} + end + ## Private Batch Query Functions # Batch query multiple string settings from database in a single operation @@ -1440,8 +1501,13 @@ defmodule PhoenixKit.Settings do _ -> true end - # Check if the repository is available and ready to accept queries - defp repo_available? do + @doc """ + Check if the repository is available and ready to accept queries. + + Returns true if the repo is configured and running, false otherwise. + Used to prevent errors during Mix tasks when repo might not be started. + """ + def repo_available? do # First check if repo is configured case PhoenixKit.Config.get(:repo, nil) do nil -> diff --git a/lib/phoenix_kit/supervisor.ex b/lib/phoenix_kit/supervisor.ex index ba0447aba..a3ffc8638 100644 --- a/lib/phoenix_kit/supervisor.ex +++ b/lib/phoenix_kit/supervisor.ex @@ -14,11 +14,17 @@ defmodule PhoenixKit.Supervisor do PhoenixKit.PubSub.Manager, PhoenixKit.Admin.SimplePresence, {PhoenixKit.Cache.Registry, []}, - {PhoenixKit.Cache, name: :settings, warmer: &PhoenixKit.Settings.warm_cache_data/0}, + # Settings cache with synchronous initialization for critical OAuth settings + # This ensures OAuth configuration is available before OAuthConfigLoader starts + {PhoenixKit.Cache, + name: :settings, + sync_init: true, + critical_warmer: &PhoenixKit.Settings.warm_critical_cache/0, + warmer: &PhoenixKit.Settings.warm_cache_data/0}, # Rate limiter backend MUST be started before any authentication requests PhoenixKit.Users.RateLimiter.Backend, - # OAuth config loader MUST be first to ensure configuration - # is available before any OAuth requests are processed + # OAuth config loader - now guaranteed to have critical settings in cache + # No longer needs retry logic as cache is pre-warmed with OAuth settings PhoenixKit.Workers.OAuthConfigLoader, PhoenixKit.Entities.Presence, # Email tracking supervisor - handles SQS Worker for automatic bounce event processing diff --git a/lib/phoenix_kit/workers/oauth_config_loader.ex b/lib/phoenix_kit/workers/oauth_config_loader.ex index 3efb1849a..2d8a35c06 100644 --- a/lib/phoenix_kit/workers/oauth_config_loader.ex +++ b/lib/phoenix_kit/workers/oauth_config_loader.ex @@ -3,14 +3,14 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do GenServer worker that ensures OAuth configuration is loaded from database before any OAuth requests are processed. - This worker runs synchronously during application startup to prevent timing - issues where Ueberauth plug is initialized before OAuth providers are configured. + This worker runs synchronously during application startup to configure + OAuth providers from database settings. ## Startup Sequence - 1. Worker starts as first child in PhoenixKit.Supervisor - 2. Waits for Settings cache to be ready (with timeout) - 3. Loads OAuth configuration from database + 1. PhoenixKit.Cache starts with sync_init, loading critical OAuth settings + 2. OAuthConfigLoader starts, OAuth settings already in cache + 3. Loads OAuth configuration from cache 4. Configures Ueberauth with available providers 5. Returns :ok when complete @@ -22,8 +22,8 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do - Parent Endpoint (router compiles, Ueberauth.init() runs) - PhoenixKit.Supervisor (OAuth config should load here) - If PhoenixKit.Supervisor starts AFTER Parent Endpoint, the OAuth configuration - arrives too late and Ueberauth fails with MatchError. + With sync_init enabled in the Cache, critical OAuth settings are loaded + synchronously before this worker starts, eliminating race conditions. This worker ensures OAuth configuration is available as early as possible during PhoenixKit.Supervisor initialization. @@ -32,15 +32,12 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do use GenServer require Logger - @max_retries 10 - @retry_delay 100 - ## Client API @doc """ Starts the OAuth configuration loader. - Blocks until OAuth configuration is successfully loaded or max retries exceeded. + Loads OAuth configuration from cache which is pre-warmed with critical settings. """ def start_link(_opts) do GenServer.start_link(__MODULE__, [], name: __MODULE__) @@ -103,26 +100,24 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do @impl true def init(_) do # Load OAuth configuration synchronously during initialization - # This ensures configuration is ready before supervisor completes startup - case load_oauth_config_with_retry() do + # With sync_init in the Cache, critical OAuth settings are already loaded + case load_oauth_config() do :ok -> Logger.info("OAuth config loaded successfully during startup") {:ok, %{status: :loaded}} - {:error, :cache_not_ready} -> - # During Mix tasks (like phoenix_kit.update --status), the cache may not be ready - # This is expected and not an error condition - log as debug instead of warning - Logger.debug( - "OAuth config loading skipped: Settings cache not ready (likely during Mix task execution)" - ) - - # Don't fail supervisor startup if cache is not ready - # The fallback plug will handle this case - {:ok, %{status: :not_loaded, reason: :cache_not_ready}} - {:error, :modules_not_loaded} -> Logger.info("OAuth modules not loaded, OAuth features will be unavailable") {:ok, %{status: :not_loaded, reason: :modules_not_loaded}} + + {:error, :repo_not_available} -> + # During Mix tasks, the repo may not be available + # This is expected and not an error condition + Logger.debug( + "OAuth config loading skipped: Repository not available (likely during Mix task execution)" + ) + + {:ok, %{status: :not_loaded, reason: :repo_not_available}} end rescue # Catch unexpected errors during initialization @@ -158,27 +153,6 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do ## Private Helpers - defp load_oauth_config_with_retry(attempt \\ 1) do - case load_oauth_config() do - :ok -> - :ok - - {:error, :cache_not_ready} when attempt < @max_retries -> - # Only log every 3rd attempt to reduce noise during Mix tasks - if rem(attempt, 3) == 0 do - Logger.debug( - "Settings cache not ready, retrying... (attempt #{attempt}/#{@max_retries})" - ) - end - - Process.sleep(@retry_delay) - load_oauth_config_with_retry(attempt + 1) - - {:error, reason} -> - {:error, reason} - end - end - defp load_oauth_config do # Check if required modules are loaded if Code.ensure_loaded?(PhoenixKit.Users.OAuthConfig) and @@ -191,25 +165,15 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do end defp do_load_oauth_config do - # CRITICAL: Verify Settings cache is FULLY warmed before configuring OAuth - # The cache warming is asynchronous (via handle_info(:warm_cache)) - # We must wait until ALL settings are loaded, not just one setting - - # Strategy 1: Check cache size (most reliable) - # Based on production data: cache should have ~50+ entries when fully warmed - cache_size = get_cache_size() - - if cache_size < 40 do - # Don't log every attempt to reduce noise during Mix tasks - # Cache will be empty during Mix tasks when repo is not available - {:error, :cache_not_ready} - else - # Strategy 2: Verify OAuth-specific settings are present - # This ensures we have actual OAuth data, not just general settings + # With sync_init enabled, critical OAuth settings are already in cache + # No need to check cache size or wait for warming + + # Check if repository is available (for Mix tasks) + if PhoenixKit.Settings.repo_available?() do + # OAuth settings are already loaded via sync_init oauth_enabled = PhoenixKit.Settings.get_setting("oauth_enabled", "false") # Check that at least one provider's credentials are accessible - # This confirms the cache contains OAuth-related data has_any_oauth_data = PhoenixKit.Settings.has_oauth_credentials?(:google) or PhoenixKit.Settings.has_oauth_credentials?(:apple) or @@ -217,34 +181,24 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do PhoenixKit.Settings.has_oauth_credentials?(:facebook) Logger.debug( - "Settings cache ready: size=#{cache_size}, oauth_enabled=#{oauth_enabled}, has_oauth_data=#{has_any_oauth_data}" + "OAuth configuration: enabled=#{oauth_enabled}, has_oauth_data=#{has_any_oauth_data}" ) - # Configure OAuth providers from database - # At this point, ALL providers' credentials should be in cache + # Configure OAuth providers from settings alias PhoenixKit.Users.OAuthConfig OAuthConfig.configure_providers() :ok + else + {:error, :repo_not_available} end rescue - # Specific error types that indicate cache not ready (retriable) - error in [RuntimeError, UndefinedFunctionError] -> - # These are expected during startup - Settings cache may not be ready - Logger.debug("Settings cache not ready: #{Exception.message(error)}") - {:error, :cache_not_ready} - - # Database connection errors (retriable) + # Database connection errors error in [DBConnection.ConnectionError, Postgrex.Error] -> Logger.warning("Database connection error: #{Exception.message(error)}") - {:error, :cache_not_ready} - - # ArgumentError when ETS table doesn't exist yet - error in [ArgumentError] -> - Logger.debug("Settings cache table not created yet: #{Exception.message(error)}") - {:error, :cache_not_ready} + {:error, :repo_not_available} - # Any other unexpected error (non-retriable) + # Any other unexpected error error -> # Log full error with stacktrace for debugging Logger.error(""" @@ -255,14 +209,4 @@ defmodule PhoenixKit.Workers.OAuthConfigLoader do # Re-raise to fail fast on unexpected errors reraise error, __STACKTRACE__ end - - # Get the size of Settings cache ETS table - # Returns 0 if table doesn't exist yet - defp get_cache_size do - :ets.info(:cache_settings, :size) - rescue - ArgumentError -> - # Table doesn't exist yet - 0 - end end From 864e19408aeb98aaa6bb32ab7345403846343e9a Mon Sep 17 00:00:00 2001 From: timujeen Date: Mon, 24 Nov 2025 21:51:44 +0000 Subject: [PATCH 2/2] Fix cache initialization to load all data synchronously Update cache warming strategy to load all settings data during synchronous initialization instead of loading critical settings first then remaining settings asynchronously. Changes: - Remove critical_warmer parameter from cache initialization - Load all data in handle_continue when sync_init is enabled - Add retry mechanism with exponential backoff for empty results - Simplify supervisor configuration to use single warmer function This prevents race conditions where critical OAuth settings are loaded first but then overwritten by async full cache loading returning empty results if repository is not yet ready. Benefits: - Single data load eliminates overwrite issues - Retry logic handles repository initialization delays - OAuth configuration loads reliably on cold starts - Maintains non-blocking supervisor initialization --- lib/phoenix_kit/cache/cache.ex | 61 ++++++++++++++++++++++------------ lib/phoenix_kit/supervisor.ex | 9 +++-- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/lib/phoenix_kit/cache/cache.ex b/lib/phoenix_kit/cache/cache.ex index d3fae7692..e4717cd8b 100644 --- a/lib/phoenix_kit/cache/cache.ex +++ b/lib/phoenix_kit/cache/cache.ex @@ -500,28 +500,35 @@ defmodule PhoenixKit.Cache do end @impl GenServer - def handle_continue({:warm_critical, critical_warmer, warmer}, %{name: name} = state) do - # Load critical data synchronously in handle_continue + def handle_continue({:warm_critical, _critical_warmer, warmer}, %{name: name} = state) do + # Load ALL data synchronously in handle_continue when sync_init is enabled # This runs after init returns, so supervisor can continue starting other processes - case safe_warm(critical_warmer) do - {:ok, data} when is_map(data) -> + # We load all data instead of just critical to avoid race conditions + + # Retry warming if data is empty (repo might not be ready yet) + case warm_with_retry(warmer, name, 3, 100) do + {:ok, data} when is_map(data) and map_size(data) > 0 -> warm_critical_data(state, data) - Logger.info("Synchronously warmed cache #{name} with #{map_size(data)} critical entries") - # Schedule loading of remaining data if main warmer exists - if warmer do - send(self(), :warm_remaining_cache) - end + Logger.info( + "Synchronously warmed cache #{name} with #{map_size(data)} entries (sync_init mode)" + ) + + {:ok, empty_data} when is_map(empty_data) -> + Logger.warning( + "Cache #{name} warmed but no data loaded - repository may not be ready yet. Will retry asynchronously." + ) + + # Schedule async retry + Process.send_after(self(), :warm_cache, 1000) {:error, error} -> - Logger.error("Failed to synchronously warm critical cache #{name}: #{inspect(error)}") - # Continue with async warming as fallback - if warmer, do: send(self(), :warm_cache) + Logger.error("Failed to synchronously warm cache #{name}: #{inspect(error)}") + # Schedule async retry + Process.send_after(self(), :warm_cache, 1000) _ -> - Logger.warning("Critical warmer for cache #{name} returned invalid data") - # Continue with async warming as fallback - if warmer, do: send(self(), :warm_cache) + Logger.warning("Warmer for cache #{name} returned invalid data") end {:noreply, state} @@ -532,12 +539,6 @@ defmodule PhoenixKit.Cache do handle_cast(:warm, state) end - @impl GenServer - def handle_info(:warm_remaining_cache, state) do - # Load all remaining data after critical data has been loaded - handle_cast(:warm, state) - end - # Private Functions defp via_tuple(name) do @@ -562,6 +563,24 @@ defmodule PhoenixKit.Cache do error -> {:error, error} end + defp warm_with_retry(warmer, name, retries, delay) do + case safe_warm(warmer) do + {:ok, data} when is_map(data) and map_size(data) > 0 -> + {:ok, data} + + {:ok, empty_data} when is_map(empty_data) and retries > 0 -> + Logger.debug( + "Cache #{name} warming returned empty data, retrying in #{delay}ms (#{retries} retries left)" + ) + + Process.sleep(delay) + warm_with_retry(warmer, name, retries - 1, delay * 2) + + result -> + result + end + end + defp maybe_evict(%{max_size: nil} = state), do: state defp maybe_evict(%{table: table, max_size: max_size} = state) do diff --git a/lib/phoenix_kit/supervisor.ex b/lib/phoenix_kit/supervisor.ex index a3ffc8638..59e14dc50 100644 --- a/lib/phoenix_kit/supervisor.ex +++ b/lib/phoenix_kit/supervisor.ex @@ -14,13 +14,12 @@ defmodule PhoenixKit.Supervisor do PhoenixKit.PubSub.Manager, PhoenixKit.Admin.SimplePresence, {PhoenixKit.Cache.Registry, []}, - # Settings cache with synchronous initialization for critical OAuth settings + # Settings cache with synchronous initialization + # Loads all settings in handle_continue (after init returns) # This ensures OAuth configuration is available before OAuthConfigLoader starts + # while not blocking supervisor initialization {PhoenixKit.Cache, - name: :settings, - sync_init: true, - critical_warmer: &PhoenixKit.Settings.warm_critical_cache/0, - warmer: &PhoenixKit.Settings.warm_cache_data/0}, + name: :settings, sync_init: true, warmer: &PhoenixKit.Settings.warm_cache_data/0}, # Rate limiter backend MUST be started before any authentication requests PhoenixKit.Users.RateLimiter.Backend, # OAuth config loader - now guaranteed to have critical settings in cache