Merge pull request #17 from ausimian/issue/10-error-returns

fix: report release-management failures, and confirm an upgrade took effect

Author: ausimian

AGENTS.md

@@ -21,6 +21,50 @@ Castle's job is configuration and release management on a running node.
 - **`unpack/1`, `install/1`, `commit/1`, `remove/1`, `releases/0`** — wrappers
   over `:release_handler`, with `generate/1` called ahead of `install` and
   `commit` so the target version's configuration exists before it is booted.
+- **`Castle.running/1`** — succeeds when the version it is given is the release
+  the system is running. `install_release/1`'s reply says only that the upgrade
+  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. So Castle answers the question and leaves the asking to
+  Forecastle: `bin/castle install` repeats it rather than trusting the reply,
+  from Forecastle 1.0.0 — the revision pinned in this project's `mix.lock`
+  installs with a single rpc and never calls this, so do not describe the
+  polling as something Castle's own integrated state does. 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.
+
+  The marker is the whole of the evidence, so it inherits whatever the selected
+  boot script does with it. `RELEASE_BOOT_SCRIPT` naming a hand-written script
+  that never reaches `{progress, started}` will never be confirmed — `install`
+  waits and then fails, and the refusal names the progress the node did reach,
+  so it is diagnosable and never a false success — and one that emits the marker
+  before its applications start defeats the check. Both are documented rather
+  than validated: Mix generates the boot scripts and offers no `rel/` template
+  for them, so reaching either state takes deliberate work. (An earlier note
+  here claimed `systools_make:add_apply_upgrade/2`'s hard match on the trailing
+  marker ruled this out. It does not: that builds the hybrid script for an
+  emulator upgrade and says nothing about a script an operator supplies.)
+
+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
+leaves a non-zero exit status behind for the shell that asked for it. Raising,
+not halting — the expression runs on the *running* node, so halting would take
+down the system under management; `Kernel.CLI` catches on the node and
+re-raises in the calling VM, and only that VM exits. `Castle.Commands` holds
+the operations themselves, returning their outcome instead of acting on the
+process, which is what makes them testable.
 
 Forecastle is what arranges for these to be reachable: it renames `sys.config`
 to `build.config` at assembly time, adds a `:preboot` script that starts
@@ -31,7 +75,10 @@ into this module.
 
 | Path | Purpose |
 | --- | --- |
-| `lib/castle.ex` | The whole of the runtime logic |
+| `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`, `:init` and a config provider |
 
 ## Working on this project
 
@@ -50,27 +97,32 @@ into this module.
 
 ## Tests
 
