fix: serialise materialisation and settle the marker on every exit

Three round-three review findings, two of them in this repo.

Materialisation raced the serialised install. `Castle.install/1` composed
`Commands.materialise/3` and then `Commands.install/4`, so two callers
both configured the target before either reached the lock. The argument
for keeping it outside was that its primitives refuse rather than
replace; that is true of the staging and of `sys.config.pristine` and
false of the step that matters, because materialising *ends* in a rename
onto `sys.config` - a replace by design, since that is the file
`:release_handler` reads. So the loser's providers could overwrite the
configuration the winner's provisional release was about to boot, after
which the loser was refused for the winner's marker: a refused install
decided what a successful one booted. Providers are not obliged to agree
across evaluations, which is why `sys.config.pristine` exists at all.

Materialising is now the third step inside `Commands.install/5`, after
the record check and after the pending-marker refusal, so a caller that
is going to be refused configures nothing. Inside the lock alone would
not have been enough: ahead of `unclaimed/3` it would still have replaced
the configuration on its way to being told no, and there is a test whose
only job is to fail against that arrangement. The boundary moved out with
it - `Castle.install/1` is one call, so "an install is serialised" is
true of the public entry point rather than of a part of it. Only the ERTS
guard stays outside, which changes what an ERTS-less deployment is told
by `install`: "Cannot install", not "Cannot configure".

An exceptional failure left an actionable marker pair. Cleanup ran only
when `install_release/1` returned an error or an unrecognised value, so
an exit, a throw or a raise skipped it - and where
`prepare_restart_new_emulator/7` had already written
`new_start_erl.data`, that left the complete pair the launcher acts on.
The region is now an implicit `try` with `catch` and `else` clauses, not
`after`: an `after` cannot see which way the block went and would disarm
the successful restart install too, taking away the marker whose whole
purpose is to outlive the call.

Failing to settle the marker is now reported rather than swallowed.
Ignoring `File.rm/1` rested on a directory the marker cannot be removed
from being one it could not have been linked into, which holds only if
nothing changed in between - and `install_release/1` runs in between.
An unreadable marker was worse than ignored, it was classified as another
attempt's and left alone. `disarm/3` now answers four ways: ours is
removed and a failed removal reported, theirs is left, gone is success,
and unverifiable is reported. What the operator is told names the file,
says OTP's own file may be beside it, says an ordinary restart will
therefore boot the version the install did not finish, and asks for the
marker to be removed first.

The read and the removal go through `Castle.Deployment`, for the reason
`stat/1` is there: the answers that decide what Castle says are the
failing ones, and every fixture that makes them fail uses a mode, which
root and some filesystems ignore.

Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

Author: ausimian

AGENTS.md

@@ -145,10 +145,33 @@
   either had, and the second would clear the `new_start_erl.data` the first one's
   reboot depends on - leaving the first system to come back on the version it was
   upgrading away from, while the second reported that nothing had been changed.
-  An install that has to wait waits, and then finds the first one's marker. What
-  is serialised is the whole operation and not only the arming: which kind of
-  transition an install is, is decided from the release the system is running,
-  and another install completing in between would change that answer.
+  An install that has to wait waits, and then finds the first one's marker.
+
+  What is serialised is `Castle.install/1` itself, and that includes
+  materialising the target's configuration. It is worth saying which parts, since
+  "the whole operation" was claimed here while the configuration step was still
+  outside: materialising ends by renaming a resolved configuration onto the
+  target's `sys.config`, so two callers doing it before either reached the lock
+  meant the loser's config providers - evaluated in a VM of their own, with
+  whatever environment that caller had - could replace the configuration the
+  winner's provisional release was about to boot, after which the loser was
+  refused for the winner's marker. The install that was refused decided what the
+  install that succeeded booted. Configuration providers are not obliged to
+  produce the same answer twice, which is the reason `sys.config.pristine` exists
+  in the first place.
+
+  So the region now runs from the release-record lookup through the
+  configuration step to `install_release/1` and the marker being settled, and a
+  caller that is going to be told a restart install is pending is told *before*
+  it configures anything. Only the ERTS guard is outside, because it reads two
+  directories and refuses without touching anything. `Castle.commit/1` still
+  configures outside any lock, which is a boundary rather than an oversight - it
+  makes permanent a version this node installed and is running, with no marker,
+  no reboot and no window between a configuration and a boot of it.
+
+  Which kind of transition an install is, is decided from the release the system
+  is running, and another install completing in between would change that answer -
+  which is the other reason the region reaches past the arming.
 
   A restart install while another one is already pending
   is refused rather than allowed to take over its marker, saying so and changing
@@ -161,6 +184,24 @@
   into place, the way the pristine configuration above is, so no start can read a
   marker that is half written and a race is refused rather than silently won.
 
+  The marker is settled on **every** way out of the install, including the ones
+  that do not return: an exit, a throw or a raise out of `install_release/1` is
+  caught, the marker dealt with, and the failure then let out unchanged. Before,
+  only a returned error cleared it - so an exception left the marker armed, and
+  where `:release_handler` had already written its own file the pair was complete
+  and the next start booted a version whose install had blown up.
+
+  And an install that cannot settle its marker now **says so, and says what it
+  means**, rather than reporting the original failure alone. A marker Castle
+  could not remove, or could not read well enough to tell whether it was still
+  its own, is a live instruction to the next start of that system: the failure
+  message names the file, says that `new_start_erl.data` may already be beside
+  it, says that an ordinary restart will therefore boot the version the install
+  did not finish, and asks for the marker to be removed first. Clearing it used
+  to be best effort on the argument that a directory the marker cannot be removed
+  from is one it could not have been linked into - which holds only if nothing
+  changed in between, and `install_release/1` runs in between.
+
   What the install *reports* is different for such a transition, because
   `install_release/1` replies the same `{ok, Vsn, Descr}` for a completed hot
   upgrade and for one that is about to reboot. Rather than say "Now running", it

RELEASE.md

@@ -42,8 +42,21 @@ defmodule Castle do
     report!(Commands.unpack(name))
   end
 
+  # Materialising is *inside* `Commands.install/5`, and this composing it here is
+  # the bug that put it there. Materialisation ends in a rename onto the target's
+  # `sys.config` - a replace by design, and it has to be, because that is the file
+  # `:release_handler` reads - so it is not the harmless idempotent work the note
+  # below used to call it. Two callers here both materialised before either
+  # entered the serialised region, and the loser's providers - evaluated in a
+  # second VM, with whatever environment that call had - overwrote the
+  # configuration the winner's provisional release was about to boot, after which
+  # the loser was refused for the winner's marker. The refused install decided the
+  # configuration of the one that succeeded.
+  #
+  # So there is nothing to compose: `Castle.install/1` is one call, and "an
+  # install is serialised" is now true of *this* function rather than of a part of
+  # it. See `Castle.Commands.install/5` and `serialised/2`.
   def install(vsn) when is_binary(vsn) do
-    materialise(vsn)
     report!(Commands.install(vsn, rel_dir()))
   end
 
@@ -65,23 +78,23 @@ defmodule Castle do
   end
 
   # Makes sure the target version's configuration exists before the version is
-  # handed to `:release_handler`, and fails here if it cannot be made to. It
-  # runs ahead of both operations that need it, and everything about the target
-  # that can refuse to go on - a peer that will not start, a boot script that is
-  # not there, a provider that raises - refuses from inside this call.
-  #
-  # `Commands.install/3` then refuses a running node whose release record OTP
-  # synthesised, which is a fact about this node rather than about the target,
-  # and so cannot be answered here. Both refusals are before `install_release/1`
-  # has been asked for anything, which is the line that matters: nothing after
-  # that point may fail without saying that an install happened.
+  # handed to `:release_handler`, and fails here if it cannot be made to.
+  # Everything about the target that can refuse to go on - a peer that will not
+  # start, a boot script that is not there, a provider that raises - refuses from
+  # inside this call.
   #
-  # The order means a node that will be refused for its record materialises the
-  # target's configuration before it hears so. That is what the record check
-  # costs by living inside the operation instead of in front of it, and it is
-  # only work: materialising writes into the target's version directory, never to
-  # the running system and never to a release record, and it is idempotent, so
-  # the refusal still leaves the system exactly as it was.
+  # **`commit/1` is the only caller, and `install/1` must not become one again.**
+  # This is a *replace*: the last thing it does is rename the resolved
+  # configuration onto `sys.config`. Composed in front of an operation it turns
+  # into two steps that another caller can get between, which is exactly what
+  # `Commands.install/5` had to take back inside its own lock. `commit/1` is
+  # different in kind rather than merely luckier - it makes permanent a version
+  # this node already installed and is running, so materialising produces what a
+  # boot at commit time would produce, and there is no marker, no reboot and no
+  # window between a configuration and a boot of it for a second caller to land
+  # in. Putting `commit` behind the install lock would instead be a deadlock
+  # dressed as caution, since an install waiting on a reboot is exactly when a
+  # commit is wanted.
   defp materialise(vsn), do: report!(Commands.materialise(rel_vsn_dir(vsn)))
 
   # The release directory, and the version directory of the release being