Skip to content

Close a window of opportunity in ElsaRegistry #19

Merged
nathanmonteleone merged 1 commit into
mainfrom
fix_registry_race_condition
Jun 16, 2026
Merged

Close a window of opportunity in ElsaRegistry #19
nathanmonteleone merged 1 commit into
mainfrom
fix_registry_race_condition

Conversation

@nathanmonteleone

@nathanmonteleone nathanmonteleone commented Jun 16, 2026

Copy link
Copy Markdown

This should take care of the random startup crashes we've had for like a year now.

https://simplifi.atlassian.net/browse/INT-11129

I found this using Claude. The comments in the actual committed code are my own paraphrasing; please let me know if they're too wordy or get anything wrong.

Note this was not a severe bug to begin with -- the supervisor tree usually just restarted everything and we would reconnect fine. But it might help alleviate some confusion in the future to have this fixed.

Original prompt and feedback:

> Sometimes on startup this library will give an error like
   Child {:elsa_registry_exuser_update_service_reader, :worker_dynamic_supervisor} of Supervisor {:elsa_registry_exuser_updat>
  Apr 03 16:05:50 user-update-service-fcap-prd-asia-southeast1-a-qbdd exuser_update_service[8918]: ** (exit) already started: #PID<0.1474.0>

Can you figure out why that might happen?
  
<snip>

There are two bugs that interact to produce this error. Here's the breakdown:

---
Bug 1 (direct crash): wait_for/1 calls the wrong whereis

dynamic_process_manager.ex:146 — the private wait_for/1 function calls Process.whereis(name) directly:

defp wait_for(name) do
  case Process.whereis(name) do   # <-- bug

But name is state.dynamic_supervisor, which is a :via tuple like {:via, ElsaRegistry, {registry, DynamicSupervisor}}. Erlang's erlang:whereis/1 only accepts atoms — passing a tuple raises badarg. The module already has a correct private whereis/1 helper (lines 138–144) that dispatches on :via tuples, but wait_for doesn't use it.

Result: any time the monitored DynamicSupervisor dies, handle_info/2 at line 85 calls wait_for, crashes immediately with badarg, and the DynamicProcessManager itself exits unexpectedly.

---
Bug 2 (the "already started" race): ElsaRegistry cleanup is async

elsa_registry.ex:84–115 — when a registered process dies, ElsaRegistry removes it from ETS inside handle_info({:EXIT, pid, _}, ...). That cleanup is asynchronous — it only runs when ElsaRegistry's mailbox gets to that message.

Meanwhile, whereis_name/1 reads from ETS synchronously. There's a window between when a process dies and when ElsaRegistry removes its entry. During that window, OTP's :via protocol calls whereis_name, sees the (dead) old PID, and returns {:error, {:already_started, dead_pid}} to the process trying to start.

The processes most likely to hit this are any :via-named processes that get restarted in rapid succession: WorkerSupervisor, Manager, Acknowledger.

---
How they combine

The typical startup scenario:
1. GroupSupervisor starts WorkerSupervisor + Manager
2. Manager.handle_continue calls start_group_coordinator — this can fail transiently if the brod client or Kafka isn't fully ready yet
3. Manager stops → GroupSupervisor (:one_for_all) immediately kills and restarts both children
4. WorkerSupervisor died but ElsaRegistry hasn't processed its EXIT yet
5. New WorkerSupervisor.start_link calls whereis_name → ETS returns dead PID → "already started"

Bug 1 makes this worse: if the DynamicSupervisor ever dies, the DPM crashes too, ElsaSupervisor's :rest_for_one terminates everything downstream simultaneously, and the flood of EXIT signals to ElsaRegistry creates a larger window for the race.


defp wait_for(name) do
case Process.whereis(name) do
case whereis(name) do

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a fairly clear oversight from the original library -- there's a whereis override that handles ":via" lookups, but it was only being used on startup and was forgotten about here.

case registry_module.whereis_name(lookup) do
:undefined -> nil
pid -> pid
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this bit just so that we'd be consistent within this module, instead of having a mixture of ":undefined" and "nil" for not-found processes.

@nathanmonteleone
nathanmonteleone merged commit d0877a9 into main Jun 16, 2026
3 checks passed
@nathanmonteleone
nathanmonteleone deleted the fix_registry_race_condition branch June 16, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants