fix: accept the two release files an unpacked release has

Castle.Peer.release_file/1 listed the version directory, filtered for
*.rel and required exactly one, refusing anything else as ambiguous. An
unpacked release always holds two, so the peer path refused every
release it will ever be asked to configure - found by Forecastle's e2e
suite the first time it drove this path.

Mix writes one release file into the version directory, <name>.rel, the
one belonging to the start script. release_handler adds a second on
unpack: it extracts the tarball's <name>-<vsn>.rel, reads that to decide
what the version is - check_rel on the way in, and RELEASES written from
the record it returns - and then copies it in beside Mix's, "keeping
this for backwards compatibility reasons with older systools:make_tar,
where there is no copy of the .rel file in the releases/<vsn> dir. See
OTP-9746." For a tarball systools built the two names are the same and
that copy overwrites Mix's, which is why OTP's comment reads as
harmless. They differ here, so both survive.

Two shapes are therefore expected and no others: one file, which is a
version as Mix assembled it, and that pair, which is an unpacked one.
release_handler's copy is the one read, because it is the file the
version was admitted on and the applications and emulator version
RELEASES records came from those very bytes - so if the two ever
disagreed, it is the release as the system understands it. They are
byte-identical in practice: Forecastle writes the tarball's copy from
Mix's with File.cp!, the tar carries it unchanged, and
release_handler's copy is a read and a write of the whole file, all
three checked rather than assumed. Nothing compares them, deliberately.
This is a choice of which file is authoritative rather than a tie-break
between equals, so it holds however the two differ, and a difference in
bytes that cannot change the answer is no reason to refuse an install
that works.

The pair is recognised by the version alone, which is the version
directory's own name - release_handler copies into releases/<Vsn> for
the Vsn it has just read out of the file, and Mix assembles into
releases/<version> - so the release name never has to be found from
anywhere. Anything else refuses as before and still names what it
found: two names that are not that pair are two release files rather
than two copies of one, and so is any third.

The comment saying a version directory holds exactly one is what
shipped this. It was true of an assembled release and false of every
unpacked one, which is the only kind this path meets, and
Castle.SyntheticRelease modelled the assembled shape faithfully enough
that seven rounds of review had nothing to notice. So the fixture now
builds the unpacked shape as well - and builds it by default, since the
peer path is reached from install and commit and so meets a version
that came out of a tarball. 29 of the 45 tests in peer_test.exs fail
against 3ec5979 for that reason alone, every one that materialises
anything, which is the gap that mattered here rather than the missing
clause.

Six tests are new. The unpacked shape resolves, and its two files are
asserted to hold the same bytes. The copy unpacking left is the one
read, shown by planting a bogus emulator version in Mix's copy: with
the two identical, nothing but a difference between them can show which
was read. A version Mix assembled still resolves from its one file. And
three directories that are genuinely ambiguous are still refused - two
unrelated names, a copy belonging to another version, and a third file
beside the pair - the second of which is what stops the pair being
recognised by suffix rather than by this version.

No RELEASE.md entry. The peer path is new in this unreleased version and
has never been reachable from a released one, so no consumer could have
seen the refusal; what the Added entry already promises is what now
works.

Verified on 1.19.5 and on 1.20.0 through mise, a recent change here
having compiled clean on 1.19 and failed 1.20's type checker: mix
compile --warnings-as-errors is clean and the suite passes 90 under
both.

Refs: #13

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4oaMvbR1cbxrZBj8qwkqN

Author: ausimian

AGENTS.md

@@ -388,6 +388,15 @@ puts the other on the peer's code path, and asserts that the answer came from
 the peer's. Peer cleanup is asserted from outside: a provider records the
 operating system pid of the VM it ran in, and the test waits for it to go.
 
+It builds the **unpacked** shape by default, and the assembled one only when a
+test asks for it — because the peer path is reached from `install` and `commit`,
+so every version it is asked to configure was unpacked from a tarball. The two
+shapes differ in the version directory's release files: Mix assembles one,
+`<name>.rel`, while unpacking leaves two, that one plus the `<name>-<vsn>.rel`
+`release_handler` copies in beside it. A fixture that only built the assembled
+shape is how `Castle.Peer` came to refuse every unpacked release as ambiguous,
+having been reviewed seven times against a directory the peer path never meets.
+
 `Castle.Peer.materialise/2` takes `:boot_timeout` and `:resolve_timeout` for the
 same reason `Castle.Commands` takes the module to talk to — a deadline nothing
 can shorten is a deadline no test can show is enforced. Two tests give it a

