fix: name a remedy that works for an unreadable RELEASES

AGENTS.md

@@ -437,6 +437,38 @@ 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
+  readable* 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.
+
+  **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

@@ -150,6 +150,14 @@
 
 ### 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
+  `releases/RELEASES` is absent or readable: the release creates that file when it
+  is missing, so a file that is present but unreadable 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. It now asks for the
+  file to be absent or readable before the restart.
+
 - 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,16 @@ 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 readable or absent, because the release creates that file only when it " <>
+           "is absent - an unreadable one left in place is stepped over on every start " <>
+           "and the system comes back on the same synthesised record. Absent, or " <>
+           "readable, 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 " <>
+           "and made readable 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 readable, 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