fix: confirm a release only once its boot has finished

running/1 asked only whether the version was the current or permanent
release, and that can be true of a node that is still booting. After a
restart_emulator reboot, release_handler's transform_release/3 makes the
target current while sasl starts, and distribution has been answering
since kernel came up - so a poll landing in that window was confirmed,
install exited 0, and an application started after sasl could still fail
the boot and take the system back to the release that was permanent.
Automation that commits after a successful install would then have made
a version that cannot boot the permanent one.

So the boot has to have finished as well. That is init:get_status/0's
*provided* status being :started, and only that element is usable: the
internal one stays :starting while the boot process lives, which is the
whole life of a release started by its boot script, so a booted node
reports {:starting, :started} and waiting for {:started, _} would wait
for ever. Checked against a running VM, and against init.erl:687-700,
where each {progress, Info} instruction sets the provided status.

:started is the last of those instructions in every boot script Mix
generates, after the one that starts the release's applications - and
systools_make:add_apply_upgrade/2 splices
release_handler:new_emulator_upgrade/2 in just before that same marker,
so on the hybrid boot of an emulator upgrade the status reaches
:started only once the continuation has run. One condition therefore
covers both restart transitions.

:init is reached through a module argument defaulting to it, as
:release_handler already is, so both sides are testable: a target that
is current but mid-boot is refused, and the same target once booted is
accepted.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

Author: ausimian

AGENTS.md

@@ -26,10 +26,20 @@ Castle's job is configuration and release management on a running node.
   was accepted: a transition that restarts the emulator is replied to and then
   rebooted, and an emulator upgrade finishes on the way back up, where it can
   still roll back. `bin/castle install` therefore polls this rather than
-  trusting the reply. The running release is the `current` one, or the
-  `permanent` one when none is current — `install` leaves its target `current`
-  and `commit` promotes it, so both count; `unpacked` (a rolled-back
-  continuation) and `tmp_current` (written before the reboot) do not.
+  trusting the reply. Two conditions. The version is the running release: the
+  `current` one, or the `permanent` one when none is current — `install` leaves
+  its target `current` and `commit` promotes it, so both count; `unpacked` (a
+  rolled-back continuation) and `tmp_current` (written before the reboot) do
+  not. And its boot has finished, which is `:init.get_status/0`'s *provided*
+  status being `:started`. Do not gate on the internal status: it stays
+  `:starting` for the life of a release started by its boot script, so a booted
+  node reports `{:starting, :started}`. The provided status is what the script's
+  `{progress, _}` instructions move along, and `started` is its last one — after
+  the applications have started, and after `new_emulator_upgrade/2` in the
+  hybrid script that continues an emulator upgrade. Without that second
+  condition a poll can confirm a node that is still booting, and automation
+  that commits straight after installing would make a version that cannot boot
+  the permanent one.
 
 Every one of them is a command entry point, so `Castle` is the command
 boundary: an operation that fails raises `Castle.Error` there, which is what
@@ -52,7 +62,7 @@ into this module.
 | `lib/castle.ex` | The command boundary: print the outcome, or raise |
 | `lib/castle/commands.ex` | The commands themselves, returning their outcome |
 | `lib/castle/error.ex` | The exception a failed command raises |
-| `test/support/` | Stubs for `:release_handler` and a config provider |
+| `test/support/` | Stubs for `:release_handler`, `:init` and a config provider |
 
 ## Working on this project
 
@@ -71,13 +81,14 @@ into this module.
 
 ## Tests
 
-`mix test` covers `Castle.Commands` as units. `:release_handler` is reached
-through a module argument that defaults to it, so the tests hand it
-`Castle.ReleaseHandlerStub` instead; `generate/1` takes the version directory
-it writes to, so the tests give it a `tmp_dir` holding a synthetic
-`build.config`. `test/castle_test.exs` drives the boundary itself against the
-real `:release_handler` — which is running under `mix test`, because castle
-depends on sasl — naming releases that do not exist.
+`mix test` covers `Castle.Commands` as units. `:release_handler` and `:init` are
+reached through module arguments that default to them, so the tests hand them
+`Castle.ReleaseHandlerStub` and `Castle.InitStub` instead; `generate/1` takes
+the version directory it writes to, so the tests give it a `tmp_dir` holding a
+synthetic `build.config`. `test/castle_test.exs` drives the boundary itself
+against the real `:release_handler` — which is running under `mix test`, because
+castle depends on sasl — and the real `:init`, naming releases that do not
+exist.
 
 What is *not* covered here is a booted release: the upgrade of a running
 system, and the exit statuses `bin/castle` returns, belong to Forecastle's

RELEASE.md

@@ -8,9 +8,16 @@
   accepted: a transition that restarts the emulator is replied to and *then*
   rebooted, and for an emulator upgrade the instructions run on the way back
   up, where they can still fail and roll back. Completion therefore has to be
