fix: add retry logic for WebSocket and SSE startup failures - #119
Conversation
When upstream services are temporarily unreachable during app startup, both WebSocket and SSE clients would fail their initial connection with no recovery path — leaving the app in a permanently broken state until manually restarted. WebSocket: Replace fire-and-forget Task with retry loop using exponential backoff (5s–60s) in the Killmail Supervisor GenServer. SSE: Retry failed map initialization up to 5 times with exponential backoff (10s–60s) via TaskSupervisor, re-attempting initialize_and_start_for_map for each failed map. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughKillmail supervisor now starts the WebSocket client via a supervised child and implements tracked exponential-backoff retries. SSE supervisor separates successful and failed map inits, starts clients for successes, and schedules bounded exponential-backoff retries for failed map initializations. Changes
Sequence Diagram(s)sequenceDiagram
participant Sup as Supervisor
participant WS as WebSocketClient
participant Sched as Scheduler
Sup->>WS: start_websocket_client()
alt Success
WS-->>Sup: {:ok, pid}
Sup->>Sup: clear :ws_retry_attempts
else Failure
WS-->>Sup: {:error, reason}
Sup->>Sup: log warning (retry_in_ms)
Sup->>Sched: schedule :retry_websocket (delay)
Sched-->>Sup: :retry_websocket
Sup->>Sup: increment :ws_retry_attempts
Sup->>WS: start_websocket_client()
alt Success
WS-->>Sup: {:ok, pid}
Sup->>Sup: clear :ws_retry_attempts
else Failure
WS-->>Sup: {:error, reason}
Sup->>Sched: schedule :retry_websocket (increased delay)
end
end
sequenceDiagram
participant Sup as SSE Supervisor
participant Init as MapInitialization
participant SSE as SSEClientManager
participant Sched as Scheduler
participant Reg as MapRegistry
Sup->>Init: run_parallel_init()
Init-->>Sup: {successful_maps, failed_maps}
Sup->>SSE: start_sse_clients(successful_maps)
alt failed_maps not empty
Sup->>Sched: schedule_failed_map_retries(failed_maps)
Sched-->>Sup: :retry_failed_maps (timer)
Sup->>Reg: fetch_fresh_configs(failed_maps)
Reg-->>Sup: fresh_configs
Sup->>Init: retry_failed_maps(fresh_configs)
Init-->>Sup: {new_successful, still_failed}
Sup->>SSE: start_sse_clients(new_successful)
alt still_failed and attempts < max
Sup->>Sched: schedule_failed_map_retries(still_failed)
else exceeded attempts or removed
Sup->>Sup: log give-up / skip retries
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- schedule_websocket_retry returns state instead of GenServer tuple - Remove double delay calculation in handle_info(:retry_websocket) - Separate side effects from predicate in retry_failed_maps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/wanderer_notifier/domains/killmail/supervisor.ex`:
- Around line 81-95: The GenServer currently starts the WebSocket linked into
its process tree via
WandererNotifier.Domains.Killmail.WebSocketClient.start_link from
handle_continue(:start_websocket) (and/or start_websocket_client), which will
crash this supervisor when the socket dies; change the startup so the WebSocket
is started unlinked or under the InternalSupervisor instead: remove any direct
start_link calls from handle_continue/2 (or start_websocket_client) and either
(a) call InternalSupervisor.start_child/1 (or the existing internal supervisor
helper) to start the WebSocket under that supervisor, or (b)
spawn/Task.start_unlinked an intermediary starter that calls
WebSocketClient.start_link so the WebSocket is not linked to this GenServer;
ensure the retry scheduling logic (schedule_websocket_retry) still runs if the
external start attempt fails.
In `@lib/wanderer_notifier/map/sse_supervisor.ex`:
- Around line 204-214: The function initialize_maps/0 should always return :ok;
after calling signal_pipeline_worker() and
start_sse_clients_staggered(successful_maps) call
schedule_failed_map_retries(failed_maps, 1) but do not return its raw
Task.Supervisor.start_child/2 result—instead pattern-match its result, log any
{:error, reason} or unexpected reply (use the module logger or existing
processLogger) and ensure initialize_maps/0 ends with :ok; apply the same change
to the other occurrence (lines ~382-395) so scheduler failures are explicitly
logged but do not change the public return value.
- Around line 398-417: The retry_failed_maps/2 function currently tries to
initialize maps from a captured batch that may include maps removed in the
meantime; before calling initialize_and_start_for_map/1 filter the incoming maps
against the current registry returned by Dependencies.map_registry() (e.g. fetch
live slugs or membership and then Enum.filter maps by &(&1.slug in live_slugs))
so you only retry maps that still exist, and then proceed with the existing
split/logging and schedule_failed_map_retries/2 behavior; update references to
MapConfig.slug, retry_failed_maps/2, initialize_and_start_for_map/1,
Dependencies.map_registry(), and schedule_failed_map_retries/2 accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85ac804b-d6da-4032-9d18-9f91c1a3a9c9
📒 Files selected for processing (2)
lib/wanderer_notifier/domains/killmail/supervisor.exlib/wanderer_notifier/map/sse_supervisor.ex
There was a problem hiding this comment.
♻️ Duplicate comments (3)
lib/wanderer_notifier/map/sse_supervisor.ex (2)
392-415:⚠️ Potential issue | 🟠 MajorRe-fetch live map configs before retrying.
The task sleeps in Lines 393-394, but Line 404 still retries the captured
%MapConfig{}list. If a map is removed or updated during that backoff window, this can recreate SSE state for a deleted map or retry against stale config.Proposed fix
defp retry_failed_maps(maps, attempt) do - Logger.info("Retrying initialization for #{length(maps)} map(s)", + active_maps = + Enum.flat_map(maps, fn %{slug: slug} -> + case Dependencies.map_registry().get_map(slug) do + {:ok, map_config} -> [map_config] + {:error, _} -> [] + end + end) + + Logger.info("Retrying initialization for #{length(active_maps)} map(s)", attempt: attempt, category: :startup ) - results = Enum.map(maps, fn map -> {map, initialize_and_start_for_map(map)} end) + results = Enum.map(active_maps, fn map -> {map, initialize_and_start_for_map(map)} end) {succeeded, still_failed} = Enum.split_with(results, fn {_map, result} -> match?({:ok, _}, result) end) if succeeded != [] do slugs = Enum.map(succeeded, fn {map, _} -> map.slug end) Logger.info("Map initialization retry succeeded", map_slugs: slugs, category: :startup) end failed_maps = Enum.map(still_failed, fn {map, _} -> map end) schedule_failed_map_retries(failed_maps, attempt + 1) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/wanderer_notifier/map/sse_supervisor.ex` around lines 392 - 415, The retry logic in retry_failed_maps currently uses the stale maps list captured earlier; before calling initialize_and_start_for_map you should re-fetch current map configs (e.g. look up each map.slug or fetch all live MapConfig entries) and only attempt initialization for configs that still exist, skipping/logging maps that were removed or changed; update the code in retry_failed_maps to map slugs -> fetch fresh configs, call initialize_and_start_for_map with the fresh config, and then pass the remaining failures into schedule_failed_map_retries (keep function names retry_failed_maps, initialize_and_start_for_map, and schedule_failed_map_retries for easy location).
204-213:⚠️ Potential issue | 🟠 MajorKeep
initialize_sse_clients/0returning:ok.Line 213 still returns the raw
schedule_failed_map_retries/2result, and Lines 392-395 still bubble upTask.Supervisor.start_child/2on partial startup failures. That changesinitialize_sse_clients/0from its declared:okcontract to{:ok, pid} | {:error, reason}exactly when map initialization is already degraded.Proposed fix
defp initialize_maps do maps = Dependencies.map_registry().all_maps() Logger.info("Initializing #{length(maps)} maps from registry", category: :startup) {successful_maps, failed_maps} = run_parallel_init(maps) # Signal PipelineWorker signal_pipeline_worker() # Start SSE clients only for successfully initialized maps start_sse_clients_staggered(successful_maps) # Schedule retry for failed maps - schedule_failed_map_retries(failed_maps, 1) + schedule_failed_map_retries(failed_maps, 1) + :ok end @@ defp schedule_failed_map_retries(failed_maps, attempt) do delay = calculate_init_retry_delay(attempt) slugs = Enum.map(failed_maps, & &1.slug) Logger.info( "Scheduling retry #{attempt}/#{`@max_init_retries`} for #{length(failed_maps)} failed map(s) in #{delay}ms", map_slugs: slugs, category: :startup ) - Task.Supervisor.start_child(WandererNotifier.TaskSupervisor, fn -> - Process.sleep(delay) - retry_failed_maps(failed_maps, attempt) - end) + case Task.Supervisor.start_child(WandererNotifier.TaskSupervisor, fn -> + Process.sleep(delay) + retry_failed_maps(failed_maps, attempt) + end) do + {:ok, _pid} -> + :ok + + {:error, reason} -> + Logger.error("Failed to schedule map initialization retry", + map_slugs: slugs, + attempt: attempt, + reason: inspect(reason), + category: :startup + ) + + :ok + end endAlso applies to: 382-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/wanderer_notifier/map/sse_supervisor.ex` around lines 204 - 213, initialize_sse_clients/0 must always return :ok; after calling run_parallel_init(maps), signal_pipeline_worker(), start_sse_clients_staggered(successful_maps) and schedule_failed_map_retries(failed_maps, 1), ignore and do not return the raw result of schedule_failed_map_retries/2—explicitly return :ok. In start_sse_clients_staggered/1 (and any code paths that call Task.Supervisor.start_child/2 around lines referenced), catch failures from Task.Supervisor.start_child/2 (use case/try/rescue or handle_info) so partial startup errors are logged/handled locally and do not bubble up as {:error, reason} from initialize_sse_clients/0; ensure any Task.Supervisor.start_child results are normalized/logged and the outer initialize_sse_clients/0 still returns :ok.lib/wanderer_notifier/domains/killmail/supervisor.ex (1)
81-94:⚠️ Potential issue | 🔴 CriticalDon't
start_link/0the WebSocket directly from this GenServer.Lines 82 and 139 still start the socket via
WebSocketClient.start_link/0, which links the WebSocket process to this GenServer. Because this module never traps exits, a later socket crash will take down the Killmail supervisor itself instead of being isolated under__MODULE__.InternalSupervisor.Use the internal supervisor (or another unlinked starter) for the WebSocket start path, then keep the retry scheduling around that call.
Run this to verify the direct
start_linkpath and the absence of exit trapping in the current tree. Expected result:WebSocketClient.start_linkis called from this module, and notrap_exithandling exists here.#!/bin/bash set -euo pipefail echo "=== Killmail supervisor start/retry path ===" sed -n '80,155p' lib/wanderer_notifier/domains/killmail/supervisor.ex echo echo "=== Link/trap-exit evidence ===" rg -n 'WebSocketClient\.start_link|trap_exit|InternalSupervisor' \ lib/wanderer_notifier/domains/killmail/supervisor.ex \ lib/wanderer_notifier/domains/killmail/websocket_client.exAlso applies to: 136-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/wanderer_notifier/domains/killmail/supervisor.ex` around lines 81 - 94, The GenServer currently calls WebSocketClient.start_link/0 directly (via start_websocket_client()) which links the socket to this process; instead, start the socket under the supervisor __MODULE__.InternalSupervisor (or via Supervisor.start_child/2/DynamicSupervisor) so the socket is isolated from this GenServer — replace direct calls to WebSocketClient.start_link/0 in handle_continue(:start_websocket) and the retry path (the places calling start_websocket_client and schedule_websocket_retry(state, 0)) with a function that starts the child through __MODULE__.InternalSupervisor (e.g., build the child_spec for WebSocketClient and call Supervisor.start_child or DynamicSupervisor.start_child on __MODULE__.InternalSupervisor) and keep the existing retry scheduling and {:noreply, state} / {:noreply, schedule_websocket_retry(...)} return handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/wanderer_notifier/domains/killmail/supervisor.ex`:
- Around line 81-94: The GenServer currently calls WebSocketClient.start_link/0
directly (via start_websocket_client()) which links the socket to this process;
instead, start the socket under the supervisor __MODULE__.InternalSupervisor (or
via Supervisor.start_child/2/DynamicSupervisor) so the socket is isolated from
this GenServer — replace direct calls to WebSocketClient.start_link/0 in
handle_continue(:start_websocket) and the retry path (the places calling
start_websocket_client and schedule_websocket_retry(state, 0)) with a function
that starts the child through __MODULE__.InternalSupervisor (e.g., build the
child_spec for WebSocketClient and call Supervisor.start_child or
DynamicSupervisor.start_child on __MODULE__.InternalSupervisor) and keep the
existing retry scheduling and {:noreply, state} / {:noreply,
schedule_websocket_retry(...)} return handling.
In `@lib/wanderer_notifier/map/sse_supervisor.ex`:
- Around line 392-415: The retry logic in retry_failed_maps currently uses the
stale maps list captured earlier; before calling initialize_and_start_for_map
you should re-fetch current map configs (e.g. look up each map.slug or fetch all
live MapConfig entries) and only attempt initialization for configs that still
exist, skipping/logging maps that were removed or changed; update the code in
retry_failed_maps to map slugs -> fetch fresh configs, call
initialize_and_start_for_map with the fresh config, and then pass the remaining
failures into schedule_failed_map_retries (keep function names
retry_failed_maps, initialize_and_start_for_map, and schedule_failed_map_retries
for easy location).
- Around line 204-213: initialize_sse_clients/0 must always return :ok; after
calling run_parallel_init(maps), signal_pipeline_worker(),
start_sse_clients_staggered(successful_maps) and
schedule_failed_map_retries(failed_maps, 1), ignore and do not return the raw
result of schedule_failed_map_retries/2—explicitly return :ok. In
start_sse_clients_staggered/1 (and any code paths that call
Task.Supervisor.start_child/2 around lines referenced), catch failures from
Task.Supervisor.start_child/2 (use case/try/rescue or handle_info) so partial
startup errors are logged/handled locally and do not bubble up as {:error,
reason} from initialize_sse_clients/0; ensure any Task.Supervisor.start_child
results are normalized/logged and the outer initialize_sse_clients/0 still
returns :ok.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a22b5f6c-713b-4464-be1d-1802771f38ca
📒 Files selected for processing (2)
lib/wanderer_notifier/domains/killmail/supervisor.exlib/wanderer_notifier/map/sse_supervisor.ex
- Start WebSocket as child of InternalSupervisor instead of linking directly to the GenServer — prevents WebSocket crash from taking down the Killmail Supervisor - Ensure initialize_maps/0 and schedule_failed_map_retries/2 always return :ok; log Task.Supervisor.start_child failures explicitly - Filter retry batch against current MapRegistry to skip maps removed since last attempt - Extract filter_live_maps/1 and attempt_map_initialization/1 to satisfy credo ABC complexity limit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of filtering stale MapConfig structs by slug existence, re-fetch current configs from MapRegistry on each retry attempt. This ensures retries use up-to-date settings if a map's config changed between attempts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/wanderer_notifier/map/sse_supervisor.ex`:
- Around line 204-215: The current flow only adds maps that failed during
run_parallel_init/1 (i.e., initialize_map_data_safely/1 failures) to
schedule_failed_map_retries/2, but maps that fail later in
start_sse_client_for_map/1 (called by start_sse_clients_staggered/1) are merely
logged and never retried; modify start_sse_clients_staggered/1 so it returns the
list of %MapConfig{} entries whose SSE startup failed (preserving map context in
the Task.async_stream results), then merge that returned list with the
failed_maps from run_parallel_init/1 before calling
schedule_failed_map_retries/2 (i.e., call
start_sse_clients_staggered(successful_maps) -> {ok_started, sse_failed_maps}
and pass failed_maps ++ sse_failed_maps to schedule_failed_map_retries/2) so all
startup failures are retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92236350-59fe-47e3-a3e4-f2c5fd765026
📒 Files selected for processing (2)
lib/wanderer_notifier/domains/killmail/supervisor.exlib/wanderer_notifier/map/sse_supervisor.ex
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/wanderer_notifier/map/sse_supervisor.ex (1)
210-213:⚠️ Potential issue | 🟠 MajorSSE startup failures are still excluded from the retry queue.
initialize_maps/0only schedules retries forfailed_mapsfrom data initialization. Failures fromstart_sse_clients_staggered/1are logged but dropped, so those maps can still stay down until an external restart.Proposed fix
defp initialize_maps do maps = Dependencies.map_registry().all_maps() Logger.info("Initializing #{length(maps)} maps from registry", category: :startup) {successful_maps, failed_maps} = run_parallel_init(maps) # Signal PipelineWorker signal_pipeline_worker() # Start SSE clients only for successfully initialized maps - start_sse_clients_staggered(successful_maps) + sse_failed_maps = start_sse_clients_staggered(successful_maps) # Schedule retry for failed maps - schedule_failed_map_retries(failed_maps, 1) + schedule_failed_map_retries(failed_maps ++ sse_failed_maps, 1) :ok end defp start_sse_clients_staggered(maps) do results = maps - |> Task.async_stream(&start_sse_client_for_map/1, + |> Task.async_stream(fn map -> {map, start_sse_client_for_map(map)} end, max_concurrency: 5, timeout: 30_000 ) |> Enum.to_list() {succeeded, failed} = Enum.split_with(results, fn - {:ok, {:ok, _pid}} -> true + {:ok, {_map, {:ok, _pid}}} -> true _ -> false end) succeeded_count = length(succeeded) failed_count = length(failed) + failed_maps = + Enum.flat_map(failed, fn + {:ok, {%MapConfig{} = map, {:error, reason}}} -> + Logger.warning("SSE client startup failed", + map_slug: map.slug, + reason: inspect(reason) + ) + [map] + + {:exit, reason} -> + Logger.warning("SSE client startup task exited", reason: inspect(reason)) + [] + + _ -> + [] + end) + if failed_count > 0 do - Enum.each(failed, fn - {:ok, {:error, reason}} -> - Logger.warning("SSE client startup failed", reason: inspect(reason)) - - {:exit, reason} -> - Logger.warning("SSE client startup task exited", reason: inspect(reason)) - - _ -> - :ok - end) - Logger.warning( "SSE client startup: #{succeeded_count} succeeded, #{failed_count} failed out of #{length(maps)}", category: :startup ) else Logger.info("Started #{succeeded_count} SSE clients", category: :startup) end + + failed_maps endAlso applies to: 303-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/wanderer_notifier/map/sse_supervisor.ex` around lines 210 - 213, initialize_maps/0 currently only calls schedule_failed_map_retries/2 with the failed_maps returned from data initialization and ignores any maps that fail during start_sse_clients_staggered/1; modify initialize_maps/0 (and the related logic in the start_sse_clients_staggered flow) to collect and return or forward any start-time failures (e.g., a failed_sse_maps list) and merge them with the existing failed_maps before calling schedule_failed_map_retries/2 so that maps which fail while starting SSE clients are enqueued for retry; apply the same change for the analogous code region around the 303-340 block so all SSE startup failures are scheduled for retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/wanderer_notifier/map/sse_supervisor.ex`:
- Around line 417-432: Refactor retry_failed_maps/2 into two function heads
using pattern matching: add a clause retry_failed_maps([], attempt) that logs
the "All previously failed maps..." message with attempt and category and
returns :ok, and a clause retry_failed_maps(maps, attempt) that logs "Retrying
initialization..." with length(maps), calls attempt_map_initialization(maps) to
get failed_maps and then calls schedule_failed_map_retries(failed_maps, attempt
+ 1); preserve the existing Logger.info messages, argument order and behavior of
attempt_map_initialization/1 and schedule_failed_map_retries/2.
---
Duplicate comments:
In `@lib/wanderer_notifier/map/sse_supervisor.ex`:
- Around line 210-213: initialize_maps/0 currently only calls
schedule_failed_map_retries/2 with the failed_maps returned from data
initialization and ignores any maps that fail during
start_sse_clients_staggered/1; modify initialize_maps/0 (and the related logic
in the start_sse_clients_staggered flow) to collect and return or forward any
start-time failures (e.g., a failed_sse_maps list) and merge them with the
existing failed_maps before calling schedule_failed_map_retries/2 so that maps
which fail while starting SSE clients are enqueued for retry; apply the same
change for the analogous code region around the 303-340 block so all SSE startup
failures are scheduled for retry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96d3cb6e-66c0-4a14-a07e-5ea74c9a7e94
📒 Files selected for processing (1)
lib/wanderer_notifier/map/sse_supervisor.ex
- start_sse_clients_staggered now returns maps that failed SSE client startup; these are merged with data-init failures before scheduling retries, so no failure path is silently dropped - retry_failed_maps split into two function heads ([], attempt) and (maps, attempt) with fetch_fresh_configs moved to the caller - Extract log_sse_startup_results/3 to satisfy credo ABC limit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When upstream services are temporarily unreachable during app startup, both WebSocket and SSE clients would fail their initial connection with no recovery path — leaving the app in a permanently broken state until manually restarted.
WebSocket: Replace fire-and-forget Task with retry loop using exponential backoff (5s–60s) in the Killmail Supervisor GenServer.
SSE: Retry failed map initialization up to 5 times with exponential backoff (10s–60s) via TaskSupervisor, re-attempting initialize_and_start_for_map for each failed map.
Summary by CodeRabbit
New Features
Improvements
Chores