test: drive the concurrent install through the command boundary

The case that pins two callers not deciding each other's configuration
drove `Commands.install/5`, and the defect it names was a composition in
`Castle.install/1` - `materialise/3` and then the install, so both callers
configured the target before either reached the lock. Putting that
composition back therefore left the case green: it was asserting about a
function the defect was never in.

`Castle.install/1` now takes the releases directory, the handler, the peer
and the deployment as defaulted arguments, the way `Commands.install/5`
already did, and that one case runs two concurrent callers through the
boundary `bin/castle` actually reaches over `rpc`. `installer/3` takes
`through: :boundary` for it; the boundary prints what succeeded and raises
what failed, so `invoke/2` puts both back into the shape
`Commands.install/5` returns - `with_io/1` inside the task, because that is
whose group leader has to be swapped.

Verified by reintroducing the composition: the case fails on the second
caller's peer having been called and on the target holding its
configuration, which is exactly what it is for.

Every other concurrency case stays on `Commands.install/5`, which is the
right level for a claim about the serialised region itself. The arity-1
form is unchanged and nothing in a release passes the extra arguments, so
no caller chooses anything about the deployment.

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

Author: ausimian

AGENTS.md

@@ -533,6 +533,18 @@ Castle's job is configuration and release management on a running node.
   version directory and is idempotent; it ends in a rename onto `sys.config`, so
   it is not. See the restart-marker section for the whole of it.
 
+  **And that claim is now tested at the boundary, which is what
+  `Castle.install/2..5` is for.** `install` takes the releases directory, the
+  handler, the peer and the deployment as defaulted arguments, exactly as
+  `Commands.install/5` does, for one reason: a concurrency test that drives
+  `Commands.install/5` cannot see anything composed in `Castle.install/1`, so the
+  defect above was reintroducible with the whole suite green. One case in
+  `commands_test.exs` runs two concurrent callers through `Castle.install/5`
+  instead, and fails if a `materialise/3` reappears in front of the install. The
+  arity-1 form is unchanged and is still what `bin/castle` calls over `rpc`;
+  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
@@ -1154,6 +1166,20 @@ what this test fails against, because it is the point of it — not just
 materialising outside the lock, but materialising *inside* the lock and in front
 of `unclaimed/3`, which is the fix that looks sufficient and is not.
 
+**It is also the one case that runs through `Castle.install/5` rather than
+`Commands.install/5`, and that is not a detail.** The defect was a composition in
+`Castle.install/1`, so a case that only ever called `Commands.install/5` was
+asserting about a function the defect was not in: putting `materialise/3` back in
+front of the install left it green. `installer/3` takes `through: :boundary` for
+this one, which runs the command boundary — so the two callers are two `rpc`s,
+which is what they are in a deployment. The boundary prints what succeeded and
+raises what failed, so `invoke/2` puts both back into the shape
+`Commands.install/5` returns: `with_io/1` inside the task, because that is whose
+group leader has to be swapped, and an implicit-`try` `attempt/1` to turn
+`Castle.Error` back into an `{:error, message}`. Every other case here stays on
+`Commands.install/5`, which is the right level for a claim about the serialised
+region itself.
+
 **The exception path has tests of its own, and the seam is
 `Castle.ReleaseHandlerStub`'s function reply again.** A raise, an exit and a
 throw out of `install_release/1`, each asserted to leave no marker behind, with

RELEASE.md

@@ -230,6 +230,11 @@
   release built by Mix that is the file OTP writes; a deployment that sets
   `RELDIR` or the `sasl` `releases_dir` parameter moves the release records
   elsewhere, and Castle does not yet follow them.
+- `Castle.install/1` accepts four further arguments, all defaulted, naming the
+  releases directory and the modules it talks to. `Castle.install("1.2.3")` is
+  unchanged and is still what `bin/castle` calls; the arguments exist so that
+  concurrent installs can be exercised through the function an operator actually
+  invokes, rather than one layer below it.
 - `unpack/1`, `install/1`, `commit/1`, `remove/1` and
   `make_releases/0` now fail when the operation fails, instead of printing the
   reason and returning normally. These are invoked over `bin/castle`, which

lib/castle.ex

@@ -5,6 +5,7 @@ defmodule Castle do
 
   alias Castle.Commands
   alias Castle.Deployment
+  alias Castle.Peer
 
   # Every function in this module is a command entry point: `bin/castle` sends
   # each one to the running node over `bin/<release> rpc`, and the launcher's
@@ -56,8 +57,30 @@ defmodule Castle do
   # 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
