Fix supervisor restart when child fails to match OTP

UncleGrumpy · UncleGrumpy · commit d081272b9bc7 · 2025-11-13T20:07:48.000Z
Fixes a function clause exception that would crash the supervisor if a child fails to restart by matching the same behavior as OTP, and continue to try restarting the child until success, or maximum `intensity` is reached within the allowed `period`. Closed #1957 Signed-off-by: Winford <winford@object.stream>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -93,6 +93,7 @@ instead `badarg`.
 - Fix `list_to_integer`, it was likely buggy with integers close to INT64_MAX
 - Added missing support for supervisor `one_for_all` strategy.
 - Supervisor now honors period and intensity options.
+- Fix supervisor crash if a `one_for_one` child fails to restart.
 
 ## [0.6.7] - Unreleased
 
diff --git a/libs/estdlib/src/supervisor.erl b/libs/estdlib/src/supervisor.erl
@@ -292,6 +292,8 @@ handle_info({'EXIT', Pid, Reason}, State) ->
         {ok, State1, restart_all_children} ->
             Children = State1#state.children,
             {noreply, State1, {timeout, 0, {restart_many_children, Children}}};
+        {ok, State1, {try_again_restart, ChildId}} ->
+            {noreply, State1, {timeout, 0, {try_again_restart, ChildId}}};
         {shutdown, State1} ->
             {stop, shutdown, State1}
     end;
@@ -305,17 +307,65 @@ handle_info({ensure_killed, Pid}, State) ->
     end;
 handle_info({restart_many_children, []}, State) ->
     {noreply, State};
