Merge pull request #31 from ausimian/issue/29-error-messages

Tighten operator-facing diagnostics

Author: ausimian

AGENTS.md

@@ -23,6 +23,9 @@ Forecastle 1.x and Elixir 1.18 or later.
 
 - Release-management commands now raise on refusal or a returned OTP error, so
   `bin/castle` exits non-zero. Successful command output is unchanged.
+- Operator-facing errors and warnings are shorter, distinguish preflight
+  refusals from attempted operations, report whether the configuration step ran,
+  and preserve paths, reasons and recovery steps.
 - `make_releases/0` now derives the release directory from the running emulator
   instead of the current working directory.
 - The minimum supported Elixir version is now 1.18.
@@ -41,9 +44,17 @@ Forecastle 1.x and Elixir 1.18 or later.
   the shared Erlang installation.
 - Give actionable recovery instructions when `:release_handler` booted without
   an accepted `RELEASES` file.
-- Fix the wording when a restart install is refused because something unusual,
-  such as a named pipe, is already at the path Castle uses for its restart
-  marker. The message used to read "a other".
+- Clarify restart-marker failures, including whether the configuration step ran,
+  whether `new_start_erl.data` was removed or already absent, and when release
+  records, `castle-restart-pending` and `new_start_erl.data` must be inspected
+  before restarting, retrying or removing a marker. Also explain the `unpacked`
+  record left by an unfinished install and fix the former "a other" wording for
+  named pipes and similar marker-path conflicts.
+- Report a failed commit as possibly partial instead of claiming the version was
+  not made permanent. `:release_handler` writes `releases/start_erl.data` before
+  it updates the release record, so an error can leave the file that selects the
+  boot version already naming the target; the message now says so and directs
+  the operator to inspect release state.
 - Report restart installs without raising `CaseClauseError`.
 - Return an empty release list without raising `Enum.EmptyError`.
 - Report `RELEASES` read and write errors instead of raising `MatchError`.

RELEASE.md

@@ -99,9 +99,10 @@ defmodule Castle do
   # to unpack - and that failure names a missing file rather than the release
   # option that did not ask for it.
   #
-  # It states the omission and stops. Nothing here knows the release was meant
-  # to be distributed: the system an upgrade is installed *onto* needs no
-  # tarball of its own, and `:tar` is Mix's own way of packing one - its
+  # It states the omission and preserves the valid qualifications. Nothing here
+  # knows the release was meant to be distributed: the system an upgrade is
+  # installed *onto* needs no tarball of its own, and `:tar` is Mix's own way of
+  # packing one - its
   # `make_tar/1` is private to `Mix.Tasks.Release` - rather than the only way,
   # so a function step in the list may be packing one itself.
   #
@@ -120,18 +121,15 @@ defmodule Castle do
   # acknowledged the counterexample in a trailing sentence without retracting
   # either claim, which is the worst of both: a definite diagnosis on the error
   # channel sending an operator to investigate a packaging failure that may not
-  # exist. Say what was seen, say what follows *unless* something else packs it,
-  # and stop.
+  # exist. Say what was seen, say what follows unless something else packs it,
+  # and keep the valid tarball-free base-deployment case explicit.
   defp warn_missing_tar(steps) do
     if :assemble in steps and :tar not in steps do
       Mix.shell().error(
-        "warning: Castle.customize/1 was given a :steps list with no :tar step. " <>
-          "Unless a step of your own packs one, this release will not produce the " <>
-          "<name>-<vsn>.tar.gz that is copied into a deployment's releases " <>
-          "directory for bin/castle unpack to read - and bin/castle unpack is how " <>
-          "a version is installed onto a running system. Add :tar after :assemble " <>
-          "if this version is meant to be installed anywhere. A deployment that is " <>
-          "only ever upgraded *from* needs no tarball of its own."
+        "warning: release :steps has no :tar step. Add :tar after :assemble to create " <>
+          "the <name>-<vsn>.tar.gz used by bin/castle unpack. No change is needed if " <>
+          "another step creates the archive. A deployment used only as an upgrade base needs " <>
+          "no tarball of its own."
       )
     end
   end

lib/castle.ex

@@ -9,19 +9,20 @@ defmodule Castle.Deployment do
   # the real rule over substituted inputs, the way the release-record check
   # exercises the real rule over a substituted `which_releases/0`.
   #
-  # Two roots, for the ERTS guard, and three filesystem operations: the `stat/1`
-  # that guard falls back to, and the `read/1` and `rm/1` that settle the restart
-  # marker's ownership on the way out of a failed install. All three are here for
-  # one reason - the answers that matter are the *failing* ones, and every way of
-  # arranging a failing `read` or `rm` from a fixture is a mode that root and some
-  # filesystems ignore. See `stat/1`.
+  # Two roots, for the ERTS guard, and four filesystem operations: the `stat/1`
+  # that guard falls back to, the `lstat/1` that classifies the restart-marker
+  # path before configuration changes, and the `read/1` and `rm/1` that settle
+  # marker ownership after a failed install. All four are here for one reason -
+  # the answers that matter are the *failing* ones, and every fixture that relies
+  # on a permission failure depends on a mode that root and some filesystems
+  # ignore. See `stat/1`.
   #
   # **This is not a general filesystem seam and must not become one.** The
   # primitives that *publish* the marker - `Castle.Peer.work_dir/1`,
   # `write_private/2` and `publish/2` - are deliberately called directly and not
   # through here: what they guarantee is the point of them, and a stub would
-  # prove nothing about it. What these two carry is the opposite kind of thing,
-  # an outcome Castle has to have something to say about and no way to cause.
+  # prove nothing about it. These operations carry the opposite kind of thing:
+  # outcomes Castle has to describe and no reliable way to cause in a fixture.
 
   @doc """
   The root `:release_handler` resolves its own relative paths against.
@@ -81,6 +82,18 @@ defmodule Castle.Deployment do
   @spec stat(Path.t()) :: {:ok, File.Stat.t()} | {:error, File.posix()}
   def stat(path), do: File.stat(path)
 
+  @doc """
+  Inspects a path without following its final symbolic link.
+
+  The restart-marker preflight has to distinguish a missing path, a regular
+  marker and some other occupant. A failure to inspect is a fourth state, but it
+  cannot be produced reliably with permissions in a test. Keeping this read here
+  lets the lifecycle test establish that Castle refuses before configuration or
+  `install_release/1` is reached.
+  """
+  @spec lstat(Path.t()) :: {:ok, File.Stat.t()} | {:error, File.posix()}
+  def lstat(path), do: File.lstat(path)
+
   @doc """
   Reads a file, for deciding whether the restart marker is still this attempt's.
 

lib/castle/commands.ex

@@ -0,0 +1,21 @@
+defmodule Castle.FileReason do
+  @moduledoc false
+
+  @spec format(term()) :: String.t()
+  def format(reason) when is_atom(reason) do
+    formatted = reason |> :file.format_error() |> to_string()
+
+    if contains_identity?(formatted, reason) do
+      formatted
+    else
+      "#{inspect(reason)} (#{formatted})"
+    end
+  end
+
+  def format(reason), do: inspect(reason)
+
+  defp contains_identity?(formatted, reason) do
+    identity = reason |> Atom.to_string() |> Regex.escape()
+    Regex.match?(Regex.compile!("(?<![[:alnum:]_])#{identity}(?![[:alnum:]_])"), formatted)
+  end
+end

lib/castle/deployment.ex

@@ -1,6 +1,8 @@
 defmodule Castle.Peer do
   @moduledoc false
 
+  alias Castle.FileReason
+
   # Materialises the configuration of the release being upgraded *to*, by
   # running that release's own `Config.Provider` pipeline in a temporary VM
   # booted from that release's own boot script and code.