-    report!(Commands.install(vsn, rel_dir()))
+  #
+  # **And that claim is tested here rather than one layer down, which is what the
+  # four defaulted arguments are for.** `Castle.Commands.install/5` already took
+  # the handler, the peer and the deployment so that its own suite could drive two
+  # concurrent callers through it; but a test that drives *it* cannot see anything
+  # composed in *this* function, so the composition that was the whole defect would
+  # have been reintroducible with every test still green. `rel_dir` joins them for
+  # the same reason it is an argument there - a suite needs a releases directory of
+  # its own to contend over, or the cases cannot run async - and the three module
+  # arguments follow it because a caller held at `which_releases/0` is the only
+  # seam the interleaving has.
+  #
+  # They are defaults rather than a separate entry point so that `bin/castle`
+  # keeps calling `Castle.install/1` over `rpc` and nothing about the deployment
+  # is chosen by a caller: see `rel_dir/0`.
+  def install(
+        vsn,
+        rel_dir \\ rel_dir(),
+        handler \\ :release_handler,
+        peer \\ Peer,
+        deployment \\ Deployment
+      )
+      when is_binary(vsn) do
+    report!(Commands.install(vsn, rel_dir, handler, peer, deployment))
   end
 
   def running(vsn) when is_binary(vsn) do

test/castle/commands_test.exs

@@ -1,6 +1,8 @@
 defmodule Castle.CommandsTest do
   use ExUnit.Case, async: true
 
+  import ExUnit.CaptureIO
+
   alias Castle.Commands
   alias Castle.DeploymentStub
   alias Castle.InitStub
@@ -796,6 +798,13 @@ defmodule Castle.CommandsTest do
       # the winner's marker. The install that was refused decided what the install
       # that succeeded booted.
       #
+      # **So this one goes through `Castle.install/5`, and that is the point of the
+      # arguments it takes.** Every other case here drives `Commands.install/5`,
+      # which is the right level for them - but the composition was one layer up,
+      # in the function `bin/castle` actually calls, and a case that never calls it
+      # would stay green while somebody put `materialise/3` back in front of the
+      # install. The two callers here are two `rpc`s, which is what they would be.
+      #
       # Two providers that yield *distinguishable* results is what makes it
       # visible. With both callers answering `{:ok, []}` the end state is identical
       # whichever of them ran, which is why every existing test passed against it.
@@ -805,6 +814,7 @@ defmodule Castle.CommandsTest do
         installer(dir, "1.2.2",
           as: :first,
           hold: true,
+          through: :boundary,
           install: prepares_then_reboots(dir),
           configure: configures("[{first, resolved}].\n")
         )
@@ -814,6 +824,7 @@ defmodule Castle.CommandsTest do
       second =
         installer(dir, "1.2.2",
           as: :second,
+          through: :boundary,
           configure: configures("[{second, resolved}].\n")
         )
 
@@ -1239,12 +1250,19 @@ defmodule Castle.CommandsTest do
   # results are told apart. That is the only way to see *whose* configuration a
   # version ended up with, which is the thing composing materialisation in front
   # of the lock got wrong.
+  #
+  # `through: :boundary` runs `Castle.install/5` instead of `Commands.install/5`.
+  # That distinction is load bearing rather than tidy: the defect was
+  # `Castle.install/1` composing `materialise/3` and the install, so a case that
+  # only ever calls `Commands.install/5` cannot see it come back. One case uses it,
+  # and says why.
   defp installer(rel_dir, from, opts) do
     test = self()
     name = Keyword.fetch!(opts, :as)
     lookup = lookup(test, name, from, Keyword.get(opts, :hold, false))
     reply = Keyword.get(opts, :install, {:ok, ~c"1.2.2", ~c"upgrade"})
     configure = Keyword.get(opts, :configure, {:ok, []})
+    through = Keyword.get(opts, :through, :commands)
 
     Task.async(fn ->
       Stub.stub(:which_releases, lookup)
@@ -1253,11 +1271,34 @@ defmodule Castle.CommandsTest do
 
       send(test, {:started, name})
 
-      {Commands.install("1.2.3", rel_dir, Stub, PeerStub), Stub.calls(:install_release),
-       PeerStub.calls()}
+      {invoke(through, rel_dir), Stub.calls(:install_release), PeerStub.calls()}
     end)
   end
 
+  defp invoke(:commands, rel_dir), do: Commands.install("1.2.3", rel_dir, Stub, PeerStub)
+
+  # Through `Castle.install/5`, which is the function `bin/castle` reaches over
+  # `rpc` and the only place a composition in front of the serialised region could
+  # live. It is a command boundary rather than an operation, so it *prints* what
+  # succeeded and *raises* what failed; both are turned back into the shape
+  # `Commands.install/5` returns so that a case can be written either way round.
+  #
+  # `with_io/1` rather than `capture_io/1` because the result is wanted as well as
+  # the output, and it runs in the task's own process because that is whose group
+  # leader has to be swapped.
+  defp invoke(:boundary, rel_dir) do
+    case with_io(fn -> attempt(rel_dir) end) do
+      {{:error, _} = refusal, _output} -> refusal
+      {:ok, output} -> {:ok, String.split(output, "\n", trim: true)}
+    end
+  end
+
+  defp attempt(rel_dir) do
+    Castle.install("1.2.3", rel_dir, Stub, PeerStub)
+  rescue
+    error in Castle.Error -> {:error, Exception.message(error)}
+  end
+
   # The `which_releases/0` a caller is given: it says that the lookup happened
   # and, when the caller is the one being held, waits there until it is let go.
   defp lookup(test, name, from, hold?) do