-There is no test coverage yet. `test/castle_test.exs` is a `doctest` stub.
+`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.
 
-Every function here talks to `:release_handler` against a real installed
-release, and `generate/1` resolves paths from `:code.root_dir()`, so none of it
-is reachable from a plain `mix test`. Testing it needs a release fixture booted
-in a workspace, the way Forecastle's `:e2e` suite does — tracked in
-[#8](https://github.com/ausimian/castle/issues/8). Forecastle's
-`test/forecastle/upgrade_test.exs` exercises this code end to end in the
-meantime.
+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
+`:e2e` suite ([#8](https://github.com/ausimian/castle/issues/8)), which
+exercises this code against a real release and asserts on the success messages
+each command prints. Those strings — `Unpacked <vsn> ok`,
+`Now running <vsn> (previously <other>).`, `Committed <vsn>. …` and the
+`releases/0` table — are a contract with that suite. Failure messages are not.
 
 ## Known limitations
 
-- **Failed operations exit 0.** Every command catches the `:release_handler`
-  error, prints it and returns normally, so `bin/castle` cannot tell a failed
-  unpack/install/commit/remove from a successful one. Tracked in
-  [#10](https://github.com/ausimian/castle/issues/10), together with letting
-  `generate/1` take a caller-chosen destination path
-  ([#15](https://github.com/ausimian/castle/issues/15)).
 - **Concurrent boots race on `sys.config`.** `generate/1` writes into the
   version directory, so simultaneous `start`/`daemon`/`eval` invocations with
-  differing environments overwrite each other's configuration. Same issue.
+  differing environments overwrite each other's configuration. Do not fix this
+  by letting callers choose where the configuration is written: it goes away
+  with [#13](https://github.com/ausimian/castle/issues/13), which materialises
+  the target release's configuration in a `:peer` running its own config
+  providers, and takes `Castle.generate/1` with it.
 - **The public API is undocumented.** `@moduledoc` is still the generated
   placeholder and there are no `@doc` or `@spec` annotations
   ([#11](https://github.com/ausimian/castle/issues/11)).

RELEASE.md

@@ -1,3 +1,65 @@
+### Added
+
+- `Castle.Error`, the exception raised by a release-management command that did
+  not succeed.
+- `Castle.running/1`, which succeeds when the version it is given is the
+  release the system is running, and fails otherwise. `install/1` reports what
+  `:release_handler` replied, and that reply says only that the upgrade was
+  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 rather than inferred, and this is what makes it observable: a caller
+  that repeats the question until it is answered - which is what Forecastle's
+  `bin/castle install` does, from its own 1.0.0 - can tell an upgrade that took
+  effect from one that did not. Castle supplies the answer; it does not do the
+  asking, and the Forecastle this release is built against does not yet ask.
+
+  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.
+
+  "Finished booting" means the boot script reached its `{progress, started}`
+  instruction, which is the last thing every boot script Mix generates does.
+  A release booted with a `RELEASE_BOOT_SCRIPT` that names a hand-written
+  script without that marker will therefore never be confirmed: `install` waits
+  and then fails, and the refusal names the progress the node did reach, so it
+  says what is wrong rather than failing silently - but it will not succeed. A
+  script that emits the marker before its applications are started defeats the
+  check instead, since the marker is all there is to go on.
+
+  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
+  hot-upgrade path is covered by Forecastle's end-to-end suite; the statuses
+  themselves are covered by unit tests here.
+
 ### Changed
 
 - Raised the minimum Elixir requirement to 1.18.
+- `unpack/1`, `install/1`, `commit/1`, `remove/1`, `generate/1` and
+  `make_releases/0` now fail when the operation fails, instead of printing the
+  reason and returning normally. These are invoked over `bin/castle`, which
+  reaches them by `rpc`, and by the launcher's preboot `eval`, so the reason
+  now arrives on the caller's standard error and the command exits non-zero:
+  `bin/castle unpack "$VSN" && bin/castle install "$VSN"` stops at the step
+  that failed, and an operation the system refused no longer exits 0. The
+  running system is unaffected - the failure is raised on the
+  node, re-raised in the short-lived VM that made the call, and it is that VM
+  which exits. What a successful command reports is unchanged.
+
+### Fixed
+
+- `install/1` reports the emulator restart that an upgrade to a new emulator,
+  or to a new kernel, stdlib or sasl, needs - rather than failing with a
+  `CaseClauseError` while the upgrade proceeds.
+- `releases/0` reports nothing at all, rather than raising `Enum.EmptyError`,
+  when no releases are installed.
+- `generate/1` and `make_releases/0` say what went wrong - which file could not
+  be read or written, and why - rather than raising `MatchError`.

lib/castle.ex

@@ -3,95 +3,64 @@ defmodule Castle do
   Documentation for `Castle`.
   """
 
-  @app Mix.Project.config()[:app]
+  alias Castle.Commands
+
+  # Every function in this module is a command entry point: `bin/castle` sends
+  # each one to the running node over `bin/<release> rpc`, and the launcher's
+  # env.sh fragment evaluates generate/1 and make_releases/0 in the preboot VM.
+  # There is no separate CLI layer to carry the process status, so these
+  # functions are the command boundary, and it is here that a failure raises.
+  #
+  # Raising, rather than halting or returning: the rpc expression runs on the
+  # running release node, so halting there would halt the system under
+  # management rather than the caller. `Kernel.CLI.rpc_eval/1` catches on the
+  # node and the local VM re-raises, printing the reason and exiting non-zero
+  # while the running node is left untouched. A returned error value would be
+  # discarded - `Kernel.CLI` only inspects the result of a command for `:ok`.
+  #
+  # `Castle.Commands` holds the operations themselves, returning their outcome
+  # rather than acting on the process, so that they can be tested.
 
   def make_releases do
-    reldir = "releases"
-    releases_file = Path.join(reldir, "RELEASES")
-
-    unless File.exists?(releases_file) do
-      {:ok, _} = Application.ensure_all_started(:sasl)
-      [{name, vsn, _, _}] = :release_handler.which_releases(:permanent)
-      relfile = Path.join([reldir, vsn, "#{name}.rel"])
-      :ok = :release_handler.create_RELEASES(to_charlist(reldir), relfile, [])
-    end
+    report!(Commands.make_releases())
   end
 
   def generate(vsn) do
-    rel_vsn_dir = Path.join([:code.root_dir(), "releases", vsn])
-    # Read the build time config from build.config
-    {:ok, [build_config]} = :file.consult(to_charlist(Path.join(rel_vsn_dir, "build.config")))
-    # Generate the sys.config by running the config providers
-    sys_config =
-      build_config
-      |> Keyword.get(@app, [])
-      |> Keyword.get(:config_providers, [])
-      |> Enum.reduce(build_config, fn {mod, arg}, cfg -> mod.load(cfg, arg) end)
-
-    File.write!(
-      Path.join(rel_vsn_dir, "sys.config"),
-      :io_lib.format(~c"%% coding: utf-8~n~tp.~n", [sys_config])
-    )
+    report!(Commands.generate(rel_vsn_dir(vsn)))
   end
 
   def unpack(name) when is_binary(name) do
-    case :release_handler.unpack_release(to_charlist(name)) do
-      {:ok, vsn} ->
-        IO.puts("Unpacked #{vsn} ok")
-
-      {:error, reason} ->
-        IO.puts("Failed to unpack #{name}. #{inspect(reason)}")
-    end
+    report!(Commands.unpack(name))
   end
 
   def install(vsn) when is_binary(vsn) do
     generate(vsn)
+    report!(Commands.install(vsn))
+  end
 
-    case :release_handler.install_release(to_charlist(vsn)) do
-      {:ok, other_vsn, _} ->
-        IO.puts("Now running #{vsn} (previously #{other_vsn}).")
-
-      {:error, reason} ->
-        IO.puts("Install of #{vsn} failed. #{inspect(reason)}")
-    end
+  def running(vsn) when is_binary(vsn) do
+    report!(Commands.running(vsn))
   end
 
   def commit(vsn) when is_binary(vsn) do
     generate(vsn)
-
-    case :release_handler.make_permanent(to_charlist(vsn)) do
-      :ok ->
-        IO.puts("Committed #{vsn}. System restarts will now boot into this version.")
-
-      {:error, reason} ->
-        IO.puts("Commit of #{vsn} failed. #{inspect(reason)}")
-    end
+    report!(Commands.commit(vsn))
   end
 
   def remove(vsn) when is_binary(vsn) do
-    case :release_handler.remove_release(to_charlist(vsn)) do
-      :ok ->
-        IO.puts("Removed #{vsn}.")
-
-      {:error, reason} ->
-        IO.puts("Removal of #{vsn} failed. #{inspect(reason)}")
-    end
+    report!(Commands.remove(vsn))
   end
 
   def releases do
-    vsns =
-      for {_, vsn, _, status} <- :release_handler.which_releases() do
-        {to_string(vsn), to_string(status)}
-      end
+    report!(Commands.releases())
+  end
 
-    width =
-      vsns
-      |> Enum.map(&elem(&1, 0))
-      |> Enum.map(&String.length/1)
-      |> Enum.max()
+  # The version directory of the running release. Where the configuration is
+  # written is derived from the release that is running, never chosen by the
+  # caller - see castle#13, which materialises target configuration in a peer
+  # rather than extending this path.
+  defp rel_vsn_dir(vsn), do: Path.join([:code.root_dir(), "releases", vsn])
 
-    Enum.each(vsns, fn {vsn, status} ->
-      IO.puts("#{String.pad_trailing(vsn, width + 2)}#{status}")
-    end)
-  end
+  defp report!({:ok, lines}), do: Enum.each(lines, &IO.puts/1)
+  defp report!({:error, message}), do: raise(Castle.Error, message)
 end