Fix supervisor restart when child fails to match OTP

UncleGrumpy · UncleGrumpy · commit 707f71b218ce · 2025-11-11T23:19:38.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
@@ -285,11 +285,13 @@ handle_cast(_Msg, State) ->
 
 handle_info({'EXIT', Pid, Reason}, State) ->
     case handle_child_exit(Pid, Reason, State) of
+        {ok, State1} ->
+            {noreply, State1};
         {ok, State1, restart_all_children} ->
             Children = State1#state.children,
             {noreply, State1, {timeout, 0, {restart_many_children, Children}}};
-        {ok, State1} ->
-            {noreply, State1};
+        {ok, State1, {try_again_restart, ChildId}} ->
+            {noreply, State1, {timeout, 0, {try_again_restart, ChildId}}};
         {shutdown, State1} ->
             {stop, shutdown, State1}
     end;
@@ -303,7 +305,7 @@ 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 | Children] = _ChildSpecs}, State) ->
     case try_start(Child) of
         {ok, NewPid, _Result} ->
             NewChild = Child#child{pid = NewPid},
@@ -313,7 +315,50 @@ handle_info({restart_many_children, [#child{id = Id} = Child | Children] = Child
             {noreply, State#state{children = NewChildren},
                 {timeout, 0, {restart_many_children, Children}}};
         {error, _Reason} ->
-            {noreply, State, {timeout, 0, {restart_many_children, ChildSpecs}}}
+            {noreply, State, {timeout, 0, {try_again_restart_continue, Child, Children}}}
+    end;
+handle_info({try_again_restart, Id}, State) ->
+    case lists:keyfind(Id, #child.id, State#state.children) of
+        false ->
+            {ok, 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, State#state.children),
+                    {stop, shutdown, State1#state{children = RemainingChildren}}
+            end
+    end;
+handle_info({try_again_restart_continue, Id, Children}, State) ->
+    case lists:keyfind(Id, #child.id, State#state.children) of
+        false ->
+            {ok, 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},
+                                {timeout, 0, {restart_many_children, Children}}};
+                        {error, {_, _}} ->
+                            {noreply, State1, {timeout, 0, {try_again_restart, Id}}}
+                    end;
+                {shutdown, State1} ->
+                    RemainingChildren = lists:keydelete(Id, #child.id, State#state.children),
+                    {stop, shutdown, State1#state{children = RemainingChildren}}
+            end
     end;
 handle_info(_Msg, State) ->
     %TODO: log unexpected message
@@ -370,7 +415,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,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
+                    stop -> exit(normal)
+                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,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 => 2000,
+            type => worker,
+            modules => [?MODULE]
+        },
+        #{
+            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 +597,86 @@ 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 ! stop,
+    ChildPid1 = wait_child_pid(),
+
+    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 ! 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 4 restart attempts, Ping1 exiting
+    %% a single time after should cause a supervisor shutdown.
+    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 = get_and_test_server(),
+
+    _Crashy1 = wait_child_pid(),
+
+    gen_server:cast(Ping1, {crash, test}),
+
+    _Ping2 = get_and_test_server(),
+    Crashy2 = wait_child_pid(),
+    %% this will exit the child triggering a one_for_all restart.
+    Crashy2 ! stop,
+
+    %% 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_child_pid() ->
+    receive
+        Pid when is_pid(Pid) ->
+            Pid
+    after 1000 ->
+        error({timeout, no_child_pid})
+    end.
