fix: report a failed commit as possibly partial

`do_make_permanent/2` writes `releases/start_erl.data` through
`set_permanent_files/5` before `write_releases/3` updates the release
record, and a throw from that write - or from the Windows service
update or the `ok = init:make_permanent/2` after it - is caught by
`handle_call/3` and returned as `{:error, reason}`. So a returned error
can arrive with the file that decides what an ordinary restart boots
already naming the target.

Saying Castle "did not make it permanent" therefore asserted the
absence of an effect that may have happened, and told an operator the
rollback still held when it may not - which is the part they act on.
Report the commit as possibly partial instead, name the file that may
already select the version, and direct them to `bin/castle releases`.

The test's refutation is the discriminator: the previous wording keeps
every other assertion in that case green.

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

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

Author: ausimian

AGENTS.md

@@ -978,9 +978,22 @@ is `<Operation> failed for <target>`. That distinction is operational state, not
 style. Install failures say the target configuration step completed and direct
 the operator to inspect release state; a non-returning install folded into a
 marker error says the install state may have changed. A failed
-`make_permanent/1` says the target configuration step completed but the version
-was not made permanent. "Completed" is deliberate: provider-less releases may
-change no configuration file.
+`make_permanent/1` says the target configuration step completed and that the
+commit **may be partial**, then directs the operator to release state.
+"Completed" is deliberate: provider-less releases may change no configuration
+file.
+
+**It must not say the version was not made permanent, and it did.**
+`do_make_permanent/2` writes `releases/start_erl.data` through
+`set_permanent_files/5` *before* `write_releases/3` updates the record, and a
+throw from that write — or from the Windows service update or the
+`ok = init:make_permanent/2` after it — is caught by `handle_call/3` and
+returned as `{:error, reason}`. So the file that decides what an ordinary
+restart boots can already name the target on a call that failed. Asserting the
+absence of an effect that may have happened is worse than reporting the
+uncertainty, because the rollback is the thing an operator acts on: the message
+states the partial case and sends them to `bin/castle releases`. Do not
+"tighten" it back into a claim about what was not done.
 
 Filesystem reasons in `Castle.Commands` and `Castle.Peer` both go through
 `Castle.FileReason`. It formats atoms with `:file.format_error/1`, retaining the

RELEASE.md

@@ -50,6 +50,11 @@ Forecastle 1.x and Elixir 1.18 or later.
   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`.

lib/castle/commands.ex

@@ -1449,6 +1449,13 @@ defmodule Castle.Commands do
   So the file that decides what an ordinary restart boots is written here and
   nowhere else, which is the whole of the rollback property: until this runs, a
   restart returns to the version that was permanent before.
+
+  **A returned error does not establish that it did not run.**
+  `set_permanent_files/5` comes before `write_releases/3`, so a failure in that
+  record write - or in the service update or `init:make_permanent/2` after it -
+  leaves `releases/start_erl.data` already naming `vsn`. Such a commit is
+  partial rather than absent, and the error says so instead of claiming the
+  version was not made permanent.
   """
   @spec commit(String.t(), Path.t(), module(), module(), module()) :: result()
   def commit(
@@ -1488,6 +1495,19 @@ defmodule Castle.Commands do
   # target configuration step has completed, but that need not mean a file was
   # changed: a provider-less release can complete it as a no-op. The message
   # reports the step, not a filesystem effect it cannot prove.
+  #
+  # **Nor may it claim the commit had no effect, and saying so was wrong.**
+  # `do_make_permanent/2` writes `releases/start_erl.data` through
+  # `set_permanent_files/5` and only *then* updates the release record through
+  # `write_releases/3` - which throws on a failed write, as do the Windows
+  # service update and the `ok = init:make_permanent/2` after it, and
+  # `handle_call/3` catches all three into the `{:error, reason}` seen here. So
+  # an error can arrive with the first write already on disk: the one file that
+  # decides what an ordinary restart boots may already name `vsn` even though
+  # the call failed. "Did not make it permanent" told an operator the rollback
+  # still held when it may not, which is the one thing they would act on. The
+  # message therefore reports the commit as possibly partial and sends them to
+  # the release state, rather than asserting an outcome this side cannot see.
   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
@@ -1497,7 +1517,9 @@ defmodule Castle.Commands do
         {:error, reason} ->
           {:error,
            "Commit failed for #{vsn}: #{inspect(reason)}. Castle completed the target " <>
-             "configuration step but did not make it permanent."}
+             "configuration step, and the commit may be partial: " <>
+             "releases/start_erl.data may already select #{vsn}. Run bin/castle " <>
+             "releases to inspect release state before restarting."}
       end
     end
   end

test/castle/commands_test.exs

@@ -1477,7 +1477,19 @@ defmodule Castle.CommandsTest do
       assert message =~ "Commit failed for 1.2.3:"
       assert message =~ "bad_status"
       assert message =~ "Castle completed the target configuration step"
-      assert message =~ "did not make it permanent"
+
+      # `set_permanent_files/5` writes releases/start_erl.data before
+      # `write_releases/3` updates the record, so a returned error can arrive
+      # with the boot already selecting the target. The message has to report
+      # that as possibly partial and send the operator to release state; the
+      # refutation is the discriminator, since claiming the version was *not*
+      # made permanent reads as reassuring and tells them the rollback holds
+      # when it may not.
+      assert message =~ "the commit may be partial"
+      assert message =~ "releases/start_erl.data may already select 1.2.3"
+      assert message =~ "Run bin/castle releases"
+      refute message =~ "did not make it permanent"
+
       assert configuration(dir, "1.2.3") == "[].\n"
       assert PeerStub.calls() == [Path.join(dir, "1.2.3")]
     end