lib/castle/peer.ex

@@ -467,9 +467,43 @@ defmodule Castle.Peer do
   end
 
   # Mix names the release file after the release, so it is found rather than
-  # named. A version directory holds exactly one: Mix renames the one belonging
-  # to the `start` script and removes the rest. Listed rather than globbed,
-  # since a release root is a path and not a pattern.
+  # named. A version directory holds one or two of them, and which it holds says
+  # how the version got there.
+  #
+  # Mix's copy is `<name>.rel` - the release file belonging to the `start`
+  # script, renamed after the release - and a version directory as Mix
+  # assembled it holds that one alone. Unpacking a tarball into it adds a
+  # second: `release_handler` extracts the tarball's `<name>-<vsn>.rel`, reads
+  # *that* to decide what the version is - `check_rel` on the way in, and the
+  # `RELEASES` entry is written from the record it returns - and then copies it
+  # into the version directory, "keeping this for backwards compatibility
+  # reasons with older systools:make_tar, where there is no copy of the .rel
+  # file in the releases/<vsn> dir. See OTP-9746." For a tarball `systools`
+  # built the two names are the same and that copy overwrites Mix's, which is
+  # why the comment reads as harmless. They differ here, so both survive, and
+  # every unpacked release has two.
+  #
+  # Of the two, `release_handler`'s copy is the authoritative one: it is the
+  # file the version was admitted on, and the applications and emulator version
+  # `RELEASES` records came from those very bytes, so it is the release as the
+  # system understands it. The two are byte-identical in practice - Forecastle
+  # writes the tarball's copy from Mix's with `File.cp!`, the tar carries it
+  # unchanged, and `release_handler`'s copy is a read and a write of the whole
+  # file - and nothing here compares them, deliberately. This is a choice of
+  # which file is authoritative rather than a tie-break between equals, so it
+  # holds however the two differ, and a difference in bytes that cannot change
+  # the answer is no reason to refuse an install that works.
+  #
+  # Anything other than those two shapes is refused, naming what it found: two
+  # names that are not that pair are two release files rather than two copies of
+  # one, and so is any third. The version is all the pair is recognised by, and
+  # it is the version directory's own name - `release_handler` copies into
+  # `releases/<Vsn>` for the `Vsn` it has just read out of the file, and Mix
+  # assembles into `releases/<version>` - so the directory is the version, and
+  # the release name never has to be known here.
+  #
+  # Listed rather than globbed, since a release root is a path and not a
+  # pattern.
   defp release_file(rel_vsn_dir) do
     case File.ls(rel_vsn_dir) do
       {:ok, entries} ->
@@ -489,11 +523,34 @@ defmodule Castle.Peer do
   end
 
   defp release_file(rel_vsn_dir, names) do
-    {:error,
-     "Found more than one release file in #{rel_vsn_dir} - #{Enum.join(names, ", ")} - so " <>
-       "the emulator to evaluate its configuration with is ambiguous."}
+    case unpacked_copy(names, Path.basename(rel_vsn_dir)) 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."}
+    end
+  end
+
+  # The copy `release_handler` made, recognised only while the file it was made
+  # from is still beside it: the one name of the two that is the other with the
+  # version in it.
+  defp unpacked_copy([first, second], vsn) do
+    cond do
+      first == copy_of(second, vsn) -> {:ok, first}
+      second == copy_of(first, vsn) -> {:ok, second}
+      true -> :none
+    end
   end
 
+  defp unpacked_copy(_names, _vsn), do: :none
+
+  defp copy_of(name, vsn), do: "#{Path.rootname(name)}-#{vsn}.rel"
+
   ## sys.config, and the base it is resolved from
 
   # The base is `sys.config.pristine` once there is one, and `sys.config` itself

test/castle/peer_test.exs

@@ -247,12 +247,86 @@ defmodule Castle.PeerTest do
 
     test "reports a version directory with no release file", %{tmp_dir: root} do
       vsn_dir = SyntheticRelease.build(root)
-      File.rm!(Path.join(vsn_dir, "synthetic.rel"))
+      for path <- Path.wildcard(Path.join(vsn_dir, "*.rel")), do: File.rm!(path)
 
       assert {:error, message} = Castle.Peer.materialise(vsn_dir)
       assert message =~ "Cannot find a release file"
     end
 
