I really think that you want to use names here instead of always going through the supervisor. This removes a bottleneck from your system and makes it easier to reason about crashes. I also want to add some nuance to what @tomekowal is saying about state.

Processes absolutely can crash for no reason. This is typically do to a supervisor being restarted due to an unrelated crash. For instance your worker and agent may be working fine but their supervisor is managed by a supervisor with an all_for_one strategy. If one of your supervisors siblings crashes then your worker and agent will be restarted as well. This is pretty rare but is worth keeping in mind.

But @tomekowal’s main point is correct. Most of the time a process will crash because it ends up in a bad state. When that happens it’s better to just allow the process to crash and come back in a good state.

The main issue with the way you’ve built your system currently is that the worker is responsible for pushing state into the agent. What that means is that if your worker state is bad than you’ll push bad state into the agent, and the agent will crash. This will continue happening repeatedly because the bad state is never being cleaned up. We aren’t allowing the agent to come back up in a known good state. These kinds of bugs crop up all the time especially when people intermingle persistence with their process state. The bad state is persisted somewhere, process crashes, process restarts and loads data into memory, next message it crashes again, etc.

What I try to do is to isolate my state to a single process as much as possible. I then send commands to that process and allow that process to update its own internal state. If something gets into a bad state the crash will be isolated and I can restart in a good state.

Here’s how I would re-write what you have so far: https://gist.github.com/keathley/36e536e7b55d7444752bd29fb8d25d4d

I’d just like to mention a good blog post on state isolation. It introduces the idea of an “error kernel”:

Erlang programs have a concept called the error kernel . The kernel is the part of the program which must be correct for its correct operation. Good Erlang design begins with identifying the error kernel of the system: What part must not fail or it will bring down the whole system? Once you have the kernel identified, you seek to make it minimal. Whenever the kernel is about to do an operation which is dangerous and might crash, you “outsource” that computation to another process, a dumb slave worker. If he crashes and is killed, nothing really bad has happened - since the kernel keeps going.

I think @jola is correct here. You want to name these processes somehow in order to look them up. Otherwise when your polling process crashes it won’t have access to the existing state. So you can either use a registry or give them well defined names. Without knowing more about your problem I’d build something similar to this:

defmodule Authenticator do
  def child_spec(args) do
    children = [
      Worker,
      {State, name: args[:name]},
    ]

    %{
      id: __MODULE__,
      type: :supervisor,
      start: {Supervisor, :start_link, [children, [strategy: :one_for_one]]}
    }
  end

  def lookup(name) do
    Agent.get(name, & &1)
  end
end

Now you can add Authenticator to your supervision tree. When you need to look things up it’ll go through the authenticator module and the details of how things are stored and accessed can be hidden away. For instance if you decide to go the ETS route because you need concurrent reads then this will be encapsulated in Authenticator and your callers won’t have to change.

• a :public table can only be accessed “globally” if it is named - otherwise any process accessing it has to somehow have to get ahold of the table ID. So it isn’t uncommon for a supervisor to create a public table for one of its child processes and hand the child process the table id. Then each time the process is restarted it takes over the existing table.

• with protected tables you can play the heir - give_away game. A simple owner process transfers ownership to a requesting process but gets it back when that process dies.

Demo script:

# file: lib/demo.ex
#
defmodule Demo do
  def run do
    cycle(3)
  end

  defp cycle(n) when n < 1 do
    :ok
  end

  defp cycle(n) do
    increment(3)
    pid = kill_and_wait()
    if(pid, do: cycle(n - 1), else: :error)
  end

  defp increment(n) when n < 1 do
    :ok
  end

  defp increment(n) do
    increment()
    increment(n - 1)
  end

  defp increment do
    {:count, value} = DontLose.Counter.increment()
    IO.puts("#{value}")
  end

  defp kill_and_wait() do
    name = DontLose.Counter
    pid = Process.whereis(name)
    ref = Process.monitor(pid)
    Process.exit(pid, :kill)

    receive do
      {:DOWN, ^ref, :process, ^pid, :killed} ->
        :ok
    end

    wait(name, 10, 10)
  end

  defp wait(_, _, left) when left < 1 do
    nil
  end

  defp wait(name, timeout, left) do
    case Process.whereis(name) do
      nil ->
        Process.sleep(timeout)
        wait(name, timeout, left - 1)

      pid ->
        pid
    end
  end
end

Demo session:

Interactive Elixir (1.8.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> Demo.run()
1
2
3
4
5
6
7
8
9
:ok
iex(2)> 

Public table:

# file: lib/dont_lose/application.ex
#
defmodule DontLose.Application do
  use Application

  def start(_type, _args) do
    DontLose.Supervisor.start_link([])
  end
end

# file: lib/dont_lose/supervisor.ex
#
defmodule DontLose.Supervisor do
  use Supervisor

  def start_link(init_arg) do
    Supervisor.start_link(__MODULE__, init_arg, name: __MODULE__)
  end

  @impl true
  def init(_init_arg) do
    table = :ets.new(:counter_storage, [:set, :public])
    :ets.insert(table, {:counter, 0})

    children = [
      {DontLose.Counter, table}
    ]

    Supervisor.init(children, strategy: :one_for_one)
  end
end

# file: lib/dont_lose/counter.ex
#
defmodule DontLose.Counter do
  use GenServer

  def start_link(table) do
    GenServer.start_link(__MODULE__, table, name: __MODULE__)
  end

  @impl true
  def init(table) do
    # retrieve value from backup
    [{:counter, count}] = :ets.lookup(table, :counter)
    {:ok, {table, count}}
  end

  @impl true
  def handle_call(:increment, _from, {table, count}) do
    new_count = count + 1
    # backup value
    :ets.insert(table, {:counter, new_count})

    {:reply, {:count, new_count}, {table, new_count}}
  end

  # --- API
  def increment,
    do: GenServer.call(__MODULE__, :increment)
end

Protected “heir” table:

# file: lib/dont_lose/application.ex
#
defmodule DontLose.Application do
  use Application

  def start(_type, _args) do
    supervisor = DontLose.Supervisor

    children = [
      {DontLose.Keeper, nil},
      {DontLose.Counter, supervisor}
    ]

    opts = [strategy: :rest_for_one, name: supervisor]
    Supervisor.start_link(children, opts)
  end
end

# file: lib/dont_lose/keeper.ex
#
defmodule DontLose.Keeper do
  use GenServer

  def start_link(args) do
    GenServer.start_link(__MODULE__, args)
  end

  @impl true
  def init(_args) do
    # create and initialize table
    table = :ets.new(:counter_storage, [:set, :protected, {:heir, self(), :counter_heir}])
    :ets.insert(table, {:counter, 0})

    {:ok, table}
  end

  @impl true
  def handle_call({:request_table, pid}, _from, table) when not is_nil(table) do
    :ets.give_away(table, pid, :counter_transfer)
    {:reply, :ok, nil}
  end

  @impl true
  def handle_info({:"ETS-TRANSFER", table, _pid, :counter_heir}, _state) do
    {:noreply, table}
  end

  # --- API
  def request_table(keeper, pid),
    do: GenServer.call(keeper, {:request_table, pid})
end

# file: lib/dont_lose/counter.ex
#
defmodule DontLose.Counter do
  use GenServer

  alias DontLose.Keeper

  def start_link(sup) do
    GenServer.start_link(__MODULE__, sup, name: __MODULE__)
  end

  @impl true
  def init(sup) do
    state = []
    {:ok, state, {:continue, sup}}
  end

  @impl true
  def handle_continue(sup, _state) do
    children = Supervisor.which_children(sup)

    case find_keeper(children) do
      nil ->
        {:stop, :no_keeper, nil}

      pid ->
        # request table from keeper
        :ok = Keeper.request_table(pid, self())
        {:noreply, nil}
    end
  end

  @impl true
  def handle_call(:increment, _from, {table, count}) do
    new_count = count + 1
    # backup value
    :ets.insert(table, {:counter, new_count})

    {:reply, {:count, new_count}, {table, new_count}}
  end

  @impl true
  def handle_info({:"ETS-TRANSFER", table, _pid, :counter_transfer}, _state) do
    # retrieve current count
    [{:counter, count}] = :ets.lookup(table, :counter)
    {:noreply, {table, count}}
  end

  defp find_keeper([]) do
    nil
  end

  defp find_keeper([{DontLose.Keeper, pid, :worker, _modules} | _tail]) do
    pid
  end

  defp find_keeper([_ | tail]) do
    find_keeper(tail)
  end

  # --- API
  def increment,
    do: GenServer.call(__MODULE__, :increment)
end

The above is for simple demonstration only as not all edge cases are covered.

Ultimately that is my beef with Agents - their state is entirely at the mercy of the functions that are sent to them by other processes. At least with a GenServer it can be easily enforced that interactions with the state are simple.

Aside: In Programming Elixir 1.3 - Chapter 18: OTP Supervisors

This tree structure was used:

Supervisor
|__ Stash
|__ SubSupervisor
    |__ Server

https://media.pragprog.com/titles/elixir13/code/otp-supervisor/2/sequence/lib/sequence.ex
https://media.pragprog.com/titles/elixir13/code/otp-supervisor/2/sequence/lib/sequence/supervisor.ex
https://media.pragprog.com/titles/elixir13/code/otp-supervisor/2/sequence/lib/sequence/stash.ex
https://media.pragprog.com/titles/elixir13/code/otp-supervisor/2/sequence/lib/sequence/sub_supervisor.ex
https://media.pragprog.com/titles/elixir13/code/otp-supervisor/2/sequence/lib/sequence/server.ex

In Programming Elixir 1.6 that was replaced with a :rest_for_one strategy on

Supervisor
|__ Stash
|__ Server

https://media.pragprog.com/titles/elixir16/code/otp-supervisor/2/sequence/lib/sequence/application.ex

The primary issue with the server code was that the stash was only updated in the terminate callback which won’t always run. The backing store needs to be updated whenever we are certain that the new state is consistent.