-  observed, and `bin/castle install` polls this to observe it. The running
-  release is the one whose status is `current`, or the `permanent` one if none
-  is current, so a version is confirmed both before and after `commit`.
+  observed, and `bin/castle install` polls this to observe it. Confirmation
+  needs two things: the version is the release the system is running - the one
+  whose status is `current`, or the `permanent` one if none is current, so a
+  version is confirmed both before and after `commit` - *and* its boot has
+  finished. The second is not redundant. A node that restarted into the new
+  version is reachable long before it has finished booting: `release_handler`
+  makes the version `current` while `sasl` starts, and distribution answers
+  from `kernel` onwards, so an application started after `sasl` can still fail
+  and take the system back to the version that was permanent. Confirming
+  earlier than that would let automation `commit` a release that cannot boot.
   Nothing can build a relup that restarts the emulator until
   [forecastle#4](https://github.com/ausimian/forecastle/issues/4), so the
   restart transitions this addresses cannot be exercised end to end yet. The

lib/castle/commands.ex

@@ -166,16 +166,50 @@ defmodule Castle.Commands do
   is - notably `:unpacked`, which is what a rolled-back continuation leaves the
   target as, and `:tmp_current`, which is written before the reboot a restart
   transition has yet to make.
+
+  Being the running release is necessary but not sufficient, because a node
+  that restarted into it can be seen part-way up. `release_handler` records the
+  new version as `:current` while `sasl` starts, and distribution is already
+  answering by then - so a reply is available before the applications after
+  `sasl` have started, and one of them can still fail the boot and take the
+  system back to the previous permanent release. Committing on the strength of
+  that would make a version that cannot boot the permanent one. So the boot has
+  to have finished too, which is what the second condition below is for.
   """
-  @spec running(String.t(), module()) :: result()
-  def running(vsn, handler \\ :release_handler) do
+  @spec running(String.t(), module(), module()) :: result()
+  def running(vsn, handler \\ :release_handler, init \\ :init) do
     case running_release(handler) do
-      ^vsn -> {:ok, []}
+      ^vsn -> booted(vsn, init)
       nil -> {:error, "#{vsn} is not the running release. No release is running."}
       other -> {:error, "#{vsn} is not the running release. #{other} is."}
     end
   end
 
+  # `init:get_status/0` answers `{InternalStatus, ProvidedStatus}`, and only the
+  # second element is any use here. The internal one stays `:starting` for as
+  # long as the boot process is alive, which is the whole life of a release
+  # started by its boot script - a booted node reports `{:starting, :started}`,
+  # so waiting for `{:started, _}` would wait forever.
+  #
+  # The provided status is what the boot script's `{progress, _}` instructions
+  # set (init.erl:692), and `:started` is the last of them. Every boot script
+  # Mix generates ends with `{progress, started}`, after the instruction that
+  # starts the release's own applications; the hybrid script that continues an
+  # emulator upgrade has `release_handler:new_emulator_upgrade/2` applied just
+  # before that same marker (systools_make.erl:336). So a provided status of
+  # `:started` means the script ran to the end: applications up, and any
+  # continuation of the upgrade finished.
+  defp booted(vsn, init) do
+    case init.get_status() do
+      {_internal, :started} ->
+        {:ok, []}
+
+      {_internal, progress} ->
+        {:error,
+         "#{vsn} is the running release but has not finished booting: #{inspect(progress)}."}
+    end
+  end
+
   defp running_release(handler) do
     releases = for {_, vsn, _, status} <- handler.which_releases(), do: {to_string(vsn), status}
 

test/castle/commands_test.exs

@@ -3,6 +3,7 @@ defmodule Castle.CommandsTest do
 
   alias Castle.Commands
   alias Castle.ConfigProviderStub
+  alias Castle.InitStub
   alias Castle.ReleaseHandlerStub, as: Stub
 
   describe "generate/1" do
@@ -122,15 +123,15 @@ defmodule Castle.CommandsTest do
     end
   end
 
-  describe "running/2" do
+  describe "running/3" do
     test "confirms the version an install has made current" do
       handler =
         Stub.stub(:which_releases, [
           {~c"sample", ~c"1.2.3", [], :current},
           {~c"sample", ~c"1.2.2", [], :permanent}
         ])
 
-      assert Commands.running("1.2.3", handler) == {:ok, []}
+      assert Commands.running("1.2.3", handler, booted()) == {:ok, []}
     end
 
     test "confirms the version a commit has made permanent" do
@@ -140,7 +141,7 @@ defmodule Castle.CommandsTest do
           {~c"sample", ~c"1.2.2", [], :old}
         ])
 
-      assert Commands.running("1.2.3", handler) == {:ok, []}
+      assert Commands.running("1.2.3", handler, booted()) == {:ok, []}
     end
 
     test "refuses the permanent version while another one is current" do
@@ -152,7 +153,7 @@ defmodule Castle.CommandsTest do
           {~c"sample", ~c"1.2.2", [], :permanent}
         ])
 
-      assert Commands.running("1.2.2", handler) ==
+      assert Commands.running("1.2.2", handler, booted()) ==
                {:error, "1.2.2 is not the running release. 1.2.3 is."}
     end
 
@@ -163,7 +164,7 @@ defmodule Castle.CommandsTest do
           {~c"sample", ~c"1.2.2", [], :permanent}
         ])
 
-      assert Commands.running("1.2.3", handler) ==
+      assert Commands.running("1.2.3", handler, booted()) ==
                {:error, "1.2.3 is not the running release. 1.2.2 is."}
     end
 
@@ -176,21 +177,47 @@ defmodule Castle.CommandsTest do
           {~c"sample", ~c"1.2.2", [], :permanent}
         ])
 
-      assert Commands.running("1.2.3", handler) ==
+      assert Commands.running("1.2.3", handler, booted()) ==
                {:error, "1.2.3 is not the running release. 1.2.2 is."}
     end
 
     test "refuses a version the system has never heard of" do
       handler = Stub.stub(:which_releases, [{~c"sample", ~c"1.2.2", [], :permanent}])
 
-      assert Commands.running("9.9.9", handler) ==
+      assert Commands.running("9.9.9", handler, booted()) ==
                {:error, "9.9.9 is not the running release. 1.2.2 is."}
     end
 
+    test "refuses a version whose boot has not finished" do
+      # A node that restarted into the new version answers rpc from the moment
+      # kernel is up, and release_handler has made the version current by the
+      # time sasl has started - so it can be seen like this, with applications
+      # still to start and the boot still able to fail back to the release that
+      # was permanent before.
+      handler = Stub.stub(:which_releases, [{~c"sample", ~c"1.2.3", [], :current}])
+      init = InitStub.stub({:starting, :applications_loaded})
+
+      assert Commands.running("1.2.3", handler, init) ==
+               {:error,
+                "1.2.3 is the running release but has not finished booting: :applications_loaded."}
+    end
+
+    test "confirms that same version once its boot has finished" do
+      handler = Stub.stub(:which_releases, [{~c"sample", ~c"1.2.3", [], :current}])
+
+      # The internal status says nothing: it stays :starting for as long as the
+      # boot process lives, which is the whole life of a release started by its
+      # boot script. Only the provided status, which the script's last
+      # {progress, _} sets, answers the question.
+      for status <- [{:starting, :started}, {:started, :started}] do
+        assert Commands.running("1.2.3", handler, InitStub.stub(status)) == {:ok, []}
+      end
+    end
+
     test "refuses everything when nothing is running" do
       handler = Stub.stub(:which_releases, [{~c"sample", ~c"1.2.3", [], :unpacked}])
 
-      assert Commands.running("1.2.3", handler) ==
+      assert Commands.running("1.2.3", handler, booted()) ==
                {:error, "1.2.3 is not the running release. No release is running."}
     end
   end
@@ -249,6 +276,9 @@ defmodule Castle.CommandsTest do
     end
   end
 
+  # What a node reports once its boot script has run to the end.
+  defp booted, do: InitStub.stub({:starting, :started})
+
   defp write_build_config(dir, config) do
     File.write!(
       Path.join(dir, "build.config"),

test/castle_test.exs

@@ -35,6 +35,10 @@ defmodule CastleTest do
     test "confirms the release that is running, silently" do
       [{_, vsn, _, _}] = :release_handler.which_releases(:permanent)
 
+      # Through the real :init as well as the real :release_handler: a VM that
+      # has finished booting reports {:starting, :started}, which is the shape
+      # the confirmation has to accept.
+      assert {_internal, :started} = :init.get_status()
       assert capture_io(fn -> assert Castle.running(to_string(vsn)) == :ok end) == ""
     end
 

test/support/init_stub.ex

@@ -0,0 +1,23 @@
+defmodule Castle.InitStub do
+  @moduledoc false
+
+  # A stand-in for `:init`, so that a node seen part-way through its boot can be
+  # tested. Answers from the calling process's dictionary, like
+  # `Castle.ReleaseHandlerStub`, and for the same reason.
+
+  @doc """
+  Registers the status `get_status/0` replies with, and returns this module so
+  that it can be passed straight to the function under test.
+  """
+  def stub(status) do
+    Process.put(__MODULE__, status)
+    __MODULE__
+  end
+
+  def get_status do
+    case Process.get(__MODULE__, :unstubbed) do
+      :unstubbed -> raise "get_status/0 was called without a registered reply"
+      status -> status
+    end
+  end
+end