fix: serialise commit, and stop two refusals claiming nothing changed

Two findings from a Castle review of the restart-marker work.

commit/1 composed materialise-then-commit at the boundary with no lock, so a
duplicate install of the version being committed could configure it between
the two steps: the commit succeeds, that install then fails as already
installed, and its configuration is what the newly permanent release boots on
the next restart. A failed caller deciding what a successful one boots, which
is the failure this protocol exists to prevent, through the one operation
still outside the serialised region.

The reason it was left outside does not hold. AGENTS.md called putting commit
behind the install lock a deadlock dressed as caution, because an install
waiting on a reboot is when a commit is wanted - but an install never holds
the lock across a reboot. install_release/1 replies before init:reboot(), the
reboot runs in release_handler's process, and after a restart transition the
VM that held the lock is gone. The only thing commit can wait for is a hot
install still inside install_release/1, where waiting is correct.

So Commands.commit/5 takes rel_dir and materialises inside serialised/2, and
the refusal an ERTS-less deployment used to hear from commit changes from
naming the configuration step to naming commit itself. That asymmetry was the
visible symptom of the composition, and erts_guard_test now pins the new
direction, so a reappearance means a composition came back at the boundary.

Separately, arm/4's occupied and stale refusals both claimed nothing had been
changed. Materialisation runs before arming, so the target's configuration has
already been replaced by then and the claim was false. They now say what did
and did not happen. unclaimed/3's refusal keeps the phrase, because it fires
before materialising and there it is true.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: ausimian

AGENTS.md

@@ -545,10 +545,15 @@ Castle's job is configuration and release management on a running node.
   nothing about the deployment is chosen by a caller in a release, because nothing
   in a release passes the extra arguments.
 
-  `Castle.commit/1` does still compose materialise-then-commit, which is why an
-  ERTS-less deployment hears "Cannot configure" from `commit` and "Cannot
-  install" from `install`. That asymmetry is exact rather than untidy, and
-  `erts_guard_test.exs` pins both.
+  **`Castle.commit/1` composes nothing either, and the asymmetry this used to
+  describe is gone.** It said an ERTS-less deployment hears "Cannot configure"
+  from `commit` and "Cannot install" from `install`, and that the difference was
+  exact rather than untidy. It was exact, and it was also the visible symptom of
+  the same composition: `commit` materialised in front of the operation, so the
+  configuration step's guard answered first. `Commands.commit/5` now materialises
+  inside its own serialised region, so every command names itself.
+  `erts_guard_test.exs` pins that in the new direction — a "Cannot configure"
+  reappearing there would mean a composition had come back at the boundary.
 
   Every refusal still falls before `install_release/1` is asked for anything,
   which is the line that matters.
@@ -711,10 +716,28 @@ Castle's job is configuration and release management on a running node.
   is told a restart install is pending: the same message as before, said about a
   pair that is complete instead of said while taking half of it away.
 
-  Only `install` takes it. `unpack`, `commit` and `remove` arm nothing and hold no
-  two-file invariant of their own — `release_handler` serialising its own record
-  writes is the whole of what they need — and putting `commit` behind an install
-  that is waiting on a reboot would be a deadlock dressed as caution.
+  **`install` and `commit` take it; `unpack` and `remove` do not.** Those two arm
+  nothing and hold no two-file invariant of their own, and `release_handler`
+  serialising its own record writes is the whole of what they need.
+
+  `commit` was left out at first, on the argument that putting it behind an
+  install "waiting on a reboot" would be a deadlock dressed as caution. **That
+  was wrong, and the error was about when the lock is held rather than about
+  commit.** An install never holds it across a reboot: `install_release/1`
+  replies *before* `init:reboot()`, the reboot runs in `release_handler`'s own
+  process, so `Commands.install/5` returns and its `trans` releases while the
+  system is still up — and after a restart transition the VM that held the lock
+  is gone entirely. `bin/castle install` then polls `Castle.running/1` over
+  separate rpcs that take no lock at all. The only thing a commit can wait for is
+  a *hot* install still inside `install_release/1`, and waiting there is correct:
+  committing part-way through an upgrade is the thing not to do.
+
+  What the omission left open was reachable and is the failure this protocol
+  exists to prevent. A duplicate install of the version being committed
+  materialises between `commit`'s two steps; the commit succeeds; that install
+  then fails as already installed; and *its* configuration is what the newly
+  permanent release boots on the next restart. A failed caller deciding what a
+  successful one boots, through the one operation left outside the region.
 
   **Materialising the target's configuration is inside the region, and the
   argument for keeping it outside was wrong.** That argument was: it writes only
@@ -746,17 +769,24 @@ Castle's job is configuration and release management on a running node.
   front of `unclaimed/3` fixes nothing, and there is a test whose only job is to
   fail against exactly that arrangement.
 
