Merge pull request #25 from ausimian/issue/13-restart-remedy

fix: name a remedy that works for an unreadable RELEASES

Author: ausimian

AGENTS.md

@@ -437,6 +437,55 @@ Castle's job is configuration and release management on a running node.
   `RELEASES` file names at least `kernel` and `stdlib`. The remedy the message
   names is a restart, because that is the only thing that changes the answer.
 
+  **A restart is necessary and not always sufficient, so the message names the
+  state the file has to be in rather than just saying "restart".** The record is
+  synthesised when `RELEASES` was missing *or* could not be read, and Forecastle's
+  `env.sh` creates it only when it is **absent** (`[ ! -f ... ]`). So a file that
+  is present and unreadable is stepped over on every start: the node comes back on
+  a freshly synthesised record, the refusal repeats, and an operator following a
+  message that named only the restart would loop forever.
+
+  **State the required condition, not the boot-time cause.** The obvious
+  correction — branch the advice on why the record was synthesised, absent versus
+  unreadable — is wrong in a third case, and that was the first attempt here. The
+  file can have been absent at boot and been created, readably, since: the node
+  keeps its synthesised record either way, so the refusal still fires, and an
+  operator told to check whether the file is "absent" or "present and unreadable"
+  finds it is neither and has no applicable advice. A plain restart is exactly
+  right for them. So the message asks for `releases/RELEASES` to be *absent or
+  consultable* before the restart, which covers all three states and is shorter
+  than the branch it replaced. Do not turn it back into a case analysis of the
+  cause, and do not collapse it into a bare "restart the system" either.
+
+  **Name the authority, not the mechanism — and this is the lesson of five
+  successive corrections to one sentence.** Each named a property of the file and
+  each was necessary but not sufficient, so each admitted a narrower
+  counterexample: "restart" missed a file that was present and unreadable;
+  "present or absent" missed one created readably since boot; "readable" missed
+  malformed terms, because `init/1` reads it with `file:consult/1`; "consultable"
+  missed a file of two valid terms, because `init/1` accepts only `{ok, [Term]}`.
+  There is no reason to think that series had ended, and the hook leaves an
+  existing file alone whatever is in it, so every one of those states loops.
+
+  The message therefore asks for a file **`:release_handler` accepts**, and says
+  that no single property of the file is the test. That cannot be narrowed further
+  because it does not claim a mechanism, and it is what an operator needs anyway:
+  the handler is the thing that has to take the file. Do not "improve" it by
+  substituting whichever internal criterion is current — that is the move that was
+  wrong five times.
+
+  **And it must not name `releases/RELEASES` unqualified**, because that is the
+  file the *release* creates, not necessarily the one the handler reads — see
+  `Castle.Deployment.root_dir/0` and
+  [#23](https://github.com/ausimian/castle/issues/23). Where `RELDIR` or
+  `{sasl, releases_dir}` points elsewhere the two are different files, and the
+  remedy is then genuinely harder rather than merely differently spelled: the
+  hook creates one at the root that the handler will not read, so "absent" does
+  not get the operator out either, and the file the handler *does* read has to be
+  put there by hand. The message says so. When #23 lands and Castle follows those
+  overrides, this paragraph and that sentence both need revisiting — the
+  divergence is the thing being described, and it is the thing #23 removes.
+
   It has to be asked of the node rather than of the filesystem — a file that
   appeared *after* the boot that looked for it passes a shell test and still
   leaves the node on the synthesised record — and it has to be asked *in the call

RELEASE.md

@@ -47,16 +47,17 @@
 - `Castle.unpack/1` and `Castle.install/1` now refuse a system that cannot be
   upgraded from, and refuse it in the same call that would otherwise have done
   the work. `:release_handler` reads `releases/RELEASES` once, as it starts, and
-  when the file is not there it makes a release record up out of the boot
-  script's name and version — a record that names no applications at all.
+  when it cannot — the file absent, or there but not something it accepts — it
+  makes a release record up out of the boot script's name and version — a record
+  that names no applications at all.
   Upgrading a system in that state is worse than being stopped: the install
   reports success, and every application whose version changed but whose code the
   upgrade does not explicitly load goes on running its old code out of the
   directory of the release that was just replaced, until a later `remove` deletes
   it. Nothing can repair the running system afterwards, because creating the file
-  changes no record the node holds — so what the refusal says is to restart,
-  which is the one thing that does: the release creates the file before it
-  starts.
+  changes no record the node holds — so what the refusal says is to restart, with
+  the file either absent or accepted first. See *Fixed* below for what that
+  condition is and why a bare restart is not always enough.
 
   The question is asked of the node's own records rather than of the filesystem,
   which is the only way to see the case where the file exists but the boot that
@@ -70,7 +71,7 @@
   operation that *writes* release records: an unpack on such a node would put the
   made-up record into `releases/RELEASES`, where the next boot would read it back
   as though it belonged there — which takes away the restart that is the way out,
-  since the file is only created when it is missing. Committing and removing are
+  since the file is only created when it is absent. Committing and removing are
   unaffected: neither can write that record back, and refusing them could strand
   a version that was already installed.
 - `Castle.upgradable/0`, which answers the same question on its own, for an
@@ -150,6 +151,24 @@
 
 ### Fixed
 
+- The refusal for a system running from a synthesised release record now names a
+  remedy that works. It said to restart, and a restart alone is enough only when
+  the `RELEASES` file `:release_handler` reads is absent or accepted by the handler
+  itself: present, readable and parsing as Erlang terms are each necessary and none
+  of them sufficient. The release creates the file only when it is *absent*, so
+  anything left in place that the handler will not accept is stepped over on every
+  start and the system comes back on another synthesised record. An operator following the old message would have restarted
+  indefinitely.
+
+  The refusal now asks for that file to be absent or accepted before the restart,
+  and identifies it rather than assuming: `releases/RELEASES` under the release
+  root, unless `RELDIR` or the `sasl` `releases_dir` parameter points elsewhere.
+  Where one of those does, the two are different files and a restart cannot fix it
+  on its own — the release creates the one at the root, which the handler will not
+  read — so the file the handler *does* read has to be put there by hand. Castle
+  following those overrides itself is
+  [#23](https://github.com/ausimian/castle/issues/23).
+
 - A release built with `include_erts: false` is now refused, by name and with
   the reason, rather than quietly managing the Erlang installation it happens to
   be running on. Such a release ships no emulator, so it runs the system one, and

lib/castle/commands.ex

@@ -344,8 +344,20 @@ defmodule Castle.Commands do
            "not be read, when the system booted. An upgrade from that record reports " <>
            "success and leaves any application whose version changed, but whose code the " <>
            "upgrade does not load, running its old code. Creating the file now would not " <>
-           "change the record this node works from. Restart the system: the release " <>
-           "creates the file before it starts."}
+           "change the record this node works from, so the system has to be restarted. " <>
+           "Before restarting, make sure the RELEASES file :release_handler reads is " <>
+           "either absent or one that :release_handler itself accepts - being present, " <>
+           "being readable and parsing as Erlang terms are each necessary and none of " <>
+           "them sufficient, so the test that matters is whether the handler takes it, " <>
+           "not any one property of the file. The release creates that file only when it " <>
+           "is absent, so anything left in place that the handler will not accept is " <>
+           "stepped over on every start and the system comes back on the same " <>
+           "synthesised record. Absent, or accepted, is what a restart needs. That file " <>
+           "is " <>
+           "releases/RELEASES under the release root unless RELDIR or the sasl " <>
+           "releases_dir parameter points elsewhere; where one of those does, the release " <>
+           "creates a file at the root that the handler will not read, so the one it does " <>
+           "read has to be put there by hand."}
 
       nil ->
         {:error, "#{refusal}: no release is running."}