+    # Every version this path is asked to configure was unpacked from a tarball,
+    # and an unpacked version directory holds two release files: the one Mix put
+    # inside the tarball's version directory, and the copy `release_handler`
+    # makes of the tarball's own `<name>-<vsn>.rel` beside it. That pair is not
+    # ambiguous, and refusing it refused every install - which is what the rest
+    # of this file now builds by default.
+    test "reads the release file in a version directory that was unpacked", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+
+      assert Enum.sort(rel_files(vsn_dir)) == ["synthetic-1.0.0.rel", "synthetic.rel"]
+
+      # The two hold the same bytes, which is why nothing but a difference
+      # planted between them can show which one was read.
+      assert File.read!(Path.join(vsn_dir, "synthetic-1.0.0.rel")) ==
+               File.read!(Path.join(vsn_dir, "synthetic.rel"))
+
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "reads the copy unpacking left, and not Mix's", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+
+      # Mix's copy, made to name an emulator that is not there. It is the copy
+      # `release_handler` admitted the version on that says what the version is -
+      # `RELEASES` was written from those bytes - so that is the one to read, and
+      # preferring this one would refuse a release that boots.
+      File.write!(
+        Path.join(vsn_dir, "synthetic.rel"),
+        :io_lib.format(~c"~tp.~n", [
+          {:release, {~c"synthetic", ~c"1.0.0"}, {:erts, ~c"0.0.0"}, []}
+        ])
+      )
+
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "reads the one release file a version Mix assembled has", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+
+      assert rel_files(vsn_dir) == ["synthetic.rel"]
+      assert Castle.Peer.materialise(vsn_dir) == {:ok, []}
+    end
+
+    test "refuses two release files that are not one release's", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "other.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "synthetic.rel"
+      assert message =~ "other.rel"
+    end
+
+    test "refuses a copy that belongs to another version", %{tmp_dir: root} do
+      # What is accepted is a release file and the copy made of it *for this
+      # version*, since the version directory's own name is the version. A name
+      # that merely looks like such a copy is two release files.
+      vsn_dir = SyntheticRelease.build(root, shape: :assembled)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "synthetic-2.0.0.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "synthetic-2.0.0.rel"
+    end
+
+    test "refuses a third release file beside the pair", %{tmp_dir: root} do
+      vsn_dir = SyntheticRelease.build(root)
+      File.cp!(Path.join(vsn_dir, "synthetic.rel"), Path.join(vsn_dir, "other.rel"))
+
+      assert {:error, message} = Castle.Peer.materialise(vsn_dir)
+      assert message =~ "Found more than one release file"
+      assert message =~ "other.rel"
+    end
+
     test "gives up on a peer that never boots, at the deadline", %{tmp_dir: root} do
       vsn_dir = SyntheticRelease.build(root, config: with_providers([{PeerProviderStub, []}], []))
       File.write!(Path.join(vsn_dir, "preboot.boot"), "not a boot script")
@@ -783,6 +857,7 @@ defmodule Castle.PeerTest do
                  "preboot.boot",
                  "preboot.script",
                  "synthetic.rel",
+                 "synthetic-1.0.0.rel",
                  "sys.config",
                  "sys.config.pristine",
                  Path.join(work, "sys.config"),
@@ -978,6 +1053,8 @@ defmodule Castle.PeerTest do
 
   defp mode(path), do: Bitwise.band(File.stat!(path).mode, 0o777)
 
+  defp rel_files(vsn_dir), do: Enum.filter(File.ls!(vsn_dir), &String.ends_with?(&1, ".rel"))
+
   # What the peer saw, as paths relative to the version directory.
   defp snapshot(path, relative_to) do
     assert {:ok, [entries]} = :file.consult(to_charlist(path))

test/support/synthetic_release.ex

@@ -15,8 +15,9 @@ defmodule Castle.SyntheticRelease do
   #     applications - kernel, stdlib, sasl, compiler, elixir and castle
   #     started, anything else on the code path only, which is the shape
   #     Forecastle's preboot script has,