-  **`Castle.commit/1` still materialises outside any lock, and that is a boundary
-  rather than a claim.** It is not the same case: commit 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 another caller to
-  land in. What is left open is an operator running `commit` of a version at the
-  same moment as an `install` of that same version, where the two renames onto one
-  `sys.config` are unordered. Nothing is known to do it, putting `commit` behind
-  the install lock would be the deadlock above, and a lock of its own on the
-  version directory would close it — that is the trade, written down rather than
-  taken.
+  **`commit` materialises inside the same region, and the argument for leaving it
+  outside was wrong twice over.** It went: commit is not the same case, because it
+  makes permanent a version this node already installed and is running, so there
+  is no marker, no reboot and no window between a configuration and a boot of it —
+  and putting it behind the install lock would be the deadlock above anyway.
+
+  The first half is true and does not license the second. The window is not
+  between a configuration and a *boot*; it is between commit's own two steps. A
+  duplicate install of the version being committed materialises there, the commit
+  succeeds, that install fails as already installed, and its configuration is what
+  the newly permanent release boots on the next restart. And the deadlock does not
+  exist — see above: an install never holds this lock across a reboot, so the only
+  thing commit can wait for is a hot install mid-`install_release/1`, which is
+  exactly when it should wait.
+
+  So `Commands.commit/5` takes `rel_dir` and materialises inside `serialised/2`,
+  the way `install` does, and `Castle.commit/1` composes nothing. The two renames
+  onto one `sys.config` are now ordered wherever they meet.
 
   **It is published the way `sys.config.pristine` is** — staged in an owner-only
   working directory and hard-linked into place — and for the same two reasons a
@@ -1281,19 +1311,9 @@ wrote, so Elixir's pipeline is still armed in the file the launcher reads.
   open again between them. Widening the lock does not fix it; a lock the
   filesystem holds would, at the price of a stale one after a hard kill blocking
   every later install. Nothing is known to do this, and Castle does not detect
-  it.
-- **`commit` materialises outside the install lock, so the two renames onto one
-  `sys.config` are unordered where they meet.** `Castle.install/1` now
-  materialises inside its serialised region, which is what stops two installs
-  deciding each other's configuration. `Castle.commit/1` still composes
-  materialise-then-commit, and the case left open is an operator running a
-  `commit` of some version at the same moment as an `install` of that *same*
-  version — a narrow one, since commit is for a version this node has already
-  installed and is running. Putting `commit` behind the install lock would be a
-  deadlock dressed as caution, since an install waiting on a reboot is exactly
-  when a commit is wanted; a second lock on the version directory, taken by
-  materialisation itself, would close it. That is the trade and it has not been
-  taken. Nothing is known to do this, and Castle does not detect it.
+  it. It is the one limitation here that a lock cannot narrow, which is why the
+  filesystem half of the protocol — `publish/2` refusing rather than replacing —
+  has to stand on its own.
 - **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

@@ -164,10 +164,15 @@
   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.
+  directories and refuses without touching anything.
+
+  `commit` is serialised the same way, and for a reason that is not obvious: it
+  configures the version too, so a duplicate install of the version being
+  committed could configure it between commit's two steps - the commit would
+  succeed, that install would then fail as already installed, and its
+  configuration would be what the newly permanent release booted on the next
+  restart. A failed caller deciding what a successful one boots. `unpack` and
+  `remove` are not serialised: they configure nothing and arm nothing.
 
   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 -

lib/castle.ex

@@ -88,8 +88,7 @@ defmodule Castle do
   end
 
   def commit(vsn) when is_binary(vsn) do
-    materialise(vsn)
-    report!(Commands.commit(vsn))
+    report!(Commands.commit(vsn, rel_dir()))
   end
 
   def remove(vsn) when is_binary(vsn) do
@@ -100,33 +99,40 @@ defmodule Castle do
     report!(Commands.releases())
   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.
-  # 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.
+  # **Nothing here composes materialisation any more, and neither entry point may
+  # start again.** It is a *replace*: the last thing it does is rename the
+  # resolved configuration onto `sys.config`. In front of an operation it is two
+  # steps another caller can get between, which is why `Commands.install/5` took
+  # it back inside its own lock — and `commit/1` has now followed, into
+  # `Commands.commit/5`.
   #
-  # **`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)))
+  # `commit` was left out on the argument that it is different in kind: it makes
+  # permanent a version this node already installed and is running, so there is no
+  # marker, no reboot and no window between a configuration and a boot of it. The
+  # part that was wrong is what the argument then concluded — that putting commit
+  # behind the install lock would be "a deadlock dressed as caution, since an
+  # install waiting on a reboot is exactly when a commit is wanted". **An install
+  # never holds the lock while waiting on a reboot.** `install_release/1` replies
+  # before `init:reboot()` and the reboot runs in `release_handler`'s process, so
+  # `Commands.install/5` returns and its `trans` releases before the node goes
+  # down; `bin/castle install` then polls `Castle.running/1` over separate rpcs
+  # that take no lock, and after a restart transition the VM that held it is gone
+  # entirely. The only thing a commit can now wait for is a hot install still
+  # inside `install_release/1` — and waiting there is right, because committing
+  # part-way through an upgrade is what should not happen.
+  #
+  # What the composition actually left open was the reachable case: a duplicate
+  # install of the version being committed, materialising between the two calls.
+  # The commit succeeds, that install then fails as already installed, and its
+  # configuration is what the newly permanent release boots on the next restart —
+  # a failed caller deciding what a successful one boots, which is the failure
+  # this protocol exists to prevent, reachable through the one operation left
+  # outside it.
 
