iframe-proxy

pguyot · 2026-07-04T04:56:50Z

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 · 2026-07-08T18:53:54Z

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 `-spec`s 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:

simple_one_for_one restart returning ignore drops the dynamic child (finding Enhancement: Port to Darwin and FreeBSD #2).
simple_one_for_one terminate_child(Sup, OldPid) while the child is restarting (finding bifs_hash.h not found #1).
simple_one_for_one restart-limit (intensity) shutdown.
simple_one_for_one non-brutal_kill (timeout) dynamic child shutdown, verifying the reply comes only after the actual EXIT.
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 · 2026-10-15T09:22:15Z

+    ok = terminate_one_for_all(Affected1),
+    {ok, NewAffected} = get_restart_children(Affected1),
+    {noreply, State#state{children = lists:reverse(NewAffected) ++ Rest},
+        {timeout, 0, {restart_many_children, NewAffected}}}.


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 · 2026-10-15T09:24:47Z

+        {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}}}


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 · 2026-10-15T09:29:26Z

            case add_restart(State) of
                {ok, State1} ->
                    case try_start(Child) of
+                        {ok, undefined, _Result} when


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 · 2026-10-15T09:30:23Z

- Added a `hex_package` build target that builds the `atomvm` Hex package, published on
-  hex.pm by CI when a release is tagged, so that build tools can check a project against
-  the AtomVM release it targets
+- Added support for `rest_for_one` and `simple_one_for_one` supervisor restart strategies


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

bettio · 2026-10-15T09:31:05Z

+        type => worker,
+        modules => [?MODULE]
+    },
+    {ok, {#{strategy => simple_one_for_one, intensity => 10, period => 60}, [Template]}};


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 · 2026-10-15T09:31:38Z

+        stop -> ok
+    end.
+
+test_rest_for_one() ->


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.

For simple_one_for_one, drop a dynamic child whose restart returns ignore and let terminate_child/2 cancel a pending restart by the original pid, as OTP does. Signed-off-by: Paul Guyot <pguyot@kallisys.net>

pguyot force-pushed the w27/estdlib-supervisor-strategies branch from 8e83c85 to 63a942a Compare July 4, 2026 07:19

pguyot force-pushed the w27/estdlib-supervisor-strategies branch 3 times, most recently from b5bdab9 to 68a7821 Compare August 1, 2026 06:29

pguyot force-pushed the w27/estdlib-supervisor-strategies branch from 68a7821 to 3e84e86 Compare September 25, 2026 15:41

bettio requested changes Oct 15, 2026

View reviewed changes

estdlib: add rest_for_one and simple_one_for_one supervisor strategies

35eafd7

For simple_one_for_one, drop a dynamic child whose restart returns ignore and let terminate_child/2 cancel a pending restart by the original pid, as OTP does. Signed-off-by: Paul Guyot <pguyot@kallisys.net>

pguyot force-pushed the w27/estdlib-supervisor-strategies branch from 3e84e86 to 35eafd7 Compare October 17, 2026 17:04

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

estdlib: add rest_for_one and simple_one_for_one supervisor strategies - #2352

estdlib: add rest_for_one and simple_one_for_one supervisor strategies#2352
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w27/estdlib-supervisor-strategies

pguyot commented Jul 4, 2026

Uh oh!

petermm commented Jul 8, 2026

Uh oh!

bettio left a comment

Uh oh!

bettio Oct 15, 2026

Uh oh!

bettio Oct 15, 2026 •

edited

Loading

Uh oh!

bettio Oct 15, 2026

Uh oh!

bettio Oct 15, 2026

Uh oh!

bettio Oct 15, 2026

Uh oh!

bettio Oct 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

pguyot commented Jul 4, 2026

Uh oh!

petermm commented Jul 8, 2026

PR Review — estdlib: add rest_for_one and simple_one_for_one supervisor strategies

Summary

What's correct (verified)

rest_for_one ordering and partial restart

Restart intensity accounting

Findings

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

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

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

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

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

Test gaps

Nits

Uh oh!

bettio left a comment

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

bettio Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PR Review — `estdlib: add rest_for_one and simple_one_for_one supervisor strategies`

`rest_for_one` ordering and partial restart

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

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

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

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

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

bettio Oct 15, 2026 •

edited

Loading