test/castle/commands_test.exs

@@ -85,7 +85,15 @@ defmodule Castle.CommandsTest do
       assert message =~ "This system cannot be upgraded: 1.2.3 is running from a release record"
       assert message =~ "names no applications"
       assert message =~ "running its old code"
-      assert message =~ "Restart the system: the release creates the file before it starts."
+      assert message =~ "make sure the RELEASES file :release_handler reads is"
+
+      # The remedy names the state the file has to be in, not the reason the
+      # record was synthesised. A bare "restart" loops forever on a file that is
+      # present and unreadable, because the hook that creates it is guarded on its
+      # absence - and a case analysis of the cause, which is what this said first,
+      # has no advice at all for a file that was absent at boot and has been
+      # created readably since, where a plain restart is all that is needed.
+      assert message =~ "Absent, or accepted, is what a restart needs."
     end
 
     test "asks the release the system is running, and not another one" do
@@ -136,7 +144,7 @@ defmodule Castle.CommandsTest do
 
       assert {:error, message} = Commands.unpack("sample-1.2.3", handler)
       assert message =~ "Cannot unpack sample-1.2.3: 1.2.2 is running from a release record"
-      assert message =~ "Restart the system"
+      assert message =~ "the system has to be restarted"
       assert Stub.calls(:unpack_release) == []
     end
 
@@ -193,7 +201,7 @@ defmodule Castle.CommandsTest do
       assert {:error, message} = Commands.install("1.2.3", handler)
       assert message =~ "Cannot install 1.2.3: 1.2.2 is running from a release record"
       assert message =~ "running its old code"
-      assert message =~ "Restart the system"
+      assert message =~ "the system has to be restarted"
       assert Stub.calls(:install_release) == []
     end