-  # The release directory, and the version directory of the release being
-  # operated on beneath it. Derived, never chosen by the caller: which file the
+  # The release directory. Derived, never chosen by the caller: which file the
   # configuration lands in, and which file the release records go in, are
   # properties of the installation rather than arguments, and a caller's working
-  # directory cannot make them name different ones. The version directory
-  # resolves for any version the running release knows about, because
-  # `:release_handler` unpacks every version into this same root.
+  # directory cannot make them name different ones.
   #
   # `Castle.Deployment.root_dir/0` says what that root does and does not decide,
   # and is the one place that says it. The part that bears on these two: it is
@@ -139,7 +145,6 @@ defmodule Castle do
   # `RELEASE_ROOT` - which is the one deployment where this derivation names the
   # wrong tree, and where every operation that would act on it refuses.
   defp rel_dir, do: Path.join(Deployment.root_dir(), "releases")
-  defp rel_vsn_dir(vsn), do: Path.join(rel_dir(), vsn)
 
   defp report!({:ok, lines}), do: Enum.each(lines, &IO.puts/1)
   defp report!({:error, message}), do: raise(Castle.Error, message)

lib/castle/commands.ex

@@ -1251,7 +1251,9 @@ defmodule Castle.Commands do
       "reboot, and there is already #{describe_type(type)} at that path. Castle " <>
       "will not write through it or replace it, and there is nowhere else it can " <>
       "arm the install - that path is what the launcher reads on the next start. " <>
-      "Nothing has been changed. Move whatever is there out of the way."
+      "The upgrade did not happen and nothing was made permanent, but the target's " <>
+      "configuration has already been expanded: that is the step before this one. " <>
+      "Move whatever is there out of the way."
   end
 
   defp describe_type(:directory), do: "a directory"
@@ -1264,7 +1266,8 @@ defmodule Castle.Commands do
       "be cleared first (#{:file.format_error(reason)}). It has to be, because one " <>
       "left by an earlier attempt would pair with the marker this install is about " <>
       "to arm and tell the launcher to boot a version that was never installed. " <>
-      "Nothing has been changed."
+      "The upgrade did not happen and nothing was made permanent, but #{vsn}'s " <>
+      "configuration has already been expanded: that is the step before this one."
   end
 
   defp unarmed(marker, reason, refusal) do
@@ -1395,9 +1398,41 @@ defmodule Castle.Commands do
   nowhere else, which is the whole of the rollback property: until this runs, a
   restart returns to the version that was permanent before.
   """
-  @spec commit(String.t(), module(), module()) :: result()
-  def commit(vsn, handler \\ :release_handler, deployment \\ Castle.Deployment) do
+  @spec commit(String.t(), Path.t(), module(), module(), module()) :: result()
+  def commit(
+        vsn,
+        rel_dir,
+        handler \\ :release_handler,
+        peer \\ Peer,
+        deployment \\ Castle.Deployment
+      ) do
     with :ok <- ensure_own_erts("Cannot commit #{vsn}", deployment) do
+      serialised(rel_dir, fn ->
+        commit_materialised(vsn, rel_dir, handler, peer, deployment)
+      end)
+    end
+  end
+
+  # Materialising and committing under the *same* lock an install takes, and for
+  # the reason install takes it: both rename a `sys.config` into the version
+  # directory, and whichever renames last decides what the version boots.
+  #
+  # This composed at the boundary until it was found to be racy. A duplicate
+  # install of the version being committed could materialise between the two
+  # calls here; the commit would then succeed, that install would fail as already
+  # installed, and its configuration would be left as the configuration the newly
+  # permanent release boots on the next restart. A failed caller deciding what a
+  # successful one boots is the failure this whole protocol exists to prevent, and
+  # it was reachable through the one operation that had been left outside.
+  #
+  # **It cannot deadlock against an install, and the earlier belief that it could
+  # was wrong.** `install_release/1` replies before `init:reboot()` and the reboot
+  # runs in `release_handler`'s process, so `Commands.install/5` returns and its
+  # `trans` releases well before the node goes down - the lock is never held
+  # across a restart. `bin/castle install` then polls `Castle.running/1` through
+  # separate rpcs, none of which takes this lock at all.
+  defp commit_materialised(vsn, rel_dir, handler, peer, deployment) do
+    with {:ok, _} <- materialise(Path.join(rel_dir, vsn), peer, deployment) do
       case handler.make_permanent(to_charlist(vsn)) do
         :ok -> {:ok, ["Committed #{vsn}. System restarts will now boot into this version."]}
         {:error, reason} -> {:error, "Commit of #{vsn} failed. #{inspect(reason)}"}