-handle_info({restart_many_children, [#child{id = Id} = Child | Children] = ChildSpecs}, State) ->
+handle_info({restart_many_children, [#child{id = Id} = Child | Siblings] = _ChildSpecs}, State) ->
     case try_start(Child) of
         {ok, NewPid, _Result} ->
             NewChild = Child#child{pid = NewPid},
             NewChildren = lists:keyreplace(
                 Id, #child.id, State#state.children, NewChild
             ),
             {noreply, State#state{children = NewChildren},
-                {timeout, 0, {restart_many_children, Children}}};
+                {timeout, 0, {restart_many_children, Siblings}}};
         {error, _Reason} ->
-            {noreply, State, {timeout, 0, {restart_many_children, ChildSpecs}}}
+            case Siblings of
+                [] ->
+                    {noreply, State, {timeout, 0, {try_again_restart, Id}}};
+                Siblings ->
+                    {noreply, State, {timeout, 0, {try_again_restart_continue, Child, Siblings}}}
+            end
+    end;
+handle_info({try_again_restart, Id}, State) ->
+    case lists:keyfind(Id, #child.id, State#state.children) of
+        false ->
+            {noreply, State};
+        Child ->
+            case add_restart(State) of
+                {ok, State1} ->
+                    case try_start(Child) of
+                        {ok, NewPid, _Result} ->
+                            UpdatedChildren = lists:keyreplace(
+                                Id, Child#child.id, State1#state.children, Child#child{pid = NewPid}
+                            ),
+                            {noreply, State1#state{children = UpdatedChildren}};
+                        {error, {_, _}} ->
+                            {noreply, State1, {timeout, 0, {try_again_restart, Id}}}
+                    end;
+                {shutdown, State1} ->
+                    RemainingChildren = lists:keydelete(Id, #child.id, State1#state.children),
+                    {stop, shutdown, State1#state{children = RemainingChildren}}
+            end
+    end;
+handle_info({try_again_restart_continue, #child{id = Id} = Child, Siblings}, State) ->
+    case lists:keyfind(Id, #child.id, State#state.children) of
+        false ->
+            {noreply, State, {timeout, 0, {restart_many_children, Siblings}}};
+        Child ->
+            case add_restart(State) of
+                {ok, State1} ->
+                    case try_start(Child) of
+                        {ok, NewPid, _Result} ->
+                            UpdatedChildren = lists:keyreplace(
+                                Child#child.id, #child.id, State1#state.children, Child#child{pid = NewPid}
+                            ),
+                            {noreply, State1#state{children = UpdatedChildren},
+                                {timeout, 0, {restart_many_children, Siblings}}};
+                        {error, {_, _}} ->
+                            {noreply, State1, {timeout, 0, {try_again_restart_continue, Child, Siblings}}}
+                    end;
+                {shutdown, State1} ->
+                    RemainingChildren = lists:keydelete(Child#child.id, #child.id, State1#state.children),
+                    {stop, shutdown, State1#state{children = RemainingChildren}}
+            end
     end;
 handle_info(_Msg, State) ->
     %TODO: log unexpected message
@@ -372,7 +422,13 @@ handle_restart_strategy(
             Children = lists:keyreplace(
                 Id, #child.id, State#state.children, NewChild
             ),
-            {ok, State#state{children = Children}}
+            {ok, State#state{children = Children}};
+        {error, _} ->
+            NewChild = Child#child{pid = {restarting, Child#child.pid}},
+            Children = lists:keyreplace(
+                Id, #child.id, State#state.children, NewChild
+            ),
+            {ok, State#state{children = Children}, {try_again_restart, Id}}
     end;
 handle_restart_strategy(
     #child{pid = Pid} = Child, #state{restart_strategy = one_for_all} = State
diff --git a/tests/libs/estdlib/test_supervisor.erl b/tests/libs/estdlib/test_supervisor.erl
@@ -37,6 +37,7 @@ test() ->
     ok = test_count_children(),
     ok = test_one_for_all(),
     ok = test_crash_limits(),
+    ok = try_again_restart(),
     ok.
 
 test_basic_supervisor() ->
@@ -220,7 +221,51 @@ child_start({trap_exit, Parent}) ->
             ok -> ok
         end
     end),
-    {ok, Pid}.
+    {ok, Pid};
+child_start({get_permission, Arbitrator, Parent}) ->
+    Arbitrator ! {can_start, self()},
+    CanStart =
+        receive
+            {do_start, Start} -> Start
+        after 2000 ->
+            {timeout, arbitrator}
+        end,
+    case CanStart of
+        true ->
+            Pid = spawn_link(fun() ->
+                receive
+                    stop -> exit(normal)
+                end
+            end),
+            register(crashy, Pid),
+            Parent ! Pid,
+            {ok, Pid};
+        false ->
+            {error, start_denied};
+        Error ->
+            {error, Error}
+    end.
+
+arbitrator_start(Deny) when is_integer(Deny) ->
+    receive
+        {can_start, From} ->
+            From ! {do_start, true}
+    end,
+    arbitrator(Deny).
+
+arbitrator(Deny) ->
+    Allow =
+        if
+            Deny =< 0 -> true;
+            true -> false
+        end,
+    receive
+        {can_start, From} ->
+            From ! {do_start, Allow},
+            arbitrator(Deny - 1);
+        shutdown ->
+            ok
+    end.
 
 test_ping_pong(SupPid) ->
     Pid1 = get_and_test_server(),
@@ -334,7 +379,39 @@ init({test_crash_limits, Intensity, Period, Parent}) ->
             modules => [ping_pong_server]
         }
     ],
-    {ok, {#{strategy => one_for_one, intensity => Intensity, period => Period}, ChildSpec}}.
+    {ok, {#{strategy => one_for_one, intensity => Intensity, period => Period}, ChildSpec}};
+init({test_try_again, Arbitrator, Parent}) ->
+    ChildSpec = [
+        #{
+            id => finicky_child,
+            start => {?MODULE, child_start, [{get_permission, Arbitrator, Parent}]},
+            restart => permanent,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [?MODULE]
+        }
+    ],
+    {ok, {#{strategy => one_for_one, intensity => 5, period => 10}, ChildSpec}};
+init({test_retry_one_for_all, Arbitrator, Parent}) ->
+    ChildSpec = [
+        #{
+            id => ping,
+            start => {ping_pong_server, start_link, [Parent]},
+            restart => permanent,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [ping_pong_server]
+        },
+        #{
+            id => crashy_child,
+            start => {?MODULE, child_start, [{get_permission, Arbitrator, Parent}]},
+            restart => permanent,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [?MODULE]
+        }
+    ],
+    {ok, {#{strategy => one_for_all, intensity => 5, period => 10}, ChildSpec}}.
 
 test_supervisor_order() ->
     {ok, SupPid} = supervisor:start_link(?MODULE, {test_supervisor_order, self()}),
@@ -521,3 +598,110 @@ get_ping_pong_pid() ->
         {ping_pong_server_ready, Pid} -> Pid
     after 2000 -> throw(timeout)
     end.
+
+try_again_restart() ->
+    process_flag(trap_exit, true),
+
+    %% Intensity is 5, use the arbitrator to prevent the child from restarting
+    %% 4 times. This should not exit the supervisor due to intensity.
+    Arbitrator1 = erlang:spawn(fun() -> arbitrator_start(4) end),
+    {ok, SupPid1} = supervisor:start_link(
+        {local, try_again_test1}, ?MODULE, {test_try_again, Arbitrator1, self()}
+    ),
+    ChildPid = wait_child_pid("ChildPid"),
+
+    ChildPid ! stop,
+    ChildPid1 = wait_child_pid("ChildPid1"),
+
+    ChildPid1 ! stop,
+    Arbitrator1 ! shutdown,
+    exit(SupPid1, normal),
+    ok =
+        receive
+            {'EXIT', SupPid1, normal} ->
+                ok
+        after 2000 ->
+            error({supervisor_not_stopped, normal})
+        end,
+
+    %% Prevent 5 restart attempts allow on the 6th, this should cause the supervisor
+    %% to shutdown on the 6th attempt, which happens before period expires and we are
+    %% already at max restart intensity.
+    Arbitrator2 = erlang:spawn(fun() -> arbitrator_start(5) end),
+    {ok, SupPid2} = supervisor:start_link(
+        {local, test_try_again2}, ?MODULE, {test_try_again, Arbitrator2, self()}
+    ),
+    ChildPid2 = wait_child_pid("ChildPid2"),
+
+    ChildPid2 ! stop,
+    ok =
+        receive
+            {'EXIT', SupPid2, shutdown} ->
+                ok
+        after 2000 ->
+            error({supervisor_not_stopped, restart_try_again_exceeded})
+        end,
+    Arbitrator2 ! shutdown,
+
+    %% Test one_for_all
+    %%  child 2 uses arbitrator to deny 3 restart attempts, succeeding on the 4th.
+    Arbitrator3 = erlang:spawn(fun() -> arbitrator_start(4) end),
+    {ok, SupPid3} = supervisor:start_link(
+        {local, try_again_test3}, ?MODULE, {test_retry_one_for_all, Arbitrator3, self()}
+    ),
+
+    Ping1 = wait_ping_server(),
+    _Crashy1 = wait_child_pid("Crashy1"),
+
+    %% this will take 5 restarts (1 to restart ping + 3 denied attempts for
+    %% crashy and succeed on the 4th)
+    gen_server:call(Ping1, {stop, normal}),
+
+    Ping2 = wait_ping_server(),
+    %% Crashy will restart without error since the deny count was reached after
+    %% first time it was stopped
+    _Crashy2 = wait_child_pid("Crashy2"),
+
+    %% this will surely exceed the limit
+    %crashy ! stop,
+    gen_server:call(Ping2, {stop, normal}),
+
+    %% ping_pong_server has 2000ms timeout, we need to wait longer.
+    ok =
+        receive
+            {'EXIT', SupPid3, shutdown} ->
+                ok
+        after 5000 ->
+            error({supervisor_not_stopped, one_for_all_restarts_exceeded})
+        end,
+    Arbitrator3 ! shutdown,
+
+    process_flag(trap_exit, false),
+    ok.
+
+wait_ping_server() ->
+    receive
+        {ping_pong_server_ready, Pid} ->
+            Pid;
+        {'EXIT', _, shutdown} ->
+            error({unexpected, supervisor_shutdown});
+        {'EXIT', _, _} = Exit ->
+            %% TODO: remove before final commit, just for debugging what might have
+            %% happend to the the ping_pong gen_server on OTP... \o/
+            io:format("~n>>> TEST wait_ping_server/0 caught exit: ~w~n", [Exit]),
+            wait_ping_server()
+    after 2000 ->
+        error({timeout, no_child_pid, ping_pong_server})
+    end.
+
+wait_child_pid(Name) ->
+    receive
+        Pid when is_pid(Pid) ->
+            Pid;
+        {'EXIT', _, shutdown} ->
+            error({unexpected, supervisor_shutdown});
+        {'EXIT', _, _} ->
+            wait_child_pid(Name)
+    after 1000 ->
+        error({timeout, no_child_pid, Name})
+    end.