@@ -468,7 +470,7 @@ defmodule Castle.Peer do
         {:error, "Cannot read #{path} as a release file. It holds #{inspect(terms)}."}
 
       {:error, reason} ->
-        {:error, "Cannot read #{path}. #{format_error(reason)}"}
+        {:error, "Cannot read #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -516,7 +518,7 @@ defmodule Castle.Peer do
         release_file(rel_vsn_dir, Enum.filter(entries, &String.ends_with?(&1, ".rel")))
 
       {:error, reason} ->
-        {:error, "Cannot list #{rel_vsn_dir}. #{format_error(reason)}"}
+        {:error, "Cannot list #{rel_vsn_dir}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -529,16 +531,17 @@ defmodule Castle.Peer do
   end
 
   defp release_file(rel_vsn_dir, names) do
-    case unpacked_copy(names, Path.basename(rel_vsn_dir)) do
+    vsn = Path.basename(rel_vsn_dir)
+
+    case unpacked_copy(names, vsn) do
       {:ok, name} ->
         {:ok, Path.join(rel_vsn_dir, name)}
 
       :none ->
         {:error,
-         "Found more than one release file in #{rel_vsn_dir} - #{Enum.join(names, ", ")}. An " <>
-           "unpacked version directory holds two, a release file and the copy unpacking " <>
-           "leaves beside it, and these are not that pair - so the emulator to evaluate its " <>
-           "configuration with is ambiguous."}
+         "Cannot determine the release file in #{rel_vsn_dir}: found " <>
+           "#{Enum.join(names, ", ")}. Expected one file or an unpacked " <>
+           "<name>.rel/<name>-#{vsn}.rel pair."}
     end
   end
 
@@ -640,13 +643,14 @@ defmodule Castle.Peer do
     end
   end
 
-  ## Writing a file that holds configuration
+  ## Private staging primitives
   #
-  # Every file this module creates holds a release's configuration - the base,
-  # and the scratch copy the providers are resolved into - so none of them may be
-  # readable by anyone the `sys.config` it came from or is about to become would
-  # not let read it. An operator who restricts that file has said something, and
-  # it has to hold for the copies too.
+  # The configuration path uses these for the base and scratch files, and the
+  # restart protocol uses them for its marker. The primitives enforce filesystem
+  # facts only; their callers say which lifecycle operation was interrupted and
+  # what had already changed. Configuration files must not become readable by
+  # anyone `sys.config` excludes, and the same owner-only staging also protects a
+  # marker from being replaced before publication.
   #
   # What protects them is the *directory* they are made in, and it has to be,
   # because OTP cannot create a file with a mode. `:file.open/2`'s modes say how
@@ -822,18 +826,14 @@ defmodule Castle.Peer do
     case File.ls(path) do
       {:ok, []} -> :ok
       {:ok, entries} -> {:error, occupied(path, entries)}
-      {:error, reason} -> {:error, "Cannot list #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot list #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp occupied(path, entries) do
-    "Cannot assemble configuration in #{path}. Castle had just created that " <>
-      "directory and written nothing to it, and it already holds " <>
-      "#{Enum.join(entries, ", ")} - so something else can write where this " <>
-      "release's configuration is about to be, and a name planted there is a name " <>
-      "the configuration could be written through. Nothing has been written and " <>
-      "the directory has been removed. Check the umask the release runs under: a " <>
-      "new directory has to be private to the account doing the install."
+    "Cannot use #{path}: newly created directory contains #{Enum.join(entries, ", ")}. " <>
+      "Castle removed it without writing to it. " <>
+      "Check that the release umask creates owner-only directories."
   end
 
   @doc false
@@ -866,7 +866,7 @@ defmodule Castle.Peer do
   def create_exclusive(path) do
     case File.open(path, [:write, :exclusive, :raw]) do
       {:ok, handle} -> {:ok, handle}
-      {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot create #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -898,14 +898,14 @@ defmodule Castle.Peer do
   defp written(handle, path, bytes) do
     case :file.write(handle, bytes) do
       :ok -> :ok
-      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp closed(handle, path) do
     case File.close(handle) do
       :ok -> :ok
-      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -915,14 +915,14 @@ defmodule Castle.Peer do
     case File.ln(staging, path) do
       :ok -> :ok
       {:error, :eexist} -> :taken
-      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp carry_mode(from, to) do
     case File.stat(from) do
       {:ok, %File.Stat{mode: mode}} -> chmod(to, Bitwise.band(mode, 0o7777))
-      {:error, reason} -> {:error, "Cannot read #{from}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot read #{from}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -1082,15 +1082,15 @@ defmodule Castle.Peer do
 
   ## Files
 
-  # Whether a file holding configuration may be created here at all: nothing
-  # granted to group or other, so there is no path through this directory for
-  # anyone else to open what is inside it by. Checked on every creation rather
-  # than assumed of the working directory, because assuming it at the call sites
-  # is the mistake this whole arrangement exists to make impossible.
+  # Whether a private staging file may be created here at all: nothing granted to
+  # group or other, so there is no path through this directory for anyone else to
+  # open what is inside it by. Checked on every creation rather than assumed of
+  # the working directory, because assuming it at the call sites is the mistake
+  # this whole arrangement exists to make impossible.
   defp private_dir(path) do
     case File.stat(path) do
       {:ok, %File.Stat{mode: mode}} -> private_mode(path, Bitwise.band(mode, 0o7777))
-      {:error, reason} -> {:error, "Cannot read #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot read #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -1099,18 +1099,16 @@ defmodule Castle.Peer do
       :ok
     else
       {:error,
-       "Cannot write in #{path}, whose mode is 0#{Integer.to_string(mode, 8)}. A file holding " <>
-         "a release's configuration is only ever created in a directory Castle has made " <>
-         "owner-only, so that nothing can open it while it is being written. Castle chmods " <>
-         "that directory to 0700 as it creates it, and a wider mode than that means the " <>
-         "filesystem holding the release did not take it."}
+       "Cannot write in #{path}: directory mode " <>
+         "0#{Integer.to_string(mode, 8)} allows access beyond its owner. Castle requires " <>
+         "owner-only mode 0700; check the release filesystem and permissions."}
     end
   end
 
   defp mkdir(path) do
     case File.mkdir(path) do
       :ok -> :ok
-      {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot create #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -1124,7 +1122,7 @@ defmodule Castle.Peer do
   defp read(path) do
     case File.read(path) do
       {:ok, contents} -> {:ok, contents}
-      {:error, reason} -> {:error, "Cannot read #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot read #{path}. #{FileReason.format(reason)}"}
     end
   end
 
@@ -1137,31 +1135,31 @@ defmodule Castle.Peer do
         {:error, "Cannot read #{path}: expected one configuration term, found #{length(terms)}."}
 
       {:error, reason} ->
-        {:error, "Cannot read #{path}. #{format_error(reason)}"}
+        {:error, "Cannot read #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp write(path, contents) do
     case File.write(path, contents) do
       :ok -> :ok
-      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp chmod(path, mode) do
     case File.chmod(path, mode) do
-      :ok -> :ok
-      {:error, reason} -> {:error, "Cannot set the mode of #{path}. #{format_error(reason)}"}
+      :ok ->
+        :ok
+
+      {:error, reason} ->
+        {:error, "Cannot set the mode of #{path}. #{FileReason.format(reason)}"}
     end
   end
 
   defp rename(source, destination) do
     case File.rename(source, destination) do
       :ok -> {:ok, []}
-      {:error, reason} -> {:error, "Cannot write #{destination}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{destination}. #{FileReason.format(reason)}"}
     end
   end
-
-  defp format_error(reason) when is_atom(reason), do: :file.format_error(reason)
-  defp format_error(reason), do: inspect(reason)
 end