[pguyot]

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[petermm]

AMP:

PR Review — estdlib: add rest_for_one and simple_one_for_one supervisor strategies

Commit: 63a942a0a8fd497c62f605b6223dc9068f4e27f3
Author: Paul Guyot
Files: libs/estdlib/src/supervisor.erl, tests/libs/estdlib/test_supervisor.erl, CHANGELOG.md

Summary

This commit adds the rest_for_one and simple_one_for_one restart strategies to
the estdlib supervisor, refactors the shared one_for_one restart into
restart_one/2, adds a template field to #state{} for dynamic children, and
updates the module docstring, types, and CHANGELOG.

Overall: solid, well-tested work. The rest_for_one ordering and its
{restarting,_} re-restart path are correct. A few simple_one_for_one
behaviors diverge from OTP and are worth fixing before merge, plus some spec/test
gaps.

Verdict: change requested (minor).

---

What's correct (verified)

rest_for_one ordering and partial restart

The children list invariant is "last-to-start at the head" (see the comment above
#state). For [C, B, A] (start order A, B, C), a crash of B:

• split_affected_rest/3 → affected [C, B], rest [A] ✅ (C started after B, restarts; A started before, untouched)
• get_restart_children/1 reverses affected to start order [B, C] for restart_many_children ✅
• stored state becomes [C, B] ++ [A], preserving the head-invariant ✅

The {restarting,_} re-restart path is also consistent with OTP: if a later child
fails to restart, only children after it are affected; earlier ones are left alone.

Restart intensity accounting

handle_child_exit/3 calls add_restart/1 exactly once before dispatching to
handle_restart_strategy/2, so a whole-group rest_for_one restart counts as a
single restart event — matching OTP and the existing one_for_all behavior.

---

Findings

1. [should-fix] terminate_child(Sup, OldPid) can't cancel a simple_one_for_one child stuck in {restarting, OldPid}

handle_call({terminate_child, Pid}, ...)
(supervisor.erl:331) only matches a live pid via
lists:keyfind(Pid, #child.pid, Children). But a failed dynamic restart is stored
as {restarting, OldPid} by restart_one/2
(supervisor.erl:599-602). So calling
terminate_child/2 with the documented original pid returns {error, not_found}
while the retry loop keeps running. OTP accepts the original pid and cancels the
pending restart.

handle_call(
    {terminate_child, Pid}, From, #state{restart_strategy = simple_one_for_one} = State
) when is_pid(Pid) ->
    Children = State#state.children,
    case lists:keyfind(Pid, #child.pid, Children) of
        #child{} = Child ->
            do_terminate(Child),
            NewChild = Child#child{restart = {terminating, temporary, From}},
            NewChildren = lists:keyreplace(Pid, #child.pid, Children, NewChild),
            {noreply, State#state{children = NewChildren}};
        false ->
            %% Child may be waiting for a restart retry as {restarting, Pid};
            %% no process is alive, so just cancel the pending retry.
            case lists:keyfind({restarting, Pid}, #child.pid, Children) of
                #child{} ->
                    NewChildren = lists:keydelete({restarting, Pid}, #child.pid, Children),
                    {reply, ok, State#state{children = NewChildren}};
                false ->
                    {reply, {error, not_found}, State}
            end
    end;

2. [should-fix] A simple_one_for_one dynamic child that returns ignore on restart is kept; OTP drops it

At start time, ignore is correctly dropped
(supervisor.erl:321-324). But a crash restart
flows through the shared restart_one/2
(supervisor.erl:593-603), whose
{ok, undefined, _Result} (== ignore) branch treats it as success and leaves the
child in children with pid = undefined. For simple_one_for_one this is a
phantom child: which_children/1 reports {undefined, undefined, ...} and the
worker/supervisor counts are skewed. OTP drops the dynamic child instead.

Split the simple restart out from one_for_one:

handle_restart_strategy(#child{} = Child, #state{restart_strategy = one_for_one} = State) ->
    restart_one(Child, State);
handle_restart_strategy(#child{} = Child, #state{restart_strategy = simple_one_for_one} = State) ->
    restart_simple_one(Child, State);

restart_simple_one(#child{id = Id} = Child, State) ->
    case try_start(Child) of
        {ok, undefined, _Result} ->
            %% Dynamic child returned ignore on restart: drop it, as in OTP.
            Children = lists:keydelete(Id, #child.id, State#state.children),
            {noreply, State#state{children = Children}};
        {ok, NewPid, _Result} ->
            NewChild = Child#child{pid = NewPid},
            Children = lists:keyreplace(Id, #child.id, State#state.children, NewChild),
            {noreply, State#state{children = Children}};
        {error, _} ->
            NewChild = Child#child{pid = {restarting, Child#child.pid}},
            Children = lists:keyreplace(Id, #child.id, State#state.children, NewChild),
            {noreply, State#state{children = Children}, {timeout, 0, {try_again_restart, Id}}}
    end.

The try_again_restart retry path
(supervisor.erl:472-494) has the same issue and
should also drop the dynamic child when the retried start returns ignore.

3. [nice-to-have] Public -specs don't mention the new simple_one_for_one error

The new {error, simple_one_for_one} returns are not reflected in the exported
specs, and terminate_child/2 doesn't allow a pid() id.

-spec terminate_child(Supervisor :: sup_ref(), ChildId :: child_id() | pid()) ->
    ok | {error, not_found | simple_one_for_one}.

-spec restart_child(Supervisor :: sup_ref(), ChildId :: child_id()) ->
    {ok, Child :: child()}
    | {ok, Child :: child(), Info :: term()}
    | {error, Reason :: running | restarting | not_found | simple_one_for_one | term()}.

-spec delete_child(Supervisor :: sup_ref(), ChildId :: child_id()) ->
    ok | {error, Reason :: running | restarting | not_found | simple_one_for_one}.

4. [nice-to-have / pre-existing] Affected pid = undefined children are not restarted by the multi-child path

get_restart_children/2 includes non-temporary pid = undefined children in the
restart set (supervisor.erl:672-675), but
handle_info({restart_many_children, [#child{pid = undefined} | ...]})
(supervisor.erl:495-496) skips them rather than
starting them. This pre-existed for one_for_all; rest_for_one now inherits it.
Effect: a manually-terminated (pid = undefined) child that is part of an affected
group won't be restarted when a sibling crashes, unlike OTP. Fixing cleanly likely
needs an id-based restart entry for undefined children (pid-keying is ambiguous
across multiple undefineds), so it's out of scope for this commit but worth a
follow-up note.

5. [nice-to-have / pre-existing] Internal {terminating, ...} leaks into the exported restart() type

restart() (supervisor.erl:73-77) includes the
internal {terminating, ..., gen_server:from()} state, which then widens the public
child_spec(). Pre-existing, but adding the new strategies is a good moment to
consider splitting the internal state out of the exported type.

---

Test gaps

The added tests (test_rest_for_one/0, test_simple_one_for_one/0) cover the happy
paths well. Consider adding:

1. simple_one_for_one restart returning ignore drops the dynamic child (finding Enhancement: Port to Darwin and FreeBSD #2).
2. simple_one_for_one terminate_child(Sup, OldPid) while the child is restarting (finding bifs_hash.h not found #1).
3. simple_one_for_one restart-limit (intensity) shutdown.
4. simple_one_for_one non-brutal_kill (timeout) dynamic child shutdown, verifying the reply comes only after the actual EXIT.
5. rest_for_one restart-intensity shutdown, and the retry path when the crashed child is {restarting,_}.

---

Nits

• CHANGELOG.md entry is correct and well-placed under the unreleased Added section.
• Docstring caveat removal (No support for simple_one_for_one or one_for_rest) is correct. Note the removed line said "one_for_rest" (a typo for rest_for_one); the fix is right regardless.

[bettio]

fd66292 and acefe17 replaced the {timeout, 0, Msg} restart driver with payload-free '$atomvm_*' messages sent to self(), plus restart_queue and retry_queue in the state. The new code in this PR still uses the old mechanism.

[bettio]

This conflicts with release-0.7 (fd66292 and acefe17).

On rebase, please switch this to the new mechanism, the same way one_for_all now does it: self() ! '$atomvm_restart_many_children' with restart_queue = restart_ids(NewAffected).

Otherwise this path brings back both problems: a call or 'EXIT' arriving mid-restart leaves the remaining children stuck in {restarting, _}, and the message carries #child{} records, so any sender can make the supervisor call an arbitrary MFA.

[bettio]

restart_one and restart_simple_one helpers return {timeout, 0, {try_again_restart, Id}}. That was the bug fixed in acefe17: the next incoming message cancels the retry, so the child stays {restarting, _} forever. Please port them to self() ! '$atomvm_try_again_restart' and append Id to retry_queue, the way the current one_for_one clause on release-0.7 does. The same applies at line 633.

[bettio]

On release-0.7, the '$atomvm_try_again_restart' handler only matches #child{pid = {restarting, _}} and re-queues with retry_queue = Ids ++ [Id]. Please carry the {ok, undefined, _} when simple_one_for_one drop branch into that handler, so the case covered by test_simple_one_for_one_ignore_on_restart still works through the retry path.

[bettio]

Let's move this to 0.7.0-beta.1.

[bettio]

Every failed retry calls add_restart, and the retry loop runs back-to-back. With intensity 10, the supervisor can shut down before terminate_child/2 gets in, especially on SMP builds. Today the test likely passes partly because the call cancels the {timeout, 0} retry, and that changes after the rebase. Please raise the intensity (e.g. 1000), or make the worker fail a bounded number of times and then block.

[bettio]

release-0.7 has test_restart_survives_messages_one_for_all and test_retry_survives_messages_one_for_one. A rest_for_one version, where a which_children call or an unrelated 'EXIT' arrives during a partial restart, would guard the rebased path above.