Add support for supervisor one_for_all strategy

UncleGrumpy · UncleGrumpy · commit ad05bf7bceec · 2025-11-11T23:19:38.000Z
Adds support for handling restarts for the `one_for_all` strategy that was documented, but lacked implementation. Makes necessary changes to ensure children are always restarted in the same order they were originally started, and shutdown in reverse order with last child first, conforming to OTP behavior. Closes #1855 Signed-off-by: Winford <winford@object.stream>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -91,6 +91,7 @@ instead `badarg`.
 - Fixed a bug where empty atom could not be created on some platforms, thus breaking receiving a message for a registered process from an OTP node.
 - Fix a memory leak in distribution when a BEAM node would monitor a process by name.
 - Fix `list_to_integer`, it was likely buggy with integers close to INT64_MAX
+- Added missing support for supervisor `one_for_all` strategy.
 
 ## [0.6.7] - Unreleased
 
diff --git a/libs/estdlib/src/supervisor.erl b/libs/estdlib/src/supervisor.erl
@@ -83,18 +83,20 @@
     }.
 
 -record(child, {
-    pid = undefined,
+    pid = undefined :: pid() | undefined | {restarting, pid()} | {restarting, undefined},
     id :: any(),
     start :: {module(), atom(), [any()] | undefined},
     restart :: restart(),
     shutdown :: shutdown(),
     type :: child_type(),
     modules = [] :: [module()] | dynamic
 }).
