Fix supervisor restart when child fails to match OTP

UncleGrumpy · UncleGrumpy · commit e0f3afd51323 · 2025-11-04T02:00:54.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
@@ -91,6 +91,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 child fails to restart.
 
 ## [0.6.7] - Unreleased
 
diff --git a/libs/estdlib/src/supervisor.erl b/libs/estdlib/src/supervisor.erl
@@ -235,7 +235,12 @@ restart_child(Pid, Reason, #state{restart_strategy = one_for_one} = State) ->
                                     Children = lists:keyreplace(
                                         Pid, #child.pid, State1#state.children, NewChild
                                     ),
-                                    {ok, State1#state{children = Children}}
+                                    {ok, State1#state{children = Children}};
+                                {error, _} ->
+                                    erlang:send_after(
+                                        50, self(), {try_again_restart, Child#child.id}
+                                    ),
+                                    {ok, State1}
                             end;
                         {shutdown, State1} ->
                             RemainingChildren = lists:keydelete(
@@ -271,8 +276,9 @@ restart_child(Pid, Reason, #state{restart_strategy = one_for_all} = State) ->
                             case restart_many_children(Child, Siblings) of
                                 {ok, NewChildren} ->
                                     {ok, State1#state{children = NewChildren}};
-                                _Error ->
-                                    {shutdown, State1}
+                                {ok, NewChildren, RetryID} ->
+                                    erlang:send_after(50, self(), {try_again_restart, RetryID}),
+                                    {ok, State1#state{children = NewChildren}}
                             end;
                         {shutdown, State1} ->
                             RemainingChildren = lists:keydelete(
@@ -380,6 +386,26 @@ handle_info({ensure_killed, Pid}, State) ->
             exit(Pid, kill),
             {noreply, State}
     end;
+handle_info({try_again_restart, Id}, State) ->
+    Child = lists:keyfind(Id, #child.id, State#state.children),
+    case add_restart(State) of
+        {ok, State1} ->
+            case try_start(Child) of
+                {ok, NewPid, _Result} ->
+                    UpdatedChildren = lists:keyreplace(
+                        Id, Child#child.id, State#state.children, Child#child{pid = NewPid}
+                    ),
+                    {noreply, State#state{children = UpdatedChildren}};
+                {error, {_, _}} ->
+                    erlang:send_after(50, self(), {try_again_restart, Id}),
+                    {noreply, State1}
+            end;
+        {shutdown, State1} ->
+            RemainingChildren = lists:keydelete(
+                Id, #child.id, State1#state.children
+            ),
+            {stop, shutdown, State1#state{children = RemainingChildren}}
+    end;
 handle_info(_Msg, State) ->
     %TODO: log unexpected message
     {noreply, State}.
@@ -488,7 +514,7 @@ trim_expired_restarts(_Threshold, Restarts) ->
 restart_many_children(#child{pid = Pid} = Child, Children) ->
     Siblings = lists:keydelete(Pid, #child.pid, Children),
     {ok, Children1} = terminate_many_children(Siblings, [Child#child{pid = {restarting, Pid}}]),
-    do_restart_children(Children1, []).
+    do_restart_children(Children1, [], []).
 
 terminate_many_children([], NewChildren) ->
     {ok, lists:reverse(NewChildren)};
@@ -510,26 +536,49 @@ terminate_many_children([Child | Children], NewChildren) ->
             ])
     end.
 
-do_restart_children([], NewChildren) ->
+do_restart_children([], NewChildren, []) ->
     {ok, lists:reverse(NewChildren)};
-do_restart_children([#child{pid = Pid} = Child | Children], NewChildren) ->
+do_restart_children([], NewChildren, [RetryChild | T] = RetryChildren) ->
+    if
+        length(T) =:= 0 ->
+            {ok, {lists:reverse(NewChildren), RetryChild#child.id}};
+        true ->
+            ok = differed_try_again(RetryChildren),
+            {ok, lists:reverse(NewChildren)}
+    end;
+do_restart_children([#child{pid = Pid} = Child | Children], NewChildren, RetryChildren) ->
     case Pid of
         {restarting, _} ->
             case try_start(Child) of
                 {ok, Pid1, {ok, Pid1}} ->
-                    do_restart_children(Children, [Child#child{pid = Pid1} | NewChildren]);
+                    do_restart_children(
+                        Children, [Child#child{pid = Pid1} | NewChildren], RetryChildren
+                    );
                 {ok, Pid1, {ok, Pid1, _Result}} ->
-                    do_restart_children(Children, [Child#child{pid = Pid1} | NewChildren]);
+                    do_restart_children(
+                        Children, [Child#child{pid = Pid1} | NewChildren], RetryChildren
+                    );
                 {ok, undefined, {ok, undefined}} ->
-                    do_restart_children(Children, [Child#child{pid = undefined} | NewChildren]);
-                {error, _} = Error ->
-                    Error
+                    do_restart_children(
+                        Children, [Child#child{pid = undefined} | NewChildren], RetryChildren
+                    );
+                {error, _} ->
+                    do_restart_children(Children, NewChildren, [Child | RetryChildren])
             end;
         _ ->
             % retain previous ignore children without starting them
-            do_restart_children(Children, [Child | NewChildren])
+            do_restart_children(Children, [Child | NewChildren], RetryChildren)
     end.
 
+%% Schedules "try again" restarts at 50ms intervals when multiple children have failed to restart
+%% on the first attempt. This is an accumulated (reverse start order) list, so the children that
+%% should start last get the longest delay before sending the try_again_restart request.
+differed_try_again([]) ->
+    ok;
+differed_try_again([Child | Children] = RetryChildren) ->
+    erlang:send_after(50 * length(RetryChildren), self(), {try_again_restart, Child#child.id}),
+    differed_try_again(Children).
+
 child_to_info(#child{id = Id, pid = Pid, type = Type, modules = Modules}) ->
     Child =
         case Pid of
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,50 @@ 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
+                    ok -> ok
+                end
+            end),
+            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 +378,19 @@ 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}}.
 
 test_supervisor_order() ->
     {ok, SupPid} = supervisor:start_link(?MODULE, {test_supervisor_order, self()}),
@@ -521,3 +577,83 @@ get_ping_pong_pid() ->
         {ping_pong_server_ready, Pid} -> Pid
     after 2000 -> throw(timeout)
     end.
+
+try_again_restart() ->
+    process_flag(trap_exit, true),
+    timer:sleep(100),
+    Arbitrator = erlang:spawn(fun() -> arbitrator_start(4) end),
+    ok =
+        case is_pid(Arbitrator) of
+            true -> ok;
+            false -> {error, no_arbitrator}
+        end,
+    {ok, SupPid} = supervisor:start_link(
+        {local, try_again_test}, ?MODULE, {test_try_again, Arbitrator, self()}
+    ),
+    ChildPid =
+        case is_pid(SupPid) of
+            true ->
+                receive
+                    Pid0 when is_pid(Pid0) ->
+                        Pid0
+                after 200 ->
+                    error({timeout, no_child_start})
+                end;
+            false ->
+                {error, no_supervisor}
+        end,
+    ok =
+        case is_pid(ChildPid) of
+            true -> ok;
+            false -> ChildPid
+        end,
+    ChildPid ! ok,
+    ChildPid1 =
+        receive
+            Pid1 when is_pid(Pid1) ->
+                Pid1
+        after 2000 ->
+            error({timeout, no_child_restart})
+        end,
+    ChildPid1 ! ok,
+    Arbitrator ! shutdown,
+    exit(SupPid, normal),
+    ok =
+        receive
+            {'EXIT', SupPid, normal} ->
+                ok
+        after 2000 ->
+            error({supervisor_not_stopped, normal})
+        end,
+
+    Arbitrator2 = erlang:spawn(fun() -> arbitrator_start(5) end),
+    {ok, SupPid2} = supervisor:start_link(
+        {local, test_try_again}, ?MODULE, {test_try_again, Arbitrator2, self()}
+    ),
+    ChildPid2 =
+        case is_pid(SupPid2) of
+            true ->
+                receive
+                    Pid2 when is_pid(Pid2) ->
+                        Pid2
+                after 200 ->
+                    error({timeout, no_child2_start})
+                end;
+            false ->
+                {error, no_supervisor2}
+        end,
+    ok =
+        case is_pid(ChildPid) of
+            true -> ok;
+            false -> ChildPid
+        end,
+    ChildPid2 ! ok,
+    ok =
+        receive
+            {'EXIT', SupPid2, shutdown} ->
+                ok
+        after 2000 ->
+            error({supervisor_not_stopped, restart_try_again_exceeded})
+        end,
+    process_flag(trap_exit, false),
+    ok.