-  #   * `releases/<vsn>/<name>.rel`, which is where the emulator version comes
-  #     from, and
+  #   * the release file or files, which is where the emulator version comes
+  #     from - see `write_release_files/4`, since a version directory holds one
+  #     of them or two depending on how the version got there, and
   #   * `releases/<vsn>/sys.config`.
   #
   # The boot script is generated by `systools` from the real application
@@ -25,6 +26,9 @@ defmodule Castle.SyntheticRelease do
 
   @started [:kernel, :stdlib, :sasl, :compiler, :elixir, :castle]
 
+  # The release name, which is what the release file or files are named after.
+  @name "synthetic"
+
   @doc """
   Builds a release under `root` and returns its version directory.
 
@@ -36,6 +40,9 @@ defmodule Castle.SyntheticRelease do
     * `:apps` - `{app, vsn, dir}` triples to add to the code path without
       starting them, which is how a provider module that lives outside the
       applications preboot starts is reached
+    * `:shape` - `:unpacked`, the default, or `:assembled`; which release files
+      the version directory holds, and so how the version is to have got there.
+      See `write_release_files/4`
   """
   def build(root, opts \\ []) do
     vsn = Keyword.get(opts, :vsn, "1.0.0")
@@ -49,7 +56,7 @@ defmodule Castle.SyntheticRelease do
     apps = apps ++ for {app, app_vsn, dir} <- extra, do: {app, app_vsn, :none, dir}
 
     make_boot_script(vsn_dir, vsn, apps, link_apps(lib, apps, extra))
-    write_rel(Path.join(vsn_dir, "synthetic.rel"), vsn, apps)
+    write_release_files(vsn_dir, vsn, apps, Keyword.get(opts, :shape, :unpacked))
     write_sys_config(vsn_dir, opts)
 
     vsn_dir
@@ -159,8 +166,10 @@ defmodule Castle.SyntheticRelease do
   end
 
   # `systools` writes the `.rel` it was given back out beside the boot script,
-  # and a version directory holds exactly one release file, so preboot's goes as
-  # soon as it has been used - which is what Mix does with its own.
+  # so preboot's goes as soon as it has been used - which is what Mix does with
+  # the release file of every boot script but `start`. Left there it would be a
+  # release file no release has, and one `Castle.Peer` would be right to refuse
+  # to choose between.
   defp make_boot_script(vsn_dir, vsn, apps, paths) do
     path = Path.join(vsn_dir, "preboot")
     write_rel(path <> ".rel", vsn, apps)
@@ -178,14 +187,41 @@ defmodule Castle.SyntheticRelease do
     File.rm!(path <> ".rel")
   end
 
+  # The release file, under the name or names the shape being built has.
+  #
+  # `:assembled` is what `mix release` leaves: one file, `<name>.rel`, the one
+  # belonging to the `start` script, which Mix renames after the release.
+  #
+  # `:unpacked` is that plus `<name>-<vsn>.rel`, and is what
+  # `release_handler:unpack_release/1` leaves behind - so it is the shape the
+  # peer path meets, every version it is asked to configure having been unpacked
+  # from a tarball. The second name is the copy Forecastle puts at the top of
+  # the tarball's `releases` directory, which `release_handler` reads to admit
+  # the version and then copies in here, for compatibility with tarballs that
+  # carried no release file inside the version directory. Made by copying,
+  # because copying is how the two come to hold the same bytes in a real
+  # release: `File.cp!` at assembly, and a read and a write of the whole file on
+  # unpack.
+  #
+  # The unpacked shape is the default because it is the one reality presents. A
+  # test that means the assembled shape asks for it.
+  defp write_release_files(vsn_dir, vsn, apps, shape) do
+    assembled = Path.join(vsn_dir, "#{@name}.rel")
+    write_rel(assembled, vsn, apps)
+
+    if shape == :unpacked do
+      File.cp!(assembled, Path.join(vsn_dir, "#{@name}-#{vsn}.rel"))
+    end
+  end
+
   defp write_rel(path, vsn, apps) do
     entries =
       for app_spec <- apps do
         {elem(app_spec, 0), to_charlist(elem(app_spec, 1)), elem(app_spec, 2)}
       end
 
     rel =
-      {:release, {~c"synthetic", to_charlist(vsn)}, {:erts, :erlang.system_info(:version)},
+      {:release, {to_charlist(@name), to_charlist(vsn)}, {:erts, :erlang.system_info(:version)},
        entries}
 
     File.write!(path, :io_lib.format(~c"~tp.~n", [rel]))