--record(state, {restart_strategy :: strategy(), children = [] :: [#child{}]}).
+%% note: the list of children should always be kept in order, with first to start at the head.
+-record(state, {restart_strategy = one_for_one :: strategy(), children = [] :: [#child{}]}).
 
 start_link(Module, Args) ->
     gen_server:start_link(?MODULE, {Module, Args}, []).
+
 start_link(SupName, Module, Args) ->
     gen_server:start_link(SupName, ?MODULE, {Module, Args}, []).
 
@@ -172,7 +174,7 @@ child_spec_to_record(#{id := ChildId, start := MFA} = ChildMap) ->
 init_state([ChildSpec | T], State) ->
     Child = child_spec_to_record(ChildSpec),
     NewChildren = [Child | State#state.children],
-    init_state(T, #state{children = NewChildren});
+    init_state(T, State#state{children = NewChildren});
 init_state([], State) ->
     State#state{children = lists:reverse(State#state.children)}.
 
@@ -182,7 +184,8 @@ start_children([Child | T], StartedC) ->
             start_children(T, [Child#child{pid = Pid} | StartedC])
     end;
 start_children([], StartedC) ->
-    StartedC.
+    %% We should always keep the start list in order for later one_for_all restarts.
+    lists:reverse(StartedC).
 
 handle_call({start_child, ChildSpec}, _From, #state{children = Children} = State) ->
     Child = child_spec_to_record(ChildSpec),
@@ -196,7 +199,9 @@ handle_call({start_child, ChildSpec}, _From, #state{children = Children} = State
             case try_start(Child) of
                 {ok, Pid, Result} ->
                     UpdatedChild = Child#child{pid = Pid},
-                    {reply, Result, State#state{children = [UpdatedChild | Children]}};
+                    %% The last child to start should always be at the end of the child
+                    %% start list.
+                    {reply, Result, State#state{children = Children ++ [UpdatedChild]}};
                 {error, _Reason} = ErrorT ->
                     {reply, ErrorT, State}
             end
@@ -255,6 +260,9 @@ handle_cast(_Msg, State) ->
 
 handle_info({'EXIT', Pid, Reason}, State) ->
     case handle_child_exit(Pid, Reason, State) of
+        {ok, State1, restart_all_children} ->
+            Children = State1#state.children,
+            {noreply, State1, {timeout, 0, {restart_many_children, Children}}};
         {ok, State1} ->
             {noreply, State1};
         {shutdown, State1} ->
@@ -268,13 +276,28 @@ handle_info({ensure_killed, Pid}, State) ->
             exit(Pid, kill),
             {noreply, State}
     end;
+handle_info({restart_many_children, []}, State) ->
+    {noreply, 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},
+            NewChildren = lists:keyreplace(
+                Id, #child.id, State#state.children, NewChild
+            ),
+            {noreply, State#state{children = NewChildren},
+                {timeout, 0, {restart_many_children, Children}}};
+        {error, _Reason} ->
+            {noreply, State, {timeout, 0, {restart_many_children, ChildSpecs}}}
+    end;
 handle_info(_Msg, State) ->
     %TODO: log unexpected message
     {noreply, State}.
 
 %% @hidden
 terminate(_Reason, #state{children = Children} = State) ->
-    RemainingChildren = loop_terminate(Children, []),
+    %% Shutdown children last to first.
+    RemainingChildren = loop_terminate(lists:reverse(Children), []),
     loop_wait_termination(RemainingChildren),
     {ok, State}.
 
@@ -298,20 +321,33 @@ handle_child_exit(Pid, Reason, State) ->
         #child{} = Child ->
             case should_restart(Reason, Child#child.restart) of
                 true ->
-                    case try_start(Child) of
-                        {ok, NewPid, _Result} ->
-                            NewChild = Child#child{pid = NewPid},
-                            Children = lists:keyreplace(
-                                Pid, #child.pid, State#state.children, NewChild
-                            ),
-                            {ok, State#state{children = Children}}
-                    end;
+                    handle_restart_strategy(Child, State);
                 false ->
                     Children = lists:keydelete(Pid, #child.pid, State#state.children),
                     {ok, State#state{children = Children}}
             end
     end.
 
+handle_restart_strategy(
+    #child{id = Id} = Child, #state{restart_strategy = one_for_one} = State
+) ->
+    case try_start(Child) of
+        {ok, NewPid, _Result} ->
+            NewChild = Child#child{pid = NewPid},
+            Children = lists:keyreplace(
+                Id, #child.id, State#state.children, NewChild
+            ),
+            {ok, State#state{children = Children}}
+    end;
+handle_restart_strategy(
+    #child{pid = Pid} = Child, #state{restart_strategy = one_for_all} = State
+) ->
+    Children = lists:keyreplace(Pid, #child.pid, State#state.children, Child#child{
+        pid = {restarting, Pid}
+    }),
+    {ok, NewChildren} = terminate_one_for_all_restart(Children),
+    {ok, State#state{children = NewChildren}, restart_all_children}.
+
 should_restart(_Reason, permanent) ->
     true;
 should_restart(_Reason, temporary) ->
@@ -367,6 +403,57 @@ try_start(#child{start = {M, F, Args}} = Record) ->
             {error, {{'EXIT', Error}, Record}}
     end.
 
+terminate_one_for_all_restart(Children) ->
+    %% Always shut down last child first
+    do_terminate_one_for_all_restart(lists:reverse(Children), []).
+
+do_terminate_one_for_all_restart([], NewChildren) ->
+    {ok, lists:reverse(NewChildren)};
+do_terminate_one_for_all_restart([Child | Children], NewChildren) ->
+    case Child of
+        #child{restart = {terminating, temporary, From}} = Child when is_pid(From) ->
+            do_terminate(Child),
+            ok = wait_child_exit(Child),
+            gen_server:reply(From, ok),
+            do_terminate_one_for_all_restart(Children, NewChildren);
+        #child{restart = {terminating, _Restart, From}} = Child when is_pid(From) ->
+            do_terminate(Child),
+            ok = wait_child_exit(Child),
+            gen_server:reply(From, ok),
+            do_terminate_one_for_all_restart(Children, [Child#child{pid = undefined} | NewChildren]);
+        #child{pid = undefined, restart = temporary} = Child ->
+            do_terminate_one_for_all_restart(Children, NewChildren);
+        #child{pid = Pid, restart = temporary} = Child when is_pid(Pid) ->
+            do_terminate(Child),
+            ok = wait_child_exit(Child),
+            do_terminate_one_for_all_restart(Children, NewChildren);
+        #child{pid = undefined} = Child ->
+            do_terminate_one_for_all_restart(Children, [Child | NewChildren]);
+        #child{pid = {restarting, _Pid}} = Child ->
+            do_terminate_one_for_all_restart(Children, [Child | NewChildren]);
+        #child{pid = Pid} = Child when is_pid(Pid) ->
+            do_terminate(Child),
+            ok = wait_child_exit(Child),
+            do_terminate_one_for_all_restart(Children, [
+                Child#child{pid = {restarting, Pid}} | NewChildren
+            ])
+    end.
+
+wait_child_exit(#child{pid = Pid, shutdown = brutal_kill} = Child) ->
+    receive
+        {'EXIT', Pid, _Reason} ->
+            ok
+    after 500 ->
+        {timeout, {no_exit, Child}}
+    end;
+wait_child_exit(#child{pid = Pid, shutdown = Timeout} = Child) ->
+    receive
+        {'EXIT', Pid, _Reason} ->
+            ok
+    after Timeout + 100 ->
+        {timeout, {no_exit, Child}}
+    end.
+
 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
@@ -35,6 +35,7 @@ test() ->
     ok = test_terminate_timeout(),
     ok = test_which_children(),
     ok = test_count_children(),
+    ok = test_one_for_all(),
     ok.
 
 test_basic_supervisor() ->
@@ -292,7 +293,35 @@ init({test_supervisor_order, Parent}) ->
     ],
     {ok, {{one_for_one, 10000, 3600}, ChildSpecs}};
 init({test_no_child, _Parent}) ->
-    {ok, {#{strategy => one_for_one, intensity => 10000, period => 3600}, []}}.
+    {ok, {#{strategy => one_for_one, intensity => 10000, period => 3600}, []}};
+init({test_one_for_all, Parent}) ->
+    ChildSpecs = [
+        #{
+            id => ping_pong_1,
+            start => {ping_pong_server, start_link, [Parent]},
+            restart => permanent,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [ping_pong_server]
+        },
+        #{
+            id => ping_pong_2,
+            start => {ping_pong_server, start_link, [Parent]},
+            restart => transient,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [ping_pong_server]
+        },
+        #{
+            id => ready_0,
+            start => {notify_init_server, start_link, [{Parent, ready_0}]},
+            restart => temporary,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [notify_init_server]
+        }
+    ],
+    {ok, {#{strategy => one_for_all, intensity => 10000, period => 3600}, ChildSpecs}}.
 
 test_supervisor_order() ->
     {ok, SupPid} = supervisor:start_link(?MODULE, {test_supervisor_order, self()}),
@@ -346,3 +375,68 @@ test_which_children() ->
     unlink(SupPid),
     exit(SupPid, shutdown),
     ok.
+
+test_one_for_all() ->
+    {ok, SupPid} = supervisor:start_link({local, allforone}, ?MODULE, {test_one_for_all, self()}),
+    % Collect startup message from permanent ping_pong_server
+    Server_1 = get_and_test_server(),
+    % Collect startup message from transient ping_pong_server
+    Server_2 = get_and_test_server(),
+    % Collect startup message from temporary notify_init_server
+    ready_0 =
+        receive
+            Msg1 -> Msg1
+        after 1000 -> error({timeout, {start, ready_0}})
+        end,
+
+    [{specs, 3}, {active, 3}, {supervisors, 0}, {workers, 3}] = supervisor:count_children(SupPid),
+
+    %% Monitor transient Server_2 to make sure it is stopped, and restarted when
+    %% permanent Server_1 is shutdown.
+    MonRef = monitor(process, Server_2),
+    ok = gen_server:call(Server_1, {stop, test_crash}),
+    %% Server_2 should exit before the first child is restarted, but exit messages from
+    %% monitored processes may take some time to be received so we may get the message
+    %% from the first restarted child first.
+    First =
+        receive
+            {'DOWN', MonRef, process, Server_2, killed} ->
+                down;
+            {ping_pong_server_ready, Restart1} when is_pid(Restart1) ->
+                ready
+        after 1000 ->
+            error({timeout, restart_after_crash})
+        end,
+    ok =
+        case First of
+            down ->
+                receive
+                    {ping_pong_server_ready, Restart_1} when is_pid(Restart_1) -> ok
+                after 1000 ->
+                    error({timeout, restart_after_crash})
+                end;
+            ready ->
+                receive
+                    {'DOWN', MonRef, process, Server_2, killed} -> ok
+                after 1000 ->
+                    error({timeout, restart_after_crash})
+                end
+        end,
+
+    demonitor(MonRef),
+
+    % Collect startup message from restarted transient ping_pong_server child
+    _Restart_2 = get_and_test_server(),
+    % Make sure temporary notify_init_server is not restarted
+    no_start =
+        receive
+            ready_0 -> error({error, restarted_temporary})
+        after 1000 -> no_start
+        end,
+
+    % Ensure correct number of children
+    [{specs, 2}, {active, 2}, {supervisors, 0}, {workers, 2}] = supervisor:count_children(SupPid),
+
+    unlink(SupPid),
+    exit(SupPid, shutdown),
+    ok.
